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

stern
2026-04-03T02:42:44Z

On Fri, Apr 03, 2026 at 01:59:58AM +0000, Thinh Nguyen wrote:
> On Wed, Apr 01, 2026, stern@rowland.harvard.edu wrote:
> > 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.

> How about this:
>
> Introduce a halted flag the following conditions:
>
> * Introduce the halted flag in usb_host_endpoint
> * The halted flag must be implemented as a bit in a unsigned long so
> we can use atomic bit operation

I don't see why it needs to use atomic operations. Unless you're
thinking that we might want to add other bitflags into the same unsigned
long?

> * Only the HCD may set the halted flag, and only upon checking the
> first URB completing with a halted status

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.

Why do you want the HCD to set the flag instead of
usb_hcd_giveback_urb()? Just one central place is simpler than in every
HCD.

> * Only the USB core may clear the halted flag, and only after
> usb_reset_endpoint returns (this makes sure the HCD drained and reset
> the endpoint before the flag is cleared and new URBs are accepted)
> * The usb_reset_endpoint must be called after clear-halt, SetInterface,
> and SetConfiguration.

That's easy to do just by adding it into the appropriate core routines.

> * 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.

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 -- it
would mean adding error handling to the submission code as well as to
the completion handler code.

(Actually, that wouldn't matter unless the driver was queuing up
multiple URBs, in which case it should be prepared to handle submission
errors in the middle.)

> * The class driver is responsible to check the halted flag to
> determine whether to initiate error recovery via usb_clear_halt
>
> I'm trying to keep a clear separation of responsibilities between HCD
> and the USB core. Also, I try to keep the halted flag more closely match
> the state of the endpoint.
>
> Let me know what you think?

Making the flag match the endpoint state is a good idea. But there is
some ambiguity: Do you mean the state in the device, or the state in the
HC hardware, or the state in the HCD? After all, these aren't always
the same. For instance, unlinking an URB won't affect the device's
state but it will affect the state on the host side.

Alan Stern