Various xhci fixes and improvements

Hi,

Here's a handful of bug fixes, cleanups and improvements for the xHCI driver.

The first is a trivial fix, the second also fixes a bug albeit a less trivial one. These two maybe deserve to go into usb-linus, the latter perhaps to stable as well, but the patch doesn't apply as-is.

The rest are new functionality or code cleanups with low anticipated user impact.

All patches were applied and tested on v6.11-rc5, all compiled cleanly and worked without regressions with some HID, storage, audio, video. Functional changes received additional functional testing described in their respective changelogs.

Notably missing is a solution to the "Underrun after Missed Service" problem. To recap, Underrun/Overrun typically has a NULL TRB pointer and ep_ring->td_list contains some missed TDs and possibly a few that have been added after the underrun occured but before we got the IRQ. No way to tell them apart, at least by the spec, as far as I see.

On USB 3.1+ hosts we can get out of it with "expedited skipping" - it ensures that the ring is never left with stale TDs in the first place.

On USB 3.0 hosts the underrun handler *will* sometimes face skipping and it needs to make a decision.

Currently, it skips everything. This may cause DMA-after-free - not great on IN endpoints - and "TRB DMA ptr not part of current TD" or "WARN Event TRB with no TDs queued" later.

The obvious alterantive is to never skip on empty ring events, but it can deadlock a driver which waits for its URBs to be given back when all of them were missed and we can't get a valid dequeue from the HC.

I wonder if it would make sense to always skip the first queued TD when we get an MSE with NULL pointer? I think it's legal, and likely sufficient to avoid the stupid deadlock.

Just a last minute idea. I will think about it, but now I'd like to flush this patch queue before it grows to infinity ;)

Thanks,
Michal

xhci_dbg_trace() appends a newline to the format string, don't call it with a newline terminated string.

---
 drivers/usb/host/xhci-ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4ea2c3e072a9..e960609dcf51 100644
@@ -723,9 +723,9 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
 	ep->queued_deq_seg = new_seg;
 	ep->queued_deq_ptr = new_deq;
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-		       "Set TR Deq ptr 0x%llx, cycle %u\n", addr, new_cycle);
+		       "Set TR Deq ptr 0x%llx, cycle %u", addr, new_cycle);
 
 	/* Stop the TD queueing code from ringing the doorbell until
 	 * this command completes.  The HC won't set the dequeue pointer
 	 * if the ring is running, and ringing the doorbell starts the

2.43.0

Some host controllers fail to produce the final completion event on an isochronous TD which experienced an error mid TD. We deal with it by flagging such TDs and checking if the next event points at the flagged TD or at the next one, and completing the flagged TD if the latter.

This is not enough, because the next TD may be missed by the xHC. And we need to do the check early, or errors on the next TD may confuse us.

If the next TD experiences a Missed Service Error, we will set the skip flag on the endpoint and then attempt skipping TDs when yet another event arrives. In such scenario, we ought to report the 'erorr mid TD' transfer as such rather than skip it. We also need to detect that it is there and in need of handling, despite never receiving a completion of the subsequent TD. For this purpose, add a check for ep->skip flag and move the whole error handling logic before the skipping logic.

Note that when we see both td->error_mid_td and ep->skip, they surely must have been set in this order. Otherwise, skip flag would have been cleared when the error mid TD was being handled.

Another problem case are Stopped Endpoint events. If we see one after an error mid TD, we naively assume that it's a Forced Stopped Event because it doesn't match the pending TD, even though it might actually be a Stopped Event on the next TD, which we fail to recognize. This is fixed similarly, by reordering erorr handling before FSE handling.

Lastly, error mid TD handling is reordered with last_td_was_short. This is harmless, because these two cases are mutually exclusive - only one can happen in any given execution of handle_tx_event().

And if we are here, add a FIXME comment on the last_td_was_short thing because it's a wholly broken idea.

Tested on the NEC host and a USB camera with flaky cable. Dynamic debug confirmed that Transaction Errors are sometimes seen, sometimes mid-TD, sometimes followed by Missed Service. In such cases, they were finished properly before skipping began.

---
 drivers/usb/host/xhci-ring.c | 78 ++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 38 deletions(-)

index e960609dcf51..401c34ff2260 100644
@@ -2798,8 +2798,39 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 		/* Is this a TRB in the currently executing TD? */
 		ep_seg = trb_in_td(xhci, td, ep_trb_dma, false);
 
