[PATCH] usb: xhci: Fix isochronous scheduling regression

[PATCH] usb: xhci: Fix isochronous scheduling regression

[Michal Pecio]

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

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>
---
 drivers/usb/host/xhci-ring.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f27bc132d0e9..8b0c27d6f12d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -4311,10 +4311,11 @@ 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))
 		xep->next_uframe = -1;
 
 	return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);
-- 
2.48.1

Re: [PATCH] usb: xhci: Fix isochronous scheduling regression

[Mathias Nyman]

On 8/21/26 12:47, Michal Pecio wrote:
> 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

I would still prefer:
if (list_empty(&ep_ring->td_list) && GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
to detect the start of a new isoch stream.

It has zero false positives mid stream, and fixes the "stopped" state race case.

I can't figure out when the false negatives it introduces is an issue.

If you can show me a usecase where a driver or specification is designed to let an
isoch endpoint intentionally run dry, leaving it in running/idle, and then continue
queuing URBs expecting ASAP scheduling, then we can change it.

UAC and UVC go to altsetting 0 and drop the isoch endpoint on stop/pause (says AI)
> 
> 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>
> ---
>   drivers/usb/host/xhci-ring.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index f27bc132d0e9..8b0c27d6f12d 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -4311,10 +4311,11 @@ 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))
>   		xep->next_uframe = -1;

I don't know why we intentionally should introduce the false positive case,
hiding a known issue.

At least we should add a debug message so audio/video developers can find the reason for
the glitches:

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, "Starting new isoc stream on running endpoint at uframe %d, killing sync...
	xep->next_uframe = -1;
}

Thanks
Mathias

Re: [PATCH] usb: xhci: Fix isochronous scheduling regression

[Michal Pecio]

