[PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()

[Marek Vasut]

On 2/16/21 6:32 PM, Patrice CHOTARD wrote:
> Hi Marek

Hi,

> On 2/11/21 12:26 PM, Marek Vasut wrote:
>> On 2/11/21 10:58 AM, Patrice CHOTARD wrote:
>>> Hi Marek
>>>
>>> On 2/10/21 3:26 PM, Marek Vasut wrote:
>>>> On 2/10/21 3:17 PM, Patrice Chotard wrote:
>>>>> Since commit 8745b9ebccae ("usb: gadget: add super speed support")
>>>>> ums was no more functional on platform which use dwc2_udc_otg driver.
>>>>>
>>>>> Remove the speed test in dwc2_gadget_start() to fix it.
>>>>> Tested on stm32mp157c-ev1 board.
>>>>
>>>> Isn't the speed check correct though ?
>>>
>>> I am not sure this speed test is needed.
>>>
>>>>
>>>> What is really going on when this fails ?
>>>
>>>
>>> Since 8745b9ebccae ("usb: gadget: add super speed support"),
>>> driver->speed is now set to USB_SPEED_SUPER in drivers/usb/gadget/composite.c
>>>
>>> and this forbids dwc2_udc_otg.c to be registered.
>>
>> That sounds like a bug in the USB gadget/otg core , no ?
> 
> After analysis, if i correctly understood, the speed test done in both usb_gadget_register_driver()
> and in dwc2_gadget_start() in dwc2_udc_otg.c is too restrictive and accepts only gadget driver with
> max speed equal to USB_SPEED_FULL or USB_SPEED_HIGH.

That should be fine for the DWC2 IP, which is LS/FS/HS.

> So all gadget driver with max speed set to higher speed than USB_SPEED_HIGH (USB_SPEED_SUPER in case of
> composite gadget driver) are not allowed, which is wrong.
> 
> This test should check that the gadget driver max speed is higher or equal to the min speed supported by
> dwc2_udc_otg driver, ie USB_SPEED_FULL.
> 
> So the test should be :
> 
> @@ -247,12 +247,11 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
>   	unsigned long flags = 0;
>   
>   	debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
>   
>   	if (!driver
> -	    || (driver->speed != USB_SPEED_FULL
> -		&& driver->speed != USB_SPEED_HIGH)
> +	    || driver->speed < USB_SPEED_FULL
>   	    || !driver->bind || !driver->disconnect || !driver->setup)
>   		return -EINVAL;
>   	if (!dev)
>   		return -ENODEV;
>   	if (dev->driver)
> @@ -319,12 +318,11 @@ static int dwc2_gadget_start(struct usb_gadget *g,
>   	struct dwc2_udc *dev = the_controller;
>   
>   	debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
>   
>   	if (!driver ||
> -	    (driver->speed != USB_SPEED_FULL &&
> -	     driver->speed != USB_SPEED_HIGH) ||
> +	    driver->speed < USB_SPEED_FULL ||

The DWC2 can't operate in LS gadget mode , i.e. emulate some old 
keyboard ? Maybe what you want is driver->speed > USB_SPEED_HIGH instead ?

>   	    !driver->bind || !driver->disconnect || !driver->setup)
>   		return -EINVAL;
>   
>   	if (!dev)
> 
> I you are agree, i will send a v2 with this.

Yes please. But you really want to get AB/RB from Lukasz, since he does 
the gadget stuff.