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

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

[Miquel Raynal]

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@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_count);
 	mutex_unlock(&udc_lock);
 	return -ENODEV;
 found:
-- 
2.34.1

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

[Heinrich Schuchardt]

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?

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

> 
> 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/

Best regards

Heinrich

> +		       udc_count);
>   	mutex_unlock(&udc_lock);
>   	return -ENODEV;
>   found:

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

[Miquel Raynal]

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