xhci: Fix isochronous scheduling gaps on CFC controllers

This series addresses a long-standing issue where the xHCI driver introduces frame-aligned gaps in isochronous streams during timing fluctuations. While many USB devices handle these gaps with a minor glitch, professional interfaces like the MOTU 1248 lose internal channel synchronization when these gaps occur.

I have identified two specific code paths in xhci-ring.c where these gaps are created:

Silent Rescheduling:
When a start frame is "late," the driver adds a +1 frame offset, creating an explicit hole in the stream.

Ring Drain Race:
On CFC-capable controllers, the driver falls back to ASAP scheduling if the ring is momentarily empty, resulting in a jump to a future frame instead of continuing sequentially.

Impact on MOTU 1248:
The MOTU 1248 is particularly sensitive to stream continuity. The two situations described above cause a
shift in the output channels, where audio intended for one set of channels is routed to the wrong physical outputs until the device is reset.

Proposed Solution:
This series modifies the xHCI driver to prefer sequential scheduling on modern CFC-capable controllers:

- Patch 1 removes the silent start_frame_id + 1 reschedule, returning -EINVAL to allow the use of the TRB_SIA (Schedule Immediate After) flag.
- Patch 2 expands the CFC sequential condition to check if a periodic completion is in progress, covering the
"ring drain" race where a new URB is submitted just as the previous one finishes.

These changes build upon recent work by Michał Pecio regarding missed TD handling.

Nicola Lunghi (2):
usb: xhci: fix isoc silent reschedule creating stream gap on CFC
controllers
usb: xhci: fix CFC sequential scheduling lost on ring drain race

 drivers/usb/host/xhci-ring.c | 42 +++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 13 deletions(-)

2.51.0

xhci_get_isoc_frame_id() silently rescheduled the first TRB to start_frame_id+1 when the requested start frame was out of the valid scheduling window or landed exactly on its boundary. This creates an explicit one-frame hole in the isochronous stream.

Most USB audio devices tolerate a brief gap with a small glitch and recover automatically. However, some devices assume that once isochronous packets start streaming they flow continuously until the stream is explicitly stopped. Any gap causes the device firmware to permanently lose channel synchronization — subsequent packets are routed to the wrong output channels until the device is reset.

This was observed with the MOTU 1248 (USB ID 0x07fd:0x0005), where after a gap the 24-channel output stream shifts by a fixed number of channels, mapping audio intended for ch1/ch2 onto ch7/ch8 or other channel pairs depending on timing.

Return -EINVAL instead so the caller falls back to TRB_SIA (Schedule Immediately After), which lets the hardware place the TRB right after the previous one without introducing a frame-aligned gap.

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

index e47e644b296e..03e47db82092 100644
@@ -4030,15 +4030,21 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci,
 			ret = -EINVAL;
 	}
 
+	/*
+	 * If the first TRB's start frame is out of the scheduling window or
+	 * lands exactly on its boundary, fall back to SIA (Schedule Immediately
+	 * After) rather than forcing start_frame_id+1. A forced +1 creates an
+	 * explicit one-frame hole that audio devices with strict continuity
+	 * requirements cannot recover from. The caller handles -EINVAL by
+	 * leaving sia_frame_id as TRB_SIA.
+	 */
 	if (index == 0) {
 		if (ret == -EINVAL || start_frame == start_frame_id) {
-			start_frame = start_frame_id + 1;
-			if (urb->dev->speed == USB_SPEED_LOW ||
-					urb->dev->speed == USB_SPEED_FULL)
-				urb->start_frame = start_frame;
-			else
-				urb->start_frame = start_frame << 3;
-			ret = 0;
+			xhci_dbg(xhci, "isoc: start frame %d %s window [%d, %d], using SIA\n",
+				 start_frame,
+				 ret == -EINVAL ? "behind" : "at boundary of",
+				 start_frame_id, end_frame_id);
+			return -EINVAL;
 		}
 	}
 

2.51.0

Agreed, setting start_frame to start_frame_id + 1 would only make sense for the very first URB, otherwise we create glitches.

Looks like the whole start_frame_id calculation is incorrect.

xhci specification 4.11.2.5 'Start Frame ID' formula sums together several values in frame units, while driver currently sums values in microframes, and then turns it into frames.

