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

Tue Feb 13 12:50:19 CET 2024

On 2/13/24 04:59, Dragan Simic wrote:
> On 2024-02-13 00:33, Marek Vasut wrote:
>> On 2/12/24 22:10, Dragan Simic wrote:
>>> On 2024-02-12 21:19, Marek Vasut wrote:
>>>> On 2/12/24 14:41, Shantur Rathore wrote:
>>>>> On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i at shantur.com> wrote:
>>>>>> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic at manjaro.org> 
>>>>>> wrote:
>>>>>>> On 2024-02-08 15:17, Dragan Simic wrote:
>>>>>>>> On 2024-02-08 15:10, Shantur Rathore wrote:
>>>>>>>>> On Thu, Feb 8, 2024 at 1:44 PM Dragan Simic <dsimic at manjaro.org>
>>>>>>>>> wrote:
>>>>>>>>>> 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 at 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 at arm.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Shantur Rathore <i at 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. :)
>>>>>>>>>
>>>>>>>>> That would be really kind of you Dragan.
>>>>>>>>
>>>>>>>> Sure, I'll write the summary and send it over.
>>>>>>>>
>>>>>>>>> I am down with the flu so that would really help me as my brain is
>>>>>>>>> working at 15% capacity.
>>>>>>>>
>>>>>>>> Oh, I'm really sorry to hear that. :(  I hope you'll get better
>>>>>>>> soon, and I know very well what's it like;  I've also been sick
>>>>>>>> recently, as a result of some kind of flu that unfortunately found
>>>>>>>> its way into my lungs, and it took me about a month to get back
>>>>>>>> to about 90% of my usual mental capacity.  I'm still not back to
>>>>>>>> exactly 100%. :/
>>>>>>>>
>>>>>>>> I really hope you'll recover much faster.
>>>>>>>
>>>>>>> I hope you're feeling better.
>>>>>>>
>>>>>>> Below are the patch subject and description that I prepared for you,
>>>>>>> please have a look.
>>>>>>>
>>>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>>>>> -------
>>>>>>> [PATCH v2] common: usb-hub: Reset USB 3.0 hubs only
>>>>>>>
>>>>>>> Additional testing of the changes introduced in commit 33e06dcbe57a
>>>>>>> ("common:
>>>>>>> usb-hub: Reset hub port before scanning") revealed that some USB 3.0
>>>>>>> flash
>>>>>>
>>>>>> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.
>>>>>>
>>>>>>> drives didn't work in U-Boot on some Allwinner SoCs that support 
>>>>>>> USB 2.0
>>>>>>> only.
>>>>>>> More precisely, some tested USB 3.0 flash drives failed to be 
>>>>>>> detected
>>>>>>> and
>>>>>>> work on an OrangePi Zero 3 with Allwinner H616 SoC, which 
>>>>>>> supports USB
>>>>>>> 2.0
>>>>>>> only, while the same USB flash drives worked just fine on a 
>>>>>>> Pine64 H64
>>>>>>> with
>>>>>>> Allwinner H6 SoC, which supports both USB 2.0 and 3.0.
>>>>>>>
>>>>>>> Resetting USB 3.0 hubs only has been tested to work as expected,
>>>>>>> resolving
>>>>>>> the previous issues on the Allwinner H616, while not introducing 
>>>>>>> any new
>>>>>>> issues on other Allwinner SoCs.  Thus, let's fix it that way.
>>>>>>>
>>>>>>> According to the USB 3.0 specification, resetting a USB 3.0 port is
>>>>>>> required
>>>>>>> when an attached USB device transitions between different states, 
>>>>>>> such
>>>>>>> as
>>>>>>> when it resumes from suspend.  Though, the Linux kernel performs
>>>>>>> additional
>>>>>>> USB 3.0 port resets upon initial USB device attachment, 
>>>>>>> presumably to
>>>>>>> ensure
>>>>>>> proper state of the USB 3.0 hub port and proper USB mode negotiation
>>>>>>> during
>>>>>>> the initial USB device attachment and enumeration.
>>>>>>>
>>>>>>> Such USB port resets don't seem to exist for USB 2.0 hubs, 
>>>>>>> according the
>>>>>>> USB
>>>>>>> 2.0 specification.  The resets seem to be added to the USB 3.0
>>>>>>> specification
>>>>>>> as part of the port and device mode negotiation.
>>>>>>>
>>>>>>> The Linux kernel also resets USB 3.0 (i.e. SuperSpeed) hubs only, as
>>>>>>> visible
>>>>>>> in file drivers/usb/core/hub.c in the kernel source, line 2859. This
>>>>>>> check
>>>>>>> also applies to newer SuperSpeed Plus (USB 3.1 or 3.2) hubs as well,
>>>>>>> which
>>>>>>> hopefully makes it future proof.
>>>>>>>
>>>>>>> Fixes: 33e06dcbe57a ("common: usb-hub: Reset hub port before 
>>>>>>> scanning")
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/u-boot/20240207102327.35125-1-i@shantur.com/T/#u
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/u-boot/20240201164604.13315fa6@donnerap.manchester.arm.com/T/#u
>>>>>>> Signed-off-by: Shantur Rathore <i at shantur.com>
>>>>>>> Helped-by: Dragan Simic <dsimic at manjaro.org>
>>>>>>> Tested-by: Andre Przywara <andre.przywara at arm.com>
>>>>>>> Reviewed-by: Dragan Simic <dsimic at manjaro.org>
>>>>>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 
>>>>>>> -------
>>>>>>
>>>>>> Regards,
>>>>>> Shantur
>>>>>
>>>>> Is this description acceptable to you Marek.
>>>>
>>>> Please send a V2 patch . If possible, include the device information
>>>> as reported by Andre, esp. which USB stick triggered it, including USB
>>>> IDs, this is important for future reference and in case someone has
>>>> similar failure.
>>>
>>> If Andre can supply the USB IDs, I'm willing to help Shantur by
>>> adjusting the wording of the patch description to include them.
>>
>> That's what I'm hoping is gonna happen , thanks !
>>
>>>> Please don't use "in the kernel source, line 2859", considering the
>>>> rate of change of the Linux kernel, it is best to also include exact
>>>> commit ID as of which this is a line 2859 and spell out this is
>>>> referring to Linux kernel.
>>>
>>> Good point.  I'm willing to adjust the wording.
>>
>> Thanks .
> 
> I'm glad to help, and I'll be happy to see this patch accepted, with
> properly detailed description for future reference.
> 
> I've already adjusted the wording of the description, and I'll send
> the adjusted version over after Andre provides the USB IDs of the USB
> flash drives that initially failed to work.

Thanks !