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 4/5] usb: xhci: Simplify the TD skipping loop further
Date: Tue, 10 Sep 2024 13:16:09 +0200 [thread overview]
Message-ID: <20240910131609.20434c3c@foxbook> (raw)
In-Reply-To: <20240910131233.409c6481@foxbook>
Now that everything not related to skipping has been removed from the
loop, its role becomes more clear. It repeatedly skips the first TD on
the ring, as long as one exists, doesn't match the event, and is isoc.
Rewrite this logic more succintly and unify the various exit paths.
The skip flag is always cleared, so do it only once (nitpick: it used
not to be cleared on non-isoc exit, no big deal). Print one universal
debug message too, and count how many TDs were skipped.
Note: the test for non-isoc URB is valuable because skip_isoc_td()
writes to URB fields which don't exist on non-isoc URBs. This avoids
memory corruption in the unlucky event of a severe HW or SW bug.
Add a helper function for getting the first TD from the ring's queue
to make the code less of an eyesore.
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
drivers/usb/host/xhci-ring.c | 61 +++++++++++++++++-------------------
1 file changed, 28 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 56b0c0e85293..db9bc7db5aac 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2591,6 +2591,15 @@ static int handle_transferless_tx_event(struct xhci_hcd *xhci, struct xhci_virt_
return 0;
}
+/*
+ * Return the first TD pending on a transfer ring, or NULL if none.
+ */
+static struct xhci_td *xhci_ring_pending_td(struct xhci_ring *ring)
+{
+ return list_first_entry_or_null(&ring->td_list,
+ struct xhci_td, td_list);
+}
+
/*
* If this function returns an error condition, it means it got a Transfer
* event with a corrupted Slot ID, Endpoint ID, or TRB DMA address.
@@ -2777,8 +2786,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
* We wait for the final IOC event, but if we get an event
* anywhere outside this TD, just give it back already.
*/
- td = list_first_entry_or_null(&ep_ring->td_list,
- struct xhci_td, td_list);
+ td = xhci_ring_pending_td(ep_ring);
if (td && td->error_mid_td && !trb_in_td(xhci, td, ep_trb_dma, false)) {
xhci_dbg(xhci, "Missing TD completion event after mid TD error\n");
@@ -2786,48 +2794,35 @@ static int handle_tx_event(struct xhci_hcd *xhci,
ep_ring->deq_seg = td->last_trb_seg;
inc_deq(xhci, ep_ring);
xhci_td_cleanup(xhci, td, ep_ring, td->status);
+
+ td = xhci_ring_pending_td(ep_ring);
}
- do {
- /* This TRB should be in the TD at the head of this ring's
- * TD list.
- */
- if (list_empty(&ep_ring->td_list)) {
- if (ep->skip) {
- ep->skip = false;
- xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
- slot_id, ep_index);
- }
- break;
- }
-
- td = list_first_entry(&ep_ring->td_list, struct xhci_td,
- td_list);
-
- /* Is this a TRB in the currently executing TD? */
+ /* Is this a TRB in the currently executing TD? */
+ if (td)
ep_seg = trb_in_td(xhci, td, ep_trb_dma, false);
+ if (ep->skip) {
+ int skipped = 0;
/*
* If ep->skip is set, it means there are missed tds on the
* endpoint ring need to take care of.
* Process them as short transfer until reach the td pointed by
* the event.
*/
- if (ep->skip) {
- if (ep_seg) {
- xhci_dbg(xhci,
- "Found td. Clear skip flag for slot %u ep %u.\n",
- slot_id, ep_index);
- ep->skip = false;
- } else {
- if (usb_endpoint_xfer_isoc(&td->urb->ep->desc))
- skip_isoc_td(xhci, td, ep, status);
- else
- break;
- }
- }
- } while (ep->skip);
+ while (td && !ep_seg &&
+ usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
+ skip_isoc_td(xhci, td, ep, status);
+ td = xhci_ring_pending_td(ep_ring);
+ if (td)
+ ep_seg = trb_in_td(xhci, td, ep_trb_dma, false);
+ skipped++;
+ }
+ ep->skip = false;
+ xhci_dbg(xhci, "Skipped %d isoc TDs, TD %sfound, clear skip flag for slot %u ep %u\n",
+ skipped, td ? "":"not ", slot_id, ep_index);
+ }
if (!ep_seg) {
--
2.43.0
next prev parent reply other threads:[~2024-09-10 11:16 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-10 11:12 [PATCH 0/5] Quick and effective handle_tx_event() cleanup Michal Pecio
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 ` Michal Pecio [this message]
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=20240910131609.20434c3c@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;
as well as URLs for NNTP newsgroup(s).