Skip to contentSkip to conversation

xHCI: Decouple updating Dequeue from giveback

PATCH In review Michal Pecio 2 participants 7 messages Nov 19, 2025

Subsystems (4)
USB Arm MediaTek USB xHCI driver MediaTek USB3 DRD IP driver
lore
Share & tools
lore

Michal Pecio

author
#0 lore raw

cover xHCI: Decouple updating Dequeue from giveback

Hi,

This introduces unified mechanism for handling all short transfers and errors mid TD plus their later events, accurately and in-spec. I have been working on this code on and off for the last few months, it's dead simple conceptually and tested a lot on various hardware.

When a TD completes early, store its end_trb on ep_ring and give it back. Use end_trb to recognize future events for the TD. Remove the SPURIOUS_SUCCESS quirk and unreliable comp code guesswork.

Isochronous TDs with errors are given back instantly, which reduces latency and eliminates the need for error_mid_td flag and some hacks like handling Missed Service only to give back error_mid_td.

Dequeue will be moved in accordance with received events, never to the next TD right away. Previous code would do that on Short Packet, allowing overwriting of remaining TRBs if it happens across segment boundary. Rare enough that no one complained, but obviously wrong.

We will need a trb_in_range(), and I used this as an opportunity to smuggle some long-discussed changes and improve trb_in_td() usage. After converting from dma_addr_t to trb* once in handle_tx_event(), pass ep_trb instead ep_trb_dma to trb_in_td().

This requires a rewrite of trb_in_td(). New version is easier and shorter. While I'm aware it could be shorter still by using segment numbers, it has two advantages which I thought are neat:

* It can detect when "suspect" TRB is on a different ring than TD. This means it has a loop, but common cases never enter it.

(And yes, I've seen this warning once, but I suspect corruption -
DMA UAF was involved. Things improved with pdev->untrusted = 1).

* Needs only one segment pointer (suspect). Call sites are tidier and I don't need to store last TD's end_seg together with end_trb.

Alternatively, segment numbers can also be used, but I really wanted to see if the code could be less noisy.

Regards,
Michal

Michal Pecio (5):
usb: xhci: Track transfer ring dequeue progress properly
usb: xhci: Find transfer TRB early in handle_tx_event()
usb: xhci: Refactor and generalize trb_in_td()
usb: xhci: Reduce error mid TD latency with a new "done TD" mechanism
usb: xhci: Handle short transfers as "done TDs"

 drivers/usb/host/xhci-mtk.c  |   5 -
 drivers/usb/host/xhci-pci.c  |   5 -
 drivers/usb/host/xhci-ring.c | 318 +++++++++++++++++++----------------
 drivers/usb/host/xhci.c      |   7 -
 drivers/usb/host/xhci.h      |   5 +-
 5 files changed, 174 insertions(+), 166 deletions(-)
author
#1 lore raw

PATCH 1/5 usb: xhci: Track transfer ring dequeue progress properly

xHCI 4.9.2 has clear requirements regarding transfer ring management:

Software uses the Dequeue Pointer to determine when a Transfer Ring is full. As it processes Transfer Events, it updates its copy of the Dequeue Pointer with the value of the Transfer Event TRB Pointer field. If advancing the Enqueue Pointer would make it equal to the Dequeue Pointer then the Transfer Ring is full and software shall wait for Transfer Events that will advance the Dequeue Pointer.

This and the first two rows of Table 4-2 in 4.6.9 imply that the xHC is not required to atomically advance Dequeue to the next TRB after completing one. The TRB referenced by latest event is still owned by the xHC and not supposed to be reused for new transfers yet.

The driver allows such reuse and then worse. When a TD completes with Short Packet, Dequeue is moved past remaining TRBs without waiting for their completion. This opens them for reuse if it happens across segment boundary and complicates handling events for those TRBs.

This is due to sloppy historic assumptions that Dequeue points to the next TD to execute. Those assumptions stopped working when unlinking was implemented and have been fully cleaned up last year. Dequeue is now only used for free space tracking and in move_dequeue_past_td().

So let's fix this. When TD is given back, set Dequeue to the current TRB pointer rather than past the TD. Future patch will also update Dequeue when (and if) events are received for the remaining TRBs.

Note: updating Dequeue before giveback would break sum_trb_lengths().

Skipping moves Dequeue past the TD because it is triggered by events outside the TD, so we know that the xHC has left it completely. That being said, replace inc_deq() with next_trb() to ensure that Dequeue doesn't get ahead of the xHC (and/or Enqueue) on Link TRBs.

Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)
drivers/usb/host/xhci-ring.c +20 -13
index bf077cd13ffa..3d5124912a09 100644
@@ -927,7 +927,7 @@ static void xhci_dequeue_td(struct xhci_hcd *xhci, struct xhci_td *td, struct xh
 {
 	ring->dequeue = td->end_trb;
 	ring->deq_seg = td->end_seg;
-	inc_deq(xhci, ring);
+	next_trb(&ring->deq_seg, &ring->dequeue);
 
 	xhci_td_cleanup(xhci, td, ring, status);
 }

@@ -2229,8 +2229,8 @@ int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int trb_comp_code)
 }
 
 static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
