[EXT] Re: [PATCH] USB: gadget: Add ID numbers to configfs-gadget driver names

[Frank Li]

> 
> Caution: EXT Email
> 
> On 13/12/2022 14:22, Christophe JAILLET wrote:
> > Le 13/12/2022 à 05:12, Chanh Nguyen a écrit :
> >> It is unable to use configfs to attach more than one gadget. When
> >> attaching the second gadget, it always fails and the kernel message
> >> prints out:
> >>
> >> Error: Driver 'configfs-gadget' is already registered, aborting...
> >> UDC core: g1: driver registration failed: -16
> >>
> >> This commit fixes the problem by a ".N" suffix added to each
> >> configfs_gadget's driver name (where N is a unique ID number),
> >> thus making the names distinct.
> >>
> >> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> >> Signed-off-by: Chanh Nguyen
> >> <chanh-shex6MNQR2J/SfDzf78azzKzEDxYleXD at public.gmane.org>
> >> ---
> >>   drivers/usb/gadget/configfs.c | 42
> +++++++++++++++++++++++++++++++++++
> >>   1 file changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/usb/gadget/configfs.c
> >> b/drivers/usb/gadget/configfs.c
> >> index 96121d1c8df4..d8c5156ad777 100644
> >> --- a/drivers/usb/gadget/configfs.c
> >> +++ b/drivers/usb/gadget/configfs.c
> >> @@ -3,6 +3,7 @@
> >>   #include <linux/module.h>
> >>   #include <linux/slab.h>
> >>   #include <linux/device.h>
> >> +#include <linux/idr.h>
> >>   #include <linux/kstrtox.h>
> >>   #include <linux/nls.h>
> >>   #include <linux/usb/composite.h>
> >> @@ -11,6 +12,16 @@
> >>   #include "u_f.h"
> >>   #include "u_os_desc.h"
> >> +static DEFINE_IDA(driver_id_numbers);
> >> +
> >> +/*
> >> + * Driver name has the form of "configfs-gadget.%d", where %d
> >> + * is id allocated by ida_alloc(). The max value returns by
> >> + * ida_alloc() is INT_MAX, in 64-bit system, it is about nine
> >> + * quintillion: 19 digits in decimal. Set the max length to 35.
> >> + */
> >> +#define DRIVER_NAME_LENGTH_MAX 35
> >
> > Hi,
> >
> > if paranoiac, the final \0 seems to be missing in the max length
> > computation, but see below.
> 
> Thanks CJ! Indeed, I have missed that.
> 
> >
> >> +
> >>   int check_user_usb_string(const char *name,
> >>           struct usb_gadget_strings *stringtab_dev)
> >>   {
> >> @@ -52,6 +63,9 @@ struct gadget_info {
> >>       char qw_sign[OS_STRING_QW_SIGN_LEN];
> >>       spinlock_t spinlock;
> >>       bool unbind;
> >> +
> >> +    /* Make driver names unique */
> >> +    int driver_id_number;
> >>   };
> >>   static inline struct gadget_info *to_gadget_info(struct config_item
> >> *item)
> >> @@ -1582,6 +1596,8 @@ static struct config_group *gadgets_make(
> >>           const char *name)
> >>   {
> >>       struct gadget_info *gi;
> >> +    char *driver_name;
> >> +    int ret;
> >>       gi = kzalloc(sizeof(*gi), GFP_KERNEL);
> >>       if (!gi)
> >> @@ -1623,6 +1639,21 @@ static struct config_group *gadgets_make(
> >>       gi->composite.gadget_driver = configfs_driver_template;
> >> +    ret = ida_alloc(&driver_id_numbers, GFP_KERNEL);
> >> +    if (ret < 0)
> >> +        goto err;
> >> +    gi->driver_id_number = ret;
> >> +
> >> +    driver_name = kmalloc(DRIVER_NAME_LENGTH_MAX, GFP_KERNEL);
> >> +    if (!driver_name)
> >> +        goto out_free_driver_id_number;
> >> +
> >> +    ret = scnprintf(driver_name, DRIVER_NAME_LENGTH_MAX,
> >> +            "configfs-gadget.%d", gi->driver_id_number);
> >
> >
> > using kasprintf() looks simpler here.
> > No need to kmalloc()+scnprintf(), and no need for
> DRIVER_NAME_LENGTH_MAX.
> >
> > Just my 2c,
> >
> > CJ
> 
> Thanks CJ for the review!
> 
> I've made some changes as below (in gadgets_make() to remove
> unnecessary
> variables) and now trying to test it as much as possible. Will re-post
> it as v2 if looks good soon.
> 
> static inline struct gadget_info *to_gadget_info(struct config_item *item)
> @@ -1623,13 +1629,25 @@ static struct config_group *gadgets_make(
> 
>       gi->composite.gadget_driver = configfs_driver_template;
> 
> +    gi->driver_id_number = ida_alloc(&driver_id_numbers, GFP_KERNEL);
> +    if (gi->driver_id_number < 0)
> +        goto err;
> +
> +    gi->composite.gadget_driver.driver.name =
> +                          kasprintf(GFP_KERNEL, "configfs-gadget.%d",
> +                                    gi->driver_id_number);
> +    if (!gi->composite.gadget_driver.driver.name)
> +        goto out_free_driver_id_number;
> +
>       gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
>       gi->composite.name = gi->composite.gadget_driver.function;
> 
>       if (!gi->composite.gadget_driver.function)
> -        goto err;
> +        goto out_free_driver_id_number;
> 
>       return &gi->group;
> +
> +out_free_driver_id_number:
> +    ida_free(&driver_id_numbers, gi->driver_id_number);
>   err:
>       kfree(gi);
>       return ERR_PTR(-ENOMEM);
> 
> 
> >
> >> +    if (ret < 0)
> >> +        goto out_free_driver_name;
> >> +
> >> +    gi->composite.gadget_driver.driver.name = driver_name;
> >>       gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
> >>       gi->composite.name = gi->composite.gadget_driver.function;
> >> @@ -1630,6 +1661,11 @@ static struct config_group *gadgets_make(
> >>           goto err;
> >>       return &gi->group;
> >> +
> >> +out_free_driver_name:
> >> +    kfree(driver_name);
> >> +out_free_driver_id_number:
> >> +    ida_free(&driver_id_numbers, gi->driver_id_number);
> >>   err:
> >>       kfree(gi);
> >>       return ERR_PTR(-ENOMEM);
> >> @@ -1637,6 +1673,12 @@ static struct config_group *gadgets_make(
> >>   static void gadgets_drop(struct config_group *group, struct
> >> config_item *item)
> >>   {
> >> +    struct gadget_info *gi = to_gadget_info(item);
> >> +
> >> +    mutex_lock(&gi->lock);
> >> +    kfree(gi->composite.gadget_driver.driver.name);
> >> +    ida_free(&driver_id_numbers, gi->driver_id_number);
> >> +    mutex_unlock(&gi->lock);

Move all free into gadget_info_attr_release(), just before kfree(gi)
Driver.name and gi create at the same place, 
Free should be the same place also. 

Best regards
Frank Li

> >>       config_item_put(item);
> >>   }
> >