[PATCH 0/3] xhci: ring queuing cleanups

[PATCH 0/3] xhci: ring queuing cleanups

[Michal Pecio]

Hi,

I was looking at all uses of enqueue/dequeue pointers and I found two
rather complex loops which appear to be doing really simple things.

I don't understand why they were written this way, it seems wasteful
and I see nothing that should go wrong if they are replaced with much
simpler code.

I rewrote them and the driver still works. I exercised Set TR Dequeue
code by starting/stopping isoc streams, using usb-storage with crappy
cable (transaction errors, halts) and also the smartctl -x trick that
results in URB unlinks (both on usb-storage and uas) with some disks.

The third patch is a dedupe. BTW, that comment there about section
6.4.4.1 of the 0.95 spec seems to be wrong, I suspect it should say
that the chain bit cannot be *cleared* because that's how the code
works and what some commit messages say. But I don't have 0.95 spec.

Regards,
Michal


Michal Pecio (3):
  usb: xhci: Simplify update_ring_for_set_deq_completion()
  usb: xhci: Simplify moving HW Dequeue Pointer past cancelled TDs
  usb: xhci: Unify duplicate inc_enq() code

 drivers/usb/host/xhci-ring.c | 242 +++++++++++++----------------------
 1 file changed, 92 insertions(+), 150 deletions(-)

-- 
2.48.1

[PATCH 1/3] usb: xhci: Simplify update_ring_for_set_deq_completion()

[Michal Pecio]

This function checks if the queued Set Deq pointer really belongs to the
ring it was queued on and updates the ring's dequeue state. It also used
to count free TRBs, but that part has been removed.

The code is needlessly complex and inefficent, walking TRBs one by one.
And it could "jump off the segment into la-la-land" if a segment doesn't
end with a link TRB or if the link points to another link.

Make if faster, simpler and more robust. Upgrade xhci_dbg() to xhci_err()
because this situation is a bug and shouldn't happen.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 965bffce301e..c983d22842dc 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -92,6 +92,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 seg->trbs <= trb && trb < seg->trbs + TRBS_PER_SEGMENT;
+}
+
 static bool last_trb_on_seg(struct xhci_segment *seg, union xhci_trb *trb)
 {
 	return trb == &seg->trbs[TRBS_PER_SEGMENT - 1];
@@ -1332,41 +1337,25 @@ void xhci_hc_died(struct xhci_hcd *xhci)
 		usb_hc_died(xhci_to_hcd(xhci));
 }
 
