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

stern
2026-04-02T02:36:41Z

On Thu, Apr 02, 2026 at 01:08:31AM +0300, Mathias Nyman wrote:
> On 3/31/26 18:31, stern@rowland.harvard.edu wrote:
> >
> > 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.
>
> Sounds like a possible solution to me.
>
> Just to clarify, core should unlink the remaining URBs queued to that endpoint
> after setting the "halted" flag, but before URB completion is called.

Above I wrote that the remaining URBs should be unlinked _after_ the
completion handler is called. If we did the unlinks before then the
class driver might submit a new URB after the unlinks were finished and
before the completion handler learned about the transaction error, and
this new URB then wouldn't get unlinked.

It's a race between completion of one URB and submission of another.

> "Halted" flag should be cleared after URB completion returns, and endpoint
> should be restarted if there are any pending URBs.

To be clear, the flag should be cleared after the completion handlers
for _all_ the unlinked URBs (as well as the URB getting the original
error) have returned.

> This allows the class driver URB completion handler to re-queue the halted URB
> without core unlinking it.

The completion handler shouldn't do this, because it would mean
resubmitting without doing a clear-halt first. (Completion handlers
can't do clear-halts because they run in atomic context.) If it does
try to do this anyway, I see nothing wrong with the core unlinking the
resubmitted URB.

(Are you going to ask about verification tests that set the endpoint's
Halt feature, submit an URB, wait for it to fail with -EPIPE, and then
submit another URB from within the completion handler? :-) )

> > > > 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?
>
> Looks like this needs more attention. Interface driver unbind/bind with
> halted endpoint could be an issue. I don't have an answer right now.

We can ponder it...

Alan Stern