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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 7.3.2025 17.44, Michał Pecio wrote:
> 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.
> 

This should work, and avoid that unnecessary stop endpoint command.

Just need to make sure we check for EP_STALLED flag after the other
(EP_STOP_CMD_PENDING | EP_HALTED | SET_DEQ_PENDING) flags in
xhci_urb_dequeue(), just like EP_CLEARING_TT case.

Also need to protect clearing the EP_STALLED flag with the lock

I'll either send an update patch next week, or during rc cycle if
that's too late.

Thanks
Mathias