Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

stern
2026-04-03T14:56:03Z

On Fri, Apr 03, 2026 at 10:51:45AM +0200, Michal Pecio wrote:
> On Thu, 2 Apr 2026 22:42:39 -0400, stern@rowland.harvard.edu wrote:
> > Every status other than 0 should mean that the endpoint's queue is
> > halted. But not all statuses require a clear-halt or reset-endpoint
> > to recover. For instance, a short transfer when the URB_SHORT_NOT_OK
> > flag is set.
>
> This doesn't work in xhci-hcd and I'm not sure if it can. I don't recall
> any way to make endpoints halt on short transfer at all.
>
> The driver does exactly two things with this flag:
> 1. Isochronus frames get EREMOTEIO status when it's detected, although
> ISTR that URB_SHORT_NOT_OK isn't defined for isochronous URBs.
> 2. All others log a dbg() message. Supposedly, core is responsible for
> updating urb->status then.

Truly? What happens if usb-storage is doing a large bulk-IN transfer
comprising multiple data URBs (with URB_SHORT_NOT_OK set in all but the
last one), and the device indicates an early end-of-data by sending a
short packet in the middle? Will xhci-hcd store the information in the
following bulk-IN status transaction to the transfer buffer for the next
URB in the queue of data URBs?

(That's a misleading question. The BOT class specification requires the
device to stall the bulk-IN endpoint following a short data transfer, so
this scenario wouldn't arise -- although not all devices follow the spec
in this regard. And it might arise in other protocols. Regardless,
it's hard for me to believe there's no way to tell an xHCI controller to
stop the endpoint on a short transfer. EHCI, OHCI, and UHCI all have
ways to do it.)

> > > * The USB core will not attempt to unlink pending URBs due to halted
> > > condition
> > > * The HCD is responsible for completing or canceling queued URBs
> > > when the halted flag is set. Cancelled and newly submitted URBs
> > > will be returned with -EPIPE as long as the halted flag is set
> >
> > Why make the HCD responsible for draining the queue? It's like
> > setting the halted flag; one central place is simpler and less
> > error-prone than lots of separate places.
>
> Is emptying the queue really a good idea at all? It obviously breaks
> lazy drivers which just ignore errors and continue with the URBs they
> have already submitted. Removing the URBs only adds more data loss.

We have to decide on a single approach that can work for everybody. If
it means that some class drivers will have to be changed, so be it.

A lazy driver that just ignores errors will currently not work very well
if at all following an error on an xHCI host controller. But yes, it
will work with a USB-2 controller.

If we want to minimize the changes needed in class drivers then we will
have to make the core and HCDs recover automatically from transaction
errors. Non-lazy class drivers still have a chance to do their own
recovery when the completion handler for the bad URB runs. To make
their lives easier, we could add a usb_purge_endpoint_queue() routine to
the core; this seems a lot safer than summarily unlinking all the URBs
all the time.

Is that what we should do?

> > For newly submitted URBs, should the submission fail with -EPIPE, or
> > should the submission succeed and then the URB complete with -EPIPE?
> > The first is simpler, but drivers probably aren't prepared for it
>
> Are they truly expecting the alternative? I bet most of them would
> usb_clear_halt() for each received EPIPE, or not at all.

They might do usb_clear_halt() if the completed URB had -EPIPE status.
But they won't do it when usb_submit_urb() returns -EPIPE (or any other
error, for that matter).

> If completion unlinks remaining URBs before returning, this question
> doesn't exist (if we fix restarting before completion). If it doesn't
> unlink them, who even knows what the driver wants?

Good point.

> For example, some drivers resubmit the URB while handling EPROTO and
> don't unlink anything. To me, it says "screw data loss and continue".
>
> It admittedly doesn't work on SuperSpeed anymore, but not all drivers
> need to worry about SuperSpeed. Including some old and lazy ones.

Think of it this way: If any URBs remain in the queue when the
completion handler returns, we can assume the class driver doesn't want
them to be unlinked. When that happens, the best thing the core/HCD can
do is issue a clear-halt and then restart the endpoint, so the queued
URBs will run. Except for the -EPIPE case, where the device halted its
side of the endpoint and sent a STALL packet -- it's reasonable to
require class drivers to handle that case themselves.

On the other hand, if the queue is empty when the completion handler
returns (or if all the URBs still in the queue have already been
unlinked but not yet given back), is there any harm in the core issuing
a clear-halt and restarting the endpoint (with the same exception as
above)? This doesn't allow the class driver to do any further error
recovery of its own, but when you get down to it, what else is there for
it to do?

Also, this way we don't have to reject any URBs submitted while endpoint
is stopped. They simply get added to the end of the queue, so they will
run when the endpoint is restarted.

Alan Stern