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

[Alan Stern]

On Mon, Aug 24, 2026 at 06:21:22PM +0300, Mathias Nyman wrote:
> On 8/22/26 05:38, Alan Stern wrote:
> > 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.
> 
> This sounds like a good idea.
> 
> Regarding theoretical race conditions:  If URBs for some odd and unknown
> reason are submitted by several callers to the same endpoint (no idea why
> or how this would happen) then, usb_hcd_submit_urb() could theoretically call
> hcd->driver->urb_enqueue(hcd) for the second URB right when .urb_enqueue(hcd)
> returned for the first URB, but _before_ increasing the atomic counter.
> 
> In this case hcd would incorrectly assume both first and second URB start a
> new isoc stream. This would be avoided by hcd still checking list_empty(td_list)

This is why my original suggestion had the HCD incrementing the counter 
while holding its private lock.

> Anyway, suggested solution still sounds like an improvement compared to what
> we currently have.

> > Possible alternative: Make the URB_ISO_ASAP flag take precedence over
> > the "queue is non-empty" condition.
> 
> xhci driver does this. If URB_ISO_ASAP is set then xhci driver always sets
> the SIA "Start Isoch ASAP" flag for the transfer blocks.

I wasn't very precise before.  I meant URB_ISO_ASAP should take 
precedence when there are no active URBs but there may still be some 
URBs being given back.  In other words, when the list_empty test 
succeeds.  If the list of queued URBs is not empty then new URBs should 
always be assigned to the next available slot -- unless we decide to 
support a new URB_USE_FRAME flag and the flag is set.

The point here is that if the class driver wants to change the alignment 
between URBs and uframes, without worrying about the BH giveback race, 
all it has to do is set URB_ISO_ASAP.  If it doesn't care about the 
alignment (implying that it also doesn't care if some URBs are assigned 
to expired slots) then the race doesn't matter.

And of course, if the class driver wants to maintain the alignment then 
it should resubmit URBs from the completion handler, so that the queue 
doesn't empty out unless there is an underrun.  Then the BH giveback 
race won't be an issue.

> The urb->start_frame value set by xhci driver might not be correct in this
> case.

Do you mean it might be wrong because xhci-hcd can't tell what uframe 
the xHC hardware actually selects?

Alan Stern