On Fri, 21 Aug 2026 15:05:53 +0300, Mathias Nyman wrote:
> On 8/21/26 12:47, Michal Pecio wrote:
> > 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  
> 
> I would still prefer:
> if (list_empty(&ep_ring->td_list) && GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
> to detect the start of a new isoch stream.
> 
> It has zero false positives mid stream, and fixes the "stopped" state
> race case.
> 
> I can't figure out when the false negatives it introduces is an issue.
> 
> If you can show me a usecase where a driver or specification is
> designed to let an isoch endpoint intentionally run dry, leaving it
> in running/idle, and then continue queuing URBs expecting ASAP
> scheduling, then we can change it.

Well, snd-usb-audio does. In snd_complete_urb(), in the
usb_pipeout(ep->pipe) case and ep->lowlatency_playback, which is
the default (controlled by 'lowlatency' module parameter).

It adds the completed URB to a queue of "ready" URBs and then tries
to submit as many URBs from that queue as possible, which may be
zero if there is no data to send. Then pending URB count is checked
for zero, which triggers xrun recovery in ALSA core.

I haven't dug into ALSA, but the observable outcome is:

snd_usb_endpoint_stop() is called on both IN and OUT, which unlinks
IN URBs and does nothing to OUT (zero URBs left).

snd_usb_endpoint_start() is called on both endpoints to submit URBs.

See dynamic debug below. Notably, "Setting usb interface" does appear,
but only at the end, when I killed jackd. And it seems suboptimal to
demand that drivers spend time on control requests in such cases.

[146029.539992] xhci_hcd 0000:06:00.0: Stopped on Transfer TRB for slot 1 ep 2
[146029.540031] xhci_hcd 0000:06:00.0: Underrun event on slot 1 ep 1
[146029.542857] usb 6-2: Starting data EP 0x81 (running 0)
[146029.542949] usb 6-2: 12 URBs submitted for EP 0x81
[146029.542958] usb 6-2: 2:2 Start Capture PCM
[146029.542962] usb 6-2: Starting data EP 0x1 (running 0)
[146029.542991] usb 6-2: 2 URBs submitted for EP 0x1
[146029.542996] usb 6-2: 1:3 Start Playback PCM
[146029.543799] usb 6-2: notify_xrun ep 01 from line 579	<-- my patch
[146029.543816] usb 6-2: Stopping data EP 0x81 (running 1)
[146029.543861] usb 6-2: 2:2 Stop Capture PCM
[146029.543863] usb 6-2: Stopping data EP 0x1 (running 1)
[146029.543865] usb 6-2: 1:3 Stop Playback PCM
[146029.543882] xhci_hcd 0000:06:00.0: Stopped on Transfer TRB for slot 1 ep 2
[146029.543886] xhci_hcd 0000:06:00.0: Underrun event on slot 1 ep 1
[146029.545849] usb 6-2: Starting data EP 0x81 (running 0)
[146029.545962] usb 6-2: 12 URBs submitted for EP 0x81
[146029.545969] usb 6-2: 2:2 Start Capture PCM
[146029.545972] usb 6-2: Starting data EP 0x1 (running 0)
[146029.545998] usb 6-2: 2 URBs submitted for EP 0x1
[146029.546001] usb 6-2: 1:3 Start Playback PCM
[146029.546793] usb 6-2: notify_xrun ep 01 from line 579
[146029.546814] usb 6-2: Stopping data EP 0x81 (running 1)
[146029.546876] usb 6-2: 2:2 Stop Capture PCM
[146029.546881] usb 6-2: Stopping data EP 0x1 (running 1)
[146029.546885] usb 6-2: 1:3 Stop Playback PCM
[146029.546903] xhci_hcd 0000:06:00.0: Underrun event on slot 1 ep 1
[146029.546909] xhci_hcd 0000:06:00.0: Stopped on Transfer TRB for slot 1 ep 2
[146029.557837] usb 6-2: Closing EP 0x81 (count 1)
[146029.557845] usb 6-2: Setting usb interface 2:0 for EP 0x81
[146029.558636] usb 6-2: EP 0x81 closed
[146029.558664] usb 6-2: Closing EP 0x1 (count 1)
[146029.558666] usb 6-2: Setting usb interface 1:0 for EP 0x1
[146029.559505] usb 6-2: EP 0x1 closed

0000:06:00.0 is ASM1042, USB 3.0, no CFC. Hence no Missed Service
before the Underrun. OUT URBs seem to complete successfully, but
ALSA apparently doesn't feed playback data until the IN endpoint
starts moving, which is scheduled to happen later, so OUT underruns
again and the whole mess repeats.

On CFC, all OUT URBs complete 1 uframe after submisison with MSE,
so it's pretty much guaranteed to happen before any IN is scheduled.

Up to v7.2, such xrun is only a brief audible glitch. On usb-next
it loops like that for seconds to minutes before recovering by
some lucky miracle (IDK why that happens, but sometimes it panics
and begins unlinking OUT URBs before the last one completes).

Hence the idea to not rock the boat too much and stay with zero
false negatives and only the unavoidable false postives which have
been here in HCD_BH drivers for ages.

The whole thing is madness. It nobody has issues with wasting 4 bytes
per endpoint, I think it could be fixed by counting all pending URBs
from submit to complete. Changes to ehci/xhci-hcd will be minimal. Or
add a flag which overrides this guesswork. But too late for v7.3.

> At least we should add a debug message so audio/video developers can
> find the reason for the glitches:
> 
> 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, "Starting new isoc stream on running endpoint at uframe %d, killing sync..
> 		xep->next_uframe = -1;
> }

That seems harmless, although the same information can be obtained by
checking urb->start_frame after submission.

Fun fact, I found only two in-tree drivers which do anything meaningful
with it. Particularly, if snd-usb-audio submits IN and OUT across frame
boundary, it can only recover by xruning and trying again, meh.

Regards,
Michal

Re: [PATCH] usb: xhci: Fix isochronous scheduling regression

[Alan Stern]

On Fri, Aug 21, 2026 at 11:47:06AM +0200, Michal Pecio wrote:
> Fix this by using the best criteria we can muster, taken from ehci-hcd.

Maybe it's time to correct the hcd_periodic_completion_in_progress() 
implementation.

For instance, we could add an atomic giveback_count field to the 
usb_host_endpoint struct.  The HCD would increment the field (while 
still holding its private lock) before doing a giveback, and 
__usb_hcd_giveback_urb() would decrement the field after calling the 
completion handler.

