[PATCH RESEND v2 2/2] usb: udc: Try to clarify an error message

[Miquel Raynal]

Hi Marek,

marex at denx.de wrote on Thu, 5 Oct 2023 15:04:25 +0200:

> On 10/2/23 15:46, Miquel Raynal wrote:
> > At some point when trying to use USB gadgets, two situations may arise
> > and lead to a failure. Either the UDC (USB Device Controller) is not
> > available at all (not described or not probed) or the UDC is already in
> > use. For instance, as the USB Ethernet gadget remains bound to the UDC,
> > the use of any other USB gadget (fastboot, dfu, etc) *after* will always
> > fail with the "couldn't find an available UDC" error.
> > 
> > Let's give a more helpful message by making a difference between the two
> > cases. Let's also hint people who would get this error and grep it into
> > the sources a better explanation of what's wrong with their workflow.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> > ---
> > While doing this I really wanted to add "much more" error comments but I
> > faced another reality: often the messages are there but use
> > pr_err/log_err which is actually silenced by default with LOGLEVEL=3, so
> > I consider this unnecessary, as decreasing the loglevel will make these
> > messages appear. I would have expected errors to be displayed, but I
> > understand it makes the binaries even bigger.
> > 
> > Resend: no change.
> > 
> > Changes in v2:
> > * s/UDC/UDCs/. I kept the sentence as it was as the suggested form did
> >    not sound well at all when there is only one UDC.
> > ---
> >   drivers/usb/gadget/udc/udc-core.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> > index 7f73926cb3e..8405b03462e 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -323,6 +323,7 @@ err1:
> >   int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
> >   {
> >   	struct usb_udc		*udc = NULL;
> > +	unsigned int		udc_count = 0;
> >   	int			ret;  
> >   >   	if (!driver || !driver->bind || !driver->setup)  
> > @@ -330,12 +331,22 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)  
> >   >   	mutex_lock(&udc_lock);  
> >   	list_for_each_entry(udc, &udc_list, list) {
> > +		udc_count++;
> > +
> >   		/* For now we take the first one */
> >   		if (!udc->driver)
> >   			goto found;
> >   	}  
> >   > -	printf("couldn't find an available UDC\n");  
> > +	if (!udc_count)
> > +		printf("No UDC available in the system\n");
> > +	else
> > +		/* When this happens, users should 'unbind <class> <index>'
> > +		 * using the output of 'dm tree' and looking at the line right
> > +		 * after the USB peripheral/device controller.
> > +		 */
> > +		printf("All UDCs in use (%d available), use the unbind command\n",
> > +		       udc_count);  
> 
> Users should really use 'bind' command in the first place, to avoid this guesswork to which UDC (on systems with multiple UDCs) a gadget gets bound to, and instead bind the gadget to specific UDC they want to bind it to. This code should then be removed entirely.

There are two cases when this can happen:
* a single UDC (common) but a function already bound, there is no
  guessing here
* two (or more) UDC and there is some guessing

Before your cleanup, drivers would get automatically unbound from the
UDC to let the one being needed to bind. This is no longer the case,
so there is a change in the CLI and I want to help people facing
this new problem with a slightly more comprehensive message because
people don't fully understand what USB is all about. We cannot
ask all U-Boot USB users to be U-Boot experts and USB experts. This
is just a little help.

In an ideal world, we will have the possibility to create
composite gadgets and all this can go away once it is well
integrated, I guess?

Thanks,
Miquèl