+		if (!ep_seg) {
+			/*
+			 * xhci 4.10.2 states isoc endpoints should continue
+			 * processing the next TD if there was an error mid TD.
+			 * So host like NEC don't generate an event for the last
+			 * isoc TRB even if the IOC flag is set.
+			 * xhci 4.9.1 states that if there are errors in mult-TRB
+			 * TDs xHC should generate an error for that TRB, and if xHC
+			 * proceeds to the next TD it should genete an event for
+			 * any TRB with IOC flag on the way. Other host follow this.
+			 * So this event might be for the next TD.
+			 */
+			if (td->error_mid_td &&
+			    !list_is_last(&td->td_list, &ep_ring->td_list)) {
+				struct xhci_td *td_next = list_next_entry(td, td_list);
+
+				/* Is this a TRB in the next queued TD? */
+				/* *OR* did we miss some TDs meanwhile? */
+				ep_seg = trb_in_td(xhci, td_next, ep_trb_dma, false);
+				if (ep_seg || ep->skip) {
+					/* give back previous TD, start handling new */
+					xhci_dbg(xhci, "Missing TD completion event after mid TD error\n");
+					ep_ring->dequeue = td->last_trb;
+					ep_ring->deq_seg = td->last_trb_seg;
+					inc_deq(xhci, ep_ring);
+					xhci_td_cleanup(xhci, td, ep_ring, td->status);
+					td = td_next;
+				}
+			}
+		}
+
 		if (!ep_seg) {
 
 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
 				skip_isoc_td(xhci, td, ep, status);

@@ -2820,53 +2851,24 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 			/*
 			 * Some hosts give a spurious success event after a short
 			 * transfer. Ignore it.
+			 * FIXME xHCI 4.10.1.1: this should be freed now, not mid-TD
 			 */
 			if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
 			    ep_ring->last_td_was_short) {
 				ep_ring->last_td_was_short = false;
 				return 0;
 			}
 
-			/*
-			 * xhci 4.10.2 states isoc endpoints should continue
-			 * processing the next TD if there was an error mid TD.
-			 * So host like NEC don't generate an event for the last
-			 * isoc TRB even if the IOC flag is set.
-			 * xhci 4.9.1 states that if there are errors in mult-TRB
-			 * TDs xHC should generate an error for that TRB, and if xHC
-			 * proceeds to the next TD it should genete an event for
-			 * any TRB with IOC flag on the way. Other host follow this.
-			 * So this event might be for the next TD.
-			 */
-			if (td->error_mid_td &&
-			    !list_is_last(&td->td_list, &ep_ring->td_list)) {
-				struct xhci_td *td_next = list_next_entry(td, td_list);
-
-				ep_seg = trb_in_td(xhci, td_next, ep_trb_dma, false);
-				if (ep_seg) {
-					/* give back previous TD, start handling new */
-					xhci_dbg(xhci, "Missing TD completion event after mid TD error\n");
-					ep_ring->dequeue = td->last_trb;
-					ep_ring->deq_seg = td->last_trb_seg;
-					inc_deq(xhci, ep_ring);
-					xhci_td_cleanup(xhci, td, ep_ring, td->status);
-					td = td_next;
-				}
-			}
-
-			if (!ep_seg) {
-				/* HC is busted, give up! */
-				xhci_err(xhci,
-					"ERROR Transfer event TRB DMA ptr not "
-					"part of current TD ep_index %d "
-					"comp_code %u\n", ep_index,
-					trb_comp_code);
-				trb_in_td(xhci, td, ep_trb_dma, true);
-
-				return -ESHUTDOWN;
-			}
+			/* HC is busted, give up! */
+			xhci_err(xhci,
+				"ERROR Transfer event TRB DMA ptr not "
+				"part of current TD ep_index %d "
+				"comp_code %u\n", ep_index,
+				trb_comp_code);
+			trb_in_td(xhci, td, ep_trb_dma, true);
+			return -ESHUTDOWN;
 		}
 
 		if (ep->skip) {
 			xhci_dbg(xhci,

2.43.0

Recent reworks left this loop ending with:

		if (ep->skip)
			ep->skip = false;
	while (ep->skip);

which obviously cannot ever repeat. Repetition is only possible by a 'continue' statement earlier in the loop body.

Move the "tail" which only executes once out of the loop body.

Bring the cases of "Found TD" and "skip isoc td" closely together to consolidate the skipping logic in one place and improve clarity. Now this code properly controls the loop condition.

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

index 401c34ff2260..e65cc80cb285 100644
@@ -2829,63 +2829,65 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				}
 			}
 		}
 