xhci 4.11.2.5
"Software should not schedule an Isoch TD with a Frame ID value that is less than
the Start Frame ID, where:

Start Frame ID = (Frame Index of the current MFINDEX register value + IST + 1) MOD 2048 where IST shall be rounded up to the nearest frame boundary if it is defined in microframes"

Looks like we don't consider the ESIT rules for frame id either (xhci 4.11.2.5.1). frame ID must be aligned with starting frames of an ESIT, so if ESIT is 2ms then allowed frame ID values are 0,2,4,6... not 1,3,5..

Thanks
Mathias

That's not the only problem, we are basically violating documented usb_submit_urb() behavior, as Alan points out in Bugzilla.

That's not what this acronym really means.

IIRC, the meaning of "Start Isoch ASAP" is like Linux URB_ISO_ASAP:
"as soon as possible *and not earlier* than possible".

To guarantee no gaps, we should use SIA=0 and incremental Frame IDs on chips with CFC (without CFC it's a lost cause).

The reason using SIA=1 helped you is because in reality:
- the start_frame_id calculated here is pessimistic
- I found that the IST reported by HCs is usually pessimistic too

Therefore, even if this function believes that it's too late to execute some transfer (and currently tries to reschedule it for later), the HC may actually still execute it immediately without gaps if SIA=1.

But the right thing to do is SIA=0 and correctly specified Frame ID. Setting SIA=1 opens the possibility of rescheduling for later (in HW) when it *really* is too late. And we don't want that, we want late submissions to result in Missed Service Error and EXDEV completion.

Note that the comment in this function is bogus too. In reality:

Software *shall* not schedule past end_frame_id. Software *should* not schedule before start_frame_id.

The former is a requirement, the latter a recommendation. And a valid one, if we want the URB to execute. But here, we want it to fail :)

Also, I'm not sure if ignoring submissions far into the future and turning them into SIA=1 is the right action. If a driver submits an untinterrupted stream (without URB_ISO_ASAP) going 895ms into the future, we should probably stop this madness and error out (TBD).

On CFC-capable xHCI controllers, xhci_queue_isoc_tx_prepare() uses next_frame_id for sequential isochronous scheduling only when the endpoint ring is non-empty and the endpoint is running. If all TDs are returned before the next URB is submitted (a ring drain race made more likely by fast TD-skip on xHCI 1.1+ controllers), the condition fails and the URB is scheduled ASAP — potentially many frames ahead of where the stream should continue, creating a gap.

Extend the condition to also use next_frame_id when a periodic completion is in progress for the endpoint, covering the transient window where the ring is empty but the stream has not actually stopped. Guard with URB_ISO_ASAP to preserve explicit ASAP scheduling.

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

index 03e47db82092..116a2dcd0bb2 100644
@@ -4308,14 +4308,24 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
 	 */
 	check_interval(urb, ep_ctx);
 
-	/* Calculate the start frame and put it in urb->start_frame. */
-	if ((xhci->hcc_params & HCC_CFC) && !list_empty(&ep_ring->td_list)) {
-		if (GET_EP_CTX_STATE(ep_ctx) ==	EP_STATE_RUNNING) {
-			urb->start_frame = xep->next_frame_id;
-			goto skip_start_over;
-		}
+	/*
+	 * Calculate the start frame and put it in urb->start_frame.
+	 * On CFC-capable controllers, use sequential scheduling from
+	 * next_frame_id whenever the stream is running or a completion
+	 * is in progress (ring transiently empty due to drain race).
+	 * Skip this for explicit URB_ISO_ASAP requests.
+	 */
+	if ((xhci->hcc_params & HCC_CFC) &&
+	    !(urb->transfer_flags & URB_ISO_ASAP) &&
+	    (!list_empty(&ep_ring->td_list) ||
+	     hcd_periodic_completion_in_progress(xhci_to_hcd(xhci), urb->ep))) {
+		urb->start_frame = xep->next_frame_id;
+		goto skip_start_over;
 	}
 
+	xhci_dbg(xhci, "isoc: CFC sequential skipped for slot %u ep %u (ring_empty=%d), using ASAP\n",
+		 slot_id, ep_index, list_empty(&ep_ring->td_list));
+
 	start_frame = readl(&xhci->run_regs->microframe_index);
 	start_frame &= 0x3fff;
 	/*

2.51.0