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

On 8/7/26 11:26, Michal Pecio wrote:
> On Fri, 7 Aug 2026 03:07:08 +0300, Mathias Nyman wrote:
>> On 8/7/26 01:01, Michal Pecio wrote:
>>> On Thu, 6 Aug 2026 16:25:15 +0300, Mathias Nyman wrote:
>>>> On 8/6/26 11:17, Michal Pecio wrote:
>>>>> Besides initialization, it's made -1 if and only if xHCI endpoint
>>>>> state isn't Running at the time of submission. This effectively
>>>>> means that to start a new "isoch data flow" aka "stream", class
>>>>> driver must unlink the last remaining URB of the previous flow. If
>>>>> the driver doesn't unlink (or tries too late), the endpoint goes
>>>>> Running-Idle.
>>>>
>>>> If class driver doesn't unlink remaining urbs then list won't be
>>>> empty and you have the exact same situation.
>>>
>>> Obviously not, it can simply stop resubmitting and wait. Or there
>>> may be no URBs left to unlink at all, when recovering from ring
>>> underrun.
>>>
>>> Then HW endpoint state will still be Running, not Stopped, and new
>>> URBs will be scheduled into the past, doomed to complete with
>>> -EXDEV.
>>
>> Isn't this exactly what we want?
> 
> This was discussed all the way back in Bugzilla - usb_submit_urb()
> kerneldoc spells out what we want and Alan was clear that it reflects
> what HCDs have been doing for 20 years:
> 

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.

The logic is from Alan's bugzilla comment. I'll quote the beginning here:

"When the queue underruns is exactly when URB_ISO_ASAP is supposed to matter.
  If the flag is set in the new URB then that URB should be scheduled for the
  first slot in the future, leaving a logical gap in the queue.
  If the flag is clear then the new URB is supposed to be scheduled for the slot
  that follows the preceding URB, which means that some of its packets will
  never be sent because their slots have already expired.
  This will still leave a physical gap in the queue, of course -- no way to
  avoid that -- but it will maintain the logical alignment of URBs and frames.

  Thus by submitting all URBs with URB_ISO_ASAP clear, drivers can help ensure
  that the queue remains synchronized to within the limits imposed by the host
  controller driver.
  If xhci-hcd doesn't behave this way then it should be changed."

So I changed it to behave this way.

>   * If the driver is unable to keep up and the queue empties out, the
>   * behavior for new submissions is governed by the URB_ISO_ASAP flag.
>   * If the flag is set, or if the queue is idle, then the URB is always
>   * assigned to the first available (and not yet expired) slot in the
>   * endpoint's schedule.

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

Isn't it more likely that 'queue is idle' means 'not yet started' than
'class driver intentionally left it to dry out and underrun in order
to later restart it, assuming it will restart ASAP on next URB enqueue
without setting URB_ISO_ASAP flag?'

Thanks
Mathias