+/* Check if queued pointer is on the ring and update dequeue state */
 static void update_ring_for_set_deq_completion(struct xhci_hcd *xhci,
 		struct xhci_virt_device *dev,
 		struct xhci_ring *ep_ring,
 		unsigned int ep_index)
 {
-	union xhci_trb *dequeue_temp;
+	union xhci_trb *deq = dev->eps[ep_index].queued_deq_ptr;
+	struct xhci_segment *seg;
 
-	dequeue_temp = ep_ring->dequeue;
-
-	/* If we get two back-to-back stalls, and the first stalled transfer
-	 * ends just before a link TRB, the dequeue pointer will be left on
-	 * the link TRB by the code in the while loop.  So we have to update
-	 * the dequeue pointer one segment further, or we'll jump off
-	 * the segment into la-la-land.
-	 */
-	if (trb_is_link(ep_ring->dequeue)) {
-		ep_ring->deq_seg = ep_ring->deq_seg->next;
-		ep_ring->dequeue = ep_ring->deq_seg->trbs;
-	}
-
-	while (ep_ring->dequeue != dev->eps[ep_index].queued_deq_ptr) {
-		/* We have more usable TRBs */
-		ep_ring->dequeue++;
-		if (trb_is_link(ep_ring->dequeue)) {
-			if (ep_ring->dequeue ==
-					dev->eps[ep_index].queued_deq_ptr)
-				break;
-			ep_ring->deq_seg = ep_ring->deq_seg->next;
-			ep_ring->dequeue = ep_ring->deq_seg->trbs;
-		}
-		if (ep_ring->dequeue == dequeue_temp) {
-			xhci_dbg(xhci, "Unable to find new dequeue pointer\n");
-			break;
+	/* Search starting from the last known position */
+	xhci_for_each_ring_seg(ep_ring->deq_seg, seg) {
+		if (seg == dev->eps[ep_index].queued_deq_seg && trb_on_seg(seg, deq)) {
+			ep_ring->deq_seg = seg;
+			ep_ring->dequeue = deq;
+			return;
 		}
 	}
+
+	xhci_err(xhci, "Set Deq pointer not on ring\n");
 }
 
 /*
-- 
2.48.1

Re: [PATCH 1/3] usb: xhci: Simplify update_ring_for_set_deq_completion()

[Mathias Nyman]

On 21.2.2025 0.44, Michal Pecio wrote:
> This function checks if the queued Set Deq pointer really belongs to the
> ring it was queued on and updates the ring's dequeue state. It also used
> to count free TRBs, but that part has been removed.
> 
> The code is needlessly complex and inefficent, walking TRBs one by one.
> And it could "jump off the segment into la-la-land" if a segment doesn't
> end with a link TRB or if the link points to another link.
> 
> Make if faster, simpler and more robust. Upgrade xhci_dbg() to xhci_err()
> because this situation is a bug and shouldn't happen.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>


update_ring_for_set_deq_completion() isn't needed anymore.
Niklas already wrote a patch to remove it.

It's sitting in my for-usb-next branch

https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-next&id=ee7dab196a7dfc48a1608274329323cb10b4340d

Thanks
Mathias

Re: [PATCH 1/3] usb: xhci: Simplify update_ring_for_set_deq_completion()

[Michał Pecio]

On Fri, 21 Feb 2025 15:23:11 +0200, Mathias Nyman wrote:
> update_ring_for_set_deq_completion() isn't needed anymore.
> Niklas already wrote a patch to remove it.
> 
> It's sitting in my for-usb-next branch
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/commit/?h=for-usb-next&id=ee7dab196a7dfc48a1608274329323cb10b4340d

I know that it's pure paranoia, but this patch removes a quiet debug
message about an obviously abnormal and likely harmful condition.

My patch turned it into a nice red error.

This will be something to keep in mind, for example somebody could
write a patch which reclaims an empty transfer ring segment without
chceking if there is a Set TR Dequeue pending to this segemnt. That
could lead to very interesting outcomes if all TRBs in the segment
are No-Op'ed and the next page is a transfer ring of other endpoint.

Other than that, I suppose there is currently no problem. The loop
for finding new dequeue position doesn't seem prone to jumping off
the ring as long as the ring itself isn't corrupted.

Michal

[PATCH 2/3] usb: xhci: Simplify moving HW Dequeue Pointer past cancelled TDs

[Michal Pecio]

xhci_move_dequeue_past_td() uses a relatively complex and inefficient
procedure to find new dequeue position after the cancelled TD.

Replace it with a simpler function which moves dequeue immediately to
the first pending TD, or to enqueue if the ring is empty.

The outcome should be basically equivalent, because we only clear xHC
cache if it stopped or halted on some cancelled TD and moving past the
TD effectively means moving to the first remaining TD, if any.

If the cancelled TD is followed by more cancelled TDs turned into No-
Ops, we will now jump over them and save the xHC some work.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c983d22842dc..46ca98066856 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -635,9 +635,9 @@ static u64 xhci_get_hw_deq(struct xhci_hcd *xhci, struct xhci_virt_device *vdev,
 	return le64_to_cpu(ep_ctx->deq);
 }
 
-static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
-				unsigned int slot_id, unsigned int ep_index,
-				unsigned int stream_id, struct xhci_td *td)
+/* Move HW dequeue to the first pending TD or to our enqueue if there are no TDs */
+static int set_ring_dequeue(struct xhci_hcd *xhci, unsigned int slot_id,
+				unsigned int ep_index, unsigned int stream_id)
 {
 	struct xhci_virt_device *dev = xhci->devs[slot_id];
 	struct xhci_virt_ep *ep = &dev->eps[ep_index];
@@ -645,58 +645,31 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
 	struct xhci_command *cmd;
 	struct xhci_segment *new_seg;
 	union xhci_trb *new_deq;
+	struct xhci_td *td;
 	int new_cycle;
 	dma_addr_t addr;
-	u64 hw_dequeue;
-	bool cycle_found = false;
-	bool td_last_trb_found = false;
 	u32 trb_sct = 0;
 	int ret;
 
-	ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
-			ep_index, stream_id);
+	ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id, ep_index, stream_id);
+
 	if (!ep_ring) {
 		xhci_warn(xhci, "WARN can't find new dequeue, invalid stream ID %u\n",
 			  stream_id);
 		return -ENODEV;
 	}
 
-	hw_dequeue = xhci_get_hw_deq(xhci, dev, ep_index, stream_id);
-	new_seg = ep_ring->deq_seg;
-	new_deq = ep_ring->dequeue;
-	new_cycle = hw_dequeue & 0x1;
+	if (!list_empty(&ep_ring->td_list)) {
+		td = list_first_entry(&ep_ring->td_list, struct xhci_td, td_list);
+		new_seg = td->start_seg;
+		new_deq = td->start_trb;
+		new_cycle = le32_to_cpu(new_deq->generic.field[3]) & TRB_CYCLE;
+	} else {
+		new_seg = ep_ring->enq_seg;
+		new_deq = ep_ring->enqueue;
+		new_cycle = ep_ring->cycle_state;
+	}
 
-	/*
-	 * We want to find the pointer, segment and cycle state of the new trb
-	 * (the one after current TD's end_trb). We know the cycle state at
-	 * hw_dequeue, so walk the ring until both hw_dequeue and end_trb are
-	 * found.
-	 */
-	do {
-		if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq)
-		    == (dma_addr_t)(hw_dequeue & ~0xf)) {
-			cycle_found = true;
-			if (td_last_trb_found)
-				break;
-		}
-		if (new_deq == td->end_trb)
-			td_last_trb_found = true;
-
-		if (cycle_found && trb_is_link(new_deq) &&
-		    link_trb_toggles_cycle(new_deq))
-			new_cycle ^= 0x1;
-
-		next_trb(&new_seg, &new_deq);
-
-		/* Search wrapped around, bail out */
-		if (new_deq == ep->ring->dequeue) {
-			xhci_err(xhci, "Error: Failed finding new dequeue state\n");
-			return -EINVAL;
-		}
-
-	} while (!cycle_found || !td_last_trb_found);
-
-	/* Don't update the ring cycle state for the producer (us). */
 	addr = xhci_trb_virt_to_dma(new_seg, new_deq);
 	if (addr == 0) {
 		xhci_warn(xhci, "Can't find dma of new dequeue ptr\n");
@@ -1060,9 +1033,8 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 	if (!cached_td)
 		return 0;
 
-	err = xhci_move_dequeue_past_td(xhci, slot_id, ep->ep_index,
-					cached_td->urb->stream_id,
-					cached_td);
+	err = set_ring_dequeue(xhci, slot_id, ep->ep_index, cached_td->urb->stream_id);
+
 	if (err) {
 		/* Failed to move past cached td, just set cached TDs to no-op */
 		list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) {
-- 
2.48.1

[PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

[Michal Pecio]

Remove a block of code copied from inc_enq(). As often happens, the two
copies have diverged somewhat - in this case, inc_enq() has a bug where
it may leave the chain bit of a link TRB unset on a quirky HC. Fix this.
Remove the pointless 'next' variable which only aliases ring->enqueue.

Note: I don't know if any 0.95 xHC ever reached series production, but
"AMD 0.96 host" appears to be the "Llano" family APU. Example dmesg at
https://linux-hardware.org/?probe=79d5cfd4fd&log=dmesg

pci 0000:00:10.0: [1022:7812] type 00 class 0x0c0330
hcc params 0x014042c3 hci version 0x96 quirks 0x0000000000000608

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 46ca98066856..f400ba85ebc5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -208,6 +208,52 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring)
 	return;
 }
 
+/*
+ * If enqueue points at a link TRB, follow links until an ordinary TRB is reached.
+ * Toggle the cycle bit of passed link TRBs and optionally chain them.
+ */
+static void inc_enq_past_link(struct xhci_hcd *xhci, struct xhci_ring *ring, u32 chain)
+{
+	unsigned int link_trb_count = 0;
+
+	while (trb_is_link(ring->enqueue)) {
+
+		/*
+		 * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit
+		 * set, but other sections talk about dealing with the chain bit set. This was
+		 * fixed in the 0.96 specification errata, but we have to assume that all 0.95
+		 * xHCI hardware can't handle the chain bit being cleared on a link TRB.
+		 *
+		 * If we're dealing with 0.95 hardware or isoc rings on AMD 0.96 host, override
+		 * the chain bit to always be set.
+		 */
+		if (!xhci_link_chain_quirk(xhci, ring->type)) {
+			ring->enqueue->link.control &= cpu_to_le32(~TRB_CHAIN);
+			ring->enqueue->link.control |= cpu_to_le32(chain);
+		} else {
+			ring->enqueue->link.control |= cpu_to_le32(TRB_CHAIN);
+		}
+
+		/* Give this link TRB to the hardware */
+		wmb();
+		ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE);
+
+		/* Toggle the cycle bit after the last ring segment. */
+		if (link_trb_toggles_cycle(ring->enqueue))
+			ring->cycle_state ^= 1;
+
+		ring->enq_seg = ring->enq_seg->next;
+		ring->enqueue = ring->enq_seg->trbs;
+
+		trace_xhci_inc_enq(ring);
+
+		if (link_trb_count++ > ring->num_segs) {
+			xhci_warn(xhci, "Link TRB loop at enqueue\n");
+			break;
+		}
+	}
+}
+
 /*
  * See Cycle bit rules. SW is the consumer for the event ring only.
  *
@@ -216,11 +262,6 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring)
  * If we've enqueued the last TRB in a TD, make sure the following link TRBs
  * have their chain bit cleared (so that each Link TRB is a separate TD).
  *
- * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit
- * set, but other sections talk about dealing with the chain bit set.  This was
- * fixed in the 0.96 specification errata, but we have to assume that all 0.95
- * xHCI hardware can't handle the chain bit being cleared on a link TRB.
- *
  * @more_trbs_coming:	Will you enqueue more TRBs before calling
  *			prepare_transfer()?
  */
