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

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

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)

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

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

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.

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