Re: [FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub

[Dragan Simic <dsimic@manjaro.org>]

On 2024-02-08 14:33, Marek Vasut wrote:
> On 2/8/24 12:30, Shantur Rathore wrote:
>> On Wed, Feb 7, 2024 at 1:07 PM Marek Vasut <marex@denx.de> wrote:
>>> On 2/7/24 11:23, Shantur Rathore wrote:
>>>> USB 3.0 spec requires hub to reset device while
>>>> enumeration. Some USB 2.0 hubs / devices don't
>>>> handle this well and after implementation of
>>>> reset some USB 2.0 disks weren't detected on
>>>> Allwinner based boards.
>>>> 
>>>> Resetting only when hub is USB 3.0 fixes it.
>>> 
>>> It would be good to include as many details about the faulty hardware 
>>> in
>>> the commit message as possible, so that when someone else runs into
>>> this, they would have all that information available.
>>> 
>>>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>>>> 
>>>> Signed-off-by: Shantur Rathore <i@shantur.com>
>>>> ---
>>>> 
>>>>    common/usb_hub.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>> index 3fb7e14d10..2e054eb935 100644
>>>> --- a/common/usb_hub.c
>>>> +++ b/common/usb_hub.c
>>>> @@ -174,8 +174,10 @@ static void usb_hub_power_on(struct 
>>>> usb_hub_device *hub)
>>>> 
>>>>        debug("enabling power on all ports\n");
>>>>        for (i = 0; i < dev->maxchild; i++) {
>>>> -             usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_RESET);
>>>> -             debug("Reset : port %d returns %lX\n", i + 1, 
>>>> dev->status);
>>>> +             if (usb_hub_is_superspeed(dev)) {
>>> 
>>> Should this condition be "all which are lower than superspeed" 
>>> instead ,
>>> so when the next generation of USB comes, this problem won't trigger 
>>> ?
>>> 
>>> What does Linux do btw ?
>> 
>> As of now Linux checks if the hub is superspeed
>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L2859
>> 
>> which is
>>   return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS; // 
>> USB_HUB_PR_SS = 3
>> 
>> This holds true for newer SuperSpeedPlus hubs as well.
>> https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.h#L155
>> 
>> We can change the check to be  bDeviceProtocol > 2 but who knows if
>> things change in the newer version of spec.
>> I am open to suggestions.
> 
> Please just include the ^ in the commit description. Use link to
> git.kernel.org , not some mirror . This is extremely useful
> information and, well, you already wrote the V2 commit message
> addition in this answer.

Shantur, if that would be easier or quicker for you, I can write
a quite detailed patch description for you, in exchange for a
"Helped-by" tag in the v2 patch submission. :)