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

[Michal Pecio <michal.pecio@gmail.com>]

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.

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

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.

> Any issues we discover later can be fixed when discovered.
> We can't keep finetuning this forever to cover potential issues by
> broken hardware

Well, that's Intel (reportedly) and ASMedia, hence AMD. And they don't
even need to violate xHCI 4.8.3 to break this code.

Regards,
Michal