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

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

[Chanh Nguyen]

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@os.amperecomputing.com>
---
 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
+
 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);
+	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);
 	config_item_put(item);
 }
 
-- 
2.17.1

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

[Christophe JAILLET]

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

> +
>   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

> +	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);
>   	config_item_put(item);
>   }
>   

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

[Christophe JAILLET]

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

> +
>   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

> +	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);
>   	config_item_put(item);
>   }
>   

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

[Chanh Nguyen]

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@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);
>>       config_item_put(item);
>>   }
> 

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

[Chanh Nguyen]

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@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);
>>       config_item_put(item);
>>   }
> 

RE: [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@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);
> >>   }
> >

RE: [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@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);
> >>   }
> >

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

[Alan Stern]

On Wed, Dec 14, 2022 at 11:15:48AM +0700, Chanh Nguyen wrote:

> 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;

This should goto out_free_driver_name.

Alan Stern

> 
>      return &gi->group;
> +
> +out_free_driver_id_number:
> +    ida_free(&driver_id_numbers, gi->driver_id_number);
>  err:
>      kfree(gi);
>      return ERR_PTR(-ENOMEM);

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

[Alan Stern]

On Wed, Dec 14, 2022 at 11:15:48AM +0700, Chanh Nguyen wrote:

> 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;

This should goto out_free_driver_name.

Alan Stern

> 
>      return &gi->group;
> +
> +out_free_driver_id_number:
> +    ida_free(&driver_id_numbers, gi->driver_id_number);
>  err:
>      kfree(gi);
>      return ERR_PTR(-ENOMEM);

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

[Chanh Nguyen]

On 14/12/2022 11:20, Frank Li wrote:
>>
>> 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@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.
> 

Thanks a lot for the quick review comment.

As per my observation through the test, on the first mount, the 
virtual-media the gadgets_make() is called, then later, when unmount, 
the gadgets_drop() is called and followed by gadget_info_attr_release().

The gadget_info_attr_release() is registered as .release() of 
gadget_root_type for the gi->group via the call 
"config_group_init_type_name(&gi->group, name, &gadget_root_type);"

In general, the .release() will be called only for the group. There is 
nothing to guarantee that the group will always be registered, ie: 
incase without the call to "config_group_init_type_name(&gi->group, 
name, &gadget_root_type);"

In this patch, what is added is an ida number to be used to make up the 
composite driver name. This is done in gadgets_make() so we'd like to 
add the cleanup code to gadgets_drop() as they are registered together 
in the same place and would be a little easier to read than adding them 
to _release() as the code below:

     static struct configfs_group_operations gadgets_ops = {
         .make_group = &gadgets_make,
         .drop_item = &gadgets_drop,
     };

Anyway, we still doubt that there might be something that we have missed 
so please let me know the reason why putting cleanup codes to _release() 
would be a better solution.

Thank you and best regards,
- Chanh

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

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

[Chanh Nguyen]

On 14/12/2022 11:20, Frank Li wrote:
>>
>> 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@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.
> 

Thanks a lot for the quick review comment.

As per my observation through the test, on the first mount, the 
virtual-media the gadgets_make() is called, then later, when unmount, 
the gadgets_drop() is called and followed by gadget_info_attr_release().

The gadget_info_attr_release() is registered as .release() of 
gadget_root_type for the gi->group via the call 
"config_group_init_type_name(&gi->group, name, &gadget_root_type);"

In general, the .release() will be called only for the group. There is 
nothing to guarantee that the group will always be registered, ie: 
incase without the call to "config_group_init_type_name(&gi->group, 
name, &gadget_root_type);"

In this patch, what is added is an ida number to be used to make up the 
composite driver name. This is done in gadgets_make() so we'd like to 
add the cleanup code to gadgets_drop() as they are registered together 
in the same place and would be a little easier to read than adding them 
to _release() as the code below:

     static struct configfs_group_operations gadgets_ops = {
         .make_group = &gadgets_make,
         .drop_item = &gadgets_drop,
     };

Anyway, we still doubt that there might be something that we have missed 
so please let me know the reason why putting cleanup codes to _release() 
would be a better solution.

Thank you and best regards,
- Chanh

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

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

[Frank Li]

]
> >
> > 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.
> >
> 
> Thanks a lot for the quick review comment.
> 
> As per my observation through the test, on the first mount, the
> virtual-media the gadgets_make() is called, then later, when unmount,
> the gadgets_drop() is called and followed by gadget_info_attr_release().
> 
> The gadget_info_attr_release() is registered as .release() of
> gadget_root_type for the gi->group via the call
> "config_group_init_type_name(&gi->group, name, &gadget_root_type);"
> 
> In general, the .release() will be called only for the group. There is
> nothing to guarantee that the group will always be registered, ie:
> incase without the call to "config_group_init_type_name(&gi->group,
> name, &gadget_root_type);"
> 
> In this patch, what is added is an ida number to be used to make up the
> composite driver name. This is done in gadgets_make() so we'd like to
> add the cleanup code to gadgets_drop() as they are registered together
> in the same place and would be a little easier to read than adding them
> to _release() as the code below:
> 
>      static struct configfs_group_operations gadgets_ops = {
>          .make_group = &gadgets_make,
>          .drop_item = &gadgets_drop,
>      };
> 
> Anyway, we still doubt that there might be something that we have missed
> so please let me know the reason why putting cleanup codes to _release()
> would be a better solution.