-		      struct xhci_ring *ep_ring, struct xhci_td *td,
-		      u32 trb_comp_code)
+		      struct xhci_ring *ep_ring, struct xhci_td *td, u32 trb_comp_code,
+		      struct xhci_segment *ep_seg, union xhci_trb *ep_trb)
 {
 	struct xhci_ep_ctx *ep_ctx;
 

@@ -2267,7 +2267,11 @@ static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		return; /* xhci_handle_halted_endpoint marked td cancelled */
 	}
 
-	xhci_dequeue_td(xhci, td, ep_ring, td->status);
+	/* update ring dequeue state */
+	ep_ring->deq_seg = ep_seg;
+	ep_ring->dequeue = ep_trb;
+
+	xhci_td_cleanup(xhci, td, ep_ring, td->status);
 }
 
 /* sum trb lengths from the first trb up to stop_trb, _excluding_ stop_trb */

@@ -2289,7 +2293,8 @@ static u32 sum_trb_lengths(struct xhci_td *td, union xhci_trb *stop_trb)
  */
 static void process_ctrl_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 			    struct xhci_ring *ep_ring,  struct xhci_td *td,
-			    union xhci_trb *ep_trb, struct xhci_transfer_event *event)
+			    struct xhci_segment *ep_seg, union xhci_trb *ep_trb,
+			    struct xhci_transfer_event *event)
 {
 	struct xhci_ep_ctx *ep_ctx;
 	u32 trb_comp_code;

@@ -2373,7 +2378,7 @@ static void process_ctrl_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		td->urb->actual_length = requested;
 
 finish_td:
-	finish_td(xhci, ep, ep_ring, td, trb_comp_code);
+	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb);
 }
 
 /*

@@ -2381,7 +2386,8 @@ static void process_ctrl_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
  */
 static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 			    struct xhci_ring *ep_ring, struct xhci_td *td,
-			    union xhci_trb *ep_trb, struct xhci_transfer_event *event)
+			    struct xhci_segment *ep_seg, union xhci_trb *ep_trb,
+			    struct xhci_transfer_event *event)
 {
 	struct urb_priv *urb_priv;
 	int idx;

@@ -2483,7 +2489,7 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		td->urb_length_set = true;
 		return;
 	}
-	finish_td(xhci, ep, ep_ring, td, trb_comp_code);
+	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb);
 }
 
 static void skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,

@@ -2511,7 +2517,8 @@ static void skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
  */
 static void process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 				 struct xhci_ring *ep_ring, struct xhci_td *td,
-				 union xhci_trb *ep_trb, struct xhci_transfer_event *event)
+				 struct xhci_segment *ep_seg, union xhci_trb *ep_trb,
+				 struct xhci_transfer_event *event)
 {
 	struct xhci_slot_ctx *slot_ctx;
 	u32 trb_comp_code;

@@ -2573,7 +2580,7 @@ static void process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		td->urb->actual_length = 0;
 	}
 
-	finish_td(xhci, ep, ep_ring, td, trb_comp_code);
+	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb);
 }
 
 /* Transfer events which don't point to a transfer TRB, see xhci 4.17.4 */

@@ -2941,11 +2948,11 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 	/* update the urb's actual_length and give back to the core */
 	if (usb_endpoint_xfer_control(&td->urb->ep->desc))
-		process_ctrl_td(xhci, ep, ep_ring, td, ep_trb, event);
+		process_ctrl_td(xhci, ep, ep_ring, td, ep_seg, ep_trb, event);
 	else if (usb_endpoint_xfer_isoc(&td->urb->ep->desc))
-		process_isoc_td(xhci, ep, ep_ring, td, ep_trb, event);
+		process_isoc_td(xhci, ep, ep_ring, td, ep_seg, ep_trb, event);
 	else
-		process_bulk_intr_td(xhci, ep, ep_ring, td, ep_trb, event);
+		process_bulk_intr_td(xhci, ep, ep_ring, td, ep_seg, ep_trb, event);
 	return 0;
 
 check_endpoint_halted:
Signature · 1 line
2.48.1
author
#2 lore raw

PATCH 2/5 usb: xhci: Find transfer TRB early in handle_tx_event()

In reply to Michal Pecio

As soon as we find the transfer ring to which an event belongs, we can proceed to locate the exact TRB referenced by the event. This enables better event handling and diagnostics, even if no TD matches the event.

