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

stern
2026-03-31T15:31:25Z

On Tue, Mar 31, 2026 at 12:34:54PM +0300, Mathias Nyman wrote:
> On 3/30/26 17:17, stern@rowland.harvard.edu wrote:
> > On Mon, Mar 30, 2026 at 03:51:46PM +0300, Mathias Nyman wrote:
> > > Ideally xhci driver would return the URB with EPIPE after STALL, and not continue
> > > processing URBs before a clear halt is sent, or a new URB is enqueued.
> > > USB core would hold off submitting URBs to xhci, buffering URBs enqueued for this
> > > STALLED endpoint until class driver urb->complete() finishes for that EPIPE URB.
> > >
> > > Usb core could flag this endpoint as "halt_pending" before adding the EPIPE URB to
> > > the bh workqueue. Then after urb->complete() work is called and core is sure class
> > > driver is aware of the EPIPE, then core would clear the flag and flush the buffered
> > > URBs to the host controller drivers, restarting the ring
> >
> > This is not practical; it would result in a big slowdown for large bulk
> > transfers. Doing this would mean the core could not send an URB to the
> > HCD until all the previous URBs for that endpoint had completed and the
> > interrupt handler had run, which would add significant latency to
> > transfers.
>
> The URB submit buffering in core would only take place if endpoint is halted
> with EPIPE/EPROTO.
>
> Additional latency should be limited to the time between interrupt handler
> returns a URB with EPIPE/EPROTO, and the bh workqueue finishing urb->complete()
> for that URB
>
> URBs would normally be sent to HCD directly.
>
> Yes, this might end up being quite complex, need to make sure it solves more
> issues than it creates.

Can that be done precisely? I doubt it -- there are races between the
core and the HCD and between the HCD and the hardware.

As for doing it imprecisely... I'm not sure the advantages outweigh the
complexity. Anyway, if the class driver is well behaved, this buffering
won't be needed.

> Usb core is aware of the halted endpoint before the class driver due to the bh
> workqueue giveback. To me it would make sense to make core responsible for babysitting
> the class driver urb submission for the time it withholds this information.
>
> Whole reason for this is to prevent new URB submission from HCD restarting an endpoint
> after HCD gave back a halted URB, and HCD assuming core/class drivers are aware of
> the halt when the new URB is submitted.
>
> Other option is that usb core would just refuse class driver from submitting URBs
> during this time. usb_submit_urb() would return with an error, but I think this might
> impact existing class drivers more.

How about this instead? We add a "halted" flag to the usb_host_endpoint
structure, and the core will set this flag whenever a bulk or interrupt
URB gets a status other than 0 (before putting the URB on the bh list).
If an URB has one of these statuses, when its completion handler returns
the core will unlink all the URBs queued to the same endpoint. Finally,
the "halted" flag should be cleared after a completion handler returns
if there are no more unlinked URBs still in the queue or URBs waiting on
the bh list to be given back.

The result of this is that any URB remaining in the queue when the flag
is cleared must have been submitted by the class driver _after_ the
failing URB's completion handler has run. We can assume the class
driver knows what it's doing in this case.

The endpoint queue shouldn't be restarted until the "halted" flag is
cleared. Either right away, if there are any URBs in the queue, or not
until the next URB is submitted. Doing this might require a new HCD
callback. (It would also mean the kerneldoc for usb_unlink_urb() would
need to be updated, because the endpoint might restart before all the
completion handlers for the unlinked URBs have run.)

What I'm trying to do here is come up with a single, consistent proposal
for exactly when halted endpoint queues should restart. Maybe someone
else has a better suggestion.

We also should add a special case for control endpoints because their
halt conditions can get cleared automatically when the device receives a
SETUP packet.

> > Exactly what should happen to URBs coming after one that completes with
> > -EPIPE is not immediately obvious. If the HCD did try to send them to
> > the device right away then they would also be stalled, which is
> > obviously non-productive. The only guarantee the kernel makes
> > about this situation is that the endpoint queue won't restart
> > until all completed or unlinked URBs have been given back (likewise for
> > -EPROTO errors).
>
> My take is that endpoint should stop processing URBs, existing pending URBs
> should be retired by class/core, new URBs should restart the endpoint but new URBs
> are only permitted _after_ submitter is aware of the halt.

Yes, that's more or less what my proposal attempts to accomplish.

> A class driver testing USB specification should be able to resubmit a stalled URB
> (EPIPE) and expect it to complete with EPIPE again until it clears the halt.

Agreed.

> > Here's a troubling consequence for people to consider: Suppose an
> > endpoint queue stops with -EPROTO or -EPIPE, and before the class driver
> > gets around to calling usb_clear_halt(), it is unbound. What happens
> > the next time a driver binds to the device and tries to use the
> > endpoint?
>
> The disable/enable interface and set config calls during unbind/bind should,
> if I remember correctly flush pending URBs and drop and re-add the endpoint,
> clearing the xhci side halt and reset toggle.

usb_probe_interface() doesn't do any of that stuff, other than a
deferred switch to altsetting 0 if needed.

usb_unbind_interface() does call usb_enable_interface() if the interface
is already in altsetting 0, but the reset_ep argument is false so the
endpoint state doesn't get affected. Should that be changed?

Alan Stern