Please find reply inline.
> -----Original Message----- > From: Andy Shevchenko [mailto:andy.shevche...@gmail.com] > Sent: Friday, January 06, 2017 3:21 PM > To: Grandhi, Sainath <sainath.gran...@intel.com> > Cc: netdev <netdev@vger.kernel.org>; David S. Miller > <da...@davemloft.net>; mah...@bandewar.net; linux- > ker...@vger.kernel.org > Subject: Re: [PATCHv1 5/7] TAP: Extending tap device create/destroy APIs > > On Sat, Jan 7, 2017 at 12:33 AM, Sainath Grandhi > <sainath.gran...@intel.com> wrote: > > Extending tap APIs get/free_minor and create/destroy_cdev to handle > > more than one type of virtual interface. > > > > Signed-off-by: Sainath Grandhi <sainath.gran...@intel.com> > > Tested-by: Sainath Grandhi <sainath.gran...@intel.com> > > Usually it implies that commiter has tested the stuff. > > > --- 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) > > > + > > +LIST_HEAD(major_list); > > + > > static ? Makes sense. Would take care of it. > > > -int tap_get_minor(struct tap_dev *tap) > > +int tap_get_minor(dev_t major, struct tap_dev *tap) > > { > > int retval = -ENOMEM; > > + struct major_info *tap_major, *tmp; > > + bool found = false; > > > > - mutex_lock(&macvtap_major.minor_lock); > > - retval = idr_alloc(&macvtap_major.minor_idr, tap, 1, TAP_NUM_DEVS, > GFP_KERNEL); > > > + list_for_each_entry_safe(tap_major, tmp, &major_list, next) { > > + if (tap_major->major == MAJOR(major)) { > > + found = true; > > + break; > > + } > > + } > > + > > + if (!found) > > + return -EINVAL; > > This is candidate to be a separate helper function. See also below. Would define a helper function. > > > > -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, *tmp; > > > + bool found = false; > > + > > + list_for_each_entry_safe(tap_major, tmp, &major_list, next) { > > + if (tap_major->major == MAJOR(major)) { > > + found = true; > > + break; > > + } > > + } > > + > > + if (!found) > > + return; > > Here is quite the same code (as above). > > > -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, *tmp; > > + bool found = false; > > > > - mutex_lock(&macvtap_major.minor_lock); > > - tap = idr_find(&macvtap_major.minor_idr, minor); > > > + list_for_each_entry_safe(tap_major, tmp, &major_list, next) { > > + if (tap_major->major == major) { > > + found = true; > > + break; > > + } > > + } > > + > > + if (!found) > > + return NULL; > > And here. > > > +static int tap_list_add(dev_t major, const char *device_name) { > > > + int err = 0; > > + struct major_info *tap_major; > > Perhaps > + struct major_info *tap_major; > + int err = 0; > > > + > > + tap_major = kzalloc(sizeof(*tap_major), GFP_ATOMIC); > > + > > + tap_major->major = MAJOR(major); > > + > > + idr_init(&tap_major->minor_idr); > > + mutex_init(&tap_major->minor_lock); > > + > > + tap_major->device_name = device_name; > > + > > + list_add_tail(&tap_major->next, &major_list); > > + return err; > > > > + err = tap_list_add(*tap_major, device_name); > > > > return err; > > return tap_list_add(); > > > void tap_destroy_cdev(dev_t major, struct cdev *tap_cdev) { > > + struct major_info *tap_major, *tmp; > > + bool found = false; > > + > > + list_for_each_entry_safe(tap_major, tmp, &major_list, next) { > > + if (tap_major->major == MAJOR(major)) { > > + found = true; > > + break; > > + } > > + } > > + > > + if (!found) > > + return; > > And here. > > -- > With Best Regards, > Andy Shevchenko
