[PATCHv3 5/7] TAP: Extending tap device create/destroy APIs
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 dd6f4e4..50fe993 100644 --- a/drivers/net/macvtap_main.c +++ b/drivers/net/macvtap_main.c @@ -163,7 +163,7 @@ static int macvtap_device_event(struct notifier_block *unused, * been registered but before register_netdevice has * finished running. */ - err = tap_get_minor(&vlantap->tap); + err = tap_get_minor(macvtap_major, &vlantap->tap); if (err) return notifier_from_errno(err); @@ -171,7 +171,7 @@ static int macvtap_device_event(struct notifier_block *unused, classdev = device_create(&macvtap_class, &dev->dev, devt, dev, tap_name); if (IS_ERR(classdev)) { - tap_free_minor(&vlantap->tap); + tap_free_minor(macvtap_major, &vlantap->tap); return notifier_from_errno(PTR_ERR(classdev)); } err = sysfs_create_link(&dev->dev.kobj, &classdev->kobj, @@ -186,7 +186,7 @@ static int macvtap_device_event(struct notifier_block *unused, sysfs_remove_link(&dev->dev.kobj, tap_name); devt = MKDEV(MAJOR(macvtap_major), vlantap->tap.minor); device_destroy(&macvtap_class, devt); - tap_free_minor(&vlantap->tap); + tap_free_minor(macvtap_major, &vlantap->tap); break; case NETDEV_CHANGE_TX_QUEUE_LEN: if (tap_queue_resize(&vlantap->tap)) diff --git a/drivers/net/tap.c b/drivers/net/tap.c index ede436a..1219ee9 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, &major_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(&macvtap_major.minor_lock); - retval = idr_alloc(&macvtap_major.minor_idr, tap, 1, TAP_NUM_DEVS, GFP_KERNEL); + mutex_lock(&tap_major->minor_lock); + retval = idr_alloc(&tap_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(&macvtap_major.minor_lock); + mutex_unlock(&tap_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(&macvtap_major.minor_lock); + struct major_info *tap_major; + + tap_major = tap_get_major(MAJOR(major)); + if (!tap_major) + return; + + mutex_lock(&tap_major->minor_lock); if (tap->minor) { - idr_remove(&macvtap_major.minor_idr, tap->minor); + idr_remove(&tap_major->minor_idr, tap->minor); tap->minor = 0; } - mutex_unlock(&macvtap_major.minor_lock); + mutex_unlock(&tap_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(&macvtap_major.minor_lock); - tap = idr_find(&macvtap_major.minor_idr, minor); + mutex_lock(&tap_major->minor_lock); + tap = idr_find(&tap_major->minor_idr, minor); if (tap) { dev = tap->dev;
[PATCHv3 5/7] tap: Extending tap device create/destroy APIs
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 dd6f4e4..50fe993 100644 --- a/drivers/net/macvtap_main.c +++ b/drivers/net/macvtap_main.c @@ -163,7 +163,7 @@ static int macvtap_device_event(struct notifier_block *unused, * been registered but before register_netdevice has * finished running. */ - err = tap_get_minor(&vlantap->tap); + err = tap_get_minor(macvtap_major, &vlantap->tap); if (err) return notifier_from_errno(err); @@ -171,7 +171,7 @@ static int macvtap_device_event(struct notifier_block *unused, classdev = device_create(&macvtap_class, &dev->dev, devt, dev, tap_name); if (IS_ERR(classdev)) { - tap_free_minor(&vlantap->tap); + tap_free_minor(macvtap_major, &vlantap->tap); return notifier_from_errno(PTR_ERR(classdev)); } err = sysfs_create_link(&dev->dev.kobj, &classdev->kobj, @@ -186,7 +186,7 @@ static int macvtap_device_event(struct notifier_block *unused, sysfs_remove_link(&dev->dev.kobj, tap_name); devt = MKDEV(MAJOR(macvtap_major), vlantap->tap.minor); device_destroy(&macvtap_class, devt); - tap_free_minor(&vlantap->tap); + tap_free_minor(macvtap_major, &vlantap->tap); break; case NETDEV_CHANGE_TX_QUEUE_LEN: if (tap_queue_resize(&vlantap->tap)) diff --git a/drivers/net/tap.c b/drivers/net/tap.c index ede436a..1219ee9 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, &major_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(&macvtap_major.minor_lock); - retval = idr_alloc(&macvtap_major.minor_idr, tap, 1, TAP_NUM_DEVS, GFP_KERNEL); + mutex_lock(&tap_major->minor_lock); + retval = idr_alloc(&tap_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(&macvtap_major.minor_lock); + mutex_unlock(&tap_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(&macvtap_major.minor_lock); + struct major_info *tap_major; + + tap_major = tap_get_major(MAJOR(major)); + if (!tap_major) + return; + + mutex_lock(&tap_major->minor_lock); if (tap->minor) { - idr_remove(&macvtap_major.minor_idr, tap->minor); + idr_remove(&tap_major->minor_idr, tap->minor); tap->minor = 0; } - mutex_unlock(&macvtap_major.minor_lock); + mutex_unlock(&tap_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(&macvtap_major.minor_lock); - tap = idr_find(&macvtap_major.minor_idr, minor); + mutex_lock(&tap_major->minor_lock); + tap = idr_find(&tap_major->minor_idr, minor); if (tap) { dev = tap->dev;
Re: [PATCHv3 5/7] tap: Extending tap device create/destroy APIs
From: Sainath Grandhi Date: Mon, 30 Jan 2017 11:12:00 -0800 > + list_for_each_entry_safe(tap_major, tmp, &major_list, next) { > + if (tap_major->major == major) { > + return tap_major; > + } > + } Single line basic blocks, such as this 'if' statement, should not be surrounded by curly braces. Also I see not mutual exclusion being implemented to protect this list. If there is, it is not obvious, so it should be documented with a comment explaining what protects the list. > @@ -1175,14 +1228,7 @@ int tap_create_cdev(struct cdev *tap_cdev, > if (err) > goto out2; > > - macvtap_major.major = MAJOR(*tap_major); > - > - idr_init(&macvtap_major.minor_idr); > - mutex_init(&macvtap_major.minor_lock); > - > - macvtap_major.device_name = device_name; > - > - return err; > + return tap_list_add(*tap_major, device_name); > > out2: > unregister_chrdev_region(*tap_major, TAP_NUM_DEVS); If tap_list_add() fails, you will leave the chrdev region registered.
RE: [PATCHv3 5/7] tap: Extending tap device create/destroy APIs
> -Original Message- > From: David Miller [mailto:da...@davemloft.net] > Sent: Tuesday, January 31, 2017 9:51 AM > To: Grandhi, Sainath > Cc: netdev@vger.kernel.org; mah...@bandewar.net; linux- > ker...@vger.kernel.org > Subject: Re: [PATCHv3 5/7] tap: Extending tap device create/destroy APIs > > From: Sainath Grandhi > Date: Mon, 30 Jan 2017 11:12:00 -0800 > > > + list_for_each_entry_safe(tap_major, tmp, &major_list, next) { > > + if (tap_major->major == major) { > > + return tap_major; > > + } > > + } > > Single line basic blocks, such as this 'if' statement, should not be > surrounded > by curly braces. > > Also I see not mutual exclusion being implemented to protect this list. If > there > is, it is not obvious, so it should be documented with a comment explaining > what protects the list. Ok. It's my fault to not include safe reading of the list. I would use the RCU API for list traversal, adding and deleting entries. Please look for it in my next version. > > > @@ -1175,14 +1228,7 @@ int tap_create_cdev(struct cdev *tap_cdev, > > if (err) > > goto out2; > > > > - macvtap_major.major = MAJOR(*tap_major); > > - > > - idr_init(&macvtap_major.minor_idr); > > - mutex_init(&macvtap_major.minor_lock); > > - > > - macvtap_major.device_name = device_name; > > - > > - return err; > > + return tap_list_add(*tap_major, device_name); > > > > out2: > > unregister_chrdev_region(*tap_major, TAP_NUM_DEVS); > > If tap_list_add() fails, you will leave the chrdev region registered. Thanks. Would take care of it.