[PATCH 0/3] xhci fixes for usb-linus

[PATCH 0/3] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

xhci fixes for usb-linus on top of v7.3-rc1

kernel v7.3-rc1 has a xhci regression reported by several users making
xhci unusable for them. So far at least QEMU and Mediatek MT8173
users are affected.

The patch by Chen-Yu Tsai addresses this.

The two other patches are also nice to have.

Thanks
Mathias

Arthur Gautier (1):
  xhci: fix lost bounce buffers on TDs spanning several ring segments

Chen-Yu Tsai (1):
  usb: xhci: Fix HCS_ERST_MAX conversion

Michal Pecio (1):
  usb: xhci: Fix isochronous scheduling regression

 drivers/usb/host/xhci-mem.c  |  2 +-
 drivers/usb/host/xhci-ring.c | 43 +++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH 1/3] usb: xhci: Fix HCS_ERST_MAX conversion

[Mathias Nyman]

From: Chen-Yu Tsai <wenst@chromium.org>

This fixes one broken line in commit 6d45e9556d4a ("usb: xhci: standardize
multi bit-field macros") included in 7.3-rc1 kernel

HCS_ERST_MAX holds power of 2 value for maximum number of segments.
In the culprit commit, this was incorrectly converted to "shift up 2".
On hardware where this field is zero, this results in xhci_alloc_erst()
calling dma_alloc_coherent() with size = 0, leading to a horrible splat
and non-usable XHCI.

Revert the shift-up-2 to the BIT() macro.

Fixes: 6d45e9556d4a ("usb: xhci: standardize multi bit-field macros")
Cc: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 7a21ac81f9c8..af8d4b74c4ba 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2301,7 +2301,7 @@ xhci_alloc_interrupter(struct xhci_hcd *xhci, unsigned int segs, gfp_t flags)
 	if (!segs)
 		segs = ERST_DEFAULT_SEGS;
 
-	max_segs = FIELD_GET(HCS_ERST_MAX, xhci->hcs_params2) << 2;
+	max_segs = BIT(FIELD_GET(HCS_ERST_MAX, xhci->hcs_params2));
 	segs = min(segs, max_segs);
 
 	ir = kzalloc_node(sizeof(*ir), flags, dev_to_node(dev));
-- 
2.43.0

[PATCH 2/3] usb: xhci: Fix isochronous scheduling regression

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

An isoc URB without URB_ISO_ASAP should be scheduled immediately after
the previous one, unless it's the first submission or prior URBs have
completed without resubmitting and the endpoint became idle.

An HCD_BH driver must consider URBs pending completion in the BH queue
in addition to its own queue. Regrettably, core doesn't provide much
information, we can only know if we are being called by completion now.
This issue is as old as HCD_BH, affects ehci-hcd too and has no known
reproducible impact, as drivers generally resubmit from completion.

A recent patch tried to address it by looking at xHCI HW state instead.
Obviously, HW has no knowledge of the BH giveback queue either, and the
whole solution amounts to testing whether prior URBs have been unlinked
instead of completing normally - then a new stream is assumed.

This leads to false negatives when a driver simply allows the endpoint
to empty out and begins a new stream. New URBs are scheduled into the
past and promptly fail with -EXDEV status, causing data loss and worse,
because drivers get confused by premature completion, particularly when
multiple endpoints are started at once and required to stay in sync.

snd-usb-audio underruns the OUT endpoint when userspace fails to supply
playback data in time. If this is detected in duplex mode, IN URBs are
unlinked and both streams restarted. OUT underruns again before IN even
begins, another recovery is attempted and the cycle repeats.

Fix this by using the best criteria we can muster, taken from ehci-hcd.
This brings false negative rate back to zero and false positive rate to
less than ever before in xhci-hcd. Traditional logic was equivalent to:

	if (list_empty(&ep_ring->td_list) ||
	    GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
		// consider this URB a new stream

While free of false negatives, it had easily avoidable false positives:
* no check for completion in progress when the list is empty
* the ep_ctx check doesn't make up for it at all, but it adds a race -
  EP state can remain "stopped" for a while after the first submission

[mn: add debug message in possible false positive case where driver might
incorrectly assume new stream starts mid stream just because td list is
empty (URB enqueue is late), and workqueue isn't processing URB
completions for this endpoint at the moment]

Link: https://lore.kernel.org/linux-usb/20260813005635.34750f8c.michal.pecio@gmail.com/
Fixes: add8469b3e00 ("xhci: fix frame id calculation and checks for isoc URBs")
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 97a1b53c18ef..9847c5bfc41b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -4312,11 +4312,16 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
 	check_interval(urb, ep_ctx);
 
 	/*
-	 * Check if this starts the isoc data flow. Relies on hw setting ep ctx
-	 * state after doorbell ring. Consider adding list_empty(td_list) check
+	 * Schedule the URB discontiguously if all previous URBs have completed.
+	 * XXX core can't tell if completions are pending but not running yet.
 	 */
-	if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
+	if (list_empty(&ep_ring->td_list) &&
+	    !hcd_periodic_completion_in_progress(xhci_to_hcd(xhci), urb->ep)) {
+		if (GET_EP_CTX_STATE(ep_ctx) == EP_STATE_RUNNING)
+			xhci_dbg(xhci, "Unexpected running ring at isoc stream start, uframe: %d\n",
+				 xep->next_uframe);
 		xep->next_uframe = -1;
+	}
 
 	return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);
 }