-		if (!ep_seg) {
-
-			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
-				skip_isoc_td(xhci, td, ep, status);
-				continue;
-			}
-
-			/*
-			 * 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. Ignore it.
-			 * FIXME xHCI 4.10.1.1: this should be freed now, not mid-TD
-			 */
-			if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
-			    ep_ring->last_td_was_short) {
-				ep_ring->last_td_was_short = false;
-				return 0;
-			}
-
-			/* HC is busted, give up! */
-			xhci_err(xhci,
-				"ERROR Transfer event TRB DMA ptr not "
-				"part of current TD ep_index %d "
-				"comp_code %u\n", ep_index,
-				trb_comp_code);
-			trb_in_td(xhci, td, ep_trb_dma, true);
-			return -ESHUTDOWN;
-		}
-
+		/*
+		 * 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) {
-			xhci_dbg(xhci,
-				 "Found td. Clear skip flag for slot %u ep %u.\n",
-				 slot_id, ep_index);
-			ep->skip = false;
+			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;
+			}
 		}
 
-	/*
-	 * 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);
 
+	if (!ep_seg) {
+
+		/*
+		 * 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. Ignore it.
+		 * FIXME xHCI 4.10.1.1: this should be freed now, not mid-TD
+		 */
+		if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
+		    ep_ring->last_td_was_short) {
+			ep_ring->last_td_was_short = false;
+			return 0;
+		}
+
+		/* HC is busted, give up! */
+		xhci_err(xhci,
+			"ERROR Transfer event TRB DMA ptr not "
+			"part of current TD ep_index %d "
+			"comp_code %u\n", ep_index,
+			trb_comp_code);
+		trb_in_td(xhci, td, ep_trb_dma, true);
+		return -ESHUTDOWN;
+	}
+
 	if (trb_comp_code == COMP_SHORT_PACKET)
 		ep_ring->last_td_was_short = true;
 	else
 		ep_ring->last_td_was_short = false;

2.43.0

xHCI spec rev. 1.0 allowed the TRB pointer of Missed Service events to be NULL. In such case we have no idea which queued TD were missed and which are still waiting for execution (and the spec forbids us guessing based on frame numbers), so all we can do is set a flag on the endpoint and wait for a new event with a valid dequeue pointer.

But hosts are also allowed to give us pointer to the last missed TRB and this became mandatory in spec rev. 1.1 and later.

Use this pointer, if available, to immediately skip all missed TDs. This may be particularly helpful for applications which queue very few URBs/TDs for the sake of lowest possible latency.

It also saves the day if the next event is an underrun/overrun and we will not get a valid dequeue pointer with it. If we then saw the skip flag, we would have no way of knowing how many TDs were missed and how many were queued after the host encountered ring underrun.

Handle Missed Service Error events as "error mid TD", if applicable, because rev. 1.0 spec excplicitly says so in notes to 4.10.3.2. The notes are no longer present in later revs, but rules of 4.9.1 should apply universally.

Note that handle_tx_event() can cope with a host reporting MSE on an early TRB of a TD and then failing to signal the final TRB. However, in such (hopefully rare) case latency is not improved by this patch.

Tested on ASMedia ASM3142. This USB 3.1 host gives valid pointers in Missed Service events and the skipping loop works until it finds the last missed TRB indicated by the host. Then the skip flag is cleared and the TRB passed to process_isoc_td() like any other.

Tested on NEC and VIA VL805. These USB 3.0 hosts give NULL pointers so the event handler sets the skip flag and returns, as expected.

Change inspired by a recent discussion about realtime USB audio.

---
 drivers/usb/host/xhci-ring.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index e65cc80cb285..cc0420021683 100644
@@ -2413,8 +2413,14 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		frame->status = -EOVERFLOW;
 		if (ep_trb != td->last_trb)
 			td->error_mid_td = true;
 		break;
