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

[Marek Vasut]

On 10/5/23 17:51, Miquel Raynal wrote:
> 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.

How did that ever work ?

> 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 that case, you have to handle the details like CMD_BIND may not be 
enabled, and CMD_DM may not be enabled. But then, maybe what we want to 
do is fix the automatic switching of gadgets to make it work again ?

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

I believe you would still have to do a disconnect/connect cycle even 
with a composite driver, you cannot just add functions to existing 
composite at runtime.