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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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