RE: [EXT] Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver
From: Frank Li
Date: Tue Dec 13 2022 - 23:16:54 EST
> -----Original Message-----
> From: Chanh Nguyen <chanh@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, December 13, 2022 9:38 PM
> To: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>; Frank Li
> <frank.li@xxxxxxx>
> Cc: balbi@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linhaoguo86@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> usb@xxxxxxxxxxxxxxx; stern@xxxxxxxxxxxxxxxxxxx; Chanh Nguyen
> <chanh@xxxxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> >>
> >> 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@xxxxxxxxx>
> >> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> >> ---
> >>
> >> 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&data=05%7C01%7CFrank.Li%40nxp.com
> %7C1c0810ddd7e94971373208dadd849636%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C638065858780321185%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&sdata=dDLDGL9ie0hzjPa44qHt2oAnIurcZo01Mcz
> xBAjUoQk%3D&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&data=05%7C01%7CFrank.Li%4
> 0nxp.com%7C1c0810ddd7e94971373208dadd849636%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C638065858780321185%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=HuD9mWg4D%2B%2BOPjUA6b
> 0DoktyubY52TwtWdEpMuMfuk0%3D&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)
> >