diff mbox series

[1/1] usb: gadget: call usb_gadget_check_config() to verify UDC capability

Message ID 20230628222437.3188441-1-Frank.Li@nxp.com
State New
Headers
Series [1/1] usb: gadget: call usb_gadget_check_config() to verify UDC capability |

Commit Message

Frank Li June 28, 2023, 10:24 p.m. UTC 
  The legacy gadget driver omitted calling usb_gadget_check_config()
to ensure that the USB device controller (UDC) has adequate resources,
including sufficient endpoint numbers and types, to support the given
configuration.

Previously, usb_add_config() was solely invoked by the legacy gadget
driver. Adds the necessary usb_gadget_check_config() after the bind()
operation to fix the issue.

Fixes: dce49449e04f ("usb: cdns3: allocate TX FIFO size according to composite EP number")
Reported-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/usb/gadget/composite.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Roger Quadros June 29, 2023, 3:23 a.m. UTC | #1 
On 29/06/2023 03:54, Frank Li wrote:
> The legacy gadget driver omitted calling usb_gadget_check_config()
> to ensure that the USB device controller (UDC) has adequate resources,
> including sufficient endpoint numbers and types, to support the given
> configuration.
> 
> Previously, usb_add_config() was solely invoked by the legacy gadget
> driver. Adds the necessary usb_gadget_check_config() after the bind()
> operation to fix the issue.

You have only fixed composite.c. Not all gadget drivers use composite.c
so it will be still broken for them.

Please also add default sane configuration in CDNS3 so it works even
if usb_gadget_check_config() is not invoked.

