[RTF,v3] xhci: process isoc TD properly when there was an error mid TD.

The last TRB of a isoc TD might not trigger an event if there was
an error event for a TRB mid TD. This is seen on a NEC Corporation
uPD720200 USB 3.0 Host

After an error mid a multi-TRB TD the xHC should according to xhci 4.9.1
generate events for passed TRBs with IOC flag set if it proceeds to the
next TD. This event is either a copy of the original error, or a
"success" transfer event.

If that event is missing then the driver and xHC host get out of sync as
the driver is still expecting a transfer event for that first TD, while
xHC host is already sending events for the next TD in the list.
This leads to
"Transfer event TRB DMA ptr not part of current TD" messages.

As a solution we tag the isoc TDs that get error events mid TD.
If an event doesn't match the first TD, then check if the tag is
set, and event points to the next TD.
In that case give back the fist TD and process the next TD normally

Make sure TD status and transferred length stay valid in both cases
with and without final TD completion event.

Reported-by: Michał Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
  v2 -> v3
  - Rework and fix setting TD transfer lengt issue in v2
  - Minor commit message tuning 

  v1 -> v2:
  - Check for empty TD list, suggested by Michał
  - Don't overwrite error status, suggested by Michał
  - Add a debug print for missed TD completion event, suggested by Michał
  - Reword commit message and code comments
  - Set correct TD transfer length in isoc error case


drivers/usb/host/xhci-ring.c | 74 +++++++++++++++++++++++++++++-------
 drivers/usb/host/xhci.h      |  1 +
 2 files changed, 61 insertions(+), 14 deletions(-)

Usual tests passed (but only transaction errors are covered).


I noticed that with your new fix to the frame length bug, error_mid_td
always equals urb_length_set. So the new flag is perhaps not necessary
after all. The test in handle_tx_event() could be:

if (usb_pipeisoc(td->urb->pipe) && td->urb_length_set &&
    !list_is_last(...  ))

I tried it and it works just as well.


While disconnecting my test camera from the VIA, I got this:

[95511.647441] xhci_hcd 0000:03:00.0: finishing TD with status -75 (comp_code 3 error_mid_td 0)
[95511.647453] xhci_hcd 0000:03:00.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 1
[95511.647457] xhci_hcd 0000:03:00.0: Looking for event-dma 0000000135121710 trb-start 0000000135121720 trb-end 0000000135121720 seg-start 0000000135121000 seg-end 0000000135121ff0
... followed by some ordinary transaction errors 2 milliseconds later.

Apparently a babble error, and it seems to have generated a "success"
which the event handler tried to match with the next TD. So a mid TD
babble may need the same treatment, which is not surprising.

Unfortunately I didn't print enough debug info to confirm this with
certainty and it is absolutely unreproducible, I have no idea why it
happened.

On 19.1.2024 23.54, Michał Pecio wrote:
> Usual tests passed (but only transaction errors are covered).

Thanks for testing, I'l add your a tested-by tag to the patch.

> 
> 
> I noticed that with your new fix to the frame length bug, error_mid_td
> always equals urb_length_set. So the new flag is perhaps not necessary
> after all. The test in handle_tx_event() could be:
> 
> if (usb_pipeisoc(td->urb->pipe) && td->urb_length_set &&
>      !list_is_last(...  ))
> 
> I tried it and it works just as well.

This is probably true here, but urb_length_set and error_mid_td have a bit
different idea behind them

For example short control transfers may send an event mid TD (during data stage).
There is no error, we wait for the completion of the status stage but don't want
to change the transfer length set in the urb.

I suspect we will need to play with both flags for other error cases in the future.

Thanks
Mathias

> I'm afraid we end up tuning that original patch forever
Point taken. This patch is more important, as it fixes total failure
to deal with trivial bit errors on uPD720200.


> Isoc endpoints should use COMP_ISOCH_BUFFER_OVERRUN to indicate
> endpoint hasn't halted like it does in the COMP_BABBLE_DETECTED_ERROR
> case.
> 
> See xhci 6.4.5 "TRB completion codes" Footnote 115
This applies to "TD Babble", which is:
submitted TD size < data from the device <= ESIT payload

Other babble conditions exist related to blatant device bugs (4.10.2.4)
and it looks like some may result in a halt or device disconnection even
on isoch endpoints(?)

Other babbles aren't included in "isoch buffer overrun" per 6.4.5.

I feel like one would need to systematically go through the whole spec
to avoid missing important details or special cases.


Thanks,
Michal