[0/5] Quick and effective handle_tx_event() cleanup

Message

Michał Pecio Sept. 10, 2024, 11:12 a.m. UTC

Hi,

I mentioned that I came up with a simple way to clean up the messy
(and buggy) event handling loop. Here are the patches.

Total line count is reduced by 26 and the sole loop remaining has no
breaks or continues and 8 lines of code. Some functionally duplicate
code is merged into one path. No functional change besides bugfixes.

Six defects identified by code review are resolved along the way.
I successfully reproduced #1, and #4 was seen in the wild on linux-usb.

1. Error mid TD followed by Missed Service is misreported as missed.
2. If EP stops on the next TD after error-mid-TD, neither TD is handled.
3. Empty list quiety ignored after short TD on hosts without the quirk.
4. Emergency stall recovery not attempted after "TRB not part of TD".
5. A race could prematurely complete a TD after an isoch ring underrun.
6. Error-mid-TD transfer on buggy HC is stuck forever if it's the last.

Debugging of TD skipping is improved - we know if/how many TDs were
skipped, in addition to whether a match was found or not. This enables
quickly catching cases when suspiciously many TDs are skipped (I have
seen a case of 150 skipped TDs, turned out to be a HW bug.)

The event handling process becomes linear - check a condition, handle
something, check for another condition, handle it, and so on. This is
much easier to reason about and to modify.

To demonstrate this point, patch 5/5 fixes a pair of stupid issues by
inserting one check, which would be duplicated 3 times before cleanup.

This series should be a good base for future work to resolve remaining
bugs. For example, the skipping loop could change from (simplified):

    while (td && !trb_in_td(td, ep_trb_dma))
to
    while (td && trb_after_td(td, ep_trb))

subject to providing a working implementation of trb_after_td(). I have
tested three implementations, some based on pre-scanning the list and
some on direct comparison, but I'm not 100% happy with any so far.

Mathias had a clever idea to use ring segment numbers for this. I tried
and it compiled and worked flawlessly on the first go, but it requires
passing all those seg pointers and total ring size around. This happens
to complicate sharing implementation with trb_in_td(), because users of
the latter don't currently provide such information. And I would like
to share implementation of these functions, as they are very similar.

Regards,
Michal