> 
> Fixes: dce49449e04f ("usb: cdns3: allocate TX FIFO size according to composite EP number")
> Reported-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/usb/gadget/composite.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 1b3489149e5e..dd9b90481b4c 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1125,6 +1125,10 @@ int usb_add_config(struct usb_composite_dev *cdev,
>  		goto done;
>  
>  	status = bind(config);
> +
> +	if (status == 0)
> +		status = usb_gadget_check_config(cdev->gadget);
> +
>  	if (status < 0) {
>  		while (!list_empty(&config->functions)) {
>  			struct usb_function		*f;
Frank Li June 29, 2023, 3:40 a.m. UTC | #2 
> -----Original Message-----
> From: Roger Quadros <rogerq@kernel.org>
> Sent: Wednesday, June 28, 2023 10:23 PM
> To: Frank Li <frank.li@nxp.com>; r-gunasekaran@ti.com; imx@lists.linux.dev;
> Jun Li <jun.li@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Elson Roy Serrao
> <quic_eserrao@quicinc.com>; Thinh Nguyen
> <Thinh.Nguyen@synopsys.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Jó Ágila Bitsch <jgilab@gmail.com>;
> Prashanth K <quic_prashk@quicinc.com>; Peter Chen
> <peter.chen@kernel.org>; open list:USB SUBSYSTEM <linux-
> usb@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Subject: [EXT] Re: [PATCH 1/1] usb: gadget: call usb_gadget_check_config()
> to verify UDC capability
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 29/06/2023 03:54, Frank Li wrote:
> > The legacy gadget driver omitted calling usb_gadget_check_config()
> > to ensure that the USB device controller (UDC) has adequate resources,
> > including sufficient endpoint numbers and types, to support the given
> > configuration.
> >
> > Previously, usb_add_config() was solely invoked by the legacy gadget
> > driver. Adds the necessary usb_gadget_check_config() after the bind()
> > operation to fix the issue.
> 
> You have only fixed composite.c. Not all gadget drivers use composite.c
> so it will be still broken for them.
> 
> Please also add default sane configuration in CDNS3 so it works even
> if usb_gadget_check_config() is not invoked.

Which one was not call usb_add_config()?
DWC3 also use
 .check_config		= dwc3_gadget_check_config, 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc3/gadget.c 

I think it is not correct by assume UDC can support specific config
at gadget function driver. 
Add default value of CDNS3 actually hidden potential problem. 

I don't suggest it before fixed hidden potential problem. 

Frank  

> 
> >
> > Fixes: dce49449e04f ("usb: cdns3: allocate TX FIFO size according to
> composite EP number")
> > Reported-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/usb/gadget/composite.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/composite.c
> b/drivers/usb/gadget/composite.c
> > index 1b3489149e5e..dd9b90481b4c 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -1125,6 +1125,10 @@ int usb_add_config(struct usb_composite_dev
> *cdev,
> >               goto done;
> >
> >       status = bind(config);
> > +
> > +     if (status == 0)
> > +             status = usb_gadget_check_config(cdev->gadget);
> > +
> >       if (status < 0) {
> >               while (!list_empty(&config->functions)) {
> >                       struct usb_function             *f;
> 
> --
> cheers,
> -roger
Ravi Gunasekaran June 29, 2023, 5:11 a.m. UTC | #3 
On 6/29/23 3:54 AM, Frank Li wrote:
> The legacy gadget driver omitted calling usb_gadget_check_config()
> to ensure that the USB device controller (UDC) has adequate resources,
> including sufficient endpoint numbers and types, to support the given
> configuration.
> 
> Previously, usb_add_config() was solely invoked by the legacy gadget
> driver. Adds the necessary usb_gadget_check_config() after the bind()
> operation to fix the issue.
> 
> Fixes: dce49449e04f ("usb: cdns3: allocate TX FIFO size according to composite EP number")
> Reported-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/usb/gadget/composite.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 1b3489149e5e..dd9b90481b4c 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1125,6 +1125,10 @@ int usb_add_config(struct usb_composite_dev *cdev,
>  		goto done;
>  
>  	status = bind(config);
> +
> +	if (status == 0)
> +		status = usb_gadget_check_config(cdev->gadget);

This will work for legacy gadgets that support only one configurations.
Take for example g_ether. It has two configurations when RNDIS is enabled.
And usb_add_config() is invoked for each configuration. 

cdns3_gadget_check_config() calculates the total IN end-points based on the
ep->claimed flag. 

        list_for_each_entry(ep, &gadget->ep_list, ep_list) {                
                if (ep->claimed && (ep->address & USB_DIR_IN))              
                        n_in++;                                             
        }  

This ep->claimed flag is later cleared by usb_ep_autoconfig_reset() which is
invoked in usb_add_config(). So for multi-configurations, actual total in end points
is not taken into consideration thus resulting in incorrect fifo size allocation/check.

g_ffs is another gadget which has multiple configurations.


> +
>  	if (status < 0) {
>  		while (!list_empty(&config->functions)) {
>  			struct usb_function		*f;
Frank Li June 29, 2023, 2:48 p.m. UTC | #4 
> -----Original Message-----
> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Sent: Thursday, June 29, 2023 12:11 AM
> To: Frank Li <frank.li@nxp.com>; rogerq@kernel.org; imx@lists.linux.dev;
> Jun Li <jun.li@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Elson Roy Serrao
> <quic_eserrao@quicinc.com>; Thinh Nguyen
> <Thinh.Nguyen@synopsys.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Jó Ágila Bitsch <jgilab@gmail.com>;
> Prashanth K <quic_prashk@quicinc.com>; Peter Chen
> <peter.chen@kernel.org>; open list:USB SUBSYSTEM <linux-
> usb@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Subject: [EXT] Re: [PATCH 1/1] usb: gadget: call usb_gadget_check_config()
> to verify UDC capability
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 6/29/23 3:54 AM, Frank Li wrote:
> > The legacy gadget driver omitted calling usb_gadget_check_config()
> > to ensure that the USB device controller (UDC) has adequate resources,
> > including sufficient endpoint numbers and types, to support the given
> > configuration.
> >
> > Previously, usb_add_config() was solely invoked by the legacy gadget
> > driver. Adds the necessary usb_gadget_check_config() after the bind()
> > operation to fix the issue.
> >
> > Fixes: dce49449e04f ("usb: cdns3: allocate TX FIFO size according to
> composite EP number")
> > Reported-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/usb/gadget/composite.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/composite.c
> b/drivers/usb/gadget/composite.c
> > index 1b3489149e5e..dd9b90481b4c 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -1125,6 +1125,10 @@ int usb_add_config(struct usb_composite_dev
> *cdev,
> >               goto done;
> >
> >       status = bind(config);
> > +
> > +     if (status == 0)
> > +             status = usb_gadget_check_config(cdev->gadget);
> 
> This will work for legacy gadgets that support only one configurations.
> Take for example g_ether. It has two configurations when RNDIS is enabled.
> And usb_add_config() is invoked for each configuration.
> 
> cdns3_gadget_check_config() calculates the total IN end-points based on the
> ep->claimed flag.
> 
>         list_for_each_entry(ep, &gadget->ep_list, ep_list) {
>                 if (ep->claimed && (ep->address & USB_DIR_IN))
>                         n_in++;
>         }
> 
> This ep->claimed flag is later cleared by usb_ep_autoconfig_reset() which is
> invoked in usb_add_config(). So for multi-configurations, actual total in end
> points
> is not taken into consideration thus resulting in incorrect fifo size
> allocation/check.
> 
> g_ffs is another gadget which has multiple configurations.

Actually it is another problem that I am trying to figure out. Configfs should have
 similar Problem. We never test multi config case in configfs also.  Generally,
composite device only have multi interfaces. 

cdns3_gadget_match_ep wrong cache all endpoints for all configs,  but actually
only one config can be active.

Frank 

> 
> 
> > +
> >       if (status < 0) {
> >               while (!list_empty(&config->functions)) {
> >                       struct usb_function             *f;
> 
> --
> Regards,
> Ravi
Roger Quadros June 30, 2023, 7:43 p.m. UTC | #5 
On 29/06/2023 06:40, Frank Li wrote:
> 
> 
>> -----Original Message-----
>> From: Roger Quadros <rogerq@kernel.org>
>> Sent: Wednesday, June 28, 2023 10:23 PM
>> To: Frank Li <frank.li@nxp.com>; r-gunasekaran@ti.com; imx@lists.linux.dev;
>> Jun Li <jun.li@nxp.com>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; Elson Roy Serrao
>> <quic_eserrao@quicinc.com>; Thinh Nguyen
>> <Thinh.Nguyen@synopsys.com>; Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com>; Jó Ágila Bitsch <jgilab@gmail.com>;
>> Prashanth K <quic_prashk@quicinc.com>; Peter Chen
>> <peter.chen@kernel.org>; open list:USB SUBSYSTEM <linux-
>> usb@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
>> Subject: [EXT] Re: [PATCH 1/1] usb: gadget: call usb_gadget_check_config()
>> to verify UDC capability
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report
>> this email' button
>>
>>
>> On 29/06/2023 03:54, Frank Li wrote:
>>> The legacy gadget driver omitted calling usb_gadget_check_config()
>>> to ensure that the USB device controller (UDC) has adequate resources,
>>> including sufficient endpoint numbers and types, to support the given
>>> configuration.
>>>
>>> Previously, usb_add_config() was solely invoked by the legacy gadget
>>> driver. Adds the necessary usb_gadget_check_config() after the bind()
>>> operation to fix the issue.
>>
>> You have only fixed composite.c. Not all gadget drivers use composite.c
>> so it will be still broken for them.
>>
>> Please also add default sane configuration in CDNS3 so it works even
>> if usb_gadget_check_config() is not invoked.
> 
> Which one was not call usb_add_config()?
> DWC3 also use
>  .check_config		= dwc3_gadget_check_config, 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc3/gadget.c 

I meant gadget drivers not UDC drivers.

e.g.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/legacy/dbgp.c

> 
> I think it is not correct by assume UDC can support specific config
> at gadget function driver. 
> Add default value of CDNS3 actually hidden potential problem. 
> 
> I don't suggest it before fixed hidden potential problem. 
> 
> Frank  
> 
>>
>>>
>>> Fixes: dce49449e04f ("usb: cdns3: allocate TX FIFO size according to
>> composite EP number")
>>> Reported-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>> ---
>>>  drivers/usb/gadget/composite.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/composite.c
>> b/drivers/usb/gadget/composite.c
>>> index 1b3489149e5e..dd9b90481b4c 100644
>>> --- a/drivers/usb/gadget/composite.c
>>> +++ b/drivers/usb/gadget/composite.c
>>> @@ -1125,6 +1125,10 @@ int usb_add_config(struct usb_composite_dev
>> *cdev,
>>>               goto done;
>>>
>>>       status = bind(config);
>>> +
>>> +     if (status == 0)
>>> +             status = usb_gadget_check_config(cdev->gadget);
>>> +
>>>       if (status < 0) {
>>>               while (!list_empty(&config->functions)) {
>>>                       struct usb_function             *f;
>>
>> --
>> cheers,
>> -roger
Frank Li June 30, 2023, 7:57 p.m. UTC | #6 
On Fri, Jun 30, 2023 at 10:43:14PM +0300, Roger Quadros wrote:
> 
> 
> >  .check_config		= dwc3_gadget_check_config, 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc3/gadget.c 
> 
> I meant gadget drivers not UDC drivers.
> 
> e.g.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/legacy/dbgp.c
> 

This is for EHCI host controller debug port, specially implement simple
tty gadget devices. Generally used for PC. This driver suppose will not
use UDC at all.
diff mbox series

Patch

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 1b3489149e5e..dd9b90481b4c 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1125,6 +1125,10 @@  int usb_add_config(struct usb_composite_dev *cdev,
 		goto done;
 
 	status = bind(config);
+
+	if (status == 0)
+		status = usb_gadget_check_config(cdev->gadget);
+
 	if (status < 0) {
 		while (!list_empty(&config->functions)) {
 			struct usb_function		*f;