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