Also set 'ep_seg' and remove its secondary use as a temporary boolean.

Bail out if event TRB pointer is not NULL and not a transfer TRB on the endpoint's ring. This indicates that either the HC executes TRBs from a wrong ring (bad Set TR Dequeue command, Link TRB damaged or ignored by the HC) or its internal state is corrupted and the event is bogus.

No such event is going to match any TD on td_list and trying to handle it would generally do nothing. On an isochronous endpoint we might skip all pending TDs and create more chaos. Just log this error and get out.

Suggested-by Mathias Nyman <[email protected]>
Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)
drivers/usb/host/xhci-ring.c +31 -4
index 3d5124912a09..531e2f207b17 100644
@@ -82,6 +82,27 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg,
 	return seg->dma + (segment_offset * sizeof(*trb));
 }
 
+/*
+ * Look up a TRB by its DMA address, return NULL if not found on the ring.
+ * Search from start_seg to let callers optimize starting point selection.
+ * Write actual segment containing returned TRB to seg_out, if provided.
+ */
+static union xhci_trb *xhci_dma_to_trb(struct xhci_segment *start_seg, dma_addr_t dma,
+					struct xhci_segment **seg_out)
+{
+	struct xhci_segment *seg;
+
+	xhci_for_each_ring_seg(start_seg, seg) {
+		if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE)) {
+			if (seg_out)
+				*seg_out = seg;
+			return seg->trbs + (dma - seg->dma) / sizeof(seg->trbs[0]);
+		}
+	}
+
+	return NULL;
+}
+
 static bool trb_is_noop(union xhci_trb *trb)
 {
 	return TRB_TYPE_NOOP_LE32(trb->generic.field[3]);

@@ -2672,6 +2693,15 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	if (!ep_ring)
 		return handle_transferless_tx_event(xhci, ep, trb_comp_code);
 
+	/* get the corresponding transfer TRB pointer */
+	ep_trb = xhci_dma_to_trb(ep_ring->deq_seg, ep_trb_dma, &ep_seg);
+	if (!ep_trb && ep_trb_dma) {
+		xhci_warn(xhci, "Ignoring '%s' event out of ring on slot %d ep %d\n",
+				xhci_trb_comp_code_string(trb_comp_code), slot_id, ep_index);
+		/* XXX: other ring's TDs may be executing on this EP, should we kill it? */
+		return 0;
+	}
+
 	/* Look for common error cases */
 	switch (trb_comp_code) {
 	/* Skip codes that require special handling depending on

@@ -2846,9 +2876,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				      td_list);
 
 		/* Is this a TRB in the currently executing TD? */
-		ep_seg = trb_in_td(td, ep_trb_dma);
-
-		if (!ep_seg) {
+		if (!trb_in_td(td, ep_trb_dma)) {
 
 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
 				/* this event is unlikely to match any TD, don't skip them all */

@@ -2931,7 +2959,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	if (ring_xrun_event)
 		return 0;
 
-	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, ep_trb_dma);
 
 	/*
Signature · 1 line
2.48.1
author
#3 lore raw

PATCH 3/5 usb: xhci: Refactor and generalize trb_in_td()

In reply to Michal Pecio

Replace the internal logic of trb_in_td() with a new one which works on (virtual) TRB pointers instead of DMA addresses. Since driver data structures use virtual pointers, the function and some of its callers will be simplified. Optimize for common case of trb == end_trb.

Extract the implementation into a lower level trb_in_range() function which works with arbitrary TRB ranges.

Create a new trb_in_td() as the obvious wrapper around trb_in_range(). Create a new dma_in_td() using xhci_dma_to_trb() and trb_in_td().

Update former trb_in_td() callers to use new functions appropriately.

Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 101 +++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 46 deletions(-)
drivers/usb/host/xhci-ring.c +55 -46
index 531e2f207b17..a2257e1dc396 100644
@@ -113,6 +113,11 @@ static bool trb_is_link(union xhci_trb *trb)
 	return TRB_TYPE_LINK_LE32(trb->link.control);
 }
 
+static bool trb_on_seg(struct xhci_segment *seg, union xhci_trb *trb)
+{
+	return in_range((uintptr_t) trb, (uintptr_t) seg->trbs, TRB_SEGMENT_SIZE);
+}
+
 static bool last_trb_on_seg(struct xhci_segment *seg, union xhci_trb *trb)
 {
 	return trb == &seg->trbs[TRBS_PER_SEGMENT - 1];

@@ -304,54 +309,58 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
 }
 
 /*
- * If the suspect DMA address is a TRB in this TD, this function returns that
- * TRB's segment. Otherwise it returns 0.
+ * Check if seg:trb is a TRB in the range between start_trb and end_trb, inclusive.
+ * If start_trb == end_trb, the range is one TRB, not the full ring. The range may
+ * cycle back and end in an earlier segment or on earlier TRB in start_trb segment.
  */
-static struct xhci_segment *trb_in_td(struct xhci_td *td, dma_addr_t suspect_dma)
+static bool trb_in_range(struct xhci_hcd *xhci, union xhci_trb *start_trb, union xhci_trb *end_trb,
+			 struct xhci_segment *seg, union xhci_trb *trb)
 {
-	dma_addr_t start_dma;
-	dma_addr_t end_seg_dma;
-	dma_addr_t end_trb_dma;
-	struct xhci_segment *cur_seg;
+	struct xhci_segment *seg_other;
 
-	start_dma = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
-	cur_seg = td->start_seg;
+	if (!trb || !seg)
+		return false;
 
-	do {
-		if (start_dma == 0)
-			return NULL;
-		/* We may get an event for a Link TRB in the middle of a TD */
-		end_seg_dma = xhci_trb_virt_to_dma(cur_seg,
-				&cur_seg->trbs[TRBS_PER_SEGMENT - 1]);
-		/* If the end TRB isn't in this segment, this is set to 0 */
-		end_trb_dma = xhci_trb_virt_to_dma(cur_seg, td->end_trb);
-
-		if (end_trb_dma > 0) {
-			/* The end TRB is in this segment, so suspect should be here */
-			if (start_dma <= end_trb_dma) {
-				if (suspect_dma >= start_dma && suspect_dma <= end_trb_dma)
-					return cur_seg;
-			} else {
-				/* Case for one segment with
-				 * a TD wrapped around to the top
-				 */
-				if ((suspect_dma >= start_dma &&
-							suspect_dma <= end_seg_dma) ||
-						(suspect_dma >= cur_seg->dma &&
-						 suspect_dma <= end_trb_dma))
-					return cur_seg;
-			}
-			return NULL;
-		}
-		/* Might still be somewhere in this segment */
-		if (suspect_dma >= start_dma && suspect_dma <= end_seg_dma)
-			return cur_seg;
+	/* Typically end_trb is near trb in the same segment */
+	if (trb_on_seg(seg, end_trb))
+		return trb <= end_trb
+			/* must not start between trb and end_trb */
+			? !(trb < start_trb && start_trb <= end_trb)
+			/* must start between end_trb and trb */
+			: end_trb < start_trb && start_trb <= trb;
 
-		cur_seg = cur_seg->next;
-		start_dma = xhci_trb_virt_to_dma(cur_seg, &cur_seg->trbs[0]);
-	} while (cur_seg != td->start_seg);
+	/* The range ends in other segment, easy if it starts here */
+	if (trb_on_seg(seg, start_trb))
+		return start_trb <= trb;
 
-	return NULL;
+	/* Walk segments and see if end_trb is reached before start_trb */
+	for (seg_other = seg->next; seg_other != seg; seg_other = seg_other->next) {
+		bool found_start = trb_on_seg(seg_other, start_trb);
+		bool found_end = trb_on_seg(seg_other, end_trb);
+
+		if (found_end)
+			return found_start ? end_trb < start_trb : true;
+
+		if (found_start)
+			return false;
+	}
+
+	xhci_err(xhci, "TRB range boundaries not on ring\n");
+	return false;
+}
+
+static bool trb_in_td(struct xhci_hcd *xhci, struct xhci_td *td,
+			struct xhci_segment *seg, union xhci_trb *trb)
+{
+	return trb_in_range(xhci, td->start_trb, td->end_trb, seg, trb);
+}
+
+static bool dma_in_td(struct xhci_hcd *xhci, struct xhci_td *td, dma_addr_t dma)
+{
+	struct xhci_segment *seg;
+	union xhci_trb *trb = xhci_dma_to_trb(td->start_seg, dma, &seg);
+
+	return trb_in_td(xhci, td, seg, trb);
 }
 
 /*

@@ -1094,7 +1103,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 					 td->urb->stream_id);
 		hw_deq &= TR_DEQ_PTR_MASK;
 
-		if (td->cancel_status == TD_HALTED || trb_in_td(td, hw_deq)) {
+		if (td->cancel_status == TD_HALTED || dma_in_td(xhci, td, hw_deq)) {
 			switch (td->cancel_status) {
 			case TD_CLEARED: /* TD is already no-op */
 			case TD_CLEARING_CACHE: /* set TR deq command already queued */

@@ -1184,7 +1193,7 @@ static struct xhci_td *find_halted_td(struct xhci_virt_ep *ep)
 		hw_deq = xhci_get_hw_deq(ep->xhci, ep->vdev, ep->ep_index, 0);
 		hw_deq &= TR_DEQ_PTR_MASK;
 		td = list_first_entry(&ep->ring->td_list, struct xhci_td, td_list);
-		if (trb_in_td(td, hw_deq))
+		if (dma_in_td(ep->xhci, td, hw_deq))
 			return td;
 	}
 	return NULL;

@@ -2843,7 +2852,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	 */
 	td = list_first_entry_or_null(&ep_ring->td_list, struct xhci_td, td_list);
 
-	if (td && td->error_mid_td && !trb_in_td(td, ep_trb_dma)) {
+	if (td && td->error_mid_td && !trb_in_td(xhci, td, ep_seg, ep_trb)) {
 		xhci_dbg(xhci, "Missing TD completion event after mid TD error\n");
 		xhci_dequeue_td(xhci, td, ep_ring, td->status);
 	}

@@ -2876,7 +2885,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				      td_list);
 
 		/* Is this a TRB in the currently executing TD? */
-		if (!trb_in_td(td, ep_trb_dma)) {
+		if (!trb_in_td(xhci, td, ep_seg, ep_trb)) {
 
 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
 				/* this event is unlikely to match any TD, don't skip them all */
Signature · 1 line
2.48.1
author
#4 lore raw

PATCH 4/5 usb: xhci: Reduce error mid TD latency with a new "done TD" mechanism

In reply to Michal Pecio

No data transfer will occur after an error mid TD, so the TD can be given back to class driver without waiting for the final completion event. This reduces latency, particularly on buggy HCs from vendors like NEC or ASMedia which ignore the IOC flag after errors, making the TD wait until next ESIT to be given back.

Since we can no longer expect the 'xhci_td' to stay around after it is given back, copy minimum metadata to 'ep_ring' which will enable recognition of subsequent events for the done TD. Existing solution with trb_in_td() proved very effective, so use a similar mechanism.

Also take over handling out-of-spec Etron events, because we can do it more accurately than the previous implementation.

We no longer need to handle Missed Service Errors with NULL 'ep_trb' for the sake of TDs stuck in error mid TD state, so drop that.

Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 116 +++++++++++++++++++----------------
 drivers/usb/host/xhci.h      |   2 +-
 2 files changed, 64 insertions(+), 54 deletions(-)
drivers/usb/host/xhci-ring.c +63 -53
index a2257e1dc396..2b889caff0f5 100644
@@ -1542,6 +1542,8 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			 */
 			ep_ring->deq_seg = ep->queued_deq_seg;
 			ep_ring->dequeue = ep->queued_deq_ptr;
+			/* If we were waiting for a done TD, we aren't anymore */
+			ep_ring->done_end_trb = NULL;
 		} else {
 			xhci_warn(xhci, "Mismatch between completed Set TR Deq Ptr command & xHCI internal state.\n");
 			xhci_warn(xhci, "ep deq seg = %p, deq ptr = %p\n",

@@ -2260,7 +2262,7 @@ int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int trb_comp_code)
 
 static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		      struct xhci_ring *ep_ring, struct xhci_td *td, u32 trb_comp_code,
-		      struct xhci_segment *ep_seg, union xhci_trb *ep_trb)
+		      struct xhci_segment *ep_seg, union xhci_trb *ep_trb, bool more_events)
 {
 	struct xhci_ep_ctx *ep_ctx;
 

@@ -2300,6 +2302,9 @@ static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	/* update ring dequeue state */
 	ep_ring->deq_seg = ep_seg;
 	ep_ring->dequeue = ep_trb;
+	/* td is done and going away, but it may still get more events */
+	if (more_events)
+		ep_ring->done_end_trb = td->end_trb;
 
 	xhci_td_cleanup(xhci, td, ep_ring, td->status);
 }

@@ -2408,7 +2413,7 @@ static void process_ctrl_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		td->urb->actual_length = requested;
 
 finish_td:
-	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb);
+	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb, false);
 }
 
 /*

@@ -2426,6 +2431,11 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	bool sum_trbs_for_length = false;
 	u32 remaining, requested, ep_trb_len;
 	int short_framestatus;
+	/* Expect more events for this TD after certain events before the last TRB */
+	bool more_events = false;
+	/* On Etron SuperSpeed endpoints some errors on any TRB trigger more events */
+	bool etron_ss_bug = xhci->quirks & XHCI_ETRON_HOST &&
+				td->urb->dev->speed == USB_SPEED_SUPER;
 
 	trb_comp_code = GET_COMP_CODE(le32_to_cpu(event->transfer_len));
 	urb_priv = td->urb->hcpriv;

