| Message ID | 20230628222437.3188441-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] usb: gadget: call usb_gadget_check_config() to verify UDC capability | expand |
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;
> -----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
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;
> -----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
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
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 --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;
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(+)