[PATCH] fixup! usb: xhci: Guard all calls to xhci_wait_for_event

[Tony Dinh]

On Sun, Oct 29, 2023 at 2:33 PM Peter Robinson <pbrobinson at gmail.com> wrote:
>
> On Sun, Oct 29, 2023 at 9:25 PM Marek Vasut <marex at denx.de> wrote:
> >
> > On 10/27/23 08:03, Hector Martin wrote:
> > > On 27/10/2023 09.36, Marek Vasut wrote:
> > >> On 10/27/23 01:26, Hector Martin wrote:
> > >>> Gah, I should've paid more attention to that rebase. Here's a dumb
> > >>> fixup for this patch. I'll squash it into a v2 if needed alongside
> > >>> any other changes, or if it looks good feel free to apply/squash
> > >>> it in directly.
> > >>>
> > >>> ---
> > >>>    drivers/usb/host/xhci-ring.c | 1 +
> > >>>    1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > >>> index e2bd2e999a8e..7f2507be0cf0 100644
> > >>> --- a/drivers/usb/host/xhci-ring.c
> > >>> +++ b/drivers/usb/host/xhci-ring.c
> > >>> @@ -532,6 +532,7 @@ static void reset_ep(struct usb_device *udev, int ep_index)
> > >>>     event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
> > >>>     if (!event)
> > >>>             return;
> > >>> +   field = le32_to_cpu(event->trans_event.flags);
> > >>>     BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id);
> > >>>     xhci_acknowledge_event(ctrl);
> > >>
> > >> Please squash, and add
> > >>
> > >> Reviewed-by: Marek Vasut <marex at denx.de>
> > >>
> > >> Also, +CC Minda,
> > >>
> > >> there has been a similar fix to this one but with much more information
> > >> about the failure, see:
> > >>
> > >> [PATCH v1] usb: xhci: Check return value of wait for TRB_TRANSFER event
> > >>
> > >> I think it would be useful to somehow include that information, so it
> > >> wouldn't be lost.
> > >
> > > The root cause for *that* failure is what I fix in patch #5. From that
> > > thread:
> > >
> > >> scanning bus xhci_pci for devices... WARN halted endpoint, queueing
> > > URB anyway.
> > >
> > > Without #5 and without #1, that situation then leads to fireworks.
> > >
> > > A bunch of things are broken, which is why this is a series and not a
> > > single patch. I have more fixes to timeout handling, mass storage, etc.
> > > coming up after this. I fixed most of this in one long day of trying
> > > random USB devices, so it's not so much subtle super specific problems I
> > > could document the crashes for as just wide-ranging problems in the
> > > u-boot USB stack. None of this is particularly hard to repro if you just
> > > try a bunch of normal consumer USB devices, including things like USB
> > > HDDs which take time to spin-up leading to timeouts etc.
> >
> > I think majority of users I can think of use USB mass storage devices,
> > like USB pen drives, not so much HDDs as far as I can tell.
>
> We see a bunch of spinning rust users in Fedora, these sorts of
> devices are used by a bunch of people that want to run up cheap home
> NAS devices so from experience I'd say while not usual to be USB
> sticks and some form of solid state storage, spinning disk isn't
> unusual.

What Peter said. A very common use case, even more so than USB flash
drives, is for the consumers to plug in a USB HDD to their routers or
home NAS, as a cheap and quick solution. I've seen this type of
timeout failure happen quite often with large >= 3TB HDDs in USB
enclosure.

All the best,
Tony

>
> > > The crash dumps
> > > aren't particularly useful, it's the subtleties of the xHCI interactions
> > > that are, but for that you need to add and enable a lot more debugging
> > > (patch #8).
> >
> > The crash dumps are more for posterity, when someone searches for a
> > problem they see. Search engines tend to pick those up and it might
> > point those people in the right direction.
> >
> > Also, it is good to include information about what triggered the crash,
> > e.g. which USB device on which hardware, in case this is needed in the
> > future.
> >
> > > I think the main reason all this stuff is broken and we're finding out
> > > now is that u-boot has traditionally been used in embedded devices with
> > > fairly narrow use cases for USB
> >
> > Yes
> >
> > >, and now we're running it on
> > > workstation-grade laptops and desktops and people are plugging in all
> > > kinds of normal USB devices and expecting their bootloader not to freak
> > > out with them (even though most of the time it doesn't need to talk to
> > > them). It's really easy to run into this stuff when that's your use case.
> >
> > It is really appreciated to see people fix things like this stuff, thanks !