@@ -2440,9 +2450,6 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	/* handle completion code */
 	switch (trb_comp_code) {
 	case COMP_SUCCESS:
-		/* Don't overwrite status if TD had an error, see xHCI 4.9.1 */
-		if (td->error_mid_td)
-			break;
 		if (remaining) {
 			frame->status = short_framestatus;
 			sum_trbs_for_length = true;

@@ -2462,14 +2469,12 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		fallthrough;
 	case COMP_ISOCH_BUFFER_OVERRUN:
 		frame->status = -EOVERFLOW;
-		if (ep_trb != td->end_trb)
-			td->error_mid_td = true;
+		more_events = ep_trb != td->end_trb || etron_ss_bug;	/* xHCI 4.9.1 */
 		break;
 	case COMP_MISSED_SERVICE_ERROR:
 		frame->status = -EXDEV;
 		sum_trbs_for_length = true;
-		if (ep_trb != td->end_trb)
-			td->error_mid_td = true;
+		more_events = ep_trb != td->end_trb;	/* xHCI 4.11.2.5.2, no Etron bug */
 		break;
 	case COMP_INCOMPATIBLE_DEVICE_ERROR:
 	case COMP_STALL_ERROR:

@@ -2478,8 +2483,7 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	case COMP_USB_TRANSACTION_ERROR:
 		frame->status = -EPROTO;
 		sum_trbs_for_length = true;
-		if (ep_trb != td->end_trb)
-			td->error_mid_td = true;
+		more_events = ep_trb != td->end_trb || etron_ss_bug;	/* xHCI 4.9.1 */
 		break;
 	case COMP_STOPPED:
 		sum_trbs_for_length = true;

@@ -2501,9 +2505,6 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		break;
 	}
 
-	if (td->urb_length_set)
-		goto finish_td;
-
 	if (sum_trbs_for_length)
 		frame->actual_length = sum_trb_lengths(td, ep_trb) +
 			ep_trb_len - remaining;

@@ -2512,14 +2513,7 @@ static void process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 
 	td->urb->actual_length += frame->actual_length;
 
-finish_td:
-	/* Don't give back TD yet if we encountered an error mid TD */
-	if (td->error_mid_td && ep_trb != td->end_trb) {
-		xhci_dbg(xhci, "Error mid isoc TD, wait for final completion event\n");
-		td->urb_length_set = true;
-		return;
-	}
-	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb);
+	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb, more_events);
 }
 
 static void skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,