@@ -228,8 +269,6 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
 			bool more_trbs_coming)
 {
 	u32 chain;
-	union xhci_trb *next;
-	unsigned int link_trb_count = 0;
 
 	chain = le32_to_cpu(ring->enqueue->generic.field[3]) & TRB_CHAIN;
 
@@ -238,48 +277,16 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
 		return;
 	}
 
-	next = ++(ring->enqueue);
-
-	/* Update the dequeue pointer further if that was a link TRB */
-	while (trb_is_link(next)) {
-
-		/*
-		 * If the caller doesn't plan on enqueueing more TDs before
-		 * ringing the doorbell, then we don't want to give the link TRB
-		 * to the hardware just yet. We'll give the link TRB back in
-		 * prepare_ring() just before we enqueue the TD at the top of
-		 * the ring.
-		 */
-		if (!chain && !more_trbs_coming)
-			break;
-
-		/* If we're not dealing with 0.95 hardware or isoc rings on
-		 * AMD 0.96 host, carry over the chain bit of the previous TRB
-		 * (which may mean the chain bit is cleared).
-		 */
-		if (!xhci_link_chain_quirk(xhci, ring->type)) {
-			next->link.control &= cpu_to_le32(~TRB_CHAIN);
-			next->link.control |= cpu_to_le32(chain);
-		}
-		/* Give this link TRB to the hardware */
-		wmb();
-		next->link.control ^= cpu_to_le32(TRB_CYCLE);
-
-		/* Toggle the cycle bit after the last ring segment. */
-		if (link_trb_toggles_cycle(next))
-			ring->cycle_state ^= 1;
-
-		ring->enq_seg = ring->enq_seg->next;
-		ring->enqueue = ring->enq_seg->trbs;
-		next = ring->enqueue;
-
-		trace_xhci_inc_enq(ring);
-
-		if (link_trb_count++ > ring->num_segs) {
-			xhci_warn(xhci, "%s: Ring link TRB loop\n", __func__);
-			break;
-		}
-	}
+	ring->enqueue++;
+
+	/*
+	 * If we are in the middle of a TD or the caller plans to enqueue more
+	 * TDs as one transfer (eg. control), traverse any link TRBs right now.
+	 * Otherwise, enqueue can stay on a link until the next prepare_ring().
+	 * This avoids enqueue entering deq_seg and simplifies ring expansion.
+	 */
+	if (chain || more_trbs_coming)
+		inc_enq_past_link(xhci, ring, chain);
 }
 
 /*
@@ -3177,7 +3184,6 @@ static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
 static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
 		u32 ep_state, unsigned int num_trbs, gfp_t mem_flags)
 {
-	unsigned int link_trb_count = 0;
 	unsigned int new_segs = 0;
 
 	/* Make sure the endpoint has been added to xHC schedule */
