public inbox for linux-usb@vger.kernel.org
 help / color / mirror / Atom feed
From: Michal Pecio <michal.pecio@gmail.com>
To: Mathias Nyman <mathias.nyman@intel.com>,
	Niklas Neronin <niklas.neronin@linux.intel.com>
Cc: linux-usb@vger.kernel.org
Subject: [PATCH 0/5] Quick and effective handle_tx_event() cleanup
Date: Tue, 10 Sep 2024 13:12:33 +0200	[thread overview]
Message-ID: <20240910131233.409c6481@foxbook> (raw)

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

             reply	other threads:[~2024-09-10 11:12 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-10 11:12 Michal Pecio [this message]
2024-09-10 11:13 ` [PATCH 1/5] usb: xhci: Fix handling errors mid TD followed by other errors Michal Pecio
2024-09-10 11:14 ` [PATCH 2/5] usb: xhci: Clean up the TD skipping loop Michal Pecio
2024-09-10 11:15 ` [PATCH 3/5] usb: xhci: Unify event handler's 'empty list' and 'no match' cases Michal Pecio
2024-09-10 11:16 ` [PATCH 4/5] usb: xhci: Simplify the TD skipping loop further Michal Pecio
2024-09-10 11:16 ` [PATCH 5/5] usb: xhci: Fix Ring Underrun/Overrun handling Michal Pecio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240910131233.409c6481@foxbook \
    --to=michal.pecio@gmail.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=niklas.neronin@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox