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

[Dragan Simic <dsimic@manjaro.org>]

Hello Shantur,

On 2024-02-12 14:40, Shantur Rathore wrote:
> On Sat, Feb 10, 2024 at 7:13 AM Dragan Simic <dsimic@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@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@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. :)
>> >>
>> >> 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

s/some USB 3.0 flash/some USB 2.0 and 3.0 flash/

> I think it was a USB 2.0 drive that didn't work on the USB 2.0 port.

As visible in the message linked below, there was one USB 3.0 flash
drive that didn't work in a USB 2.0 port.  Though, you're right, there
was also a USB 2.0 drive that didn't work.  Thus, it's perhaps best to
adjust the wording as I suggested above and below.

https://lore.kernel.org/u-boot/20240202001218.63b4e9c3@minigeek.lan/

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

s/some tested USB 3.0 flash drives/some tested USB 2.0 and 3.0 flash 
drives/

>> 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@shantur.com>
>> Helped-by: Dragan Simic <dsimic@manjaro.org>
>> Tested-by: Andre Przywara <andre.przywara@arm.com>
>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------