+	case COMP_MISSED_SERVICE_ERROR:
+		frame->status = -EXDEV;
+		sum_trbs_for_length = true;
+		if (ep_trb != td->last_trb)
+			td->error_mid_td = true;
+		break;
 	case COMP_INCOMPATIBLE_DEVICE_ERROR:
 	case COMP_STALL_ERROR:
 		frame->status = -EPROTO;
 		break;

@@ -2730,13 +2736,17 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		 * When encounter missed service error, one or more isoc tds
 		 * may be missed by xHC.
 		 * Set skip flag of the ep_ring; Complete the missed tds as
 		 * short transfer when process the ep_ring next time.
+		 * If the xHC tells us the last missed TRB (ep_trb_dma != NULL)
+		 * perform the skipping right away.
 		 */
 		ep->skip = true;
 		xhci_dbg(xhci,
-			 "Miss service interval error for slot %u ep %u, set skip flag\n",
-			 slot_id, ep_index);
+			 "Miss service interval error for slot %u ep %u, set skip flag, go ahead %d\n",
+			 slot_id, ep_index, !!ep_trb_dma);
+		if (ep_trb_dma)
+			break;
 		return 0;
 	case COMP_NO_PING_RESPONSE_ERROR:
 		ep->skip = true;
 		xhci_dbg(xhci,

2.43.0

It doesn't matter whether we set this flag or not on the final TRB, because the TD is about to be immediately "finished" anyway.

Without these checks the code looks cleaner and easier to follow.

---
 drivers/usb/host/xhci-ring.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

index cc0420021683..c777cb897579 100644
@@ -2410,26 +2410,23 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		sum_trbs_for_length = true;
 		fallthrough;
 	case COMP_ISOCH_BUFFER_OVERRUN:
 		frame->status = -EOVERFLOW;
-		if (ep_trb != td->last_trb)
-			td->error_mid_td = true;
+		td->error_mid_td = true;
 		break;
 	case COMP_MISSED_SERVICE_ERROR:
 		frame->status = -EXDEV;
 		sum_trbs_for_length = true;
-		if (ep_trb != td->last_trb)
-			td->error_mid_td = true;
+		td->error_mid_td = true;
 		break;
 	case COMP_INCOMPATIBLE_DEVICE_ERROR:
 	case COMP_STALL_ERROR:
 		frame->status = -EPROTO;
 		break;
 	case COMP_USB_TRANSACTION_ERROR:
 		frame->status = -EPROTO;
 		sum_trbs_for_length = true;
-		if (ep_trb != td->last_trb)
-			td->error_mid_td = true;
+		td->error_mid_td = true;
 		break;
 	case COMP_STOPPED:
 		sum_trbs_for_length = true;
 		break;

2.43.0

It's not spurious, it's expected, it's required by the spec since its final 1.0 revision 14 years ago, and it's handled incorrectly here. But until somebody puts some effort to get it all right, try at least not to do obviously wrong things here.

This code claims to handle "spurious" Success events, but in reality it is ready and willing to silently swallow any kind of event, on most host controllers these days, after any short transfer.

It got in my way while debugging genuinely incorrect events from the xHC, which this code thought were meant for it, because it has no way of knowing better, because it's utterly broken.

Limit it at least to only accept Success and Short Packet completions so that rightful warnings will be printed in other cases.

So far I found no instances of this change exposing previously hidden errors, besides the aforementioned case of a buggy xHC. The buggy xHC completely fails to acknowledge some TDs in any way. It proceeeds to the next TD, whose event then doesn't match the current TD, so if the prior TD got a short packet, the "spurious" code swallows the event.

---
 drivers/usb/host/xhci-ring.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index c777cb897579..e19c8a17b59c 100644
@@ -2785,9 +2785,10 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 			 */
 
 			if (!(trb_comp_code == COMP_STOPPED ||
 			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
-			      ep_ring->last_td_was_short)) {
+			      (trb_comp_code == COMP_SUCCESS && ep_ring->last_td_was_short) ||
+			      (trb_comp_code == COMP_SHORT_PACKET && ep_ring->last_td_was_short))) {
 				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
 					  slot_id, ep_index);
 			}
 			if (ep->skip) {

@@ -2878,9 +2879,11 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		 * transfer. Ignore it.
 		 * FIXME xHCI 4.10.1.1: this should be freed now, not mid-TD
 		 */
 		if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
-		    ep_ring->last_td_was_short) {
+		    ep_ring->last_td_was_short &&
+		    (trb_comp_code == COMP_SUCCESS ||
+		    trb_comp_code == COMP_SHORT_PACKET)) {
 			ep_ring->last_td_was_short = false;
 			return 0;
 		}
 

