Re: [RFT PATCHv3 1/3] xhci: fix frame id calculation and checks for isoc URBs

On 8/13/26 01:56, Michal Pecio wrote:
> On Fri, 7 Aug 2026 14:10:35 +0300, Mathias Nyman wrote:
>> I'm doing exactly what kerneldoc and comments in bugzilla state.
>> If queue empties out we schedule the next TD based on URB_ISO_ASAP
>> flag.
> 
>>> "If the queue is idle" means all completions have already returned.
>>
>> That's an odd interpretation of "queue is idle"
>>
>> This would mean that an URB queued after "queue runs out" (underrun)
>> should always be treated as USB_ISO_ASAP case, making the flag
>> useless.
> 
> Well, there are HW queues and SW queues, and HW and SW underruns.
> 
> If the HW queue underruns but the SW queue still has pending URBs
> (not completed yet) then we do care about URB_ISO_ASAP. If the SW
> queue is completely empty (all completed), we aren't expected to.
> 
> 
> This does make a difference with snd-usb-audio. If I run
> 
> jackd -d alsa -d hw:... -p 24 -n 2
> 
> Ring Underrun and Missed Service are reported every now and then,
> sometimes repeatedly, and it doesn't take long to enter this loop:
> 
> 1. playback Ring Underrun (xHCI EP state is Running)
> 2. capture URB unlink (xHCI EP state is Stopped)
> 3. multiple capture URBs scheduled to the same start_frame before
>     EP state becomes Running

agree, and this needs to be fixed with something like:

-       if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
+       if (list_empty(&ep_ring->td_list) &&
+           GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)

> 4. new playback URB is scheduled into a distant past due Running EP
> 5. 2 uframes later OUT endpoint reports Missed Service for all
>     those misscheduled TDs
> 6. goto 1
> 
> Changing the condition from "running endpoint" back to "empty list"
> breaks this pathological cycle. Experimental patch below.
> 

But this is different from incorrectly assuming a new stream started
mid stream just because list_empty(&ring->td_list) is true and the urb completion
workqueue isn't at this instance processing a work item belonging to
this endpoint.
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1ff84ab9955a..8e380d1c066e 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -4303,6 +4303,12 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>   	return ret;
>   }
>   
> +static int _ep(int ep_index)
> +{
> +	ep_index++;
> +	return ep_index / 2 + 0x80 * (ep_index & 1);
> +}
> +
>   /*
>    * Check transfer ring to guarantee there is enough room for the urb.
>    * Update ISO URB start_frame and interval.
> @@ -4348,7 +4354,22 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
>   	 * 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
>   	 */
> -	if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
> +	bool running = GET_EP_CTX_STATE(ep_ctx) == EP_STATE_RUNNING;
> +
> +	bool pending = !list_empty(&ep_ring->td_list) ||
> +		       hcd_periodic_completion_in_progress(xhci_to_hcd(xhci), urb->ep);

The 'pending' is still not reliable and may be false mid stream even when
list_empty() is true .

hcd_periodic_completion_in_progress() only returns true if workqueue is
currently handling a URB that belongs to this endpoint.

If workqueue doesn't yet handle any work item, or handles an URB belonging to any other
endpoint on this or any other device on this bus it will return false.

All periodic devices on this bus share this one workqueue.

If we incorrectly assume a new stream started mid stream then it will become
out of sync even if CFC is supported.

Thanks
Mathias