Re: [PATCH 12/15] xhci: Prevent early endpoint restart when handling STALL errors.

["Michał Pecio" <michal.pecio@gmail.com>]

On Fri, 7 Mar 2025 16:23:17 +0200, Mathias Nyman wrote:
> > Any flag added to this list needs to be added to xhci_urb_dequeue()
> > too so it knowns that the endpoint is held in Stopped state and
> > URBs can be unlinked without trying to stop it again.  
> 
> In this case it's intentional.
> 
> If we prevent xhci_urb_dequeue() from queuing a stop endpoint command
> due to a flag, then we must make sure the cancelled URB is given back
> in the same place we clear the flag, like we do in the command
> completion handlers that clear EP_HALTED and SET_DEQ_PENDING.

I'm not sure why this would be, what's the problem with the approach
used for EP_CLEARING_TT currently? And if there is a problem, doesn't
EP_CLEARING_TT also have this problem?

In this case, xhci_urb_dequeue() simply takes xhci->lock and calls:

void xhci_process_cancelled_tds(struct xhci_virt_ep *ep)
{
        xhci_invalidate_cancelled_tds(ep);
        xhci_giveback_invalidated_tds(ep);
}

Unlinked URBs are either given back instantly, or Set TR Dequeue is
queued (and flagged on ep->ep_state) and the rest of the process goes
same way as usual when called from xhci_handle_cmd_stop_ep(). 

The EP will be restarted when the last flag is cleared, which may be
either SET_DEQ_PENDING or EP_CLEARING_TT/EP_STALLED.

It's practically an optimization which eliminates the dummy Stop EP
command from the process. I thought EP_STALLED could use it.

> The EP_STALLED flag is cleared after a ClearFeature(ENDPOINT_HALT)
> control transfer request is (successfully?) sent to the device.
> If we only give back those cancelled URBs after this then we create a
> situation where cancelled urb giveback is blocked and depend on the
> completion of another transfer on a different endpoint.
> I don't want this dependency.

No doubt, that would be unbounded latency and asking for trouble.

> It's possible that this could create some type of deadlock where
> class driver ends up waiting for cancelled URBs to be given back
> before it sends the request to clear the halt, and  xhci won't give
> back the cancelld URBs before the ClearFeature(ENDPOINT_HALT) request
> completes..
> 
> Lets look at the cases where xhci_urb_dequeue() is called between
> setting and clearing this new EP_STALLED flag.
> 
> The EP_HALTED is set during same spinlock as EP_STALLED, so urbs
> dequeued during this time will be added to cancelled list, and given
> back in xhci_handle_cmd_reset_ep() completion handler where also
> EP_HALTED is cleared. If dequeue needs to be moved then
> SET_DEQ_PENDING is set, and cancelled urbs will be given back in
> xhci_handle_cmd_set_deq() completion handler.
> 
> At this stage we know endpoint is in stopped state. and will remauin
> so until EP_STALLED is cleared. if xhci_urb_dequeue() is called now
> then a stop endpoint command will ne queued, it will complete with a
> context state error due to endpoint already being stopped, but URB
> will be given back in one of the completion handlers. mentioned
> before.

Yes, it works, but in this case the "shortcut" will also work.

One problems with pointless Stop EP commands I remember is that there
is code in xhci-hub.c:xhci_stop_device() which avoids queuing Stop EP
on stopped endpoints, supposedly because it triggers some HW bug.

So the idea of these Stop EP patches was to eliminate such cases. It
also simplifies the completion handler and avoids needing:

> We could improve this codepath a bit by adding:
> [...]


Michal