-- 
2.43.0

[PATCH 3/3] xhci: fix lost bounce buffers on TDs spanning several ring segments

[Mathias Nyman]

From: Arthur Gautier <baloo@superbaloo.net>

When a TD reaches a link TRB with data that is not aligned to the
endpoint's wMaxPacketSize, xhci_align_td() stages the unalignable tail
through the bounce buffer of the ring segment holding that link TRB.
xhci_unmap_td_bounce_buffer() later unmaps it and, for IN transfers,
copies the data back into the URB's buffer.

The enqueue path records the segment that was bounced in td->bounce_seg,
under the assumption that a TD never spans more than two ring segments.
That assumption does not hold: a TD large enough to span three or more
segments crosses several link TRBs and can be bounced at each of them.
Only the last one survives in td->bounce_seg, so every earlier bounce
buffer is neither copied back nor DMA unmapped.

The URB still completes with actual_length equal to the requested length
and no error, so the transfer looks successful while a wMaxPacketSize
sized hole in the destination buffer silently keeps its previous
contents. It also leaks a DMA mapping per dropped bounce.

Any sufficiently large and fragmented bulk transfer can hit this. It was
found with a USB mass storage device behind xHCI backing a dm-verity
target with 512 byte hash blocks, where the stale data is detected rather
than silently consumed. The device enumerates as SuperSpeed, so
wMaxPacketSize is 1024, while dm-bufio issues one 512 byte bio per hash
block. verity_prefetch_io() makes the block layer merge hundreds of them
into a single request of up to 512 scatterlist entries of 512 bytes each.
At 256 TRBs per ring segment such a TD spans three segments, and every
segment boundary falls on an odd multiple of 512, i.e. unaligned to
wMaxPacketSize. dm-bufio then caches a hash block holding stale data and
dm-verity declares the metadata block corrupted:

  device-mapper: verity: 8:2: metadata block 10850 is corrupted

A reproducer running this under qemu is available at
https://github.com/baloo/xhci-verity

The bounce state (bounce_buf, bounce_dma, bounce_len, bounce_offs)
already lives on the ring segment, so there is nothing extra to track.
Keep recording the last bounced segment in td->bounce_seg and, on
completion, walk the segments from td->start_seg up to it, unmapping
every segment that still has a pending bounce.

Stopping at td->bounce_seg rather than td->end_seg matters: a bounce
implies the TD continues past that segment's link TRB, so bounce_seg is
always strictly before end_seg, and a later TD may already have started
in end_seg and been bounced there. Walking that far would copy a foreign
bounce buffer into this URB and unmap it twice. It also keeps the walk
correct if a TD ever wraps the whole ring so that end_seg == start_seg.

[mn: Add ring->num_segs check to prevent unlikely infinite for loop.]

Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
Cc: stable@vger.kernel.org
Suggested-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9847c5bfc41b..ec278a9f9540 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -824,21 +824,18 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
 	usb_hcd_giveback_urb(hcd, urb, status);
 }
 
-static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
-		struct xhci_ring *ring, struct xhci_td *td)
+static void xhci_unmap_one_bounce_buffer(struct xhci_hcd *xhci,
+		struct xhci_ring *ring, struct xhci_td *td,
+		struct xhci_segment *seg)
 {
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
-	struct xhci_segment *seg = td->bounce_seg;
 	struct urb *urb = td->urb;
 	size_t len;
 
-	if (!ring || !seg || !urb)
-		return;
-
 	if (usb_urb_dir_out(urb)) {
 		dma_unmap_single(dev, seg->bounce_dma, ring->bounce_buf_len,
 				 DMA_TO_DEVICE);
-		return;
+		goto done;
 	}
 
 	dma_unmap_single(dev, seg->bounce_dma, ring->bounce_buf_len,
@@ -854,10 +851,29 @@ static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
 		memcpy(urb->transfer_buffer + seg->bounce_offs, seg->bounce_buf,
 		       seg->bounce_len);
 	}
+done:
 	seg->bounce_len = 0;
 	seg->bounce_offs = 0;
 }
 
+static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
+		struct xhci_ring *ring, struct xhci_td *td)
+{
+	struct xhci_segment *seg;
+	int i = 0;
+
+	if (!td->bounce_seg || !ring || !td->urb)
+		return;
+
+	/* td->bounce_seg is the last one bounced, unmap them all */
+	for (seg = td->start_seg; i++ < ring->num_segs; seg = seg->next) {
+		if (seg->bounce_len)
+			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
+		if (seg == td->bounce_seg)
+			break;
+	}
+}
+
 static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
 			    struct xhci_ring *ep_ring, int status)
 {
@@ -3685,7 +3701,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 						  &trb_buff_len,
 						  ring->enq_seg)) {
 					send_addr = ring->enq_seg->bounce_dma;
-					/* assuming TD won't span 2 segs */
+					/* TD bounced at least, and last on this seg */
 					td->bounce_seg = ring->enq_seg;
 				}
 			}
-- 
2.43.0