@@ -3225,33 +3231,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
 		}
 	}
 
-	while (trb_is_link(ep_ring->enqueue)) {
-		/* If we're not dealing with 0.95 hardware or isoc rings
-		 * on AMD 0.96 host, clear the chain bit.
-		 */
-		if (!xhci_link_chain_quirk(xhci, ep_ring->type))
-			ep_ring->enqueue->link.control &=
-				cpu_to_le32(~TRB_CHAIN);
-		else
-			ep_ring->enqueue->link.control |=
-				cpu_to_le32(TRB_CHAIN);
-
-		wmb();
-		ep_ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE);
-
-		/* Toggle the cycle bit after the last ring segment. */
-		if (link_trb_toggles_cycle(ep_ring->enqueue))
-			ep_ring->cycle_state ^= 1;
-
-		ep_ring->enq_seg = ep_ring->enq_seg->next;
-		ep_ring->enqueue = ep_ring->enq_seg->trbs;
-
-		/* prevent infinite loop if all first trbs are link trbs */
-		if (link_trb_count++ > ep_ring->num_segs) {
-			xhci_warn(xhci, "Ring is an endless link TRB loop\n");
-			return -EINVAL;
-		}
-	}
+	/* Ensure that new TRBs won't overwrite a link */
+	inc_enq_past_link(xhci, ep_ring, 0);
 
 	if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue)) {
 		xhci_warn(xhci, "Missing link TRB at end of ring segment\n");
-- 
2.48.1

Re: [PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

[Mathias Nyman]

On 21.2.2025 0.47, Michal Pecio wrote:
> Remove a block of code copied from inc_enq(). As often happens, the two
> copies have diverged somewhat - in this case, inc_enq() has a bug where
> it may leave the chain bit of a link TRB unset on a quirky HC. Fix this.
> Remove the pointless 'next' variable which only aliases ring->enqueue.

The linnk TRB chain bit should be set when the ring is created, and never cleared
on those quirky hosts.

> 
> Note: I don't know if any 0.95 xHC ever reached series production, but
> "AMD 0.96 host" appears to be the "Llano" family APU. Example dmesg at
> https://linux-hardware.org/?probe=79d5cfd4fd&log=dmesg
> 
> pci 0000:00:10.0: [1022:7812] type 00 class 0x0c0330
> hcc params 0x014042c3 hci version 0x96 quirks 0x0000000000000608
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---
>   drivers/usb/host/xhci-ring.c | 135 +++++++++++++++--------------------
>   1 file changed, 58 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 46ca98066856..f400ba85ebc5 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -208,6 +208,52 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring)
>   	return;
>   }
>   
> +/*
> + * If enqueue points at a link TRB, follow links until an ordinary TRB is reached.
> + * Toggle the cycle bit of passed link TRBs and optionally chain them.
> + */
> +static void inc_enq_past_link(struct xhci_hcd *xhci, struct xhci_ring *ring, u32 chain)
> +{
> +	unsigned int link_trb_count = 0;
> +
> +	while (trb_is_link(ring->enqueue)) {
> +
> +		/*
> +		 * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit
> +		 * set, but other sections talk about dealing with the chain bit set. This was
> +		 * fixed in the 0.96 specification errata, but we have to assume that all 0.95
> +		 * xHCI hardware can't handle the chain bit being cleared on a link TRB.
> +		 *
> +		 * If we're dealing with 0.95 hardware or isoc rings on AMD 0.96 host, override
> +		 * the chain bit to always be set.
> +		 */
> +		if (!xhci_link_chain_quirk(xhci, ring->type)) {
> +			ring->enqueue->link.control &= cpu_to_le32(~TRB_CHAIN);
> +			ring->enqueue->link.control |= cpu_to_le32(chain);
> +		} else {
> +			ring->enqueue->link.control |= cpu_to_le32(TRB_CHAIN);
> +		}
> +
> +		/* Give this link TRB to the hardware */
> +		wmb();
> +		ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE);
> +
> +		/* Toggle the cycle bit after the last ring segment. */
> +		if (link_trb_toggles_cycle(ring->enqueue))
> +			ring->cycle_state ^= 1;
> +
> +		ring->enq_seg = ring->enq_seg->next;
> +		ring->enqueue = ring->enq_seg->trbs;
> +
> +		trace_xhci_inc_enq(ring);
> +
> +		if (link_trb_count++ > ring->num_segs) {
> +			xhci_warn(xhci, "Link TRB loop at enqueue\n");
> +			break;
> +		}
> +	}
> +}
> +
>   /*
>    * See Cycle bit rules. SW is the consumer for the event ring only.
>    *
> @@ -216,11 +262,6 @@ void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring)
>    * If we've enqueued the last TRB in a TD, make sure the following link TRBs
>    * have their chain bit cleared (so that each Link TRB is a separate TD).
>    *
> - * Section 6.4.4.1 of the 0.95 spec says link TRBs cannot have the chain bit
> - * set, but other sections talk about dealing with the chain bit set.  This was
> - * fixed in the 0.96 specification errata, but we have to assume that all 0.95
> - * xHCI hardware can't handle the chain bit being cleared on a link TRB.
> - *
>    * @more_trbs_coming:	Will you enqueue more TRBs before calling
>    *			prepare_transfer()?
>    */
> @@ -228,8 +269,6 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
>   			bool more_trbs_coming)
>   {
>   	u32 chain;
> -	union xhci_trb *next;
> -	unsigned int link_trb_count = 0;
>   
>   	chain = le32_to_cpu(ring->enqueue->generic.field[3]) & TRB_CHAIN;
>   
> @@ -238,48 +277,16 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
>   		return;
>   	}
>   
> -	next = ++(ring->enqueue);
> -
> -	/* Update the dequeue pointer further if that was a link TRB */
> -	while (trb_is_link(next)) {
> -
> -		/*
> -		 * If the caller doesn't plan on enqueueing more TDs before
> -		 * ringing the doorbell, then we don't want to give the link TRB
> -		 * to the hardware just yet. We'll give the link TRB back in
> -		 * prepare_ring() just before we enqueue the TD at the top of
> -		 * the ring.
> -		 */
> -		if (!chain && !more_trbs_coming)
> -			break;
> -
> -		/* If we're not dealing with 0.95 hardware or isoc rings on
> -		 * AMD 0.96 host, carry over the chain bit of the previous TRB
> -		 * (which may mean the chain bit is cleared).
> -		 */
> -		if (!xhci_link_chain_quirk(xhci, ring->type)) {
> -			next->link.control &= cpu_to_le32(~TRB_CHAIN);
> -			next->link.control |= cpu_to_le32(chain);
> -		}
> -		/* Give this link TRB to the hardware */
> -		wmb();
> -		next->link.control ^= cpu_to_le32(TRB_CYCLE);
> -
> -		/* Toggle the cycle bit after the last ring segment. */
> -		if (link_trb_toggles_cycle(next))
> -			ring->cycle_state ^= 1;
> -
> -		ring->enq_seg = ring->enq_seg->next;
> -		ring->enqueue = ring->enq_seg->trbs;
> -		next = ring->enqueue;
> -
> -		trace_xhci_inc_enq(ring);
> -
> -		if (link_trb_count++ > ring->num_segs) {
> -			xhci_warn(xhci, "%s: Ring link TRB loop\n", __func__);
> -			break;
> -		}
> -	}
> +	ring->enqueue++;
> +
> +	/*
> +	 * If we are in the middle of a TD or the caller plans to enqueue more
> +	 * TDs as one transfer (eg. control), traverse any link TRBs right now.
> +	 * Otherwise, enqueue can stay on a link until the next prepare_ring().
> +	 * This avoids enqueue entering deq_seg and simplifies ring expansion.
> +	 */
> +	if (chain || more_trbs_coming)> +		inc_enq_past_link(xhci, ring, chain);

