linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver
@ 2022-12-13 22:03 Frank Li
  2022-12-13 22:20 ` Christophe JAILLET
  2022-12-14  0:49 ` Alan Stern
  0 siblings, 2 replies; 6+ messages in thread
From: Frank Li @ 2022-12-13 22:03 UTC (permalink / raw)
  To: frank.li; +Cc: balbi, gregkh, imx, linhaoguo86, linux-kernel, linux-usb, stern

From: Rondreis <linhaoguo86@gmail.com>

When use configfs to attach more than one gadget.

Error: Driver 'configfs-gadget' is already registered, aborting...

	UDC core: g1: driver registration failed: -16

The problem is that when creating multiple gadgets with configfs and
binding them to different UDCs, the UDC drivers have the same name
"configfs-gadget". Because of the addition of the "gadget" bus, naming
conflicts will occur when more than one UDC drivers registered to the
bus.

It's not an isolated case, this patch refers to the commit f2d8c2606825
("usb: gadget: Fix non-unique driver names in raw-gadget driver").
Each configfs-gadget driver will be assigned a unique name
"configfs-gadget.N", with a different value of N for each driver
instance.

Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")

Signed-off-by: Rondreis <linhaoguo86@gmail.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

This patch is based on https://lore.kernel.org/lkml/20220907112210.11949-1-linhaoguo86@gmail.com/
fixed the all greg's comments.

I met the same issue.  Look likes Rodrieis have not continue to work this
patch since Sep, 2022.

I don't know full name of Rondreis.

 drivers/usb/gadget/configfs.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 3a6b4926193e..785be6aea720 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -4,12 +4,17 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/nls.h>
+#include <linux/idr.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/gadget_configfs.h>
 #include "configfs.h"
 #include "u_f.h"
 #include "u_os_desc.h"
 