2.43.0

This variable normally is an endian-corrected copy of the completion code received from hardware, except for one case where it is changed in order to trick some later code into setting some flag.

This can be confusing when analyzing or debugging the function and the false completion code is sometimes written to dmesg too.

For even more confusion, functions called by handle_tx_event() also have same-named variables but they initialize them from scratch from the hardware event, undoing this change within their scope.

Use a dedicated local variable instead of such machinations.

---
 drivers/usb/host/xhci-ring.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index e19c8a17b59c..58e6d0280e00 100644
@@ -2613,8 +2613,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	union xhci_trb *ep_trb;
 	int status = -EINPROGRESS;
 	struct xhci_ep_ctx *ep_ctx;
 	u32 trb_comp_code;
+	bool short_packet = false;
 
 	slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
 	ep_index = TRB_TO_EP_ID(le32_to_cpu(event->flags)) - 1;
 	trb_comp_code = GET_COMP_CODE(le32_to_cpu(event->transfer_len));

@@ -2645,14 +2646,15 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	 * transfer type
 	 */
 	case COMP_SUCCESS:
 		if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
-			trb_comp_code = COMP_SHORT_PACKET;
+			short_packet = true;
 			xhci_dbg(xhci, "Successful completion on short TX for slot %u ep %u with last td short %d\n",
 				 slot_id, ep_index, ep_ring->last_td_was_short);
 		}
 		break;
 	case COMP_SHORT_PACKET:
+		short_packet = true;
 		break;
 	/* Completion codes for endpoint stopped state */
 	case COMP_STOPPED:
 		xhci_dbg(xhci, "Stopped on Transfer TRB for slot %u ep %u\n",

@@ -2896,12 +2898,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		trb_in_td(xhci, td, ep_trb_dma, true);
 		return -ESHUTDOWN;
 	}
 
-	if (trb_comp_code == COMP_SHORT_PACKET)
-		ep_ring->last_td_was_short = true;
-	else
-		ep_ring->last_td_was_short = false;
+	ep_ring->last_td_was_short = short_packet;
 
 	ep_trb = &ep_seg->trbs[(ep_trb_dma - ep_seg->dma) / sizeof(*ep_trb)];
 	trace_xhci_handle_transfer(ep_ring, (struct xhci_generic_trb *) ep_trb);
 

2.43.0

This gives a better picture of what was happening when the ring was found unexpectedly empty.

---
 drivers/usb/host/xhci-ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 58e6d0280e00..9d1af76955cd 100644
@@ -2789,10 +2789,10 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 			if (!(trb_comp_code == COMP_STOPPED ||
 			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
 			      (trb_comp_code == COMP_SUCCESS && ep_ring->last_td_was_short) ||
 			      (trb_comp_code == COMP_SHORT_PACKET && ep_ring->last_td_was_short))) {
-				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
-					  slot_id, ep_index);
+				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d comp_code %d with no TDs queued?\n",
+					  slot_id, ep_index, trb_comp_code);
 			}
 			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",

2.43.0

Function handle_transferless_tx_event() pretends that it sometimes deals with certain isochronous events, but it really doesn't.

All isochronous events are handled in the big switch statement of handle_tx_event(). The above function is never called on isoch EPs because xhci_dma_to_transfer_ring() never returns NULL on those.

The only conceivable way we could end up there is if an isoch EP is marked EP_HAS_STREAMS or its ring pointer is NULL, or we get an X-run event on a non-isoch EP. Either way it's a blatant bug, so don't "handle" this with a no-op, but print the default warning.

The actual meaningful work done by this function is cleaning up after various problems on streams in questionable state, so give it a more specific name and add a bit of comment.

---
 drivers/usb/host/xhci-ring.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index 9d1af76955cd..aebbdbfd8da8 100644
@@ -2566,10 +2566,14 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 
 	return finish_td(xhci, ep, ep_ring, td, trb_comp_code);
 }
 
