>
> 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@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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);
> >> }
> >
