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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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