Re: [PATCHv2 5/7] TAP: Extending tap device create/destroy APIs

[Andy Shevchenko]

On Wed, Jan 18, 2017 at 2:03 AM, Sainath Grandhi
 wrote:
> Extending tap APIs get/free_minor and create/destroy_cdev to handle more than 
> one
> type of virtual interface.
>

Yes, looks better now.

FWIW:
Reviewed-by: Andy Shevchenko 

> Signed-off-by: Sainath Grandhi 
> ---
>  drivers/net/macvtap_main.c |  6 +--
>  drivers/net/tap.c  | 98 
> +++---
>  include/linux/if_tap.h |  4 +-
>  3 files changed, 80 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/macvtap_main.c b/drivers/net/macvtap_main.c
> index 6326a82..3f047b4 100644
> --- a/drivers/net/macvtap_main.c
> +++ b/drivers/net/macvtap_main.c
> @@ -160,7 +160,7 @@ static int macvtap_device_event(struct notifier_block 
> *unused,
>  * been registered but before register_netdevice has
>  * finished running.
>  */
> -   err = tap_get_minor(>tap);
> +   err = tap_get_minor(macvtap_major, >tap);
> if (err)
> return notifier_from_errno(err);
>
> @@ -168,7 +168,7 @@ static int macvtap_device_event(struct notifier_block 
> *unused,
> classdev = device_create(_class, >dev, devt,
>  dev, tap_name);
> if (IS_ERR(classdev)) {
> -   tap_free_minor(>tap);
> +   tap_free_minor(macvtap_major, >tap);
> return notifier_from_errno(PTR_ERR(classdev));
> }
> err = sysfs_create_link(>dev.kobj, >kobj,
> @@ -183,7 +183,7 @@ static int macvtap_device_event(struct notifier_block 
> *unused,
> sysfs_remove_link(>dev.kobj, tap_name);
> devt = MKDEV(MAJOR(macvtap_major), vlantap->tap.minor);
> device_destroy(_class, devt);
> -   tap_free_minor(>tap);
> +   tap_free_minor(macvtap_major, >tap);
> break;
> case NETDEV_CHANGE_TX_QUEUE_LEN:
> if (tap_queue_resize(>tap))
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index 43d9d54..7f38dbe 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -99,12 +99,16 @@ static struct proto tap_proto = {
>  };
>
>  #define TAP_NUM_DEVS (1U << MINORBITS)
> +
> +static LIST_HEAD(major_list);
> +
>  struct major_info {
> dev_t major;
> struct idr minor_idr;
> struct mutex minor_lock;
> const char *device_name;
> -} macvtap_major;
> +   struct list_head next;
> +};
>
>  #define GOODCOPY_LEN 128
>
> @@ -385,44 +389,73 @@ rx_handler_result_t tap_handle_frame(struct sk_buff 
> **pskb)
> return RX_HANDLER_CONSUMED;
>  }
>
> -int tap_get_minor(struct tap_dev *tap)
> +static struct major_info *tap_get_major(int major)
> +{
> +   struct major_info *tap_major, *tmp;
> +
> +   list_for_each_entry_safe(tap_major, tmp, _list, next) {
> +   if (tap_major->major == major) {
> +   return tap_major;
> +   }
> +   }
> +
> +   return NULL;
> +}
> +
> +int tap_get_minor(dev_t major, struct tap_dev *tap)
>  {
> int retval = -ENOMEM;
> +   struct major_info *tap_major;
> +
> +   tap_major = tap_get_major(MAJOR(major));
> +   if (!tap_major)
> +   return -EINVAL;
>
> -   mutex_lock(_major.minor_lock);
> -   retval = idr_alloc(_major.minor_idr, tap, 1, TAP_NUM_DEVS, 
> GFP_KERNEL);
> +   mutex_lock(_major->minor_lock);
> +   retval = idr_alloc(_major->minor_idr, tap, 1, TAP_NUM_DEVS, 
> GFP_KERNEL);
> if (retval >= 0) {
> tap->minor = retval;
> } else if (retval == -ENOSPC) {
> netdev_err(tap->dev, "Too many tap devices\n");
> retval = -EINVAL;
> }
> -   mutex_unlock(_major.minor_lock);
> +   mutex_unlock(_major->minor_lock);
> return retval < 0 ? retval : 0;
>  }
>
> -void tap_free_minor(struct tap_dev *tap)
> +void tap_free_minor(dev_t major, struct tap_dev *tap)
>  {
> -   mutex_lock(_major.minor_lock);
> +   struct major_info *tap_major;
> +
> +   tap_major = tap_get_major(MAJOR(major));
> +   if (!tap_major)
> +   return;
> +
> +   mutex_lock(_major->minor_lock);
> if (tap->minor) {
> -   idr_remove(_major.minor_idr, tap->minor);
> +   idr_remove(_major->minor_idr, tap->minor);
> tap->minor = 0;
> }
> -   mutex_unlock(_major.minor_lock);
> +   mutex_unlock(_major->minor_lock);
>  }
>
> -static struct tap_dev *dev_get_by_tap_minor(int minor)
> +static struct tap_dev *dev_get_by_tap_file(int major, int minor)
>  {
> struct net_device *dev = NULL;
> struct tap_dev *tap;
> +   struct major_info *tap_major;
> +
> +   tap_major = tap_get_major(major);
> +   if (!tap_major)

[PATCHv2 5/7] TAP: Extending tap device create/destroy APIs

[Sainath Grandhi]

Extending tap APIs get/free_minor and create/destroy_cdev to handle more than 
one
type of virtual interface.

Signed-off-by: Sainath Grandhi 
---
 drivers/net/macvtap_main.c |  6 +--
 drivers/net/tap.c  | 98 +++---
 include/linux/if_tap.h |  4 +-
 3 files changed, 80 insertions(+), 28 deletions(-)

diff --git a/drivers/net/macvtap_main.c b/drivers/net/macvtap_main.c
index 6326a82..3f047b4 100644
--- a/drivers/net/macvtap_main.c
+++ b/drivers/net/macvtap_main.c
@@ -160,7 +160,7 @@ static int macvtap_device_event(struct notifier_block 
*unused,
 * been registered but before register_netdevice has
 * finished running.
 */
-   err = tap_get_minor(>tap);
+   err = tap_get_minor(macvtap_major, >tap);
if (err)
return notifier_from_errno(err);
 
@@ -168,7 +168,7 @@ static int macvtap_device_event(struct notifier_block 
*unused,
classdev = device_create(_class, >dev, devt,
 dev, tap_name);
if (IS_ERR(classdev)) {
-   tap_free_minor(>tap);
+   tap_free_minor(macvtap_major, >tap);
return notifier_from_errno(PTR_ERR(classdev));
}
err = sysfs_create_link(>dev.kobj, >kobj,
@@ -183,7 +183,7 @@ static int macvtap_device_event(struct notifier_block 
*unused,
sysfs_remove_link(>dev.kobj, tap_name);
devt = MKDEV(MAJOR(macvtap_major), vlantap->tap.minor);
device_destroy(_class, devt);
-   tap_free_minor(>tap);
+   tap_free_minor(macvtap_major, >tap);
break;
case NETDEV_CHANGE_TX_QUEUE_LEN:
if (tap_queue_resize(>tap))
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 43d9d54..7f38dbe 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -99,12 +99,16 @@ static struct proto tap_proto = {
 };
 
 #define TAP_NUM_DEVS (1U << MINORBITS)
+
+static LIST_HEAD(major_list);
+
 struct major_info {
dev_t major;
struct idr minor_idr;
struct mutex minor_lock;
const char *device_name;
-} macvtap_major;
+   struct list_head next;
+};
 
 #define GOODCOPY_LEN 128
 
@@ -385,44 +389,73 @@ rx_handler_result_t tap_handle_frame(struct sk_buff 
**pskb)
return RX_HANDLER_CONSUMED;
 }
 
-int tap_get_minor(struct tap_dev *tap)
+static struct major_info *tap_get_major(int major)
+{
+   struct major_info *tap_major, *tmp;
+
+   list_for_each_entry_safe(tap_major, tmp, _list, next) {
+   if (tap_major->major == major) {
+   return tap_major;
+   }
+   }
+
+   return NULL;
+}
+
+int tap_get_minor(dev_t major, struct tap_dev *tap)
 {
int retval = -ENOMEM;
+   struct major_info *tap_major;
+
+   tap_major = tap_get_major(MAJOR(major));
+   if (!tap_major)
+   return -EINVAL;
 
-   mutex_lock(_major.minor_lock);
-   retval = idr_alloc(_major.minor_idr, tap, 1, TAP_NUM_DEVS, 
GFP_KERNEL);
+   mutex_lock(_major->minor_lock);
+   retval = idr_alloc(_major->minor_idr, tap, 1, TAP_NUM_DEVS, 
GFP_KERNEL);
if (retval >= 0) {
tap->minor = retval;
} else if (retval == -ENOSPC) {
netdev_err(tap->dev, "Too many tap devices\n");
retval = -EINVAL;
}
-   mutex_unlock(_major.minor_lock);
+   mutex_unlock(_major->minor_lock);
return retval < 0 ? retval : 0;
 }
 
-void tap_free_minor(struct tap_dev *tap)
+void tap_free_minor(dev_t major, struct tap_dev *tap)
 {
-   mutex_lock(_major.minor_lock);
+   struct major_info *tap_major;
+
+   tap_major = tap_get_major(MAJOR(major));
+   if (!tap_major)
+   return;
+
+   mutex_lock(_major->minor_lock);
if (tap->minor) {
-   idr_remove(_major.minor_idr, tap->minor);
+   idr_remove(_major->minor_idr, tap->minor);
tap->minor = 0;
}
-   mutex_unlock(_major.minor_lock);
+   mutex_unlock(_major->minor_lock);
 }
 
-static struct tap_dev *dev_get_by_tap_minor(int minor)
+static struct tap_dev *dev_get_by_tap_file(int major, int minor)
 {
struct net_device *dev = NULL;
struct tap_dev *tap;
+   struct major_info *tap_major;
+
+   tap_major = tap_get_major(major);
+   if (!tap_major)
+   return NULL;
 
-   mutex_lock(_major.minor_lock);
-   tap = idr_find(_major.minor_idr, minor);
+   mutex_lock(_major->minor_lock);
+   tap = idr_find(_major->minor_idr, minor);
if (tap) {
dev = tap->dev;
dev_hold(dev);
}
-   mutex_unlock(_major.minor_lock);
+   mutex_unlock(_major->minor_lock);
return tap;
 }
 
@@ -454,7 +487,7 @@ static int