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

Thinh Nguyen
2026-04-04T20:42:05Z

On Fri, Apr 03, 2026, stern@rowland.harvard.edu wrote:
> On Sat, Apr 04, 2026 at 01:15:52AM +0000, Thinh Nguyen wrote:
> > > > 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?
> >
> > Now that I think about it again, it's not needed if we have the
> > requirement for clearing the flag only after usb_reset_endpoint.
> >
> > >
> > > > * 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.
> >
> > It seems we're using the "halted" flag for different things. The halted
> > flag to me is the endpoint's state and rather than the endpoint queue
> > state. That is, if the endpoint is halted, there's an error that was
> > reported on the pipe. So, an -ECONNRESET would not cause a halted
> > endpoint.
> >
> > >
> > > 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.
> >
> > I'm just thinking in term of whose role to return the pending URBs.
> > Typically the HCD handles when to give back URBs. If the HCD were to
> > handle giving back pending URBs due to halted endpoint, then it should
> > be the one to set the halted flag. However, if the core tracks and does
> > both setting and clearing of the halted flag, then the core should
> > handle canceling the URBs. As you said, if we can shift the burden to
> > the core, that would help keep the flow consistent and centralized.
> >
> > >
> > > > * 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.
> >
> > Yes.
> >
> > >
> > > > * 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.)
> >
> > I didn't think about failing immediately on submission because we would
> > need to audit every class driver for that. Perhaps the core can
> > intercept the URBs and immediately schedule a completion handler with
> > the updated URB's status?
> >
> > >
> > > > * 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.
> > >
> >
> > The HCD state. If we can let the core manage the halted state, then here
> > are the points where the halted state is updated (hopefully I didn't
> > miss any other ones):
> >
> > Endpoint is halted (reported by HCD):
> > * URB returned with -EPROTO or -EPIPE
> >
> > Endpoint is active (updated by the core):
> > * SetConfiguration, SetInterface, clear-halt
>
> Hmmm. What did you think of my recent proposal in a message to Michal?
> Summarizing:
>
> If the class driver wants to unlink queued URBs when a transaction error
> occurs, it has to do so itself in the failed URB's completion handler.
> We can make this easier by adding a usb_flush_endpoint_queue() routine
> to the core. In the meantime, the HCD ensures that the queue is stopped
> before giving back the URB. (Note: -EPIPE, -ENOENT, -ECONNRESET, and
> -EREMOTEIO aren't considered to be transaction errors.)
>
> When the completion handler returns, the core will automatically call
> usb_clear_halt(), which will also reset the endpoint on the host side
> and will restart the queue. This also happens after SetConfiguration
> and SetInterface, either explicitly or implicitly.

I like that the core will handle this automatically. But one concern:
How will the class driver know when the clear-halt complete so it can
perform the recovery? (ie. it shouldn't perform recovery immediately
after seeing -EPROTO)

>
> For -EPIPE (device sent a STALL token), the class driver has to clear
> the halt itself. This is because stalls aren't errors; they are an
> intentional part of the USB protocol.
>
> -ENOENT and -ECONNRESET (URB was unlinked) and -EREMOTEIO (short packet
> received with URB_SHORT_NOT_OK set) are a little trickier. The HCD may
> or may not need to stop the queue for an unlink, possibly depending on
> whether the URB being unlinked is the active one. When a short packet
> is received, the HC hardware may or may not stop its queue. Either way,
> the class driver shouldn't need to take any special recovery action; any
> necessary actions should be taken automatically by the HCD and the core.
>
> All of this applies only to bulk and interrupt endpoints. Control
> endpoints generally need error recovery only on the host side, because
> the device resets automatically when it gets a new SETUP packet, and so
> the HCD should handle whatever is needed. Isochronous endpoints don't
> ever halt and they shouldn't need to be reset when an error occurs.
>
> Overall, this seems simpler than anything else we have discussed.
>

The rest sounds good to me!

Thanks Alan,
Thinh