Let's simply the logic

Gadget_make()
{
        gi=kazalloc();
        gi->composite.gadget_driver.driver.name = kasprintf();
        gi->compositr.gadget_drver.function = kstrdup();
}

this is only place to create gi and driver.name. 
no other place to set driver.name,  so I think gi take ownship of driver.name. 

so we only track when gi free,   just free driver.name before gi free. 

Related malloc and free should be pair together. 

Gadget_info_attr_release()
{
	Kfree(gi->composite.gadget_driver.function)
	Kfree(gi->composiste.gadget_driver.driver.name);
	Kfree(gi);
}

Gadget_driver.driver.name should be similar as gadget_driver.function

If driver.name is freed in gadget_drop(), gi can not actually be reused again.



> 
> Thank you and best regards,
> - Chanh
> 
> >
> >>>>        config_item_put(item);
> >>>>    }
> >>>

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

[Frank Li]

]
> >
> > 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.
> >
> 
> Thanks a lot for the quick review comment.
> 
> As per my observation through the test, on the first mount, the
> virtual-media the gadgets_make() is called, then later, when unmount,
> the gadgets_drop() is called and followed by gadget_info_attr_release().
> 
> The gadget_info_attr_release() is registered as .release() of
> gadget_root_type for the gi->group via the call
> "config_group_init_type_name(&gi->group, name, &gadget_root_type);"
> 
> In general, the .release() will be called only for the group. There is
> nothing to guarantee that the group will always be registered, ie:
> incase without the call to "config_group_init_type_name(&gi->group,
> name, &gadget_root_type);"
> 
> In this patch, what is added is an ida number to be used to make up the
> composite driver name. This is done in gadgets_make() so we'd like to
> add the cleanup code to gadgets_drop() as they are registered together
> in the same place and would be a little easier to read than adding them
> to _release() as the code below:
> 
>      static struct configfs_group_operations gadgets_ops = {
>          .make_group = &gadgets_make,
>          .drop_item = &gadgets_drop,
>      };
> 
> Anyway, we still doubt that there might be something that we have missed
> so please let me know the reason why putting cleanup codes to _release()
> would be a better solution.

Let's simply the logic

Gadget_make()
{
        gi=kazalloc();
        gi->composite.gadget_driver.driver.name = kasprintf();
        gi->compositr.gadget_drver.function = kstrdup();
}

this is only place to create gi and driver.name. 
no other place to set driver.name,  so I think gi take ownship of driver.name. 

so we only track when gi free,   just free driver.name before gi free. 

Related malloc and free should be pair together. 

Gadget_info_attr_release()
{
	Kfree(gi->composite.gadget_driver.function)
	Kfree(gi->composiste.gadget_driver.driver.name);
	Kfree(gi);
}

Gadget_driver.driver.name should be similar as gadget_driver.function

If driver.name is freed in gadget_drop(), gi can not actually be reused again.



> 
> Thank you and best regards,
> - Chanh
> 
> >
> >>>>        config_item_put(item);
> >>>>    }
> >>>

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

[Chanh Nguyen]

On 14/12/2022 22:24, Alan Stern wrote:
> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.]
> 
> 
> On Wed, Dec 14, 2022 at 11:15:48AM +0700, Chanh Nguyen wrote:
> 
>> 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;
> 
> This should goto out_free_driver_name.
> 

Thanks Alan! I'll update that.

> 
>>
>>       return &gi->group;
>> +
>> +out_free_driver_id_number:
>> +    ida_free(&driver_id_numbers, gi->driver_id_number);
>>   err:
>>       kfree(gi);
>>       return ERR_PTR(-ENOMEM);

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

[Chanh Nguyen]

On 14/12/2022 22:24, Alan Stern wrote:
> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.]
> 
> 
> On Wed, Dec 14, 2022 at 11:15:48AM +0700, Chanh Nguyen wrote:
> 
>> 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;
> 
> This should goto out_free_driver_name.
> 

Thanks Alan! I'll update that.

> 
>>
>>       return &gi->group;
>> +
>> +out_free_driver_id_number:
>> +    ida_free(&driver_id_numbers, gi->driver_id_number);
>>   err:
>>       kfree(gi);
>>       return ERR_PTR(-ENOMEM);