[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.