@@ -2610,7 +2604,34 @@ static void process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		td->urb->actual_length = 0;
 	}
 
-	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb);
+	finish_td(xhci, ep, ep_ring, td, trb_comp_code, ep_seg, ep_trb, false);
+}
+
+/*
+ * Process events for an already finished TD. See xHCI 4.9.1.
+ */
+static void process_done_td(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
+				 struct xhci_segment *ep_seg, union xhci_trb *ep_trb,
+				 u32 trb_comp_code)
+{
+	switch (trb_comp_code) {
+	case COMP_STOPPED:
+	case COMP_STOPPED_LENGTH_INVALID:
+	case COMP_STOPPED_SHORT_PACKET:
+		/* stopped events don't terminate the TD */
+		break;
+	default:
+		/* clear done TD if this is the final event */
+		if (ep_trb == ep_ring->done_end_trb)
+			ep_ring->done_end_trb = NULL;
+		else	/* unexpected */
+			xhci_dbg(xhci, "Done TD '%s' before end_trb\n",
+					xhci_trb_comp_code_string(trb_comp_code));
+	}
+
+	/* release TRBs completed by the xHC */
+	ep_ring->deq_seg = ep_seg;
+	ep_ring->dequeue = ep_trb;
 }
 
 /* Transfer events which don't point to a transfer TRB, see xhci 4.17.4 */