-/* Transfer events which don't point to a transfer TRB, see xhci 4.17.4 */
-static int handle_transferless_tx_event(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
+/*
+ * Transfer events which don't point to a transfer TRB, see xhci 4.17.4
+ * Specifically, this deals with cases where the EP has streams and the event
+ * TRB pointer is NULL or otherwise doesn't point into any known stream ring.
+ */
+static int handle_bad_stream_event(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 					u32 trb_comp_code)
 {
 	switch (trb_comp_code) {
 	case COMP_STALL_ERROR:

@@ -2581,10 +2585,8 @@ static int handle_transferless_tx_event(struct xhci_hcd *xhci, struct xhci_virt_
 			xhci_handle_halted_endpoint(xhci, ep, NULL, EP_HARD_RESET);
 		else
 			xhci_handle_halted_endpoint(xhci, ep, NULL, EP_SOFT_RESET);
 		break;
-	case COMP_RING_UNDERRUN:
-	case COMP_RING_OVERRUN:
 	case COMP_STOPPED_LENGTH_INVALID:
 		break;
 	default:
 		xhci_err(xhci, "Transfer event %u for unknown stream ring slot %u ep %u\n",

@@ -2637,9 +2639,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		goto err_out;
 	}
 
 	if (!ep_ring)
-		return handle_transferless_tx_event(xhci, ep, trb_comp_code);
+		return handle_bad_stream_event(xhci, ep, trb_comp_code);
 
 	/* Look for common error cases */
 	switch (trb_comp_code) {
 	/* Skip codes that require special handling depending on

2.43.0

[...]

Hi!

I have a 6.11-rc5 tree here (tar.gz from kernel.org) and trying to apply this series I get:

$ for n in ~/Downloads/michal\ patches/20240827-\[PATCH\ *; do echo "$n"; patch --dry-run -p 1 < "$n"; done
/home/fps/Downloads/michal patches/20240827-[PATCH 1_9] usb_ xhci_ Fix double newline in a deb-148577.txt
checking file drivers/usb/host/xhci-ring.c
/home/fps/Downloads/michal patches/20240827-[PATCH 2_9] usb_ xhci_ Fix handling errors mid TD -148571.txt
checking file drivers/usb/host/xhci-ring.c
/home/fps/Downloads/michal patches/20240827-[PATCH 3_9] usb_ xhci_ Clean up the TD skipping lo-148572.txt
checking file drivers/usb/host/xhci-ring.c
Hunk #1 FAILED at 2829.
1 out of 1 hunk FAILED
/home/fps/Downloads/michal patches/20240827-[PATCH 4_9] usb_ xhci_ Expedite processing missed -148579.txt
checking file drivers/usb/host/xhci-ring.c
/home/fps/Downloads/michal patches/20240827-[PATCH 5_9] usb_ xhci_ Simplify error_mid_td marki-148573.txt
checking file drivers/usb/host/xhci-ring.c
Hunk #1 FAILED at 2410.
1 out of 1 hunk FAILED
/home/fps/Downloads/michal patches/20240827-[PATCH 6_9] usb_ xhci_ Sanity check _spurious succ-148574.txt
checking file drivers/usb/host/xhci-ring.c
Hunk #1 succeeded at 2778 (offset -7 lines).
Hunk #2 FAILED at 2879.
1 out of 2 hunks FAILED
/home/fps/Downloads/michal patches/20240827-[PATCH 7_9] usb_ xhci_ Don't lie about trb_comp_co-148575.txt
checking file drivers/usb/host/xhci-ring.c
Hunk #1 succeeded at 2610 (offset -3 lines).
Hunk #2 succeeded at 2643 (offset -3 lines).
Hunk #3 FAILED at 2898.
1 out of 3 hunks FAILED
/home/fps/Downloads/michal patches/20240827-[PATCH 8_9] usb_ xhci_ Print completion code in th-148576.txt
checking file drivers/usb/host/xhci-ring.c
Hunk #1 FAILED at 2789.
1 out of 1 hunk FAILED
/home/fps/Downloads/michal patches/20240827-[PATCH 9_9] usb_ xhci_ Clean up and rename bad str-148580.txt
checking file drivers/usb/host/xhci-ring.c
Hunk #1 succeeded at 2563 (offset -3 lines).
Hunk #2 succeeded at 2582 (offset -3 lines).
Hunk #3 succeeded at 2635 (offset -4 lines).

Are you sure these are against 6.11-rc5? Or did I screw up on my side?

Kind regards,
FPS

Absolutely for 6.11-rc5 (but -rc4 works too and -rc6 likely will).

Please try again without --dry-run, some of them will only apply on top of earlier ones because they touch the same lines. If that doesn't work then I may have screwed up mailing them.

BTW, #3 is logically dependent on #2, but the others could be reordered at will IIRC.

As for your audio case, #4 is probably the only potentially interesting one, but it didn't help on my sluggish ASMedia host. It should, however, at least eliminate the "WARN Event TRB with no TDs queued" on USB 3.1 and newer hosts.

But the root cause of those audio issues is likely elsewhere.

Regards,
Michal

Oh, of course! My bad. Sorry for the noise!

Yeah, just giving it a shot :)

Kind regards,
FPS

Thanks Michal for this series.

I need to go through the patches individually and pick fixes.

We are in the middle of reworking transfer event handling with Niklas as well.

I'd be grateful if you could take a quick look at our solution and give your opinion on it as you have expertise in this area.

Main idea is simple.
If a transfer event points to a TRB between dequeue and enqueue we give back all queued TDs passed up to this TRB.
And if the event points to a valid TD then handle that. (normal case)

Code is much simpler, we get rid of skip flag, and could probably merge error_mid_td and urb_length_set flags into one flag.

The code isn't complete or tested yet. It doesn't handle under/overruns, but that could probably be fixed by just turning "return 0" for those cases into "break"

Code on top of 6.11-rc4 can be found in my feature_transfer_event_rework branch:

git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git feature_transfer_event_rework
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_transfer_event_rework

Thanks
Mathias

Beware that I may only have enough of that to be dangerous ;)

But I also have a few of those PCIe cards with various chips and all three public revisions of the spec, so I'm trying to stay in spec and verify things on hardware.

BTW, I only really touched isochronous transfers so far, but it looks like some things are not entirely right in others too. The handling of short packets for instance - it looks like TRB_ISP is set on various transfers, but process_xxx_td() functions don't care if the event is mid-TD and give back the URB immediately.

I though about fixing this (I have a naive hope that some xHC crashes that I see are "undefined behavior" caused by driver bugs, even though in reality the hardware is probably simply FUBAR) but I'd need to read more spec chapters to get there.

Yes, lots of things could be better if the code walked the ring from the last known dequeue to the current one and analyzed what's there. We could know if the event is for something that we have already freed (a bug?), or for any known pending TD and which one exactly, or for a non-transfer TD (like Forced Stop Event).

Many of those thing are currently detected with heuristics which only work about right if everything goes as planned (no bugs in HW or SW).

And first find the TD, then worry how to handle it. I like that Niklas managed to shorten this giant loop already, and my patches #2 and #3 shorten it further and move TD selection logic up (#2) and all else down (#3). This way we can work on selection and handling separately, with less worrying about how they influence each other.

Problem is that not all events give valid dequeue pointers. These two, for example, are not guaranteed to. Then we have no idea where on the ring the HC currently is. The EP doesn't stall and it may have already been resumed by a simple doorbell ring.

Regards,
Michal

Hi Mathias,

Thanks for pointing me to this.

It does look like a very promising approach. This is the principle of "matching before handling" taken to the extreme, as nothing is changed during the first pass through the TD list. It seems to work well.

I feel like the counting of distances from ring->dequeue is a little, I don't know, unexpected maybe, but it does indeed produce surprisingly compact and simple code thanks to those segment numbers.

That being said, I've always been a strong case of NIH syndrome, so I implemented a similar search for passed TDs which relies solely on walking the ring in order from dequeue to the event DMA pointer.

So I guess we will have two implementations to compare and choose from.

I will try to develop this approach into a complete implementation that actually works and can be tested. I want to maximally preserve original behavior, except for obvious bugs (I found an obscure one already) to simplify validation and review.

So, for example, no removing the skip flag for now. I'm not sure if it's a good idea at all, as it frankly is sweeping bugs under the rug. I have one HC which sometimes silently misses TDs for no known reason and it misses so many of them that auto-skipping wouldn't help anyway. OTOH, when I get the TRB errors, at least I know the HC is screwed.

Regards,
Michal