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 <[email protected]>
---
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
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/[email protected]>
> ---
> 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);
> }
>
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/[email protected]>
>> ---
>> 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);
>> }
>
>
> 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/[email protected]>
> >> ---
> >> 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);
> >> }
> >
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);
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/[email protected]>
>>>> ---
>>>> 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);
>>>> }
>>>
]
> >
> > 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);
> >>>> }
> >>>
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);