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

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

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.
> > 
> > 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?  :-) )
> 

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
* Only the HCD may set the halted flag, and only upon checking the
  first URB completing with a halted status
* 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.
* 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
* 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?

BR,
Thinh