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

[Peter Robinson]

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.

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