[PATCH 0/5] xhci: Sort out the TD skipping business

[PATCH 0/5] xhci: Sort out the TD skipping business

[Michal Pecio]

Hi,

This series is motivated by a recently found rare regression due to
my commit from last year and the solution suggested by Mathias Nyman.

https://lore.kernel.org/linux-usb/al_hchyOdPoPWKEo@spiral/

I think it's a good solution not only for this specific case, but also
in general, because the next event after Missed Service Error almost
always references some TD - exceptions are Ring Underrun, which we
have special handling for, and Stopped - Length Invalid, which would
be a rare occurrence, not currently supported anyway, and possible
to support within the proposed framework by making find_td_by_dma()
calculate accurate 'missed_tds' value while still returning NULL.

The first two patches fix bugs, because I found another obscure one.
The next two patches prepare for the last one by simplifying things.

The last patch implements the big change, the whole matching/skipping
loop is replaced with a more straightforward and robust version. New
functionality is paid for with a net increase of 4 LOC, not too bad.

I gave this a bit of testing and it seems to be working, including
weird cases like: Missed Service Error retires a waiting TD with
error_mid_td, then skipping is triggered by another Transaction Error
immediately afterwards, and it turns out that two TDs were missed.

[ 1665.224921] xhci_hcd 0000:0a:00.0: Transfer error for slot 1 ep 2 on endpoint
[ 1665.225151] xhci_hcd 0000:0a:00.0: Missed Service Error for slot 1 ep 2, skip 1, try now 0
[ 1665.225156] xhci_hcd 0000:0a:00.0: Missing TD completion event after mid TD error
[ 1665.225305] xhci_hcd 0000:0a:00.0: Transfer error for slot 1 ep 2 on endpoint
[ 1665.225308] xhci_hcd 0000:0a:00.0: Skipped 2 TDs on slot 1 ep 2 comp_code 4, TD found 1, skip flag 0

Additional testing of patch 1 in isolation would be appreciated from
the reporter of the regression (Cc). That patch would go to v6.18.

Regards,
Michal

Michal Pecio (5):
  usb: xhci: Handle bogus TRB pointers in Missed Service Error events
  usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun
  usb: xhci: Don't set the skip flag on non-isoc endpoints
  usb: xhci: Shorten the TD skipping loop
  usb: xhci: Rework and improve the TD matching and skipping logic

 drivers/usb/host/xhci-ring.c | 200 ++++++++++++++++++-----------------
 1 file changed, 102 insertions(+), 98 deletions(-)

-- 
2.48.1

[PATCH 1/5] usb: xhci: Handle bogus TRB pointers in Missed Service Error events

[Michal Pecio]

xHCI 1.0 allowed these pointers to be zero. Some Intel chipsets from the
era usually set it to zero, but sometimes (apparently) to the next TRB
after the one referenced by the previous transfer event on the endpoint.

Usually that's indeed the missed TD, but it may also be the last TRB of
a two-TRB TD already completed with Short Packet on its first TRB. Then
the driver skips all pending TDs, failing to find a match.

When handling Missed Service Error, scan TD list twice and only really
skip TDs in the second pass if the first pass found a match. This won't
catch bogus pointers to wrong TDs, but such a bug would be practically
impossible to detect automatically and isn't known to exist.

Reported-by: Bart Nagel <bart@tremby.net>
Closes: https://lore.kernel.org/linux-usb/al_hchyOdPoPWKEo@spiral/
Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Fixes: d0b619599e52 ("usb: xhci: Expedite skipping missed isoch TDs on modern HCs")
Cc: stable@vger.kernel.org
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index dfe42822dde5..38a0f895553a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2605,6 +2605,17 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci,
 	}
 }
 
