[PATCH] usb: gadget: don't create new string_container if already exist

[PATCH] usb: gadget: don't create new string_container if already exist

[Neil Zhang]

Don't create new usb_gadget_string_container if the current strings are
already exist in the usb_composite_dev.
Otherwise the ids_tab will overflow soon if we bind / unbind usb
functions frequently like android does.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 drivers/usb/gadget/composite.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index a8c18df..6fe3c6b 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1183,6 +1183,12 @@ struct usb_string *usb_gstrings_attach(struct usb_composite_dev *cdev,
 	if (!n_gstrings)
 		return ERR_PTR(-EINVAL);
 
+	list_for_each_entry(uc, &cdev->gstrings, list) {
+		n_gs = get_containers_gs(uc);
+		if (!strcmp(n_gs[0]->strings[0].s, sp[0]->strings[0].s))
+			return n_gs[0]->strings;
+	}
+
 	uc = copy_gadget_strings(sp, n_gstrings, n_strings);
 	if (IS_ERR(uc))
 		return ERR_CAST(uc);
-- 
1.7.9.5

Re: [PATCH] usb: gadget: don't create new string_container if already exist

[Andrzej Pietrasiewicz]

Hi,

W dniu 28.10.2014 o 12:33, Neil Zhang pisze:
> Don't create new usb_gadget_string_container if the current strings are
> already exist in the usb_composite_dev.
> Otherwise the ids_tab will overflow soon if we bind / unbind usb
> functions frequently like android does.

The problem you are describing does not exist in mainline kernel,
where functions are always unbound as part of the whole gadget's
unbind - regardless of whether it is a legacy gadget or configfs-composed
gadget. When the whole gadget is unbound, composite_dev_cleanup()
is called which zeroes cdev->next_string_id and frees all gadget
strings containers.

>
> Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> ---
>   drivers/usb/gadget/composite.c |    6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index a8c18df..6fe3c6b 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1183,6 +1183,12 @@ struct usb_string *usb_gstrings_attach(struct usb_composite_dev *cdev,
>   	if (!n_gstrings)
>   		return ERR_PTR(-EINVAL);
>
> +	list_for_each_entry(uc, &cdev->gstrings, list) {
> +		n_gs = get_containers_gs(uc);
> +		if (!strcmp(n_gs[0]->strings[0].s, sp[0]->strings[0].s))

To me it looks like it is a big assumption that if the first string matches,
the rest are the same, too. Isn't it?

Anyway, this solution looks more like pushing the moment when cdev->next_string_id
becomes 254 to a later time rather than preventing such a situation.

If usb_gstrings_attach() happens at function bind time, perhaps there
should be some usb_gstrings_detach() called at function unbind?

AP

RE: [PATCH] usb: gadget: don't create new string_container if already exist

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2499 bytes --]


> -----Original Message-----
> From: Andrzej Pietrasiewicz [mailto:andrzej.p@samsung.com]
> Sent: 2014Äê10ÔÂ28ÈÕ 21:10
> To: Neil Zhang; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: balbi@ti.com; gregkh@linuxfoundation.org
> Subject: Re: [PATCH] usb: gadget: don't create new string_container if already
> exist
> 
> Hi,
> 
> W dniu 28.10.2014 o 12:33, Neil Zhang pisze:
> > Don't create new usb_gadget_string_container if the current strings
> > are already exist in the usb_composite_dev.
> > Otherwise the ids_tab will overflow soon if we bind / unbind usb
> > functions frequently like android does.
> 
> The problem you are describing does not exist in mainline kernel, where
> functions are always unbound as part of the whole gadget's unbind - regardless
> of whether it is a legacy gadget or configfs-composed gadget. When the whole
> gadget is unbound, composite_dev_cleanup() is called which zeroes cdev-
> >next_string_id and frees all gadget strings containers.
> 

Yes, you are right that the current mainline won't suffer this issue.
But it will be needed if we want to implement similar features like android do.
It will only remove config rather than whole gadget driver.


> >
> > Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> > ---
> >   drivers/usb/gadget/composite.c |    6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/composite.c
> > b/drivers/usb/gadget/composite.c index a8c18df..6fe3c6b 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -1183,6 +1183,12 @@ struct usb_string *usb_gstrings_attach(struct
> usb_composite_dev *cdev,
> >   	if (!n_gstrings)
> >   		return ERR_PTR(-EINVAL);
> >
> > +	list_for_each_entry(uc, &cdev->gstrings, list) {
> > +		n_gs = get_containers_gs(uc);
> > +		if (!strcmp(n_gs[0]->strings[0].s, sp[0]->strings[0].s))
> 
> To me it looks like it is a big assumption that if the first string matches,
> the rest are the same, too. Isn't it?
> 
> Anyway, this solution looks more like pushing the moment when cdev-
> >next_string_id becomes 254 to a later time rather than preventing such a
> situation.
> 
> If usb_gstrings_attach() happens at function bind time, perhaps there should
> be some usb_gstrings_detach() called at function unbind?
> 
> AP


Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] usb: gadget: don't create new string_container if already exist

[Andrzej Pietrasiewicz]

W dniu 04.11.2014 o 12:05, Neil Zhang pisze:
>
>> -----Original Message-----
>> From: Andrzej Pietrasiewicz [mailto:andrzej.p@samsung.com]
>> Sent: 2014年10月28日 21:10
>> To: Neil Zhang; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: balbi@ti.com; gregkh@linuxfoundation.org
>> Subject: Re: [PATCH] usb: gadget: don't create new string_container if already
>> exist
>>

<snip>

>>
>> The problem you are describing does not exist in mainline kernel, where
>> functions are always unbound as part of the whole gadget's unbind - regardless
>> of whether it is a legacy gadget or configfs-composed gadget. When the whole
>> gadget is unbound, composite_dev_cleanup() is called which zeroes cdev-
>>> next_string_id and frees all gadget strings containers.
>>
>
> Yes, you are right that the current mainline won't suffer this issue.
> But it will be needed if we want to implement similar features like android do.

What features do you think of?
Mainlining the android gadget has been attempted a number of times
and is unlikely to succeed.

A configurable gadget can be composed with configfs interface,
so no need to add anything new.

That said, I think that the proper way of eliminating the problem
described is freeing the resources on function unbind rather than
preventing composite from allocating more (duplicate) resources
on bind, which your patch does.

AP

Re: [PATCH] usb: gadget: don't create new string_container if already exist

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

On Tue, Nov 04, 2014 at 03:05:57AM -0800, Neil Zhang wrote:
> 
> > -----Original Message-----
> > From: Andrzej Pietrasiewicz [mailto:andrzej.p@samsung.com]
> > Sent: 2014年10月28日 21:10
> > To: Neil Zhang; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: balbi@ti.com; gregkh@linuxfoundation.org
> > Subject: Re: [PATCH] usb: gadget: don't create new string_container if already
> > exist
> > 
> > Hi,
> > 
> > W dniu 28.10.2014 o 12:33, Neil Zhang pisze:
> > > Don't create new usb_gadget_string_container if the current strings
> > > are already exist in the usb_composite_dev.
> > > Otherwise the ids_tab will overflow soon if we bind / unbind usb
> > > functions frequently like android does.
> > 
> > The problem you are describing does not exist in mainline kernel, where
> > functions are always unbound as part of the whole gadget's unbind - regardless
> > of whether it is a legacy gadget or configfs-composed gadget. When the whole
> > gadget is unbound, composite_dev_cleanup() is called which zeroes cdev-
> > >next_string_id and frees all gadget strings containers.
> > 
> 
> Yes, you are right that the current mainline won't suffer this issue.
> But it will be needed if we want to implement similar features like android do.

once you have the features, then implement it. I don't want any unused
code here, specially since it won't be tested by anything.

> It will only remove config rather than whole gadget driver.

also, this is just wrong. You can't remove just one configuration, you
need to remove the whole thing and the host needs to see a
disconnection.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]