[FIX PATCH v1] Fix: common: usb_hub: Reset only USB3.0 hub
Shantur Rathore
i at shantur.com
Wed Feb 14 10:56:51 CET 2024
On Wed, Feb 14, 2024 at 3:48 AM Dragan Simic <dsimic at manjaro.org> wrote:
>
> Hello Shantur and Marek,
>
> 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 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.
>
> 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 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 -------
>
> Shantur, I hope you're feeling better.
Thanks Dragan, V2 is submitted.
I am feeling much better, thanks for asking.
More information about the U-Boot
mailing list