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 Superseded
Headers show
Series [1/1] usb: gadget: call usb_gadget_check_config() to verify UDC capability | expand

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;
Ravi Gunasekaran June 29, 2023, 5:11 a.m. UTC | #2 
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 | #3 
> -----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
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;