Re: [PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

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

On Fri, 22 Aug 2025 10:15:14 +0200, Michał Pecio wrote:
> Set TR Dequeue code is pretty dumb and it will jump to the first TRB
> after the first cancelled TD (or something like that). If the endpoint
> may have been running when Set TR Dequeue failed, is it guaranteed that
> trying again won't rewind it to some already completed TD?

Answering my own question, rewinding back to completed TDs could only
happen if TD cancel status were TD_HALTED, but it's made TD_DIRTY here.
In such case xhci_invalidate_cancelled_tds() will perform trb_in_td()
check and find that another Set TR Deq is not necessary if the HW has
already moved past the TD we tried to get out of. (It may still issue
the command for some other TD, of course.)

BTW, if the HW completed this TD, the TD *should* have already been
given back and this patch should have no effect then. But this is not
guaranteed, because fe49df60cdb7 ("xhci: Mitigate failed set dequeue
pointer commands") turned the TD into No-Ops. (Note that the contrary
isn't guaranteed either, since the xHC is allowed to cache this TD.)

Perhaps fe49df60cdb7 isn't really right? It's not illegal to complete
an unlinked URB due to race with normal execution. Sure, the condition
discussed here is abnormal, but callers won't know the difference.

And in the ASMedia case mentioned by fe49df60cdb7:

1. The unhandled Babble Error makes no real difference. IME, after
   a TRB Error from Set TR Dequeue, the endpoint stays stuck anyway.
   Some other HCs also get stuck similarly, but without TRB Error.

2. Babble Error wasn't handled because the TD had already been given
   back earlier when Set TR Deq failed with TRB Error. Perhaps that's
   the real bug which deserves fixing?

The ASMedia TRB Error case:
https://lore.kernel.org/linux-usb/ZsjgmCjHdzck9UKd@alphanet.ch/
https://lore.kernel.org/linux-usb/20241113002658.0a031681@foxbook/

> Is it impossible that the TD we tried to skip has been given back and
> the endpoint and/or slot are being disabled right now?

It's not impossible, but if the TD is given back and no other TDs
remain then nothing more will be done, so no problem.


In general, this patch looks like an improvement over the status quo -
pretending that the command succeeded and then unconditionally giving
back the TD, even though the HW may still complete it later.

That being said,

1. I'm not aware of any known cases leading to this situation.

2. A loop which finds and updates the TD_CLEARING_CACHE item already
   exists, so I think it would be better to modify this loop instead
   of adding another one. And the loop prints some xhci_dbg(), so it
   would be nice if they showed up in this case as well.


Regards,
Michal