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

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

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?

Lets take the underrun case. Audio playback as an relatable example.

If class driver fails to queue more URBs in time mid audio stream then
the ring will trigger underrun. td_list will be empty but hardware
ep ctx will still show the more correct "running" state.

A list empty check would incorrectly assume a new stream started when
the next late URB is queued. It will be given a new future frame_id,
so instead of dropping the data we will delay playback mid stream.
Not the intention with isoc transfers.

With the hardware ep ctx check we will set the frame_id of the late URB to
point to the past, to the real time it was supposed to be played back.
xHC hardware will then trigger a 'missed service event' for that TD as it
was incapable of playing it at the correct time, and then fast forward with MSE
until it finds a TD with a frame_id it can play, and play at the correct timeslot.
Here we lost the late data but playback stays in sync, as isoc transfer are
intended to work.


>>> Moreover, we know that Endpoint Context state is one of the least
>>> trustworthy piecese of information, and notably it may remain
>>> Stopped for a while after a doorbell ring. This means that multiple
>>> URBs may be scheduled as new "streams" and get assigned the same
>>> start_frame.
>>
>> Ok, we basically have different opinion on how to identify when a new
>> isoc data flow starts. You would rely on checking if list is empty,
>> and I'm looking at harware endpoint state.
> 
> I would rely on SW state, sure.
> 
> 1. The check implemented here isn't even right - see above.
> 2. Bogosity of xHCI HW is well known - see below.
> 3. For consistency with other HCDs, which use similar SW state.
> 
>> I don't trust the 'empty list check' as I'm not convinced there can't
>> be cases in software where list is empty even mid isoc data flow.
>> Something like extreme long interval and BH delay in class driver
>> receiving URB.
> 
> Other HCDs implement a rule that resubmission from completion callback
> on the same endpoint always counts as continuation of the stream, to
> work around BH. This seems to have worked so far and is trivial to do,
> somebody submitted a patch earlier this year.
> 
>> We can add empty list check as well in some places.
>> For example xhci_isoc_tx_prepare() could benefit from having both
>> empty list and hw endpoint state check when setting next_uframe = -1;
> 
> The HW state check is bogus. Before removing it I patched to log when
> URBs are pending but HW state is still Stopped. It happens sometimes.
> 
> The only robust solution is to completely disregard EP Context state.
> xHCI 4.8.3 doesn't promise much and outright advises SW to track actual
> HW state itself, based on issued commands etc.
> 

Disagree, see underrun example above.

But we can add an empty list check in addition to checking hw ep ctx state.
This should cover both empty list mid stream cases, and slow ep ctx
state change by HW.

That's a one line change during during 7.3 rc cycle

> The Stopped => Running transition is only guaranteed to become visible
> to SW when the first service interval passes and an event is posted.
> 
>> This patch is tested, solves an existing issue, and moves the
>> frame_id code in the right direction.
> 
> To be fair, it may be that in practice it fixes more than it breaks,
> but it also strays away from documented USB subsystem rules and looks
> half-baked, so I was surprised to see it queued in such state.

It's been a year, nobody else cooked anything, it improves current situation.
And honestly, it's not that bad.
Real disagreement is one line regarding how we check if we are mid stream.

Thanks
Mathias