@@ -2646,11 +2667,6 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci,
 	switch (ring->old_trb_comp_code) {
 	case COMP_SHORT_PACKET:
 		return xhci->quirks & XHCI_SPURIOUS_SUCCESS;
-	case COMP_USB_TRANSACTION_ERROR:
-	case COMP_BABBLE_DETECTED_ERROR:
-	case COMP_ISOCH_BUFFER_OVERRUN:
-		return xhci->quirks & XHCI_ETRON_HOST &&
-			ring->type == TYPE_ISOC;
 	default:
 		return false;
 	}

@@ -2800,15 +2816,17 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		break;
 	case COMP_MISSED_SERVICE_ERROR:
 		/*
-		 * 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.
+		 * One or more isoc TDs were missed by the xHC and ep_trb points to the
+		 * last missed TD, or it may be NULL. Flag the endpoint and run skipping
+		 * now if we know the missed TDs or leave them to be skipped later.
+		 * See xHCI 4.10.3.2, note differences between spec rev 1.0 and later.
 		 */
 		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" : "");
+			 slot_id, ep_index, ep_trb ? ", skip now" : "");
+		if (!ep_trb)
+			return 0;
 		break;
 	case COMP_NO_PING_RESPONSE_ERROR:
 		ep->skip = true;

@@ -2838,29 +2856,21 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	}
 
 	/*
-	 * 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.
-	 *
-	 * We wait for the final IOC event, but if we get an event
-	 * anywhere outside this TD, just give it back already.
+	 * Check if we are expecting more events for a "done" TD, which has been given back before
+	 * the xHC finished traversing all its TRBs, because it completed with an error.
 	 */