maybe

if (trb_is_link(ring->enqueue) && (chain || more_trbs_coming))
	inc_eng_past_link(xhci, ring, chain);

Avoids calling inc_enq_past_link() every time we increase enqueue, and explains why we call it.


>   }
>   
>   /*
> @@ -3177,7 +3184,6 @@ static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
>   static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
>   		u32 ep_state, unsigned int num_trbs, gfp_t mem_flags)
>   {
> -	unsigned int link_trb_count = 0;
>   	unsigned int new_segs = 0;
>   
>   	/* Make sure the endpoint has been added to xHC schedule */
> @@ -3225,33 +3231,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
>   		}
>   	}
>   
> -	while (trb_is_link(ep_ring->enqueue)) {
> -		/* If we're not dealing with 0.95 hardware or isoc rings
> -		 * on AMD 0.96 host, clear the chain bit.
> -		 */
> -		if (!xhci_link_chain_quirk(xhci, ep_ring->type))
> -			ep_ring->enqueue->link.control &=
> -				cpu_to_le32(~TRB_CHAIN);
> -		else
> -			ep_ring->enqueue->link.control |=
> -				cpu_to_le32(TRB_CHAIN);
> -
> -		wmb();
> -		ep_ring->enqueue->link.control ^= cpu_to_le32(TRB_CYCLE);
> -
> -		/* Toggle the cycle bit after the last ring segment. */
> -		if (link_trb_toggles_cycle(ep_ring->enqueue))
> -			ep_ring->cycle_state ^= 1;
> -
> -		ep_ring->enq_seg = ep_ring->enq_seg->next;
> -		ep_ring->enqueue = ep_ring->enq_seg->trbs;
> -
> -		/* prevent infinite loop if all first trbs are link trbs */
> -		if (link_trb_count++ > ep_ring->num_segs) {
> -			xhci_warn(xhci, "Ring is an endless link TRB loop\n");
> -			return -EINVAL;
> -		}
> -	}
> +	/* Ensure that new TRBs won't overwrite a link */

