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

Dragan Simic dsimic at manjaro.org
Wed Feb 14 04:18:58 CET 2024 

Hello Andre,

On 2024-02-14 03:04, Andre Przywara wrote:
> On Mon, 12 Feb 2024 21:19:13 +0100 Marek Vasut <marex at denx.de> 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.
> 
> 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.

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

More information about the U-Boot mailing list