What do you think?

Alan Stern

Re: [PATCH] usb: xhci: Fix isochronous scheduling regression

[Michal Pecio]

On Fri, 21 Aug 2026 10:44:07 -0400, Alan Stern wrote:
> Maybe it's time to correct the hcd_periodic_completion_in_progress() 
> implementation.
> 
> For instance, we could add an atomic giveback_count field to the 
> usb_host_endpoint struct.  The HCD would increment the field (while 
> still holding its private lock) before doing a giveback, and 
> __usb_hcd_giveback_urb() would decrement the field after calling the 
> completion handler.
> 
> What do you think?

I would go as far as incrementing it on successful usb_submit_urb() and
completely doing away with those list_empty(td_list) checks in HCDs.

I wrote an xhci-only (less compilation and module reloading) prototype
which relies on hijacking completions of isoc URBs for counting, it
worked, results identical as with the standard solution in a few test
runs with snd-usb-audio.

Theoretical race condition: it seems we can't prevent new submissions
after completion releases its lock and class driver considers the pipe
idle, but before the counter is decremented to zero. That would be
another case of "scheduling to the past" unexpectedly.

Seems low probability, but this type of bug hasn't existed so far, we
generally have the opposite problem.

Or does it exist in non-BH HCDs?

Maybe the documented API guarantee just isn't feasible?

Entirely out of the box alternative: new URB flag. And really, if
only xhci-hcd existed, it wouldn't be hard to even implement explicit
start_frame requests or hints as MOTU wished for, which would make
the whole business of starting synchronized endpoints cleaner.

Regards,
Michal

Re: [PATCH] usb: xhci: Fix isochronous scheduling regression

[Alan Stern]

On Fri, Aug 21, 2026 at 06:03:50PM +0200, Michal Pecio wrote:
> On Fri, 21 Aug 2026 10:44:07 -0400, Alan Stern wrote:
> > Maybe it's time to correct the hcd_periodic_completion_in_progress() 
> > implementation.
> > 
> > For instance, we could add an atomic giveback_count field to the 
> > usb_host_endpoint struct.  The HCD would increment the field (while 
> > still holding its private lock) before doing a giveback, and 
> > __usb_hcd_giveback_urb() would decrement the field after calling the 
> > completion handler.
> > 
> > What do you think?
> 
> I would go as far as incrementing it on successful usb_submit_urb() and
> completely doing away with those list_empty(td_list) checks in HCDs.
> 
> I wrote an xhci-only (less compilation and module reloading) prototype
> which relies on hijacking completions of isoc URBs for counting, it
> worked, results identical as with the standard solution in a few test
> runs with snd-usb-audio.
> 
> Theoretical race condition: it seems we can't prevent new submissions
> after completion releases its lock and class driver considers the pipe
> idle, but before the counter is decremented to zero. That would be
> another case of "scheduling to the past" unexpectedly.
> 
> Seems low probability, but this type of bug hasn't existed so far, we
> generally have the opposite problem.
> 
> Or does it exist in non-BH HCDs?

It does.  It's impossible for a non-BH HCD to reacquire its private lock 
exactly when the completion handler returns, so somewhere around that 
time the class driver and the HCD will inevitably have differing 
opinions about whether the iso queue is empty.

> Maybe the documented API guarantee just isn't feasible?

I'm not averse to your proposed solution.  As for that race, just update 
the API documentation to take it into account.  Seriously -- it's an 
unavoidable defect and that's all we can do about it.

Possible alternative: Make the URB_ISO_ASAP flag take precedence over 
the "queue is non-empty" condition.

> Entirely out of the box alternative: new URB flag. And really, if
> only xhci-hcd existed, it wouldn't be hard to even implement explicit
> start_frame requests or hints as MOTU wished for, which would make
> the whole business of starting synchronized endpoints cleaner.

Yes.  Something like an URB_USE_START_FRAME flag.  It wouldn't help much 
for starting a single endpoint, but it would allow drivers to 
synchronize new iso streams with existing ones more easily.  (Although 
there would be a little uncertainty in cases where the schedules for the 
two streams don't have the same phase.)

Alan Stern