Maybe add: if (trb_is_link(ep_ring->enqueue)) check here as well

> +	inc_enq_past_link(xhci, ep_ring, 0);
>   
>   	if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue)) {
>   		xhci_warn(xhci, "Missing link TRB at end of ring segment\n");


Otherwise this is a nice cleanup, and the chain bit seems correct, in short:

if quirky host, set chain bit in link TRB before a TD, otherwise clear it.
if quirky host, set chain bit in link TRB mid TD, otherwise set chain bit to
same value as previous TRB.

Re: [PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

[Michał Pecio]

On Fri, 21 Feb 2025 16:54:11 +0200, Mathias Nyman wrote:
> On 21.2.2025 0.47, Michal Pecio wrote:
> > Remove a block of code copied from inc_enq(). As often happens, the
> > two copies have diverged somewhat - in this case, inc_enq() has a
> > bug where it may leave the chain bit of a link TRB unset on a
> > quirky HC. Fix this. Remove the pointless 'next' variable which
> > only aliases ring->enqueue.  
> 
> The linnk TRB chain bit should be set when the ring is created, and
> never cleared on those quirky hosts.

OK, I see, there is stuff in xhci-mem.c. I'll remove the above text
and any code which touches the bit on quirky HCs.

Speaking of which, I have some evidence that NEC uPD720200 has the
exact same bug as AMD, namely after a Missed Service Error near the
end of a segment it fetches TRBs out of bounds and trips the IOMMU
or stops with Ring Underrun. Link chain quirk seems to fix it.

> maybe
> 
> if (trb_is_link(ring->enqueue) && (chain || more_trbs_coming))
> 	inc_eng_past_link(xhci, ring, chain);
> 
> Avoids calling inc_enq_past_link() every time we increase enqueue,
> and explains why we call it.

I can do that too. By the way, do we actually want this while loop in
inc_enq_past_link() at all? Currently links only exist at the end of a
segment and always point to the beginning of the next segment.

I noticed that per xHCI 4.11.7, "Software shall not define consecutive
Link TRBs within a TD". I suppose "consecutive" means "one pointing to
another". And if it's prohibited inside a TD, it will likely always be
easier to avoid doing it at all than try to manage special cases.

Regards,
Michal

Re: [PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

[Mathias Nyman]

On 24.2.2025 1.45, Michał Pecio wrote:
> On Fri, 21 Feb 2025 16:54:11 +0200, Mathias Nyman wrote:
>> On 21.2.2025 0.47, Michal Pecio wrote:
>>> Remove a block of code copied from inc_enq(). As often happens, the
>>> two copies have diverged somewhat - in this case, inc_enq() has a
>>> bug where it may leave the chain bit of a link TRB unset on a
>>> quirky HC. Fix this. Remove the pointless 'next' variable which
>>> only aliases ring->enqueue.
>>
>> The linnk TRB chain bit should be set when the ring is created, and
>> never cleared on those quirky hosts.
> 
> OK, I see, there is stuff in xhci-mem.c. I'll remove the above text
> and any code which touches the bit on quirky HCs.
> 
> Speaking of which, I have some evidence that NEC uPD720200 has the
> exact same bug as AMD, namely after a Missed Service Error near the
> end of a segment it fetches TRBs out of bounds and trips the IOMMU
> or stops with Ring Underrun. Link chain quirk seems to fix it.

Interesting, I wonder if setting the link TRB chain bit would
also help with the TRB prefetch issue on VIA VL805 hosts.

Maybe we could avoid allocating the dummy page by just setting this
quirk instead.
   
> 
>> maybe
>>
>> if (trb_is_link(ring->enqueue) && (chain || more_trbs_coming))
>> 	inc_eng_past_link(xhci, ring, chain);
>>
>> Avoids calling inc_enq_past_link() every time we increase enqueue,
>> and explains why we call it.
> 
> I can do that too. By the way, do we actually want this while loop in
> inc_enq_past_link() at all? Currently links only exist at the end of a
> segment and always point to the beginning of the next segment.
> 
> I noticed that per xHCI 4.11.7, "Software shall not define consecutive
> Link TRBs within a TD". I suppose "consecutive" means "one pointing to
> another". And if it's prohibited inside a TD, it will likely always be
> easier to avoid doing it at all than try to manage special cases.

I don't think that loop is needed. The xhci driver is the only creator
of link TRBs, and at the moment they are placed exactly as you said.

Thanks
Mathias

Re: [PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

[Michał Pecio]

On Mon, 24 Feb 2025 13:49:29 +0200, Mathias Nyman wrote:
> Interesting, I wonder if setting the link TRB chain bit would
> also help with the TRB prefetch issue on VIA VL805 hosts.

Good idea, but unfortunately not.

With xhci_hcd quirks=1, which is XHCI_LINK_TRB_QUIRK:

[    0.543465] pci 0000:0a:00.0: [1106:3483] type 00 class 0x0c0330 PCIe Endpoint

[  406.745905] xhci_hcd 0000:0a:00.0: xHCI Host Controller
[  406.745916] xhci_hcd 0000:0a:00.0: new USB bus registered, assigned bus number 11
[  406.747265] xhci_hcd 0000:0a:00.0: hcc params 0x002841eb hci version 0x100 quirks 0x0000000000000891

[  407.475672] usb 11-1.4: Found UVC 1.00 device USB2.0 Camera (1e4e:0103)

[  407.697768] usb 11-1.4: Selecting alternate setting 12 (3060 B/frame bandwidth)
[  407.700432] usb 11-1.4: Allocated 5 URB buffers of 32x3060 bytes each
[  407.732047] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000]
[  407.732122] xhci_hcd 0000:0a:00.0: WARNING: Host System Error
[  407.732133] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000]
[  407.732151] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000]

Link TRBs from debugfs:

 0 0x00000000ffccbff0: LINK 00000000ffcca000 intr 0 type 'Link' flags i:C:t:C
 1 0x00000000ffccaff0: LINK 00000000ffccb000 intr 0 type 'Link' flags i:C:T:c

Re: [PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

[Mathias Nyman]

On 24.2.2025 23.01, Michał Pecio wrote:
> On Mon, 24 Feb 2025 13:49:29 +0200, Mathias Nyman wrote:
>> Interesting, I wonder if setting the link TRB chain bit would
>> also help with the TRB prefetch issue on VIA VL805 hosts.
> 
> Good idea, but unfortunately not.
> 
> With xhci_hcd quirks=1, which is XHCI_LINK_TRB_QUIRK:
> 
> [    0.543465] pci 0000:0a:00.0: [1106:3483] type 00 class 0x0c0330 PCIe Endpoint
> 
> [  406.745905] xhci_hcd 0000:0a:00.0: xHCI Host Controller
> [  406.745916] xhci_hcd 0000:0a:00.0: new USB bus registered, assigned bus number 11
> [  406.747265] xhci_hcd 0000:0a:00.0: hcc params 0x002841eb hci version 0x100 quirks 0x0000000000000891
> 
> [  407.475672] usb 11-1.4: Found UVC 1.00 device USB2.0 Camera (1e4e:0103)
> 
> [  407.697768] usb 11-1.4: Selecting alternate setting 12 (3060 B/frame bandwidth)
> [  407.700432] usb 11-1.4: Allocated 5 URB buffers of 32x3060 bytes each
> [  407.732047] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000]
> [  407.732122] xhci_hcd 0000:0a:00.0: WARNING: Host System Error
> [  407.732133] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000]
> [  407.732151] xhci_hcd 0000:0a:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffccc000 flags=0x0000]
> 
> Link TRBs from debugfs:
> 
>   0 0x00000000ffccbff0: LINK 00000000ffcca000 intr 0 type 'Link' flags i:C:t:C
>   1 0x00000000ffccaff0: LINK 00000000ffccb000 intr 0 type 'Link' flags i:C:T:c

It was worth a shot,
thanks for trying it out

I'll send the original fix to Greg

-Mathias