+#define DRIVER_NAME "configfs-gadget"
+
+static DEFINE_IDA(driver_id_numbers);
+
 int check_user_usb_string(const char *name,
 		struct usb_gadget_strings *stringtab_dev)
 {
@@ -46,6 +51,7 @@ struct gadget_info {
 
 	struct usb_composite_driver composite;
 	struct usb_composite_dev cdev;
+	int driver_id_number;
 	bool use_os_desc;
 	char b_vendor_code;
 	char qw_sign[OS_STRING_QW_SIGN_LEN];
@@ -392,6 +398,8 @@ static void gadget_info_attr_release(struct config_item *item)
 	WARN_ON(!list_empty(&gi->string_list));
 	WARN_ON(!list_empty(&gi->available_func));
 	kfree(gi->composite.gadget_driver.function);
+	kfree(gi->composite.gadget_driver.driver.name);
+	ida_free(&driver_id_numbers, gi->driver_id_number);
 	kfree(gi);
 }
 
@@ -1571,7 +1579,6 @@ static const struct usb_gadget_driver configfs_driver_template = {
 	.max_speed	= USB_SPEED_SUPER_PLUS,
 	.driver = {
 		.owner          = THIS_MODULE,
-		.name		= "configfs-gadget",
 	},
 	.match_existing_only = 1,
 };
@@ -1581,6 +1588,7 @@ static struct config_group *gadgets_make(
 		const char *name)
 {
 	struct gadget_info *gi;
+	int ret = 0;
 
 	gi = kzalloc(sizeof(*gi), GFP_KERNEL);
 	if (!gi)
@@ -1622,16 +1630,30 @@ 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;
+
+	gi->composite.gadget_driver.driver.name =
+		kasprintf(GFP_KERNEL, DRIVER_NAME ".%d", gi->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;
+	if (!gi->composite.gadget_driver.function) {
+		ret = -ENOMEM;
+		goto err_func;
+	}
 
 	return &gi->group;
+
+err_func:
+	kfree(gi->composite.gadget_driver.driver.name);
+	ida_free(&driver_id_numbers, gi->driver_id_number);
 err:
 	kfree(gi);
-	return ERR_PTR(-ENOMEM);
+	return ERR_PTR(ret);
 }
 
 static void gadgets_drop(struct config_group *group, struct config_item *item)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver
  2022-12-13 22:03 [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver Frank Li
@ 2022-12-13 22:20 ` Christophe JAILLET
  2022-12-13 23:46   ` [EXT] " Frank Li
  2022-12-14  3:37   ` Chanh Nguyen
  2022-12-14  0:49 ` Alan Stern
  1 sibling, 2 replies; 6+ messages in thread
From: Christophe JAILLET @ 2022-12-13 22:20 UTC (permalink / raw)
  To: Frank Li
  Cc: balbi, gregkh, imx, linhaoguo86, linux-kernel, linux-usb, stern,
	Chanh Nguyen

Le 13/12/2022 à 23:03, Frank Li a écrit :
> From: Rondreis <linhaoguo86@gmail.com>
> 
> When use configfs to attach more than one gadget.
> 
> Error: Driver 'configfs-gadget' is already registered, aborting...
> 
> 	UDC core: g1: driver registration failed: -16
> 
> The problem is that when creating multiple gadgets with configfs and
> binding them to different UDCs, the UDC drivers have the same name
> "configfs-gadget". Because of the addition of the "gadget" bus, naming
> conflicts will occur when more than one UDC drivers registered to the
> bus.
> 
> It's not an isolated case, this patch refers to the commit f2d8c2606825
> ("usb: gadget: Fix non-unique driver names in raw-gadget driver").
> Each configfs-gadget driver will be assigned a unique name
> "configfs-gadget.N", with a different value of N for each driver
> instance.
> 
> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> 
> Signed-off-by: Rondreis <linhaoguo86@gmail.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 
> This patch is based on https://lore.kernel.org/lkml/20220907112210.11949-1-linhaoguo86@gmail.com/
> fixed the all greg's comments.
> 
> I met the same issue.  Look likes Rodrieis have not continue to work this
> patch since Sep, 2022.
> 
> I don't know full name of Rondreis.

Hi,

Also, out of curiosity, any link with this patch:
 
https://lore.kernel.org/all/20221213041203.21080-1-chanh@os.amperecomputing.com/
?

Not exactly the same, but not very different.

(adding Chanh Nguyen in cc)

CJ

> 
>   drivers/usb/gadget/configfs.c | 30 ++++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 3a6b4926193e..785be6aea720 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -4,12 +4,17 @@
>   #include <linux/slab.h>
>   #include <linux/device.h>
>   #include <linux/nls.h>
> +#include <linux/idr.h>
>   #include <linux/usb/composite.h>
>   #include <linux/usb/gadget_configfs.h>
>   #include "configfs.h"
>   #include "u_f.h"
>   #include "u_os_desc.h"
>   
> +#define DRIVER_NAME "configfs-gadget"
> +
> +static DEFINE_IDA(driver_id_numbers);
> +
>   int check_user_usb_string(const char *name,
>   		struct usb_gadget_strings *stringtab_dev)
>   {
> @@ -46,6 +51,7 @@ struct gadget_info {
>   
>   	struct usb_composite_driver composite;
>   	struct usb_composite_dev cdev;
> +	int driver_id_number;
>   	bool use_os_desc;
>   	char b_vendor_code;
>   	char qw_sign[OS_STRING_QW_SIGN_LEN];
> @@ -392,6 +398,8 @@ static void gadget_info_attr_release(struct config_item *item)
>   	WARN_ON(!list_empty(&gi->string_list));
>   	WARN_ON(!list_empty(&gi->available_func));
>   	kfree(gi->composite.gadget_driver.function);
> +	kfree(gi->composite.gadget_driver.driver.name);
> +	ida_free(&driver_id_numbers, gi->driver_id_number);
>   	kfree(gi);
>   }
>   
> @@ -1571,7 +1579,6 @@ static const struct usb_gadget_driver configfs_driver_template = {
>   	.max_speed	= USB_SPEED_SUPER_PLUS,
>   	.driver = {
>   		.owner          = THIS_MODULE,
> -		.name		= "configfs-gadget",
>   	},
>   	.match_existing_only = 1,
>   };
> @@ -1581,6 +1588,7 @@ static struct config_group *gadgets_make(
>   		const char *name)
>   {
>   	struct gadget_info *gi;
> +	int ret = 0;
>   
>   	gi = kzalloc(sizeof(*gi), GFP_KERNEL);
>   	if (!gi)
> @@ -1622,16 +1630,30 @@ 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;
> +
> +	gi->composite.gadget_driver.driver.name =
> +		kasprintf(GFP_KERNEL, DRIVER_NAME ".%d", gi->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;
> +	if (!gi->composite.gadget_driver.function) {
> +		ret = -ENOMEM;
> +		goto err_func;
> +	}
>   
>   	return &gi->group;
> +
> +err_func:
> +	kfree(gi->composite.gadget_driver.driver.name);
> +	ida_free(&driver_id_numbers, gi->driver_id_number);
>   err:
>   	kfree(gi);
> -	return ERR_PTR(-ENOMEM);
> +	return ERR_PTR(ret);
>   }
>   
>   static void gadgets_drop(struct config_group *group, struct config_item *item)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [EXT] Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver
  2022-12-13 22:20 ` Christophe JAILLET
@ 2022-12-13 23:46   ` Frank Li
  2022-12-14  3:37   ` Chanh Nguyen
  1 sibling, 0 replies; 6+ messages in thread
From: Frank Li @ 2022-12-13 23:46 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: balbi, gregkh, imx, linhaoguo86, linux-kernel, linux-usb, stern,
	Chanh Nguyen

> 
> Hi,
> 
> Also, out of curiosity, any link with this patch:
> 	
> https://lore.kernel.org/all/20221213041203.21080-1-chanh@os.amperecomputing.com/
> 
> Not exactly the same, but not very different.
> 
> (adding Chanh Nguyen in cc)

Yes, it is almost the same. 

I think it is not good place to free driver.name at gadget_drop function(), it should be close to kfree(gi).

> 
> CJ
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver
  2022-12-13 22:03 [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver Frank Li
  2022-12-13 22:20 ` Christophe JAILLET
@ 2022-12-14  0:49 ` Alan Stern
  1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2022-12-14  0:49 UTC (permalink / raw)
  To: Frank Li; +Cc: balbi, gregkh, imx, linhaoguo86, linux-kernel, linux-usb

On Tue, Dec 13, 2022 at 05:03:54PM -0500, Frank Li wrote:
> From: Rondreis <linhaoguo86@gmail.com>
> 
> When use configfs to attach more than one gadget.
> 
> Error: Driver 'configfs-gadget' is already registered, aborting...
> 
> 	UDC core: g1: driver registration failed: -16
> 
> The problem is that when creating multiple gadgets with configfs and
> binding them to different UDCs, the UDC drivers have the same name
> "configfs-gadget". Because of the addition of the "gadget" bus, naming
> conflicts will occur when more than one UDC drivers registered to the
> bus.
> 
> It's not an isolated case, this patch refers to the commit f2d8c2606825
> ("usb: gadget: Fix non-unique driver names in raw-gadget driver").
> Each configfs-gadget driver will be assigned a unique name
> "configfs-gadget.N", with a different value of N for each driver
> instance.
> 
> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> 
> Signed-off-by: Rondreis <linhaoguo86@gmail.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 
> This patch is based on https://lore.kernel.org/lkml/20220907112210.11949-1-linhaoguo86@gmail.com/
> fixed the all greg's comments.
> 
> I met the same issue.  Look likes Rodrieis have not continue to work this
> patch since Sep, 2022.
> 
> I don't know full name of Rondreis.
> 
>  drivers/usb/gadget/configfs.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 3a6b4926193e..785be6aea720 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -4,12 +4,17 @@
>  #include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/nls.h>
> +#include <linux/idr.h>
>  #include <linux/usb/composite.h>
>  #include <linux/usb/gadget_configfs.h>
>  #include "configfs.h"
>  #include "u_f.h"
>  #include "u_os_desc.h"
>  
> +#define DRIVER_NAME "configfs-gadget"
> +
> +static DEFINE_IDA(driver_id_numbers);
> +
>  int check_user_usb_string(const char *name,
>  		struct usb_gadget_strings *stringtab_dev)
>  {
> @@ -46,6 +51,7 @@ struct gadget_info {
>  
>  	struct usb_composite_driver composite;
>  	struct usb_composite_dev cdev;
> +	int driver_id_number;
>  	bool use_os_desc;
>  	char b_vendor_code;
>  	char qw_sign[OS_STRING_QW_SIGN_LEN];
> @@ -392,6 +398,8 @@ static void gadget_info_attr_release(struct config_item *item)
>  	WARN_ON(!list_empty(&gi->string_list));
>  	WARN_ON(!list_empty(&gi->available_func));
>  	kfree(gi->composite.gadget_driver.function);
> +	kfree(gi->composite.gadget_driver.driver.name);
> +	ida_free(&driver_id_numbers, gi->driver_id_number);
>  	kfree(gi);
>  }
>  
> @@ -1571,7 +1579,6 @@ static const struct usb_gadget_driver configfs_driver_template = {
>  	.max_speed	= USB_SPEED_SUPER_PLUS,
>  	.driver = {
>  		.owner          = THIS_MODULE,
> -		.name		= "configfs-gadget",
>  	},
>  	.match_existing_only = 1,
>  };
> @@ -1581,6 +1588,7 @@ static struct config_group *gadgets_make(
>  		const char *name)
>  {
>  	struct gadget_info *gi;
> +	int ret = 0;
>  
>  	gi = kzalloc(sizeof(*gi), GFP_KERNEL);
>  	if (!gi)
> @@ -1622,16 +1630,30 @@ 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;
> +
> +	gi->composite.gadget_driver.driver.name =
> +		kasprintf(GFP_KERNEL, DRIVER_NAME ".%d", gi->driver_id_number);

Here you forgot to put:

	if (!gi->composite.gadget_driver.driver.name) {
		ret = -ENOMEM;
		goto err_func;
	}

Or if you prefer, combine this test with the one below.

Alan Stern

> +
>  	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;
> +	if (!gi->composite.gadget_driver.function) {
> +		ret = -ENOMEM;
> +		goto err_func;
> +	}
>  
>  	return &gi->group;
> +
> +err_func:
> +	kfree(gi->composite.gadget_driver.driver.name);
> +	ida_free(&driver_id_numbers, gi->driver_id_number);
>  err:
>  	kfree(gi);
> -	return ERR_PTR(-ENOMEM);
> +	return ERR_PTR(ret);
>  }
>  
>  static void gadgets_drop(struct config_group *group, struct config_item *item)
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver
  2022-12-13 22:20 ` Christophe JAILLET
  2022-12-13 23:46   ` [EXT] " Frank Li
@ 2022-12-14  3:37   ` Chanh Nguyen
  2022-12-14  4:16     ` [EXT] " Frank Li
  1 sibling, 1 reply; 6+ messages in thread
From: Chanh Nguyen @ 2022-12-14  3:37 UTC (permalink / raw)
  To: Christophe JAILLET, Frank Li
  Cc: balbi, gregkh, imx, linhaoguo86, linux-kernel, linux-usb, stern,
	Chanh Nguyen


On 14/12/2022 05:20, Christophe JAILLET wrote:
> Le 13/12/2022 à 23:03, Frank Li a écrit :
>> From: Rondreis <linhaoguo86@gmail.com>
>>
>> When use configfs to attach more than one gadget.
>>
>> Error: Driver 'configfs-gadget' is already registered, aborting...
>>
>>     UDC core: g1: driver registration failed: -16
>>
>> The problem is that when creating multiple gadgets with configfs and
>> binding them to different UDCs, the UDC drivers have the same name
>> "configfs-gadget". Because of the addition of the "gadget" bus, naming
>> conflicts will occur when more than one UDC drivers registered to the
>> bus.
>>
>> It's not an isolated case, this patch refers to the commit f2d8c2606825
>> ("usb: gadget: Fix non-unique driver names in raw-gadget driver").
>> Each configfs-gadget driver will be assigned a unique name
>> "configfs-gadget.N", with a different value of N for each driver
>> instance.
>>
>> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
>>
>> Signed-off-by: Rondreis <linhaoguo86@gmail.com>
>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>> ---
>>
>> This patch is based on 
>> https://lore.kernel.org/lkml/20220907112210.11949-1-linhaoguo86@gmail.com/
>> fixed the all greg's comments.
>>
>> I met the same issue.  Look likes Rodrieis have not continue to work this
>> patch since Sep, 2022.
>>
>> I don't know full name of Rondreis.
> 
> Hi,
> 
> Also, out of curiosity, any link with this patch:
> 
> https://lore.kernel.org/all/20221213041203.21080-1-chanh@os.amperecomputing.com/
> ?
> 
> Not exactly the same, but not very different.
> 
> (adding Chanh Nguyen in cc)
> 

What a coincident :-)

I did not aware there are some similar attempts to fix the same issue 
and see both patches posted same time.

We start to see the issue when OpenBMC started to adopt kernel 6.0 and 
try to fix the issue since then (beginning of Oct 2022)

We then could be able to identify the issue and try to fix it follow the 
commit f2d8c2606825317b77db1f9ba0fc26ef26160b30

Going forward, as we have both Frank and Rondreis interested in the 
patch, we are really happy if you both could review and share the 
comment. I'd really appreciate if you could help with that part.

FYI, we have reviewed and made some changes based on CJ's comment in my 
last patch (v1) yesterday. We are trying to test it as much as possible. 
If it looks good we will re-post it as v2 shortly for further comment.

Thanks a lot for interesting in the patch.
- Chanh

> 
>>
>>   drivers/usb/gadget/configfs.c | 30 ++++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c 
>> b/drivers/usb/gadget/configfs.c
>> index 3a6b4926193e..785be6aea720 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -4,12 +4,17 @@
>>   #include <linux/slab.h>
>>   #include <linux/device.h>
>>   #include <linux/nls.h>
>> +#include <linux/idr.h>
>>   #include <linux/usb/composite.h>
>>   #include <linux/usb/gadget_configfs.h>
>>   #include "configfs.h"
>>   #include "u_f.h"
>>   #include "u_os_desc.h"
>> +#define DRIVER_NAME "configfs-gadget"
>> +
>> +static DEFINE_IDA(driver_id_numbers);
>> +
>>   int check_user_usb_string(const char *name,
>>           struct usb_gadget_strings *stringtab_dev)
>>   {
>> @@ -46,6 +51,7 @@ struct gadget_info {
>>       struct usb_composite_driver composite;
>>       struct usb_composite_dev cdev;
>> +    int driver_id_number;
>>       bool use_os_desc;
>>       char b_vendor_code;
>>       char qw_sign[OS_STRING_QW_SIGN_LEN];
>> @@ -392,6 +398,8 @@ static void gadget_info_attr_release(struct 
>> config_item *item)
>>       WARN_ON(!list_empty(&gi->string_list));
>>       WARN_ON(!list_empty(&gi->available_func));
>>       kfree(gi->composite.gadget_driver.function);
>> +    kfree(gi->composite.gadget_driver.driver.name);
>> +    ida_free(&driver_id_numbers, gi->driver_id_number);
>>       kfree(gi);
>>   }
>> @@ -1571,7 +1579,6 @@ static const struct usb_gadget_driver 
>> configfs_driver_template = {
>>       .max_speed    = USB_SPEED_SUPER_PLUS,
>>       .driver = {
>>           .owner          = THIS_MODULE,
>> -        .name        = "configfs-gadget",
>>       },
>>       .match_existing_only = 1,
>>   };
>> @@ -1581,6 +1588,7 @@ static struct config_group *gadgets_make(
>>           const char *name)
>>   {
>>       struct gadget_info *gi;
>> +    int ret = 0;
>>       gi = kzalloc(sizeof(*gi), GFP_KERNEL);
>>       if (!gi)
>> @@ -1622,16 +1630,30 @@ 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;
>> +
>> +    gi->composite.gadget_driver.driver.name =
>> +        kasprintf(GFP_KERNEL, DRIVER_NAME ".%d", gi->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;
>> +    if (!gi->composite.gadget_driver.function) {
>> +        ret = -ENOMEM;
>> +        goto err_func;
>> +    }
>>       return &gi->group;
>> +
>> +err_func:
>> +    kfree(gi->composite.gadget_driver.driver.name);
>> +    ida_free(&driver_id_numbers, gi->driver_id_number);
>>   err:
>>       kfree(gi);
>> -    return ERR_PTR(-ENOMEM);
>> +    return ERR_PTR(ret);
>>   }
>>   static void gadgets_drop(struct config_group *group, struct 
>> config_item *item)
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [EXT] Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver
  2022-12-14  3:37   ` Chanh Nguyen
@ 2022-12-14  4:16     ` Frank Li
  0 siblings, 0 replies; 6+ messages in thread
From: Frank Li @ 2022-12-14  4:16 UTC (permalink / raw)
  To: Chanh Nguyen, Christophe JAILLET
  Cc: balbi, gregkh, imx, linhaoguo86, linux-kernel, linux-usb, stern,
	Chanh Nguyen



> -----Original Message-----
> From: Chanh Nguyen <chanh@amperemail.onmicrosoft.com>
> Sent: Tuesday, December 13, 2022 9:38 PM
> To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Frank Li
> <frank.li@nxp.com>
> Cc: balbi@kernel.org; gregkh@linuxfoundation.org; imx@lists.linux.dev;
> linhaoguo86@gmail.com; linux-kernel@vger.kernel.org; linux-
> usb@vger.kernel.org; stern@rowland.harvard.edu; Chanh Nguyen
> <chanh@os.amperecomputing.com>
> Subject: [EXT] Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for
> each configfs driver
> 
> Caution: EXT Email
> 
> On 14/12/2022 05:20, Christophe JAILLET wrote:
> > Le 13/12/2022 à 23:03, Frank Li a écrit :
> >> From: Rondreis <linhaoguo86@gmail.com>
> >>
> >> When use configfs to attach more than one gadget.
> >>
> >> Error: Driver 'configfs-gadget' is already registered, aborting...
> >>
> >>     UDC core: g1: driver registration failed: -16
> >>
> >> The problem is that when creating multiple gadgets with configfs and
> >> binding them to different UDCs, the UDC drivers have the same name
> >> "configfs-gadget". Because of the addition of the "gadget" bus, naming
> >> conflicts will occur when more than one UDC drivers registered to the
> >> bus.
> >>
> >> It's not an isolated case, this patch refers to the commit f2d8c2606825
> >> ("usb: gadget: Fix non-unique driver names in raw-gadget driver").
> >> Each configfs-gadget driver will be assigned a unique name
> >> "configfs-gadget.N", with a different value of N for each driver
> >> instance.
> >>
> >> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> >>
> >> Signed-off-by: Rondreis <linhaoguo86@gmail.com>
> >> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> >> ---
> >>
> >> This patch is based on
> >>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Flkml%2F20220907112210.11949-1-
> linhaoguo86%40gmail.com%2F&amp;data=05%7C01%7CFrank.Li%40nxp.com
> %7C1c0810ddd7e94971373208dadd849636%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C638065858780321185%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&amp;sdata=dDLDGL9ie0hzjPa44qHt2oAnIurcZo01Mcz
> xBAjUoQk%3D&amp;reserved=0
> >> fixed the all greg's comments.
> >>
> >> I met the same issue.  Look likes Rodrieis have not continue to work this
> >> patch since Sep, 2022.
> >>
> >> I don't know full name of Rondreis.
> >
> > Hi,
> >
> > Also, out of curiosity, any link with this patch:
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Fall%2F20221213041203.21080-1-
> chanh%40os.amperecomputing.com%2F&amp;data=05%7C01%7CFrank.Li%4
> 0nxp.com%7C1c0810ddd7e94971373208dadd849636%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C638065858780321185%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=HuD9mWg4D%2B%2BOPjUA6b
> 0DoktyubY52TwtWdEpMuMfuk0%3D&amp;reserved=0
> > ?
> >
> > Not exactly the same, but not very different.
> >
> > (adding Chanh Nguyen in cc)
> >
> 
> What a coincident :-)
> 
> I did not aware there are some similar attempts to fix the same issue
> and see both patches posted same time.
> 
> We start to see the issue when OpenBMC started to adopt kernel 6.0 and
> try to fix the issue since then (beginning of Oct 2022)
> 
> We then could be able to identify the issue and try to fix it follow the
> commit f2d8c2606825317b77db1f9ba0fc26ef26160b30
> 
> Going forward, as we have both Frank and Rondreis interested in the
> patch, we are really happy if you both could review and share the
> comment. I'd really appreciate if you could help with that part.
> 
> FYI, we have reviewed and made some changes based on CJ's comment in
> my
> last patch (v1) yesterday. We are trying to test it as much as possible.
> If it looks good we will re-post it as v2 shortly for further comment.

I almost just reused rondreis patch. 
I can review your v2 version.  

I prefer move kfree(gi->composite.gadget_driver.driver.name) into
 gadget_info_attr_release,  just before free(gi).

Best regards
Frank Li

> 
> Thanks a lot for interesting in the patch.
> - Chanh
> 
> >
> >>
> >>   drivers/usb/gadget/configfs.c | 30 ++++++++++++++++++++++++++----
> >>   1 file changed, 26 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/configfs.c
> >> b/drivers/usb/gadget/configfs.c
> >> index 3a6b4926193e..785be6aea720 100644
> >> --- a/drivers/usb/gadget/configfs.c
> >> +++ b/drivers/usb/gadget/configfs.c
> >> @@ -4,12 +4,17 @@
> >>   #include <linux/slab.h>
> >>   #include <linux/device.h>
> >>   #include <linux/nls.h>
> >> +#include <linux/idr.h>
> >>   #include <linux/usb/composite.h>
> >>   #include <linux/usb/gadget_configfs.h>
> >>   #include "configfs.h"
> >>   #include "u_f.h"
> >>   #include "u_os_desc.h"
> >> +#define DRIVER_NAME "configfs-gadget"
> >> +
> >> +static DEFINE_IDA(driver_id_numbers);
> >> +
> >>   int check_user_usb_string(const char *name,
> >>           struct usb_gadget_strings *stringtab_dev)
> >>   {
> >> @@ -46,6 +51,7 @@ struct gadget_info {
> >>       struct usb_composite_driver composite;
> >>       struct usb_composite_dev cdev;
> >> +    int driver_id_number;
> >>       bool use_os_desc;
> >>       char b_vendor_code;
> >>       char qw_sign[OS_STRING_QW_SIGN_LEN];
> >> @@ -392,6 +398,8 @@ static void gadget_info_attr_release(struct
> >> config_item *item)
> >>       WARN_ON(!list_empty(&gi->string_list));
> >>       WARN_ON(!list_empty(&gi->available_func));
> >>       kfree(gi->composite.gadget_driver.function);
> >> +    kfree(gi->composite.gadget_driver.driver.name);
> >> +    ida_free(&driver_id_numbers, gi->driver_id_number);
> >>       kfree(gi);
> >>   }
> >> @@ -1571,7 +1579,6 @@ static const struct usb_gadget_driver
> >> configfs_driver_template = {
> >>       .max_speed    = USB_SPEED_SUPER_PLUS,
> >>       .driver = {
> >>           .owner          = THIS_MODULE,
> >> -        .name        = "configfs-gadget",
> >>       },
> >>       .match_existing_only = 1,
> >>   };
> >> @@ -1581,6 +1588,7 @@ static struct config_group *gadgets_make(
> >>           const char *name)
> >>   {
> >>       struct gadget_info *gi;
> >> +    int ret = 0;
> >>       gi = kzalloc(sizeof(*gi), GFP_KERNEL);
> >>       if (!gi)
> >> @@ -1622,16 +1630,30 @@ 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;
> >> +
> >> +    gi->composite.gadget_driver.driver.name =
> >> +        kasprintf(GFP_KERNEL, DRIVER_NAME ".%d", gi-
> >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;
> >> +    if (!gi->composite.gadget_driver.function) {
> >> +        ret = -ENOMEM;
> >> +        goto err_func;
> >> +    }
> >>       return &gi->group;
> >> +
> >> +err_func:
> >> +    kfree(gi->composite.gadget_driver.driver.name);
> >> +    ida_free(&driver_id_numbers, gi->driver_id_number);
> >>   err:
> >>       kfree(gi);
> >> -    return ERR_PTR(-ENOMEM);
> >> +    return ERR_PTR(ret);
> >>   }
> >>   static void gadgets_drop(struct config_group *group, struct
> >> config_item *item)
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-12-14  4:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 22:03 [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver Frank Li
2022-12-13 22:20 ` Christophe JAILLET
2022-12-13 23:46   ` [EXT] " Frank Li
2022-12-14  3:37   ` Chanh Nguyen
2022-12-14  4:16     ` [EXT] " Frank Li
2022-12-14  0:49 ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).