RE: [EXT] Re: [PATCH v2 1/1] usb: gadget: Assign an unique name for each configfs driver

[Frank Li <frank.li@nxp.com>]

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