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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Heinrich,

heinrich.schuchardt@canonical.com wrote on Mon, 7 Aug 2023 11:29:08
+0200:

> On 04.08.23 21:14, 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.  
> 
> Are the UDCs real devices or only logical ones?

In this context there is a real device behind. But UDC is also the name
for kind of an interface for gadget drivers. In theory, on USB device
controller could expose several different functions.

> If they are only logical ones, can we generate new ones on the fly?

In general, it's not impossible, but it's not supported at all in U-Boot
(and for a good reason: it would be way too complex to handle) and I
think even in Linux, it's possible, but possibly only using
configfs (not 100% sure on that one).

> > 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@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.
> > ---
> >   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..30f6d73f36e 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 UDC in use (%d available), use the unbind command\n",  
> 
> 'UDC' should be plural, e.g.
> 
> %s/All UDC in use (%d available)/All %d UDCs bound/

Does not work well with one: "All 1 UDCs bound" does not look great.

I would prefer keeping the original sentence, s/UDC/UDCs/:
"All UDCs in use (%d available)".

Thanks,
Miquèl