Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

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

On 29.10.2024 10.28, Michał Pecio wrote:
> 
> By the way, I think this race is already possible today, without my
> patches. There is no testing for SET_DEQ_PENDING in xhci_urb_dequeue()
> and no testing in handle_cmd_stop_ep(). If this happens in the middle
> of a Set TR Deq chain on a streams endpoint, nothing seems to stop the
> Stop EP handler from attempting invalidation under SET_DEQ_PENDING.
> 
> Maybe invalidate_cancelled_tds() should bail out if SET_DEQ_PENDING
> and later Set Deq completion handler should unconditionally call the
> invalidate/giveback combo before it exits.
> 

I think you are on to something.
If we add invalidate/givaback to Set TR deq completion handler, allowing
it to possible queue new Set TR Deq commands, then we can bail out in
xhci_urb_dequeue() if SET_DEQ_PENDING is set.

xhci_urb_dequeue() would not queue a extra stop endpoint command, only
set td->cancel_status to TD_DIRTY dirty, and Set TR Deq handler would
not ring the doorbell unnecessary.

Sounds like a plan to ne.

>> We could ring the doorbell before giving back the invalidated tds in
>> xhci_handle_cmd_stop_ep(), and possibly xhci_handle_cmd_set_deq().
>> This gives hardware a bit more time to start the endpoint.
> 
> This unfortunately doesn't buy much time, because giveback is a very
> cheap operation - it just adds the URBs to a queue and wakes a worker
> which runs all those callbacks. It finishes under 1us on my system.

Probably true, but change is simple and free so might be worth it.

Thanks
Mathias