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

[Patrice CHOTARD]

Hi Marek

On 2/16/21 10:15 PM, Marek Vasut wrote:
> 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 ?
The test is correct, in case driver->speed is lower than FS, we return -EINVAL.
All others speed FS/HS/SS and higher are allowed.

> 
>>           !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.

Ok, i will add Lukasz for the V2 review.

Thanks
Patrice