-	td = list_first_entry_or_null(&ep_ring->td_list, struct xhci_td, td_list);
-
-	if (td && td->error_mid_td && !trb_in_td(xhci, td, ep_seg, ep_trb)) {
-		xhci_dbg(xhci, "Missing TD completion event after mid TD error\n");
-		xhci_dequeue_td(xhci, td, ep_ring, td->status);
+	if (ep_ring->done_end_trb) {
+		if (trb_in_range(xhci, ep_ring->dequeue, ep_ring->done_end_trb, ep_seg, ep_trb)) {
+			process_done_td(xhci, ep_ring, ep_seg, ep_trb, trb_comp_code);
+			return 0;
+		}
+		/*
+		 * Some HCs don't generate these events and silently move forward. We get an event
+		 * for the next TD, or maybe Missed Service or Ring Underrun. Handle it normally.
+		 */
+		ep_ring->done_end_trb = NULL;
 	}
 
-	/* 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)
-		return 0;
-
 	if (list_empty(&ep_ring->td_list)) {
 		/*
 		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
drivers/usb/host/xhci.h +1 -1
index 485ea7fc0433..a1dd9ce5f8aa 100644
@@ -1314,7 +1314,6 @@ struct xhci_td {
 	struct xhci_segment	*bounce_seg;
 	/* actual_length of the URB has already been set */
 	bool			urb_length_set;
-	bool			error_mid_td;
 };
 
 /*

@@ -1380,6 +1379,7 @@ struct xhci_ring {
 	unsigned int		num_trbs_free; /* used only by xhci DbC */
 	unsigned int		bounce_buf_len;
 	enum xhci_ring_type	type;
+	union xhci_trb		*done_end_trb;
 	u32			old_trb_comp_code;
 	struct radix_tree_root	*trb_address_map;
 };
Signature · 1 line
2.48.1
author
#5 lore raw

PATCH 5/5 usb: xhci: Handle short transfers as "done TDs"

In reply to Michal Pecio

Short transfers (on any endpoint type) are similar to isochronous errors mid TD - the TD can be given back, but we need to keep track of xHC's progress through its TRBs and handle some more events. And here too there are buggy HCs which don't generate expected events.

Reuse the "done TD" mechanism to also handle short transfers on all endpoints other than control, which have some special requirements. Done TD is more accurate than the previous code, because it matches events with TDs by TRB pointers, not by completion codes alone.

Remove the SPURIOUS_SUCCESS quirk, as we can now detect missing end TRB events automatically and reliably. Ensure that no xhci_dbg() shows on every short transfer, which can be 8000 times per second.

The patch was tested on isochronous and bulk endpoints at FS/HS/SS with several host controllers from various vendors. Behavior after Short Packet mid TD varies:

1. NEC uPD720200 and Etron EJ168 ignore IOC on the last TRB.
2. Renesas uPD720202 and AMD Carrizo APU generate Success with residual equal to last TRB size.
3. Fresco Logic FL1100 generates Short Packet, residual as above.
4. Various ASMedia HCs and AMD Promontory generate Short Packet again with repeated residual, even if the last TRB is shorter.
5. VIA VL805 generates Success with zero residual.

No case of a Success with non-zero residual was observed, but commit 1530bbc6272d ("xhci: Add new short TX quirk for Fresco Logic host.") says that some Fresco Logic hardware (FL1000?) does that.

Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-mtk.c  |  5 ----
 drivers/usb/host/xhci-pci.c  |  5 ----
 drivers/usb/host/xhci-ring.c | 51 +++++++++---------------------------
 drivers/usb/host/xhci.c      |  7 -----
 drivers/usb/host/xhci.h      |  3 +--
 5 files changed, 13 insertions(+), 58 deletions(-)
drivers/usb/host/xhci-mtk.c +0 -5
index 208558cf822d..d85f84e39291 100644
@@ -454,11 +454,6 @@ static void xhci_mtk_quirks(struct device *dev, struct xhci_hcd *xhci)
 	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
 
 	xhci->quirks |= XHCI_MTK_HOST;