+static struct xhci_td *find_td_by_dma(struct xhci_ring *ep_ring, dma_addr_t dma)
+{
+	struct xhci_td *td;
+
+	if (dma)
+		list_for_each_entry(td, &ep_ring->td_list, td_list)
+			if (trb_in_td(td, dma))
+				return td;
+	return NULL;
+}
+
 /*
  * 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.
@@ -2799,8 +2810,11 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		xhci_dequeue_td(xhci, td, ep_ring, td->status);
 	}
 
-	/* If the TRB pointer is NULL, missed TDs will be skipped on the next event */
-	if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb_dma)
+	/*
+	 * We don't know how many TDs were missed when ep_trb_dma is zero (as permitted by
+	 * xHCI 1.0) or bogus. Bail out leaving ep->skip set, next event will sort it out.
+	 */
+	if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !find_td_by_dma(ep_ring, ep_trb_dma))
 		return 0;
 
 	if (list_empty(&ep_ring->td_list)) {
-- 
2.48.1

Re: [PATCH 1/5] usb: xhci: Handle bogus TRB pointers in Missed Service Error events

[Mathias Nyman]

On 8/4/26 13:02, Michal Pecio wrote:
> xHCI 1.0 allowed these pointers to be zero. Some Intel chipsets from the
> era usually set it to zero, but sometimes (apparently) to the next TRB
> after the one referenced by the previous transfer event on the endpoint.
> 
> Usually that's indeed the missed TD, but it may also be the last TRB of
> a two-TRB TD already completed with Short Packet on its first TRB. Then
> the driver skips all pending TDs, failing to find a match.
> 
> When handling Missed Service Error, scan TD list twice and only really
> skip TDs in the second pass if the first pass found a match. This won't
> catch bogus pointers to wrong TDs, but such a bug would be practically
> impossible to detect automatically and isn't known to exist.
> 
> Reported-by: Bart Nagel <bart@tremby.net>
> Closes: https://lore.kernel.org/linux-usb/al_hchyOdPoPWKEo@spiral/
> Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Fixes: d0b619599e52 ("usb: xhci: Expedite skipping missed isoch TDs on modern HCs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---

Thanks, I'll send this [PATCH 1/5] forward to 7.3 with rest of patches in for-usb-next

-Mathias

[PATCH 2/5] usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun

[Michal Pecio]

In this case we know that the xHC has released ownership of all missed
TDs, we only don't know which were missed and which were queued later.

URBs are queued atomically, so we can safely give back all TDs of the
currently executing URB. Unlike the previous policy, this does actually
ensure that the class driver will learn about the error and won't see
all of its URBs still in progress when all TDs are missed on xHCI 1.0.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 38a0f895553a..8eed56b72c30 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2630,6 +2630,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	unsigned int slot_id;
 	int ep_index;
 	struct xhci_td *td = NULL;
+	struct urb *missed_urb = NULL;
 	dma_addr_t ep_trb_dma;
 	union xhci_trb *ep_trb;
 	int status = -EINPROGRESS;
@@ -2849,26 +2850,31 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 					return 0;
 
 				/*
-				 * TD was missed, skip it. Core already initialized frame->status
-				 * to -EXDEV and frame->actual_length to 0, nothing more to do.
+				 * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB
+				 * pointer is zero again. All missed TDs can be given back, but we
+				 * don't know which were missed and which were queued after the xrun
+				 * occurred. We can safely give back the first pending URB.
 				 */
-				xhci_dequeue_td(xhci, td, ep_ring, 0);
+				if (ring_xrun_event) {
+					if (!missed_urb)
+						missed_urb = td->urb;
 
-				if (!list_empty(&ep_ring->td_list)) {
-					if (ring_xrun_event) {
-						/*
-						 * If we are here, we are on xHCI 1.0 host with no
-						 * idea how many TDs were missed or where the xrun
-						 * occurred. New TDs may have been added after the
-						 * xrun, so skip only one TD to be safe.
-						 */
-						xhci_dbg(xhci, "Skipped one TD for slot %u ep %u",
+					if (td->urb != missed_urb) {
+						xhci_dbg(xhci, "Skipped one URB for slot %u ep %u",
 								slot_id, ep_index);
 						return 0;
 					}
-					continue;
 				}
 
+				/*
+				 * TD was missed, skip it. Core already initialized frame->status
+				 * to -EXDEV and frame->actual_length to 0, nothing more to do.
+				 */
+				xhci_dequeue_td(xhci, td, ep_ring, 0);
+
+				if (!list_empty(&ep_ring->td_list))
+					continue;
+
 				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
 					 slot_id, ep_index);
 				ep->skip = false;
-- 
2.48.1

[PATCH 3/5] usb: xhci: Don't set the skip flag on non-isoc endpoints

[Michal Pecio]

These events are unique to isochronous endpoints, ignore them otherwise.
Update debug messages to reflect new policies. We could also log invalid
events as errors, but it seems nobody has ever had problems with that,
so don't bother.

This allows dropping the isoc check when skipping TDs.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 8eed56b72c30..8270c63ec3bf 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2759,16 +2759,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		 * Set skip flag of the ep_ring; Complete the missed tds as
 		 * short transfer when process the ep_ring next time.
 		 */
-		ep->skip = true;
+		if (ep_ring->type == TYPE_ISOC)
+			ep->skip = true;
 		xhci_dbg(xhci,
-			 "Miss service interval error for slot %u ep %u, set skip flag%s\n",
-			 slot_id, ep_index, ep_trb_dma ? ", skip now" : "");
+			 "Missed Service Error for slot %u ep %u, skip %d, try now %d\n",
+			 slot_id, ep_index, ep->skip, !!ep_trb_dma);
 		break;
 	case COMP_NO_PING_RESPONSE_ERROR:
-		ep->skip = true;
+		if (ep_ring->type == TYPE_ISOC)
+			ep->skip = true;
 		xhci_dbg(xhci,
-			 "No Ping response error for slot %u ep %u, Skip one Isoc TD\n",
-			 slot_id, ep_index);
+			 "No Ping response error for slot %u ep %u, skip %d\n",
+			 slot_id, ep_index, ep->skip);
 		return 0;
 
 	case COMP_INCOMPATIBLE_DEVICE_ERROR:
@@ -2844,7 +2846,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		/* Is this TRB not part of the currently executing TD? */
 		if (!trb_in_td(td, ep_trb_dma)) {
 
-			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
+			if (ep->skip) {
 				/* this event is unlikely to match any TD, don't skip them all */
 				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
 					return 0;
-- 
2.48.1

[PATCH 4/5] usb: xhci: Shorten the TD skipping loop

[Michal Pecio]

Half of this loop is code which only executes once to deal with cases
where no TD matches the event and then it returns. This code needs not
to be in any kind of loop, so get it out.

Optimize conditionals remaining in the loop body.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 69 +++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 8270c63ec3bf..1f0cb6a701c5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2843,10 +2843,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
 				      td_list);
 
-		/* Is this TRB not part of the currently executing TD? */
-		if (!trb_in_td(td, ep_trb_dma)) {
+		if (ep->skip) {
 
-			if (ep->skip) {
+			if (!trb_in_td(td, ep_trb_dma)) {
 				/* this event is unlikely to match any TD, don't skip them all */
 				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
 					return 0;
@@ -2884,38 +2883,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				goto check_endpoint_halted;
 			}
 
-			/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
-			if (ring_xrun_event)
-				return 0;
-
-			/*
-			 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
-			 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
-			 * pointer still at the previous TRB of the current TD. The previous TRB
-			 * maybe a Link TD or the last TRB of the previous TD. The command
-			 * completion handle will take care the rest.
-			 */
-			if (trb_comp_code == COMP_STOPPED ||
-			    trb_comp_code == COMP_STOPPED_LENGTH_INVALID) {
-				return 0;
-			}
-
-			/*
-			 * Some hosts give a spurious success event after a short
-			 * transfer or error on last TRB. Ignore it.
-			 */
-			if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
-				xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
-					 &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
-				ep_ring->old_trb_comp_code = 0;
-				return 0;
-			}
-
-			/* HC is busted, give up! */
-			goto debug_finding_td;
-		}
-
-		if (ep->skip) {
 			xhci_dbg(xhci,
 				 "Found td. Clear skip flag for slot %u ep %u.\n",
 				 slot_id, ep_index);
@@ -2932,10 +2899,40 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 	ep_ring->old_trb_comp_code = trb_comp_code;
 
-	/* Get out if a TD was queued at enqueue after the xrun occurred */
+	/*
+	 * Underrun handling ends here. Any TD pointed by the event was enqueued after the event
+	 * occurred, so wait for its completion now. And it's not a bug if no such TD exists.
+	 */
 	if (ring_xrun_event)
 		return 0;
 
+	/* Handle events not referencing the current TD */
+	if (!trb_in_td(td, ep_trb_dma)) {
+		/*
+		 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
+		 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
+		 * pointer still at the previous TRB of the current TD. The previous TRB
+		 * maybe a Link TD or the last TRB of the previous TD. The command
+		 * completion handle will take care the rest.
+		 */
+		if (trb_comp_code == COMP_STOPPED || trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
+			return 0;
+
+		/*
+		 * Some hosts give a spurious success event after a short
+		 * transfer or error on last TRB. Ignore it.
+		 */
+		if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
+			xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
+					&ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
+			ep_ring->old_trb_comp_code = 0;
+			return 0;
+		}
+
+		/* HC is busted, give up! */
+		goto debug_finding_td;
+	}
+
 	trace_xhci_handle_transfer(ep_ring, (struct xhci_generic_trb *) ep_trb, ep_trb_dma);
 
 	/*
-- 
2.48.1

Re: [PATCH 4/5] usb: xhci: Shorten the TD skipping loop

[Mathias Nyman]

On 8/4/26 13:04, Michal Pecio wrote:
> Half of this loop is code which only executes once to deal with cases
> where no TD matches the event and then it returns. This code needs not
> to be in any kind of loop, so get it out.
> 
> Optimize conditionals remaining in the loop body.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>

Much needed refactoring

Thanks
-Mathias

[PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic

[Michal Pecio]

Matching events with TDs and giving back missed TDs is carried out
by a complicated loop. Replace it with a simpler linear logic:

0. Having verified that 'td_list' isn't empty,
1. Scan it to find the matching TD and count missed TDs,
2. Perform necessary adjustments for corner cases,
3. Give back missed TDs, if applicable, using a short and tidy loop,
4. Check if the event refers to the expected TD and proceed as usual.

Besides cleaning up the code, this provides a few improvements:
- when the skip flag is set, no TD is given back unless we found a match
  or otherwise know how many TDs should be given back
- when the skip flag is clear, we know if the event refers to a "future"
  TD so we can log this in the Scary Error Message to aid debugging.

While altering the error message, drop a pointless goto.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 137 ++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 76 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1f0cb6a701c5..d94146ceb178 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -125,11 +125,16 @@ static bool link_trb_toggles_cycle(union xhci_trb *trb)
 	return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
 }
 
-static bool last_td_in_urb(struct xhci_td *td)
+static int num_tds_not_done(struct urb *urb)
 {
-	struct urb_priv *urb_priv = td->urb->hcpriv;
+	struct urb_priv *urb_priv = urb->hcpriv;
 
-	return urb_priv->num_tds_done == urb_priv->num_tds;
+	return urb_priv->num_tds - urb_priv->num_tds_done;
+}
+
+static bool last_td_in_urb(struct xhci_td *td)
+{
+	return !num_tds_not_done(td->urb);
 }
 
 static bool unhandled_event_trb(struct xhci_ring *ring)
@@ -2605,14 +2610,19 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci,
 	}
 }
 
-static struct xhci_td *find_td_by_dma(struct xhci_ring *ep_ring, dma_addr_t dma)
+static struct xhci_td *find_td_by_dma(struct xhci_ring *ep_ring, int *missed_tds, dma_addr_t dma)
 {
 	struct xhci_td *td;
 
-	if (dma)
+	if (dma) {
 		list_for_each_entry(td, &ep_ring->td_list, td_list)
 			if (trb_in_td(td, dma))
 				return td;
+			else
+				(*missed_tds)++;
+	}
+
+	*missed_tds = 0;
 	return NULL;
 }
 
@@ -2629,8 +2639,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	struct xhci_ring *ep_ring;
 	unsigned int slot_id;
 	int ep_index;
-	struct xhci_td *td = NULL;
-	struct urb *missed_urb = NULL;
+	struct xhci_td *td;
+	int missed_tds = 0;
 	dma_addr_t ep_trb_dma;
 	union xhci_trb *ep_trb;
 	int status = -EINPROGRESS;
@@ -2813,13 +2823,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		xhci_dequeue_td(xhci, td, ep_ring, td->status);
 	}
 
-	/*
-	 * We don't know how many TDs were missed when ep_trb_dma is zero (as permitted by
-	 * xHCI 1.0) or bogus. Bail out leaving ep->skip set, next event will sort it out.
-	 */
-	if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !find_td_by_dma(ep_ring, ep_trb_dma))
-		return 0;
-
 	if (list_empty(&ep_ring->td_list)) {
 		/*
 		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
@@ -2839,63 +2842,47 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		goto check_endpoint_halted;
 	}
 
-	do {
-		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
-				      td_list);
-
-		if (ep->skip) {
-
-			if (!trb_in_td(td, ep_trb_dma)) {
-				/* this event is unlikely to match any TD, don't skip them all */
-				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
-					return 0;
-
-				/*
-				 * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB
-				 * pointer is zero again. All missed TDs can be given back, but we
-				 * don't know which were missed and which were queued after the xrun
-				 * occurred. We can safely give back the first pending URB.
-				 */
-				if (ring_xrun_event) {
-					if (!missed_urb)
-						missed_urb = td->urb;
-
-					if (td->urb != missed_urb) {
-						xhci_dbg(xhci, "Skipped one URB for slot %u ep %u",
-								slot_id, ep_index);
-						return 0;
-					}
-				}
-
-				/*
-				 * TD was missed, skip it. Core already initialized frame->status
-				 * to -EXDEV and frame->actual_length to 0, nothing more to do.
-				 */
-				xhci_dequeue_td(xhci, td, ep_ring, 0);
+	td = find_td_by_dma(ep_ring, &missed_tds, ep_trb_dma);
 
-				if (!list_empty(&ep_ring->td_list))
-					continue;
+	if (ep->skip) {
+		if (!td) {
+			/*
+			 * xHCI 1.0 allowed MSE events to have zero TRB pointers. Some old chips
+			 * also generate bogus non-zero pointers. We know, don't bother warning.
+			 * Missed TDs will be given back by the next event with a valid pointer.
+			 */
+			if (trb_comp_code == COMP_MISSED_SERVICE_ERROR &&
+			    xhci->hci_version <= 0x100)
+				return 0;
+			/*
+			 * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB pointer
+			 * is zero again. All missed TDs can be given back, but we don't know which
+			 * were missed and which were queued after the xrun occurred. We can safely
+			 * give back the first pending URB to let the class driver know.
+			 */
+			if (ring_xrun_event)
+				missed_tds = num_tds_not_done(list_first_entry(&ep_ring->td_list,
+								struct xhci_td, td_list)->urb);
+			/* In other cases missed_tds is zero */
+		}
 
-				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
-					 slot_id, ep_index);
-				ep->skip = false;
-				td = NULL;
-				goto check_endpoint_halted;
-			}
+		/*
+		 * Give back missed TDs. Core already initialized their frame->status to -EXDEV
+		 * and frame->actual_length to 0, nothing more to do.
+		 */
+		for (int i = 0; i < missed_tds; i++)
+			xhci_dequeue_td(xhci,
+				list_first_entry(&ep_ring->td_list, struct xhci_td, td_list),
+				ep_ring, 0);
 
-			xhci_dbg(xhci,
-				 "Found td. Clear skip flag for slot %u ep %u.\n",
-				 slot_id, ep_index);
+		/* the list may become empty on ring_xrun_event */
+		if (td || list_empty(&ep_ring->td_list))
 			ep->skip = false;
-		}
 
-	/*
-	 * 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.
-	 */
-	} while (ep->skip);
+		xhci_dbg(xhci, "Skipped %d TDs on slot %u ep %u comp_code %u, TD found %d, skip flag %d\n",
+				missed_tds, slot_id, ep_index, trb_comp_code, !!td, ep->skip);
+		missed_tds = 0;
+	}
 
 	ep_ring->old_trb_comp_code = trb_comp_code;
 
@@ -2907,7 +2894,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		return 0;
 
 	/* Handle events not referencing the current TD */
-	if (!trb_in_td(td, ep_trb_dma)) {
+	if (!td || missed_tds) {
 		/*
 		 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
 		 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
@@ -2930,7 +2917,13 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		}
 
 		/* HC is busted, give up! */
-		goto debug_finding_td;
+		td = list_first_entry(&ep_ring->td_list, struct xhci_td, td_list);
+		xhci_err(xhci, "Event dma %pad for ep %d comp_code %u not part of TD at %016llx - %016llx, missed %d\n",
+				&ep_trb_dma, ep_index, trb_comp_code,
+				(u64)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
+				(u64)xhci_trb_virt_to_dma(td->end_seg, td->end_trb),
+				missed_tds);
+		return -ESHUTDOWN;
 	}
 
 	trace_xhci_handle_transfer(ep_ring, (struct xhci_generic_trb *) ep_trb, ep_trb_dma);
@@ -2962,14 +2955,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 	return 0;
 
-debug_finding_td:
-	xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
-		 &ep_trb_dma, ep_index, trb_comp_code,
-		 (unsigned long long)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
-		 (unsigned long long)xhci_trb_virt_to_dma(td->end_seg, td->end_trb));
-
-	return -ESHUTDOWN;
-
 err_out:
 	xhci_err(xhci, "@%016llx %08x %08x %08x %08x\n",
 		 (unsigned long long) xhci_trb_virt_to_dma(
-- 
2.48.1

Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic

[Mathias Nyman]

On 8/4/26 13:05, Michal Pecio wrote:
> Matching events with TDs and giving back missed TDs is carried out
> by a complicated loop. Replace it with a simpler linear logic:
> 
> 0. Having verified that 'td_list' isn't empty,
> 1. Scan it to find the matching TD and count missed TDs,
> 2. Perform necessary adjustments for corner cases,
> 3. Give back missed TDs, if applicable, using a short and tidy loop,
> 4. Check if the event refers to the expected TD and proceed as usual.
> 
> Besides cleaning up the code, this provides a few improvements:
> - when the skip flag is set, no TD is given back unless we found a match
>    or otherwise know how many TDs should be given back
> - when the skip flag is clear, we know if the event refers to a "future"
>    TD so we can log this in the Scary Error Message to aid debugging.
> 
> While altering the error message, drop a pointless goto.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>

How about modifying step 1 a bit and store the last passed td instead of
count missed tds?

If the event points to a valid trb ahead of last trb in td, but
before the enqueue pointer, then we know hardware has passed this td and we
can give it back.

This should work even if event trb points to a link trb or no-op trb.

It should also cover the "td->error_mid_td && !trb_in_td(td, ep_trb_dma))" case

something like:

static struct xhci_td *find_td_by_dma(struct xhci_ring *ring, struct xhci_td **passed_td, dma_addr_t dma)
{
	struct xhci_td *td;

	if (!dma)
		return NULL;

	list_for_each_entry(td, &ring->td_list, td_list) {
		if (trb_in_td(td, dma))
			return td;
		/* event points to a valid trb passed this td */
		else if (dma_in_range(dma, td->end_seg, td->end_trb,
				      ring->enq_seg, ring->enqueue))
			*passed_td = td;
	}

	return NULL;
}

static int handle_tx_event(struct xhci_hcd *xhci,
			   struct xhci_interrupter *ir,
			   struct xhci_transfer_event *event)
{
	...
	struct xhci_td *passed_td = NULL;

	...
	td = find_td_by_dma(ep_ring, &passed_td, ep_trb_dma);

	if (passed_td) {
		struct xhci_td *tmp_td;
		list_for_each_entry_safe(td, tmp_td, &ep_ring->td_list, td_list)
		{
			xhci_dequeue_td(xhci, td, ep_ring, td->status);
			if (td == passed_td)
				break;
		}
	}

-Mathias

Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic

[Michal Pecio]

On Wed, 5 Aug 2026 20:39:12 +0300, Mathias Nyman wrote:
> On 8/4/26 13:05, Michal Pecio wrote:
> > Matching events with TDs and giving back missed TDs is carried out
> > by a complicated loop. Replace it with a simpler linear logic:
> > 
> > 0. Having verified that 'td_list' isn't empty,
> > 1. Scan it to find the matching TD and count missed TDs,
> > 2. Perform necessary adjustments for corner cases,
> > 3. Give back missed TDs, if applicable, using a short and tidy loop,
> > 4. Check if the event refers to the expected TD and proceed as usual.
> > 
> > Besides cleaning up the code, this provides a few improvements:
> > - when the skip flag is set, no TD is given back unless we found a match
> >    or otherwise know how many TDs should be given back
> > - when the skip flag is clear, we know if the event refers to a "future"
> >    TD so we can log this in the Scary Error Message to aid debugging.
> > 
> > While altering the error message, drop a pointless goto.
> > 
> > Signed-off-by: Michal Pecio <michal.pecio@gmail.com>  
> 
> How about modifying step 1 a bit and store the last passed td instead
> of count missed tds?

End result would be about the same, but I think counting results in
the simplest possible giveback loop. And I can easily log the number
of dropped TDs, which is something I was sometimes curious to know.

> If the event points to a valid trb ahead of last trb in td, but
> before the enqueue pointer, then we know hardware has passed this td
> and we can give it back.

I mentioned possibility of such change in the cover letter, but kept
it out of this patch, because it's something that has never been done
before and, like "Expedite skipping missed TDs on modern hosts", it
could bring unwanted side effects on less than perfect HW.

For the record, I know that ASMedia can sporadically generate Stopped
events with semi-random, bogus TRB pointers, at least with streams:

[ 4541.443642] xhci_hcd 0000:02:00.0: Transfer event 26 for unknown stream ring slot 5 ep 6

But not sure if with streams only. Without streams, the driver has no
trouble finding the transfer ring and silently ignores bogus Stopped
events. If they (hypothetically) accidentally match some TD, the only
result is updating urb->actual_length (which may even get fixed later
if the TD receives a proper event).

> This should work even if event trb points to a link trb or no-op trb.

Counting could do it too. I consider the two above issues separate.

> It should also cover the "td->error_mid_td && !trb_in_td(td, ep_trb_dma))" case

Now, that's a third separate issue :)

I had this idea that isoc error could immediately give back the TD
to minimize latency, and only keep around enough metadata (basically
td->end_trb) to recognize future events and confidently ignore them.
I even sent a patch, but it was dropped due to last minute conflicts.

> something like:
> 
> static struct xhci_td *find_td_by_dma(struct xhci_ring *ring, struct xhci_td **passed_td, dma_addr_t dma)
> {
> 	struct xhci_td *td;
> 
> 	if (!dma)
> 		return NULL;
> 
> 	list_for_each_entry(td, &ring->td_list, td_list) {
> 		if (trb_in_td(td, dma))
> 			return td;
> 		/* event points to a valid trb passed this td */
> 		else if (dma_in_range(dma, td->end_seg, td->end_trb,
> 				      ring->enq_seg, ring->enqueue))
> 			*passed_td = td;

This could equally well be (*missed_tds)++.

> 	}
> 
> 	return NULL;
> }
> 
> static int handle_tx_event(struct xhci_hcd *xhci,
> 			   struct xhci_interrupter *ir,
> 			   struct xhci_transfer_event *event)
> {
> 	...
> 	struct xhci_td *passed_td = NULL;
> 
> 	...
> 	td = find_td_by_dma(ep_ring, &passed_td, ep_trb_dma);
> 
> 	if (passed_td) {

This seems to be a fourth independent change - getting rid of ep->skip.
Not sure if worthwile, the cost of managing ep->skip state is just a
few LOC, and it provides some degree of sanity checking.

It would be sad if an avoidable SW bug went unnoticed until it causes
a "user impact", because the driver silently ignored obvious problems.
Tolerating bugs that don't exist seems to invite people to create them.

> 		struct xhci_td *tmp_td;
> 		list_for_each_entry_safe(td, tmp_td, &ep_ring->td_list, td_list)
> 		{
> 			xhci_dequeue_td(xhci, td, ep_ring, td->status);

On isoc, you can replace td->status with 0, it will be ignored.

Non-isoc - is dropping TDs like that a can of worms worth opening?
Also, this td->status would be assigned to urb->status, so it
could become zero, EINPROGRESS, or whatever the initial value is.

> 			if (td == passed_td)
> 				break;
> 		}
> 	}
> 
> -Mathias

Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic

[Michal Pecio]

> On Wed, 5 Aug 2026 20:39:12 +0300, Mathias Nyman wrote:
> If the event points to a valid trb ahead of last trb in td, but
> before the enqueue pointer, then we know hardware has passed this td
> and we can give it back.

Actually, we don't, because of a race condition (and driver bug).

A very long TD completes with Short Packet on the first TRB.
We give back the TD and advance ep_ring->dequeue past it (bug).
Some event is generated later in the TD but we don't know yet.
We queue a lot of TRBs and move enqueue into the completed TD.

We handle the event and it appears to be "before" enqueue. But
the HW not only hasn't passed, it hasn't even begun executing
any TD after the initially completed one.

We give them back and later get their events, or IOMMU faults.

Regards,
Michal

Re: [PATCH 5/5] usb: xhci: Rework and improve the TD matching and skipping logic

[Mathias Nyman]

On 8/6/26 14:00, Michal Pecio wrote:
>> On Wed, 5 Aug 2026 20:39:12 +0300, Mathias Nyman wrote:
>> If the event points to a valid trb ahead of last trb in td, but
>> before the enqueue pointer, then we know hardware has passed this td
>> and we can give it back.
> 
> Actually, we don't, because of a race condition (and driver bug).
> 
> A very long TD completes with Short Packet on the first TRB.
> We give back the TD and advance ep_ring->dequeue past it (bug).
> Some event is generated later in the TD but we don't know yet.
> We queue a lot of TRBs and move enqueue into the completed TD.
> 
> We handle the event and it appears to be "before" enqueue. But
> the HW not only hasn't passed, it hasn't even begun executing
> any TD after the initially completed one.
> 
> We give them back and later get their events, or IOMMU faults.
> 

This isn't a realistic scenario.

Ring expansion needs to fail and class driver need to fill entire
ring buffer in the extremely short time between the short transfer
event mid TD, and the final completion event for the same TD.

xHC isn't really processing the rest of the TD's TRBs (no data
transfer) after a short transfer event.
A second event for that TD will likely point to the last TRB as
it has the IOC flag set, and that event will come almost instantly,
probably so close that both events are handled during one interrupt
handler call.

We would already now see severe issues if this could happen.
Queuing TRBs beyond hw dequeue on a full ring would overwrite the
cycle bit and hardware would likely just stop processing any TRBs

I do support changing the software dequeue to better match hardware
dequeue.

Thanks
Mathias

Re: [PATCH 0/5] xhci: Sort out the TD skipping business

[Bart Nagel]

At 2026-08-04 12:01:10 +0200, Michal Pecio wrote:
...
> Additional testing of patch 1 in isolation would be appreciated from
> the reporter of the regression (Cc). That patch would go to v6.18.

That's me! I'm new here so please forgive me if I'm asking stupid
questions:

I found the patch applies cleanly to the linux-6.18.y branch in the
stable repo; am I in the right place?

And then am I looking for anything in particular in the logs while
running it, or do you simply want an observation on whether I can
reproduce the bug or not?

Thanks.

Re: [PATCH 0/5] xhci: Sort out the TD skipping business

[Michal Pecio]

On Wed, 5 Aug 2026 12:06:48 -0700, Bart Nagel wrote:
> At 2026-08-04 12:01:10 +0200, Michal Pecio wrote:
> ...
> > Additional testing of patch 1 in isolation would be appreciated from
> > the reporter of the regression (Cc). That patch would go to v6.18.  
> 
> That's me! I'm new here so please forgive me if I'm asking stupid
> questions:
> 
> I found the patch applies cleanly to the linux-6.18.y branch in the
> stable repo; am I in the right place?

That's fine, there were hardly any changes in this area this year
so 6.18 should be about equivalent to any 7.x. And it's the only one
of them which will stay supported (here) for a few years.

> And then am I looking for anything in particular in the logs while
> running it, or do you simply want an observation on whether I can
> reproduce the bug or not?

At this point just see if it works. These patches don't affect HW
behavior, they only ignore bogus events to avoid creating problems.
And this patch should ignore every case ignored by the old one.

Regard,
Michal

Re: [PATCH 0/5] xhci: Sort out the TD skipping business

[Bart Nagel]

At 2026-08-05 22:21:20 +0200, Michal Pecio wrote:
> On Wed, 5 Aug 2026 12:06:48 -0700, Bart Nagel wrote:
> > At 2026-08-04 12:01:10 +0200, Michal Pecio wrote:
> > ...
> > > Additional testing of patch 1 in isolation would be appreciated from
> > > the reporter of the regression (Cc). That patch would go to v6.18.  
> > 
> > That's me! I'm new here so please forgive me if I'm asking stupid
> > questions:
> > 
> > I found the patch applies cleanly to the linux-6.18.y branch in the
> > stable repo; am I in the right place?
> 
> That's fine, there were hardly any changes in this area this year
> so 6.18 should be about equivalent to any 7.x. And it's the only one
> of them which will stay supported (here) for a few years.
> 
> > And then am I looking for anything in particular in the logs while
> > running it, or do you simply want an observation on whether I can
> > reproduce the bug or not?
> 
> At this point just see if it works. These patches don't affect HW
> behavior, they only ignore bogus events to avoid creating problems.
> And this patch should ignore every case ignored by the old one.

OK, I ran it with high system load (which as discussed earlier made
the bug pop up more readily) for a little over 12 hours, and no
freeze.

In case logs are interesting, they are temporarily (~48h) here:

dmesg log with handle_tx_event, but with "spurious event" messages
filtered out otherwise the file is 400 times the size:
https://tmpfiles.org/wvw5Fxo00DQt/dmesg-6.18.42-patch-2026-08-04-no-spurious.txt

Filtered debugfs trace as directed by Michal earlier in the process:
https://tmpfiles.org/w2wTFcoQ02zV/trace-6.18.42-patch-2026-08-04.txt

Thanks for all your work.