[PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Nicola Lunghi]

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.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=220748
Assisted-by: Claude:claude-sonnet-4-6 sparse checkpatch
Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index e47e644b296e..03e47db82092 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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

Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Mathias Nyman]

On 5/5/26 02:31, Nicola Lunghi wrote:
> 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.

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

Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Dylan Robinson]

On 5/5/26 16:32, Mathias Nyman wrote:
> 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.

I'd also like to call attention to the fact that index in
xhci_get_isoc_frame_id() refers to the isoc packet index within the
URB, not the position of the transfer in the overall stream. A driver
could (although probably shouldn't) submit multiple URBs, each
describing less than a microframe's worth of transfers, so index == 0
does not guarantee that the computation is for a frame-aligned
transfer.

Additionally, urb->start_frame is initially computed in
xhci_queue_isoc_tx_prepare(), and in the current implementation it is
validated, and potentially modified again in xhci_get_isoc_frame_id().
It is worth considering that xhci_queue_isoc_tx_prepare() computes a
start frame close to the current IST, and if the system is preempted
before xhci_queue_isoc_tx() runs and calls xhci_get_isoc_frame_id(),
that start frame may already fall outside the valid scheduling window.

Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Alan Stern]

On Wed, May 06, 2026 at 03:55:27PM -0400, Dylan Robinson wrote:
> On 5/5/26 16:32, Mathias Nyman wrote:
> > 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.
> 
> I'd also like to call attention to the fact that index in
> xhci_get_isoc_frame_id() refers to the isoc packet index within the
> URB, not the position of the transfer in the overall stream. A driver
> could (although probably shouldn't) submit multiple URBs, each
> describing less than a microframe's worth of transfers, ...

How can this happen?  The only way for an URB to describe less than a 
microframe's worth of transactions is if it describes no transactions at 
all.

Did you mean less than a frame's worth?

Alan Stern

Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Dylan Robinson]

On Wed, May 6, 2026 at 10:25 PM Alan Stern wrote:
> How can this happen?  The only way for an URB to describe less than a
> microframe's worth of transactions is if it describes no transactions at
> all.
>
> Did you mean less than a frame's worth?

Sorry for the confusion. Yes, less than a frame’s worth.

Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Mathias Nyman]

On 5/6/26 22:55, Dylan Robinson wrote:
> On 5/5/26 16:32, Mathias Nyman wrote:
>> 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.
> 
> I'd also like to call attention to the fact that index in
> xhci_get_isoc_frame_id() refers to the isoc packet index within the
> URB, not the position of the transfer in the overall stream. A driver
> could (although probably shouldn't) submit multiple URBs, each
> describing less than a microframe's worth of transfers, so index == 0
> does not guarantee that the computation is for a frame-aligned
> transfer.

ep->next_frame_id should be used for frame ID calculation after
the first URB is enqueued and endpoint running.

Need to makse sure it's correct, and in microframe units, and rounded to
to the correct frame for the TRB Frame ID field.

> 
> Additionally, urb->start_frame is initially computed in
> xhci_queue_isoc_tx_prepare(), and in the current implementation it is
> validated, and potentially modified again in xhci_get_isoc_frame_id().
> It is worth considering that xhci_queue_isoc_tx_prepare() computes a
> start frame close to the current IST, and if the system is preempted
> before xhci_queue_isoc_tx() runs and calls xhci_get_isoc_frame_id(),
> that start frame may already fall outside the valid scheduling window.

Hmm, looks like we are doing way too much checking here.
All the above are done with spin_lock_irqsave() held, meaning there
shouldn't be any delay by preemptions or interrupts.

re-reading the MFINDEX (microframe index) register for every TD in this
URB looks like a waste of time.
xhci_queue_isoc_tx_prepare() should read MFINDEX once, then:

a) If first URB (no ep->next_frame_id value) then calculate a proper
    start frame ID, and check last TD of urb still fits the widow

b) if ep->next_frame_id is set, then check if first TD of this URB
    can make it in time, also check last TD of urb is within window.
    I think all other register reads and checks in xhci_get_isoc_frame_id()
    can be skipped.
   (I'm confident that the non-preemtable loop queuing TRBs
    does a cycle faster than shortest isoc interval (125us)

Can I ask you to test some additional patches if I write them?
I don't have a good setup with frame sensitive usb devices to test this

Thanks
Mathias

Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Dylan Robinson]

On Thu, May 7, 2026 at 8:12 AM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> ep->next_frame_id should be used for frame ID calculation after
> the first URB is enqueued and endpoint running.

Agreed.

> Hmm, looks like we are doing way too much checking here.
> All the above are done with spin_lock_irqsave() held, meaning there
> shouldn't be any delay by preemptions or interrupts.
>
> re-reading the MFINDEX (microframe index) register for every TD in this
> URB looks like a waste of time.

Ah, I missed the spinlock, but I agree that the current implementation
involves unnecessary checks.

>    (I'm confident that the non-preemtable loop queuing TRBs
>     does a cycle faster than shortest isoc interval (125us)

That's good. I think a related issue to watch out for is preemption
occurring between URB submissions.

For example, snd-usb-audio submits multiple 1ms URBs at startup. If
preemption occurs between these URB submissions the first URB might
complete before the next one gets enqueued.

While working on an experimental audio driver to avoid this issue, I
found that many test systems required the first submitted URB to
contain more than 10 ms of transfers in order to reliably submit a
second URB before the stream underruns.

> Can I ask you to test some additional patches if I write them?
> I don't have a good setup with frame sensitive usb devices to test this

I would be happy to test some patches!

Dylan

Re: [PATCH 1/2] usb: xhci: fix isoc silent reschedule creating stream gap on CFC controllers

[Michal Pecio]

On Tue,  5 May 2026 01:31:43 +0200, Nicola Lunghi wrote:
> 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.

That's not the only problem, we are basically violating documented
usb_submit_urb() behavior, as Alan points out in Bugzilla.
 
> 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)

That's not what this acronym really means.

> which lets the hardware place the TRB right after the previous one
> without introducing a frame-aligned gap.

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).

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=220748
> Assisted-by: Claude:claude-sonnet-4-6 sparse checkpatch
> Signed-off-by: Nicola Lunghi <nick83ola@gmail.com>
> ---
>  drivers/usb/host/xhci-ring.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c
> b/drivers/usb/host/xhci-ring.c index e47e644b296e..03e47db82092 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -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);

I'm not very fond of debug prints which can show every microframe.

Even better than a "debuggable" code is one which is right, well tested
and known to work without worries.

> +			return -EINVAL;
>  		}
>  	}
>  
> -- 
> 2.51.0
>