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

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

On 8/6/26 11:17, Michal Pecio wrote:

>> +	/* calculate valid frame window, in frame units, see xhci 4.11.2.5 */
>> +	curr_frame = MFINDEX_TO_FRAME(mfindex);
>> +	win_start = (curr_frame + DIV_ROUND_UP_POW2(ist, 8) + 1) % MAX_FRAMES;
>> +	win_end = (curr_frame + 895) % MAX_FRAMES;
>>   
>> -	start_frame &= 0x7ff;
>> -	start_frame_id = (start_frame_id >> 3) & 0x7ff;
>> -	end_frame_id = (end_frame_id >> 3) & 0x7ff;
>> +	// FIXME, used to check if !list_empty(&ep_ring->td_list)), is that reliable
> 
> This doesn't look very professional :)
> 
> (Surprised to find that checkpatch doesn't warn about C++ comments...)
> 

I'll remove that comment,  but didn't it help highlight the !list_empty
case for you :)

> But most importantly, as Alan explained, list_empty() was indeed the
> specific check which should be performed.
> 
> * If there are pending URBs on the endpoint, a new non-ASAP URB is
>    scheduled contiguously after the last one.
> * If there are no pending URBs, new non-ASAP URB is scheduled ASAP.
> 
> (The exact meaning of "pending" is a little tricky in presence of BH
> giveback, but that's another issue and it also affects ehci-hcd).
> 
>>   
>> -	if (start_frame_id < end_frame_id) {
>> -		if (start_frame > end_frame_id ||
>> -				start_frame < start_frame_id)
>> -			ret = -EINVAL;
>> -	} else if (start_frame_id > end_frame_id) {
>> -		if ((start_frame > end_frame_id &&
>> -				start_frame < start_frame_id))
>> -			ret = -EINVAL;
>> +	/* Is this the first URB starting the whole isoc transfer */
> 
> Nitpick: xHCI calls this "isoch data flow" and ehci-hcd "stream".
> 
> Isn't "isoc transfer", per USB spec, things that happens during one
> interval? But not 100% sure about it and too lazy to check now...
> 
>> +	if (ep->next_uframe < 0) {
> 
> And where is ep->next_uframe coming from?

First set in xhci_queue_isoc_tx_prepare(), and then set after every URB
enque in xhci_queue_isoc_tx()

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

Checking if list is empty doesn't help here.

> 
> So you have removed the wrong condition here. It should be empty list,
> not xHCI endpoint state, which drivers don't care or even know about.

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

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;

But we are late in the rc cycle and merge window opens soon, so I'm going
to submit this anyway to get things forward.

This patch is tested, solves an existing issue, and moves the frame_id code
in the right direction.

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

Thanks
Mathias