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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-02-14 11:01, Shantur Rathore wrote:
> On Wed, Feb 14, 2024 at 9:56 AM Shantur Rathore <i@shantur.com> wrote:
>> On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>> > On 2024-02-14 04:18, Dragan Simic wrote:
>> > > On 2024-02-14 03:04, Andre Przywara wrote:
>> > >> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut <marex@denx.de> wrote:
>> > >>> On 2/12/24 14:41, Shantur Rathore wrote:
>> > >>> > On Mon, Feb 12, 2024 at 1:40 PM Shantur Rathore <i@shantur.com> 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
>> > >>> >>
>> > >>> >> 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@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 -------
>> > >>> >>
>> > >>> >> 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.
>> > >>
>> > >> So the USB 2.0 stick is some no-name, unlabelled and super cheap one,
>> > >> I
>> > >> think we bought a pack of it, just for boot-strapping machines. The
>> > >> USB
>> > >> ID of "abcd:1234" kind of gives away that this is bogus AF.
>> > >> The USB 3.0 stick is a PNY 32GB one, the USB ID is:
>> > >> 1f75:0917 Innostor Technology Corporation IS917 Mass storage
>> > >>
>> > >> Hope that helps.
>> > >
>> > > Thank you for replying.  I'll include the available information into
>> > > the revised commit description and send it over a bit later.
>> >
>> > Here are the revised commit subject and description, please have a look
>> > and let me know if further improvements are needed.
>> >
>> > ------- >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 2.0 and
>> > 3.0
>> > flash drives didn't work in U-Boot on some Allwinner SoCs that support
>> > USB
>> > 2.0 interfaces only.  More precisely, some of the tested USB 2.0 and 3.0
>> > flash drives failed to be detected and work on an OrangePi Zero 3, based
>> > on
>> > the Allwinner H616 SoC that supports USB 2.0 only, while the same USB
>> > flash
>> > drives worked just fine on a Pine64 H64, based on the Allwinner H6 SoC
>> > that
>> > supports both USB 2.0 and USB 3.0 interfaces.
>> >
>> > The USB ID of the above-mentioned USB 3.0 flash drive that failed to
>> > work is
>> > 1f75:0917 (Innostor Technology Corporation IS917 Mass storage), it is 32
>> > GB
>> > in size and sold under the PNY brand.  The mentioned USB 2.0 drive is
>> > some
>> > inexpensive no-name drive with an invalid USB ID.
>> >
>> > Resetting USB 3.0 hubs only, which this patch introduces to the USB hub
>> > resets, has been tested to work as expected, resolving the identified
>> > issues
>> > on the Allwinner H616, while not introducing any new issues on other
>> > tested
>> > 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, as visible in
>> > commit
>> > 07194ab7be63 ("USB: Reset USB 3.0 devices on (re)discovery") in the
>> > kernel
>> > source, to ensure proper state of the USB 3.0 hub port and proper USB
>> > mode
>> > negotiation during the initial USB device attachment and enumeration.
>> >
>> > These additional types of USB port resets don't 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 resets USB 3.0 (i.e. SuperSpeed) hubs only, as visible
>> > in
>> > commit 10d674a82e55 ("USB: When hot reset for USB3 fails, try warm
>> > reset.")
>> > in the kernel source.  The check for SuperSpeed hubs is performed in a
>> > way
>> > that 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 -------
>> >
>> > Shantur, I hope you're feeling better.
>> 
>> Thanks Dragan, V2 is submitted.
>> I am feeling much better, thanks for asking.

It's good to hear that you're feeling better!

> PS : I don't think I could ever explain as well as you did Dragan.
> Thanks a lot for the help.

I'm glad to help. :)  We're here to work together by combining
our skills and strengths.

> I will bring another pending matter to your attention on the Linux list 
> now ;P

Is it about the USB regulators on the RockPro64? :)  I haven't
forgotten that, and I expect to work on that rather soon.