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

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

[Frank Li]

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>
---
change from v1 to v2
- not change for this patch, just add new patch to fix multi-config case

 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 403563c06477..cb0a4e2cdbb7 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1029,6 +1029,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;
-- 
2.34.1

[PATCH v2 2/2] usb: cdns3: fix incorrect calculation of ep_buf_size when more than one config

[Frank Li]

Previously, the cdns3_gadget_check_config() function in the cdns3 driver
mistakenly calculated the ep_buf_size by considering only one
configuration's endpoint information because "claimed" will be clear after
call usb_gadget_check_config().

The fix involves checking the private flags EP_CLAIMED instead of relying
on the "claimed" flag.

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/cdns3/cdns3-gadget.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index ccfaebca6faa..3326955f6991 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -2999,12 +2999,14 @@ static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
 static int cdns3_gadget_check_config(struct usb_gadget *gadget)
 {
 	struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
+	struct cdns3_endpoint *priv_ep;
 	struct usb_ep *ep;
 	int n_in = 0;
 	int total;
 
 	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
-		if (ep->claimed && (ep->address & USB_DIR_IN))
+		priv_ep = ep_to_cdns3_ep(ep);
+		if ((priv_ep->flags & EP_CLAIMED) && (ep->address & USB_DIR_IN))
 			n_in++;
 	}
 
-- 
2.34.1

Re: [PATCH v2 2/2] usb: cdns3: fix incorrect calculation of ep_buf_size when more than one config

[Greg Kroah-Hartman]

On Sat, Jul 08, 2023 at 05:18:38PM +0200, Markus Elfring wrote:
> …
> > The fix involves checking the private flags EP_CLAIMED instead of relying
> > on the "claimed" flag.
> …
> 
> Please choose an imperative change suggestion.

Please do not review USB kernel patches, the comments you make are not
helpful for developers and will be ignored.

greg k-h

Re: [PATCH v2 2/2] usb: cdns3: fix incorrect calculation of ep_buf_size when more than one config

[Peter Chen]

On 23-07-07 19:00:15, Frank Li wrote:
> Previously, the cdns3_gadget_check_config() function in the cdns3 driver
> mistakenly calculated the ep_buf_size by considering only one
> configuration's endpoint information because "claimed" will be clear after
> call usb_gadget_check_config().
> 
> The fix involves checking the private flags EP_CLAIMED instead of relying
> on the "claimed" flag.
> 
> 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/cdns3/cdns3-gadget.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index ccfaebca6faa..3326955f6991 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -2999,12 +2999,14 @@ static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
>  static int cdns3_gadget_check_config(struct usb_gadget *gadget)
>  {
>  	struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
> +	struct cdns3_endpoint *priv_ep;
>  	struct usb_ep *ep;
>  	int n_in = 0;
>  	int total;
>  
>  	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> -		if (ep->claimed && (ep->address & USB_DIR_IN))
> +		priv_ep = ep_to_cdns3_ep(ep);
> +		if ((priv_ep->flags & EP_CLAIMED) && (ep->address & USB_DIR_IN))
>  			n_in++;
>  	}

Acked-by: Peter Chen <peter.chen@kernel.org>

-- 

Thanks,
Peter Chen

Re: [PATCH v2 2/2] usb: cdns3: fix incorrect calculation of ep_buf_size when more than one config

[Ravi Gunasekaran]

On 7/8/23 4:30 AM, Frank Li wrote:
> Previously, the cdns3_gadget_check_config() function in the cdns3 driver
> mistakenly calculated the ep_buf_size by considering only one
> configuration's endpoint information because "claimed" will be clear after
> call usb_gadget_check_config().
> 
> The fix involves checking the private flags EP_CLAIMED instead of relying
> on the "claimed" flag.
> 
> 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/cdns3/cdns3-gadget.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index ccfaebca6faa..3326955f6991 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -2999,12 +2999,14 @@ static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
>  static int cdns3_gadget_check_config(struct usb_gadget *gadget)
>  {
>  	struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
> +	struct cdns3_endpoint *priv_ep;
>  	struct usb_ep *ep;
>  	int n_in = 0;
>  	int total;
>  
>  	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> -		if (ep->claimed && (ep->address & USB_DIR_IN))
> +		priv_ep = ep_to_cdns3_ep(ep);
> +		if ((priv_ep->flags & EP_CLAIMED) && (ep->address & USB_DIR_IN))
>  			n_in++;
>  	}
>  

Tested-by: Ravi Gunasekaran <r-gunasekaran@ti.com>

-- 
Regards,
Ravi

Re: [PATCH v2 1/2] usb: gadget: call usb_gadget_check_config() to verify UDC capability

[Frank Li]

On Fri, Jul 07, 2023 at 07:00:14PM -0400, 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>
> ---

@greg
        ping

> change from v1 to v2
> - not change for this patch, just add new patch to fix multi-config case
> 
>  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 403563c06477..cb0a4e2cdbb7 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1029,6 +1029,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;
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] usb: gadget: call usb_gadget_check_config() to verify UDC capability

[Greg KH]

On Tue, Jul 25, 2023 at 10:37:11AM -0400, Frank Li wrote:
> On Fri, Jul 07, 2023 at 07:00:14PM -0400, 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>
> > ---
> 
> @greg
>         ping

It's in my review queue, please give me a chance to catch up.

While you wait, doing review of other USB patches would be most
appreciated.

thanks,

greg k-h