-	/*
-	 * MTK host controller gives a spurious successful event after a
-	 * short transfer. Ignore it.
-	 */
-	xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
 	if (mtk->lpm_support)
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 	if (mtk->u2_lpm_disable)
drivers/usb/host/xhci-pci.c +0 -5
index d292adc65e5a..bcfe45d23f17 100644
@@ -454,11 +454,6 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 
 	if (pdev->vendor == PCI_VENDOR_ID_ASMEDIA &&
 		pdev->device == PCI_DEVICE_ID_ASMEDIA_1042_XHCI) {
-		/*
-		 * try to tame the ASMedia 1042 controller which reports 0.96
-		 * but appears to behave more like 1.0
-		 */
-		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
 		xhci->quirks |= XHCI_BROKEN_STREAMS;
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_ASMEDIA &&
drivers/usb/host/xhci-ring.c +12 -39

119 lines not on this page · raw message

drivers/usb/host/xhci.c +0 -7
index 6da583c7778b..4e32cbe1c6b8 100644
@@ -5473,13 +5473,6 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 	if (get_quirks)
 		get_quirks(dev, xhci);
 
-	/* In xhci controllers which follow xhci 1.0 spec gives a spurious
-	 * success event after a short transfer. This quirk will ignore such
-	 * spurious event.
-	 */
-	if (xhci->hci_version > 0x96)
-		xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
-
 	if (xhci->hci_version == 0x95 && link_quirk) {
 		xhci_dbg(xhci, "QUIRK: Not clearing Link TRB chain bits");
 		xhci->quirks |= XHCI_LINK_TRB_QUIRK;
drivers/usb/host/xhci.h +1 -2
index a1dd9ce5f8aa..e72eda6cee62 100644
@@ -1380,7 +1380,6 @@ struct xhci_ring {
 	unsigned int		bounce_buf_len;
 	enum xhci_ring_type	type;
 	union xhci_trb		*done_end_trb;
-	u32			old_trb_comp_code;
 	struct radix_tree_root	*trb_address_map;
 };
 

@@ -1587,7 +1586,7 @@ struct xhci_hcd {
 #define XHCI_RESET_EP_QUIRK	BIT_ULL(1) /* Deprecated */
 #define XHCI_NEC_HOST		BIT_ULL(2)
 #define XHCI_AMD_PLL_FIX	BIT_ULL(3)
-#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4)
+#define XHCI_SPURIOUS_SUCCESS	BIT_ULL(4) /* Deprecated */
 /*
  * Certain Intel host controllers have a limit to the number of endpoint
  * contexts they can handle.  Ideally, they would signal that they can't handle
Signature · 1 line
2.48.1
maintainer
#6 lore raw
In reply to Michal Pecio
Quoted 32 lines · Michal Pecio

Hi,

This introduces unified mechanism for handling all short transfers and errors mid TD plus their later events, accurately and in-spec. I have been working on this code on and off for the last few months, it's dead simple conceptually and tested a lot on various hardware.

When a TD completes early, store its end_trb on ep_ring and give it back. Use end_trb to recognize future events for the TD. Remove the SPURIOUS_SUCCESS quirk and unreliable comp code guesswork.

Isochronous TDs with errors are given back instantly, which reduces latency and eliminates the need for error_mid_td flag and some hacks like handling Missed Service only to give back error_mid_td.

Dequeue will be moved in accordance with received events, never to the next TD right away. Previous code would do that on Short Packet, allowing overwriting of remaining TRBs if it happens across segment boundary. Rare enough that no one complained, but obviously wrong.

We will need a trb_in_range(), and I used this as an opportunity to smuggle some long-discussed changes and improve trb_in_td() usage. After converting from dma_addr_t to trb* once in handle_tx_event(), pass ep_trb instead ep_trb_dma to trb_in_td().

This requires a rewrite of trb_in_td(). New version is easier and shorter. While I'm aware it could be shorter still by using segment numbers, it has two advantages which I thought are neat:

* It can detect when "suspect" TRB is on a different ring than TD. This means it has a loop, but common cases never enter it.

Quoted 8 lines

(And yes, I've seen this warning once, but I suspect corruption -

    DMA UAF was involved. Things improved with pdev->untrusted = 1).

* Needs only one segment pointer (suspect). Call sites are tidier and I don't need to store last TD's end_seg together with end_trb.

Alternatively, segment numbers can also be used, but I really wanted to see if the code could be less noisy.

Looks like we both picked this up again.

I have queued up a couple patches that does basically the same trb_in_range() changes as your patches 2/5 and 3/5, but uses segment indexes.

I'm sending all for-usb-next patches forward today for 6.19, including those two patches. Just need to do one small test.

I'll take a closer look at the rest of the series (patches 1/5, 4/5, and 5/5) They all seam like a good idea, but will need to be rebased on current for-usb-next

Links to the two patches I mentioned:
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-next&id=7cca2b801bae19bfda2e54d7187f78e8af3f3700
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-next&id=b78fafee824b964c1db718662ef9854f9d7154dc

Link to my for-usb-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-next

Thanks
Mathias