New: usb: xhci_queue_isoc_tx_prepare ignore start_frame and always assumes URB_ISO_ASAP is set

https://bugzilla.kernel.org/show_bug.cgi?id=220748

Bug ID: 220748
Summary: usb: xhci_queue_isoc_tx_prepare ignore start_frame and
always assumes URB_ISO_ASAP is set
Product: Drivers
Version: 2.5
Hardware: All
OS: Linux
Status: NEW
Severity: normal
Priority: P3
Component: USB
Assignee: [email protected]
Reporter: [email protected]
Regression: No

This commit implemented Isochronous transfer for the xHCI controllers and in the first implementation always assumes URB_ISO_ASAP is set and ignores the start_frame field.

"Always assume URB_ISO_ASAP is set, and never use urb->start_frame as input."

https://github.com/torvalds/linux/commit/04e51901dd44f40a5a385ced897f6bca87d5f40a

```
USB: xHCI: Isochronous transfer implementation
This patch implements isochronous urb enqueue and interrupt handler part.

When an isochronous urb is passed to xHCI driver, first check the transfer
ring to guarantee there is enough room for the whole urb. Then update the
start_frame and interval field of the urb. Always assume URB_ISO_ASAP
is set, and never use urb->start_frame as input.

```

On this line the scheduler always schedule as soon as possible

https://github.com/torvalds/linux/blob/17d85f33a83b84e7d36bc3356614ae06c90e7a08/drivers/usb/host/xhci-ring.c#L4332-L4333

        start_frame = readl(&xhci->run_regs->microframe_index);
        start_frame &= 0x3fff;

instead for example in drivers/usb/host/xhci-ring.c, function xhci_queue_isoc_tx_prepare() the URB_ISO_ASAP flag is correctly used

https://github.com/torvalds/linux/blob/17d85f33a83b84e7d36bc3356614ae06c90e7a08/drivers/usb/host/ehci-sched.c#L1638-L1654

                /* URB_ISO_ASAP: make sure that start >= next */
                if (unlikely(start < next &&
                                (urb->transfer_flags & URB_ISO_ASAP)))
                        goto do_ASAP;


                /* Otherwise use start, if it's not in the past */
                if (likely(start >= now2))
                        goto use_start;

...

use_start:

        /* Tried to schedule too far into the future? */
        if (unlikely(start + span - period >= mod + wrap)) {
                ehci_dbg(ehci, "request %p would overflow (%u+%u >= %u)\n",
                                urb, start, span - period, mod + wrap);
                status = -EFBIG;
                goto fail;
        }


        start += base;
        stream->next_uframe = (start + skip) & (mod - 1);


        /* report high speed start in uframes; full speed, in frames */
        urb->start_frame = start & (mod - 1);
        if (!stream->highspeed)
                urb->start_frame >>= 3;
        return status;

This causes issues with devices that require a specific frame interval (like professional audio interfaces, for example the Motu AVB line)

See also this thread on linuxmusicians.com

https://linuxmusicians.com/viewtopic.php?p=179979#p179979

Thanks,
Nicola Lunghi

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

Michał Pecio ([email protected]) changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]

--- Comment #1 from Michał Pecio ([email protected]) --- Looks like you are right. I guess it means that synchronizing multiple soundcards or playback/capture streams isn't working either? (If class drivers implement that in the first place.)

I wonder if this could be the reason for some weird snd-usb-audio behavior reported in the past (repeatedly unlinking and retrying URBs for unclear reason when running full-duplex with very low latency). But it's just a wild guess.

Did you try the obvious?

+ if (!(urb->transfer_flags & URB_ISO_ASAP))

+               goto skip_start_over;
        /* Calculate the start frame and put it in urb->start_frame. */

Actually, this function looks OK but it isn't called correctly because this shouldn't be limited to CFC *if* we are scheduling the first TD in a new "data flow" (xHCI 4.11.2.5). But on modern HW it seems to be correct, and if you skip the earlier urb->start_frame overwrite, chances are things may work for you.

Does it mean there is hardware which breaks when polled at the rate specified in its own endpoint descriptor? Would it still break if you insert zero-length frames in the "forbidden" periods?

Using start_frame like that seems complicated and it's not feasible on Gen-1 controllers (no CFC). Per Documentation/driver-api/usb/URB drivers may specify any service interval on per-URB basis, but this of course can't work with xHCI and results in a warning. However, as suggested in check_interval() comments, there could be a way of setting different polling interval at endpoint creation. I see no reason for xHCI HW not to accept that, even Gen-1.

Hmm, using the sideband API to bypass kernel bugs seems even more extreme :)

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #2 from Nicola Lunghi ([email protected]) ---
I haven't tried any changes as I'm not familiar with usb devices and wanted some feedback first

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #3 from Nicola Lunghi ([email protected]) ---
sorry haven't finished responding. I can try to skip directly to skip_start_over but the logic in ehci-sched seems more complicated than that and not being familiar with usb protocol I don't know how for example debug this. what could be a test setup to see the frames or record the frame that are sent and the timing? with the ASAP scheduling if a driver try to send the frames at a specific intervals the xhci driver 'breaks' all the syncronizations in the way is this implemented.

the issue arised from a Motu AVB interface that expect a precise message rate and without it it looses the channel synchronization as it expect a specific rate for the messages.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #4 from Michał Pecio ([email protected]) --- As for debugging, you can obviously print anything to dmesg with printk()/pr_info()/xhci_info(). But this may generate quite a noise on isoc endpoints if you print in every interval.

If you mount debugfs and go to

/sys/kernel/debug/usb/xhci/<controller-pci-address>/devices/<device-slot-id>/

you will find epXX directories and trbs file in each. The file contains a human-readable dump of the transfer ring written by the driver for given endpoint, so you can inspect if the TRBs make sense. This only works while the endpoint is enabled, i.e. if you stop streaming and driver switches to an empty altsetting zero, the endpoint will disappear from debugfs.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #5 from Alan Stern ([email protected]) --- For anyone who wants to know more about how isochronous scheduling is intended to work in the Linux USB stack, see the description in the kerneldoc for usb_submit_urb() in drivers/usb/core/urb.c.

In short, the start_frame field in struct urb is output-only. The host controller driver doesn't pay any attention to it when an URB is submitted, but the frame assigned to the URB's first isochronous packet gets stored in start_frame before the URB completes. Furthermore, the URB_ISO_ASAP flag only affects what happens to an URB that was submitted too late to keep the endpoint's queue going (say, because interrupts were blocked for too long). If the queue is just starting up, or the URB isn't already too late, URB_ISO_ASAP is ignored.

Also, host controller drivers are not likely to pay attention to the interval field. They will use the interval specified in the endpoint's descriptor.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #6 from Nicola Lunghi ([email protected]) --- (In reply to Alan Stern from comment #5)

Sorry Alan I don't understand your considerations. are you saying that this is not valid? the documentation you pointed out states that

With few exceptions, USB device drivers should never access URB fields
* provided by usbcore or the HCD until its complete() is called.
* The exceptions relate to periodic transfer scheduling. For both
* interrupt and isochronous urbs, as part of successful URB submission
* urb->interval is modified to reflect the actual transfer period used
* (normally some power of two units). And for isochronous urbs,
* urb->start_frame is modified to reflect when the URB's transfers were
* scheduled to start.

I also found this documentation from intel
https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/extensible-host-controler-interface-usb-xhci.pdf

pag 219 4.11.2.5 describes the start frame field.

I don't have time today I'll try to play with this during the we.

Can someone tell me if my considerations at the beginning are valid and the behaviour that we have is missing the start_frame handling, or there's something I don't see and the driver is actually doing the right thing?

Thanks,
Nicola

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #7 from Nicola Lunghi ([email protected]) ---
Sorry just rereading Michael comments they are useful! I'll take a look this we thanks!

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #8 from Alan Stern ([email protected]) --- Sorry, yes, urb->interval and urb->start_frame do get modified during submission. Drivers are allowed to read those fields before the URB completes.

The documentation from Intel is irrelevant. It discusses the xHCI hardware, but we're talking about the Linux software interface.

As for the considerations at the start of this bug report... xhci-hcd is indeed supposed to ignore urb->start_frame during submission and overwrite it with the actual starting (micro)frame number. And most of the time, xhci-hcd is supposed to ignore URB_ISO_ASAP. But it's not supposed to ignore URB_ISO_ASAP in cases where the URB was submitted too late. By "too late", I mean that at the time the URB was submitted, the (micro)frame that it should have been scheduled for -- one period after the last frame of the preceding URB -- had already elapsed.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #9 from Michał Pecio ([email protected]) --- Thanks Alan, that's good to know, because Documentation/ is horribly out of date and the kerneldoc doesn't really say that this field is ignored as input.

(Also, it says that if the whole URB is already expired, it would fail to submit, but this behavior was apparently later changed to completing with -EXDEV).

After a bit of digging, I see that urb->start_frame and URB_ISO_ASAP used to actually work the intuitive way on UHCI in early 2000s, but then EHCI began to act as if ASAP is always set, xHCI got the same behavior, UHCI was converted to EHCI behavior, and ASAP got a new and different meaning.

So today it is unrealistic to expect host controller drivers to respect start_frame, class drivers aren't setting it to anything meaningful, and the patch I suggested earlier would likely break some class drivers unless a new flag separate from URB_ISO_ASAP is added to force using the requested start_frame.

I still wonder if providing means of overriding the polling interval wouldn't be easier overall (less changes to snd-usb-audio required to support this new HW).

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #10 from Alan Stern ([email protected]) --- Figuring out how to handle isochronous transfers has been very difficult; it's not obvious what scheme would work best. Compounding that difficulty is the fact that different host controller drivers store their (micro)frame numbers differently, so drivers will never know exactly what to expect.

You say that uhci-hcd used to work "the intuitive way", but in fact experience has shown that no way is truly intuitive. Or to put it another differently, what's intuitive for one person is not intuitive for another.

And don't forget that host controller drivers have to handle isochronous endpoint queues that can be in three or four different possible states: empty, partially or completely full, and partially full but totally expired. It's not at all easy to come up with a set of flags and values that can handle all the possibilities in ways that drivers will find convenient to use.

The matter of specifying polling intervals different from what the endpoint descriptor says is yet another can of worms. For example, host controllers generally can't change polling intervals in mid-stream. And some might reserve the necessary bandwidth at the time when the alternate setting is installed, before any URBs have been submitted -- how would one go about changing it from the default value then?

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #11 from Michał Pecio ([email protected]) ---

        if (!qh->bandwidth_reserved) {
                qh->period = urb->interval;
                [...]
        } else if (qh->period != urb->interval) {
                return -EINVAL;         /* Can't change the period */

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #12 from Michał Pecio ([email protected]) --- Oops, sorry for accidental post. This was meant to say that the solution to not changing polling interval mid-stream already exists in UHCI and isn't very hard.

As for bandwidth reservation, the problem could be ignored ("you are requesting something really weird, deal with it"). Or in case of xHCI I think it could also be solved - changing BW allocation involves similar steps as changing the interval, so they could be done both (as long as there are no pending URBs).

BTW, purely for testing purposes, I suppose one could hack the kernel to modify device's descriptor and reduce polling interval. If this helps, then there would be grounds for wondering about how to implement interval switching more cleanly.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #13 from Alan Stern ([email protected]) --- The solution that's in uhci-hcd is specific to that driver. It won't necessarily work with other drivers. Or rather, it might work but probably not in the same way (since qh->bandwidth_reserved will be set and cleared at different times in different host controller drivers). It wouldn't be surprising if it doesn't work at all for some drivers (for example, if bandwidth_reserved gets set when the altsetting is installed, before any URBs can be submitted). The scheme simply is not usable as a general-purpose approach.

Ignoring bandwidth reservation is the same as ignoring attempts to change the polling interval, since reserved bandwidth = maxpacket size / polling interval (roughly speaking). Besides, the USB spec does not allow USB-2 drivers to ignore bandwidth reservation, and xHCI hardware enforces bandwidth reservation.

Now, if this turns out to be sufficiently important to enough people, we could always add a new kernel API specifically meant for changing an endpoint's interval. Maybe by adding a new ioctl to usbfs -- I don't know. This would complicate the existing usbfs API for reporting endpoint descriptors: Should the new interval be stored in the descriptor or should it be reported separately somehow?

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #14 from Michał Pecio ([email protected]) ---
I skimmed the linked forum thread and it seems the issue is not so much with "needing a specific frame interval", but not tolerating any missing packets.

https://linuxmusicians.com/viewtopic.php?p=177688#p177688

So no-gaps scheduling is already the default *without* URB_ISO_ASAP and drivers don't need to fill sequential frame IDs themselves if that's what they want. Note that this stops working when the queue underruns - next URB is assumed to be a new stream and will be scheduled into the future, not into the past.

If there are gaps despite the driver resubmitting on time, that may be a bug, but a different one than originally assumed.

If drivers want to insert a gap to resynchronize after detected lost packets, I'm afraid that's not supported in the current model. It would be technically possible on CFC xHCI hosts and likely EHCI. A concern is how drivers would know when frame ID overflows back to zero and how much into the future they can schedule.

The urb->start_frame field is now output, so drivers can use it to confirm if the URBs are scheduled sequentially as intended. At least with CFC, it appears that these numbers should be consistent with reality, though I have never tested it. Debugfs/tracing/printk may be used to check them against TRBs written to the HW.

Non-CFC looks weird, maybe it could be improved somewhat but not sure. The old xHCI 1.0 spec is limited in what it guarantees.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #15 from Alan Stern ([email protected]) --- 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.

As for inserting a gap to resynchronize after detected lost packets, it's simple enough for drivers to submit isochronous URBs with some of the packet lengths set to 0. Figuring out how many packets should have length 0 will be tricky, but that can be adjusted after the URBs complete, by checking their starting frame numbers.

I believe drivers can safely assume that the 8 low-order bits of the frame number will be valid (11 bits for microframe numbers), and that they are allowed to submit URBs up to 50 ms in the future. The kerneldoc says 10 - 200 ms, but it's being conservative. When a driver wants to keep latency to a minimum, it won't go more than 1 or 2 ms into the future, anyway.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #16 from Michał Pecio ([email protected]) --- (In reply to Alan Stern from comment #15)

I meant cases when the queue empties completely and there is no preceding URB.

But I see that there is a subtle difference in EHCI - it still considers a new submission to be continuation of the earlier stream if earlier URBs have been given back but their completion callbacks are still running. We can do it too.

But the real bug is xhci_get_isoc_frame_id(). This should convert urb->start_frame and packet index into frame_id to be passed to HW, but it also reschedules URBs which it doesn't like (and does some nonsense if the queue gets too long).

This happens silently under normal conditions and the warnings at the end of this function are bogus, almost impossible to trigger. So that's why no one noticed. The only indicator is urb->start_frame discontinuity, but it seems that class drivers trust HCDs and aren't validating that.

I don't know if that's the cause of those Motu issues, but it's clearly involved in the other problem with low-latency duplex operation on CFC controllers which was reported last year and which I can repro on my HW.

The above applies to xHCI 1.1 controllers (typically USB 3.1 and newer). On older ones urb->start_frame returned to drivers is kinda bogus and everything goes ASAP. Which actually seems to work better in most cases than the buggy CFC stuff, but gaps due to late submission remain undetected.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #17 from Michał Pecio ([email protected]) --- Created attachment 308930
--> https://bugzilla.kernel.org/attachment.cgi?id=308930&action=edit
schedule isoc packets sequentially on CFC

This is a test patch which knocks out the problematic code and adds some debugging on CFC controllers - interrupts are enabled on *all* packets, even if the URB isn't done yet, and for each packet we check if its completion is handled in the expected microframe. This may show false positives (the packet moved as intended, but IRQ is handled later) but it would also catch cases when HW executes (or reports execution) of a packet later than expected.

For example, it warns about every single packet for full speed devices behind high speed hubs on my ASMedia controllers. Don't do it ;)

It needs a fair bit of testing to see if this approach is feasible and I still see remaining issues in some tests (which may be due to xhci-hcd or snd-usb-audio) but it seems to work generally better than unpatched driver.

May or may not help with Motu devices, I don't know what exactly goes wrong with them. And again, for now, only CFC is expected to be scheduled accurately.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #18 from Nicola Lunghi ([email protected]) ---
hi thanks for all the suggestions!
I've finally found the time to work on this and I submitted some patches on the linux usb kernel mailing list to address the issues on the usb audio stream.

https://lkml.org/lkml/2026/5/5/59

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #19 from Nicola Lunghi ([email protected]) --- Sorry better link

https://lore.kernel.org/linux-usb/[email protected]/T/#ma81ee2dadce78c645d1a4320e0f5f55b5b58ef8f

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

[email protected] changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]

--- Comment #20 from [email protected] ---
The issue, as it relates to the MOTU devices mentioned here, is that after enabling a non-zero alternate setting with usb_set_interface(), once a transfer is sent in a service interval (zero-length or otherwise), there must be a transfer in every subsequent service interval. If, for any reason, a service interval is missed, the alternate setting must be toggled back to zero and then re-enabled, and the stream restarted.

MOTU's drivers on Windows and macOS take advantage of the ability on those platforms to schedule the start frame of isochronous streams sufficiently far into the future to ensure that enough URBs can be submitted before the first URB completes.

On Linux, I have found that with snd-usb-audio, it is fairly typical for streams to "stutter start." Because of the ASAP/SIA behavior discussed above, when the first URB is submitted, it can sometimes complete before subsequent URBs are enqueued. This creates a gap in the stream and causes the device buffers to fall out of sync.

At one point, I investigated monitoring the start_frame in the completion handler, but on non-CFC controllers it was not reliable (there were many duplicated frame numbers or gaps that did not reflect reality).

Although more recent MOTU devices have improved buffer management and won't fall out of sync due to missed intervals, the ability to specify a start frame is still important for reasoning about the timing relationship between IN and OUT streams, which helps guarantee consistent round-trip latency.

On non-Linux platforms, we have found that as long as a sufficient number of transfers are enqueued before the first transfer starts, a missed service interval, even on hosts that don't support CFC is quite rare.

That would not be necessary for MOTU devices.

This sounds like an issue I just started investigating. I am exploring the possibility that when an isochronous endpoint is restarted quickly on a CFC host (all URBs are killed and immediately then resubmitted), if the new URBs are submitted before all TRBs have been dequeued, their start_frame values may be set as if they were contiguous with the canceled TRBs. This might be causing the entire batch to fail with missed service intervals. This then happens over and over again.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #21 from Michał Pecio ([email protected]) --- Thanks for taking the time to explain.

(In reply to dylan_robinson from comment #20)

That would be something for snd-usb-audio to do. Does the same apply to missed service on IN endpoints? Or IN endpoints with lost/corrupted packets?

And for the record, are you saying there is no hope to predict what the HW expects to send and receive in the next interval and handle it right? Users say that streams go out of sync but the HW continues to function in some manner.

I remember noticing this tendency to restart streams (unlink and resubmit URBs) during startup with short buffers/low latency, presumably without going through altsetting 0. So I can see how this can be a problem. IIRC it was at least partly due xhci-hcd scheduling too far into the future. I will try to find some time to look into it again.

These fields are filled by xhci-hcd so they may be bogus due to bugs.

For reliable information it's better to log "Frame ID" written to Isoch TDs and MFINDEX register in process_isoc_td() (sensitive only to IRQ latency).

IIRC, on non-CFC even if things are initially right, they may fall out of sync due to missed service, with the MSE event being the only sign of trouble.

The "Linux way" appears to be guaranteeing two things to drivers:
- start_frame in completed URBs accurately reflects the time of execution
- streams are always continuous - even if an interval is missed, the next URB frame reliably completes (or is missed) in the next interval

For continuity, class driver only needs to ensure that the URB is resubmitted from its completion handler. If that happens several ms later, the URB is expected to immediately "complete in the past" with EXDEV.

AFAIK that's how ehci-hcd does it, but xhci-hcd doesn't and maybe should.

Wouldn't this be enough to make IN/OUT synchronization work? What's people's experience using MOTU HW and audio in general on ehci-hcd?

IIRC the repeated unlink/resubmit cycles during startup were due to snd-usb-audio getting some "xrun" callback from ALSA core for some reason. Then it would unlink everything and try again. I didn't know why it happens.

New URBs may be contiguous with past ones if you don't wait for them to complete
after usb_unlink_urb(). OTOH, usb_kill_urb() is synchronous. Once all URBs are killed synchronously or completed (including due to unlink) and the queue is empty, then submitting a new URB should schedule it far enough into the future to ensure no missed service. That's the theory.

See usb_submit_urb() and usb_unlink_urb() comments in drivers/usb/core/urb.c

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #22 from Nicola Lunghi ([email protected]) ---
(In reply to dylan_robinson from comment #20)
(In reply to Michał Pecio from comment #21)
Hi Dylan, Michal
I just want to report that with the patches I've submitted to the kernel the issue with clock synchronization with the motu is gone and the interface is super stable.
I've tested the interface running continuosly playing 12 audio channels connected to an usb 2 hub (via another hub) while strees testing the usb connection using fio on a separate usb disk on the same hub

Running kernel 7.1.0-rc2 with the patches applied, I can start and restart playback repeatedly without any channel desync. usbmon confirms the frame numbers are now perfectly sequential throughout — every
URB completion on EP 0x09 OUT comes back status 0 with start_frame
incrementing by exactly +8, no gaps, no EXDEV. I've set the motu at 16 samples buffer / 16 samples host safety and is still rock solid.

I've implemented another patch to do a channel reinizialization if the driver detect gap in the packets (that would break the synchronization) but on my system I cannot trigger it. I think at this point is probably not needed. I'll attach the extra patches to this post so you can have a look

Thanks,
Nicola

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #23 from Nicola Lunghi ([email protected]) --- Created attachment 310037
--> https://bugzilla.kernel.org/attachment.cgi?id=310037&action=edit
first utility patch

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #24 from Nicola Lunghi ([email protected]) --- Created attachment 310038
--> https://bugzilla.kernel.org/attachment.cgi?id=310038&action=edit
second patch to enable automatic link reestablishment

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #25 from Nicola Lunghi ([email protected]) ---
Subject: [PATCH 0/2] ALSA: usb-audio: stream restart on isochronous gap for MOTU AVB devices

---

MOTU AVB devices (USB ID 0x07fd:0x0005, including the 1248, 624, 8A, 8M, 16A and UltraLite AVB) cannot recover from a gap in the isochronous OUT stream. Once a service interval is missed, the device's internal channel demultiplexer loses synchronization and subsequent audio is routed to the wrong output channels. The device remains in this state until the alternate setting is toggled: set to 0, wait, then re-enable it.

This behaviour was confirmed by Dylan Robinson from MOTU in bugzilla comment #20: "once a transfer is sent in a service interval (zero-length or otherwise), there must be a transfer in every subsequent service interval. If, for any reason, a service interval is missed, the alternate setting must be toggled back to zero and then re-enabled, and the stream restarted."

This series introduces QUIRK_FLAG_STREAM_RESTART_ON_GAP for devices with this requirement. The snd-usb-audio driver already receives -EXDEV on isochronous descriptors when xhci-hcd misses a service interval, providing a reliable gap indication regardless of controller type. When the flag is set, retire_playback_urb() checks for this condition and schedules a workqueue item to perform the altsetting reset and signal an XRUN to userspace. Normal ALSA recovery then calls snd_pcm_prepare(), which executes the interface reset via snd_usb_endpoint_prepare() before restarting URB submission.

Patch 1 adds a small helper in endpoint.c to encapsulate marking an endpoint for a full interface reset, needed because struct snd_usb_iface_ref is not visible outside that translation unit.

Patch 2 implements the gap detection and recovery mechanism, and registers the quirk for USB ID 0x07fd:0x0005.

This series complements a separate xHCI patch series that addresses the root causes of isochronous scheduling gaps on CFC-capable controllers. That series reduces the frequency of gaps; this series handles recovery when they do occur.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #26 from [email protected] ---
(In reply to Michał Pecio from comment #21)

If the stream is interrupted, the alternate setting should be toggled for the input streaming interface as well.

This would only affect the ability to maintain implicit feedback synchronization. As long as the endpoint is serviced during the service interval, the device's buffer will not overrun. However, if a packet is lost or corrupted, the driver cannot determine the payload size, which means it cannot correctly derive the corresponding OUT transfer sizes.

Correct, the hardware offers no mechanism to predict this. The device relies on implicit feedback, so the correct OUT transfer size depends on the actual length received on the corresponding IN endpoint.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220748

--- Comment #27 from [email protected] ---
Regarding the start_frame, I want to advocate for treating urb->start_frame as an input parameter when URB_ISO_ASAP is not set. In looking into porting our drivers to Linux, we've found the current behavior challenging to accommodate.

As Alan mentioned, different host controllers have different representations of frame numbers. Other platforms address this by only exposing a normalized, frame (not microframe) index, while handling controller-specific conversions in the host controller driver.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Taking this side discussion off Bugzilla]

For one thing, that would be very impractical, as every driver using isochronous transfers would then have to be modified.

For another, what's the point? In an ongoing stream, all this would allow the driver to do would be to break the continuity of the stream. At the start of a fresh stream, the driver could easily end up requesting the HCD to put the first transaction in a (micro)frame that the endpoint isn't scheduled to use or is beyond the end of the HCD's scheduling window.

In what way? What is it you want to do that you find challenging?

And thereby forcing every URB to contain an integral number of frames' worth of transactions, at the risk of breaking the stream's continuity?

Alan Stern

Maybe adding a new flag would make more sense then.

I was confused because the documentation regarding URB_ISO_ASAP describes this behavior.

- For ISO there are two startup behaviors: Specified start_frame or ASAP.
- For ASAP set ``URB_ISO_ASAP`` in transfer_flags.

However, it looks like URB_ISO_ASAP actually means something
different: that the URB can be delayed. Is that correct?

The point would be to allow the driver to start a stream on a particular frame relative to another stream, ensuring deterministic latency between the two.

For an ongoing stream I would expect that submitting a start_frame that is discontinuous and in the past would result in either a submission failure or immediate completion with an error status. Such an error would provide useful feedback to the driver because its occurrence would mean the stream is no longer synchronized as the driver expects.

If the driver requests a specific start frame, it should be the driver's responsibility to ensure it is a valid frame within the valid scheduling window. If the requested start frame is invalid or outside the host controller driver's valid scheduling window, the request should fail. This would be more helpful than silently fixing it upon submission.

In order to offer predictable round trip latency though the host, we have to be able to correlate the frame time of transfers on the IN pipe to transfers on the OUT pipe. Therefore, our drivers need to determine the frame in which a transfer occurs.

Currently, with non-CFC host controllers, there is no way to determine which frame a transfer occurs in. This is because without CFC, the start_frame returned in the URB is only approximate.

AFAIK, xHCI doesn't allow scheduling transfers on arbitrary microframe indexes, only on frame boundaries. URBs could still contain a non-integral number of frames by putting the same start_frame in multiple URBs (just like the TDs used by the hardware). Alternatively, the initial frame-aligned URB could have a specific start_frame and the subsequent ones could be submitted as start immediately after.

Ultimately, we care about the ability to start a stream on a particular frame and maintain its continuity after starting.

Currently, there is no straightforward way to inform the xhci driver whether a URB represents a stream start or continuation. This makes it difficult to detect missed service intervals or other scheduling discontinuities.

Dylan

Sort of; a more accurate description would be that the URB is scheduled for the next available unexpired time slot (as opposed to the next time slot, regardless of whether it has expired or not).

Also, there is no "specified start_frame" behavior. The start_frame field is output only. Some of the documentation is out of date. The most up-to-date information about isochronous scheduling is in the kerneldoc comments for usb_submit_urb() and usb_unlink_urb() in drivers/usb/core/urb.c.

I don't believe that's going to be possible. Or at least, not possible until someone adds an API for getting an isochronous stream's current schedule information. Until/unless that happens, the only way to know what frames or microframes the stream will use is to submit an URB and look at the start_frame and interval fields after it completes.

If you take that approach, submitting a few milliseconds worth of innocuous data at the start (or ignoring it in the case of an input stream), you should be able then to synchronize the data for your new stream with the data in another one with little trouble. You won't be able to change how they are scheduled, but you will know exactly how the two streams' schedules are related.

Not possible, because the driver doesn't know the scheduling window or other parameters supported by the host controller driver.

Not if the driver doesn't know what the window is.

Is this a deficiency of xhci-hcd, or of some types of xHCI hardware? Other kinds of host controllers don't have these problems.

xhci-hcd is not the only host controller driver in the Linux USB stack. What you're talking about would require changes to all of them (not to mention the changes needed in the class drivers).

Informing xhci-hcd about this shouldn't be necessary; it should already know. ehci-hcd does, for example. In short, if the submission occurs before the last scheduled URB in a stream has completed, it is a continuation. Otherwise it's a start.

What might be more difficult is distinguishing between a start and a continuation after a gap. The boundary between those two is pretty subjective. We deal with this by ignoring the possibility; it's always treated as a new start.

Alan Stern

Thanks for the clarification.

Adding "specified start_frame" behavior is exactly what I am proposing. If it existed, the same driver submitting URBs for both endpoints could track scheduling information internally, no additional API needed. The device's endpoint descriptor determines the interval. Is there additional scheduling information the HCD has that the driver cannot observe?

FWIW, this is what our drivers for Windows and macOS do and they do not require any such API. They just read the current frame and schedule streams to start on a desirable future frame.

I have tried this. It can work if the frame information returned after submission is accurate. That said, considering endpoint feedback and synchronization realities, an innocuous data transmission may not always be available.

If a host controller cannot accommodate a requested start_frame, we would consider that host controller incompatible with our device. If we wanted to try working anyway, we could always fall back to non-specified scheduling.

I don't think it matters if the driver knows what the window is, because ultimately, the host controller can either support the requested start_frame or not. All the driver needs to know is whether its submission worked.

I believe this is an xhci-hcd deficiency. The xhci-hcd computes a start frame close to the IST, but only uses it if the controller supports CFC and URB_ISO_ASAP is not set, otherwise it uses SIA. All xHCI revisions support specifying a start_frame in the first TD, but controllers without CFC require SIA = '1' for subsequent TDs.

I understand this would touch more than xhci-hcd, and I'm not proposing every HCD implement it. The minimal change would be for each HCD to advertise whether it supports "specified start_frame," allowing drivers to query that capability.

What ultimately matters is the continuity of the stream on the wire. Submitting an URB before the completion of a previous one does not guarantee it was submitted in time to prevent a discontinuity on the wire, which we have observed.

Are you saying it's difficult to distinguish if the gap was intentional?

I was talking about having a way for a driver submitting URBs to tell the host controller driver whether they are intended to be continuous, so that an error can be observed if the submitter's indication disagrees with the stream's current state.

Dylan

It would surely need to be optional on per-URB basis, to avoid updating all drivers and to enable sub-1ms URBs without gaps on HW like xHCI, where we can't request starting frame with microframe granularity.

I suppose Windows/OSX have to be doing it this way too?

Yes, there are many errors in Documentation/driver-api/usb. I thought about fixing this when the bug was opened, but maybe it's better to wait till the dust settles around xhci-hcd.

Alan pointed out that kerneldocs are more up to date, but there is still a crucial error there: on ehci-hcd, an URB submitted too late completes with -EXDEV instead of the submission failing with -EXDEV. And it seems we should strive for the same in xhci-hcd, instead of silently rescheduling such URBs to run later.

The idea is that in absence of bugs (which xhci-hcd has plenty), the stream is always synchronized as long as all URBs are completing successfully and resubmitting before returning from their complete() callback. Drivers don't need to "start_frame++" for this to work.

If an IRQ delay causes resubmission to be late, some or all "frames" of the URB will complete with EXDEV.

Also, I believe such completion should occur in submission order, i.e. only after all URBs submitted earlier have completed in some manner.

There would need to be some convention how to deal with bInterval > 1. Such endpoint doesn't use every (micro)frame and can't be scheduled into any arbitrary one.

In such cases, xHCI spec only permits the first frame of every ESIT to be specified, but the actual transfer will take place anywhere within the ESIT, and we don't know when. See 4.11.2.5.

Drivers would need to be aware of such low level details.

That's partly a bug.

Filling start_frame is only implemented for CFC HCs. On non-CFC we could still set start_frame accurately on the first URB and later increment it blindly. What we cannot do is ensure that the HW will stay in sync after Missed Service. All bets are off after EXDEV.

On non-CFC we would also be forced to ignore any driver-specified start_frame except at the beginning of a new stream.

It's supposed to be implied that submission from a complete() callback is a continuation. I believe one of the submitted patches fixes that.

Regards,
Michal

There certainly is!

Different HCDs handle periodic transfer scheduling in different ways. In some cases the bandwidth (and corresponding (u)frames) may be reserved when an alternate setting is installed; in others it doesn't happen until the first URB of the new stream is submitted (in which case the interval may be taken from the URB instead of from the endpoint descriptor).

Either way, the HCD has to decide how to schedule the isochronous stream based on the constraints of how much bandwidth is already dedicated to other devices and endpoints, and what (u)frames are already in use. Of course drivers have no way to know this, or to handle it if they did.

In addition, when the scheduling decision has been made, the HCD knows not only the interval that has been allocated to the stream but also its phase. That is, which (u)frames it will use, in cases where the interval is larger than one (u)frame. This phase information is not provided to the class driver other than implicitly through the start_frame values of completed URBs.

In fact, IIRC the USB spec doesn't require isochronous transfers to occur at uniformly spaced times. It only requires that there be one transfer somewhere within each interval.

Let's say the interval is 8 frames, and the driver requests to start in frame 71. If that isn't one of the frames in the allocated schedule, presumably the URB submission will fail. What does the driver do then? Try again with 72, then 73, 74, and so on, until it finds a value that works? Make random choices?

What if the HCD only supports scheduling up to 256 ms in the future, but the driver asks for a start frame that is 400 ms in the future?

What if the periodic schedule is already full and there is no bandwidth remaining to schedule the new stream? How will the driver find out? By getting a different error code from the URB submission?

All right. Something else will be needed for such situations. Right now I don't know what the requirements are.

In view of my remarks above, doesn't this amount to saying that you consider all host controllers incompatible with your device?

See above.

I'm not conversant with all the intricate details of xHCI or the differences between various versions, so I'll take your word for it.

Then what will you do if your device lies behind a host controller that doesn't support it? Fall back to unspecified scheduling? If that is acceptable for such situations, why can't you use the same strategy with xhci-hcd?

In fact, _nothing_ guarantees that URBs will be submitted in time to prevent a gap in the schedule (except perhaps on some RT systems). The CPU can get overcommitted at any time, and drivers have to be prepared to handle unpredictable delays.

No, I'm saying that after a gap has occurred it's difficult to tell whether the submitter of the next URB wants it to continue using the same schedule as before and starting from the next upcoming time slot, or to continue using the same schedule as before with some of the URB's packets assigned to the expired time slots (so they never get sent), or to start using a completely new schedule.

The current intention is to make this determination based on whether the next URB is submitted before the last of the previous URBs completes and on whether the next URB has the ISO_ASAP flag set.

Aren't they almost always intended to be continuous? Particularly if the new URB is submitted by an old URB's completion handler? About the only unambiguous exception I can think of is the first URB submitted after a Set-Interface request, where there's nothing for it to be continuous with.

It's always possible to observe these errors after the fact (assuming the start_frame values are reliable). I'm not sure what value there is in observing them when the URB is submitted, however. What would the driver do differently in these two cases?

Alan Stern

I don't remember the details exactly, but are you perhaps not taking care to distinguish between URBs that are partially too late (some of their packets' time slots have expired but some have not) and URBs that are entirely too late (all of their packets' time slots are expired)?

As I recall, the first case is supposed to complete with some of the packet statuses set to -EXDEV, whereas in the second case the entire submission is supposed to fail with -EXDEV.

But my memory could easily be wrong. And I'm too tired to check the source code now...

Alan Stern

... which was an artificial distinction requiring drivers to handle substantially the same problem in two ways. They didn't, therefore

24f531371de1 USB: EHCI: accept very late isochronous URBs

changed it to EXDEV completion in all cases. AFAIK, this code still stands. I can't find the string "EXDEV" anywhere in ehci-hcd source.
Note: every frame start with this status so in case of missed transfer the driver simply leaves it unchanged.

Regards,
Michal

I had completely forgotten about that commit. And you're right; the kerneldoc for usb_submit_urb() needs to be updated. I guess that can be done after the xHCI changes have settled down, when we'll have a better idea of what to say.

Alan Stern

Understood.

Our High Speed and SuperSpeed devices always use a bInterval of 1, so they expect transfers in every microframe. I'm less familiar with the behavior and expectations of endpoints that use larger polling intervals.

Perhaps, rather than treating this as a "specified start frame", it would be better to think of it as a "specified start service interval".

If a driver requests a start frame that is not specifically available within the periodic schedule, it seems reasonable for the HCD to place the transfer at another valid position within that same service interval. The actual start frame used could then still be reported back through the completed URB, which can be inspected by the driver if it needs to know the precise scheduling decision.

For endpoints with larger polling intervals, this "nearest valid start frame" approach seems appropriate, since the device cannot expect timing granularity finer than its polling interval anyway.

See above.

On macOS and Windows the submission would fail with an error. That seems appropriate.

For example, macOS has the following return status:
- kIOReturnIsoTooNew (Too far in the future)
- kIOReturnIsoTooOld (Too far in the past)

On Windows, the status of the URB is set to USBD_STATUS_BAD_START_FRAME.

Our drivers on those platforms request start frames that are about 20ms in the future. We haven't encountered controllers that reject this.

The driver needs to know if the stream cannot start. What happens currently?

The goal of our drivers is to provide low-latency, highly reliable audio streaming. In general, the more visibility and control the driver has over scheduling, the better the latency and reliability can be.

The practical requirement is for the driver to be able to start a stream at a predictable point in the future and to understand how that stream is related to other active streams.

As long as the stream can be started reliably (without gaps), and the resulting scheduling decision is observable to the driver, it is generally possible to make the system work. However, reducing the driver's visibility into or influence over scheduling typically means trading away some combination of latency, synchronization accuracy, or speed of recovery.

Our drivers already operate with "specified start_frame" behavior across a variety of host controllers on both macOS and Windows, where we consistently observe the requested start frame being honored in practice.

That leads me to believe that our devices are compatible with the vast majority of host controllers they are likely to encounter.

We can ultimately only provide functionality within the limits of what the host platform and HCD are capable of supporting.

In general, the more precise and predictable the scheduling behavior is, the lower latency and better overall performance the driver can achieve. Falling back to unspecified scheduling may allow basic functionality to work, but at the cost of increased buffering, reduced synchronization accuracy and longer startup delay.

Understood. We are well acquainted with that reality, and a significant amount of our driver work goes into mitigating and recovering from exactly those kinds of disruptions.

Ultimately, I am trying to understand how to achieve a similar level of reliability on Linux to what we are currently able to achieve on macOS and Windows platforms.

In the case of an unavoidable scheduling gap, what matters most is that the driver has some way to detect that the discontinuity occurred, so it can respond appropriately.

Generally, yes. For audio streaming we always want the stream to be continuous.

That said, we do have some MIDI devices that intentionally use discontinuous isochronous transfers in order to allow the driver to schedule MIDI output with more precise timing.

I agree the two cases are quite similar in terms of observability.

The main practical benefit of detecting this at submission time might be the potential to reduce recovery latency. If the driver can determine immediately that scheduling has failed, it may be able to restart or resynchronize the stream sooner, which for an audio interface, could translate to a smaller audible glitch.

Dylan

Windows isochronous transfers behave similarly to the outdated documentation in Documentation/driver-api/usb.

Setting USBD_START_ISO_TRANSFER_ASAP in the URB instructs the USB driver stack to schedule the transfer in the next appropriate frame. For the first URB on a pipe, transfers with this flag are scheduled as soon as possible. For subsequent URBs, the stack automatically tracks stream continuity.

Otherwise, the UrbIsochronousTransfer.StartFrame member specifies the starting USB frame number for the transfer.

Source: https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/transfer-data-to-isochronous-endpoints

The USBDriverKit API on macOS for submitting isochronous transfers is:

virtual kern_return_t IsochIO(

    IOMemoryDescriptor *dataBuffer,
    IOMemoryDescriptor *frameList,
    uint64_t firstFrameNumber,
    OSAction *completion);

firstFrameNumber: The starting frame number for the request. You can get the current frame number from the GetFrameNumber method of IOUSBHostDevice or IOUSBHostInterface. Specify 0 to begin the transfer on the next available frame (XHCI only).

Source: https://developer.apple.com/documentation/usbdriverkit/iousbhostpipe/isochio

Because our devices use the implicit feedback synchronization model, our drivers typically submit OUT transfers from the IN pipe's completion callbacks. Would those submissions also be considered continuations of the existing OUT stream, or is continuity only inferred when a URB is submitted from the completion handler associated with the same pipe?

Dylan

That makes sense, and it should be doable. At least for ehci-hcd, and likely for xhci-hcd as well.

We ought to be able to do something similar.

Submission fails with a -ENOSPC error code. On the other hand, if the schedule is set up when the alternate setting is installed, and that can't be done, then the usb_set_interface() call fails.

All right, let's say what Mathias and Michal have to say about this.

Alan Stern

Looks like Windows ASAP is similar to Linux non-ASAP behavior, except that they consider new URBs to continue the stream for 1024 USB frames after the last completed URB. In Linux that number is 0 instead, as there is no way to indicate a new stream by other means.

Windows requires isoc URBs to be frame-aligned. Linux doesn't have this limitation. I can run a high-speed UAC dongle with two 0.5ms URBs just fine, though ALSA won't let me go shorter for some reason.

Not sure how Apple and MS deal with the "phase" issue, i.e. does a bInterval=6 endpoint run in frames 0,4,8,... or 3,7,11,... and how are drivers supposed to set firstFrameNumber then.

Looking at the MS article, I suspect they want drivers to pretend it's always 0,4,8, which is what the xHCI spec requires too.

I meant completion at the same endpoint because it's a common case, but submission while any URBs are pending and not completed yet also is supposed to continues the stream, regardless of who submits.

Or at least that's the intent stated by Alan and what it was before "BH giveback". I think today there is a short window after completion by HW and before complete() runs, where even ehci-hcd may fail to detect that the last remaining URB is still "in flight". In this case it's likely too late to avoid a gap on OUT, but the gap should produce EXDEV completions rather than delay the next URB by a few uframes.

Perhaps this should be fixed for all HCDs. The alternative is to ask drivers to make the IN completion give the OUT completion any required information to resubmit, which increases latency and chance of EXDEV, or they risk potential hard to detect gaps due to SW latency. (The gaps could be detected by looking at frame_desc->start_frame, at least when xhci-hcd is fixed to fill it properly on non-CFC chips).

Regards,
Michal

This appears to be true. After reading the documentation, it was not clear to me whether it might be possible to do something similar on Windows by submitting two URBs with the same StartFrame value, each describing four microframes.

So, I tried an experiment against a bInterval = 1 high-speed device, and any submission describing less than a full frame's worth of transfers was not scheduled and was immediately completed with USBD_STATUS_BUFFER_UNDERRUN.

The Windows documentation suggests that high-speed isochronous endpoint descriptors are only supported up to bInterval = 4.

Source: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/usbspec/ns-usbspec-_usb_endpoint_descriptor

It also may be the case that Microsoft only supports bInterval = 1 for full-speed isochronous endpoints. That descriptor documentation suggests that other values are valid, but the isochronous transfer article includes the following comment in the example code:

// Microsoft USB stack only supports bInterval value of 1 for // full-speed isochronous endpoints.

Source: https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/transfer-data-to-isochronous-endpoints

For Apple, I have not been able to find documentation describing how start frame alignment is handled for isochronous endpoints with polling intervals greater than 1 ms.

I noticed you weren't CC'd on my last e-mail exchange with Alan.

I wonder what you think about adopting this approach for Linux.

Alternatively, the requested start frame could be interpreted as a "not-before" constraint (i.e. the HCD would be allowed to schedule it on the first valid frame that is equal to or greater than the requested value).

Another option would be to require that the submitted start frame be aligned to the polling interval, but that seems unnecessarily restrictive.

Dylan

IDK if drivers ever need to know. They better not, because there is no way to get "phase" information out of xHCI, besides timing IRQs. There is also no way to set it. And no way to schedule a new stream not on frame boundary. See xHCI 4.11.2.5.1, the rules are simple.

The urb->start_frame you get is the first (u)frame of target interval. Requests from drivers would need to be rounded, ignored or rejected.

I believe gaps between URBs are not supposed to be happening at all with proper resubmission, according to documented rules, so drivers can detect gaps by watching for EXDEV status in frame_desc, if we manage to bring xhci-hcd into compliance with the rules.

As for predicting EXDEV at submission, it's a gable. We can only be 100% sure whether it's too late or not when the interval completes with Missed Service or Success.

Admittedly if it fails, the driver unfortunately only learns about it after the end of the whole URB. Short URBs could be better here.

We can predict it by looking at the Isochronous Scheduling Threshold reported by the HC, but it's a single number which may not accurately reflect all cases (full-speed devices behind high-speed hubs are a fascinating topic) and IME it's quite pessimistic on many chips - in reality, they can do better than they claim.

So being conservative and failing submissions which missed the IST might cause a driver to panic and reset the endpoint when no glitch would actually occur. And of course delaying such URBs (as currently done) has the known effects that this bug is about.

I wrote some experimental patches to debug and fix this back then, but nobody was commenting on the bug so I dropped this topic because I'm reluctant to submit significant changes without real testing. I can rehash this stuff.

The most important question is if actual HW properly handles URBs submitted too late. It would be nice to get testing on various xHCI chips from varous vendors. If there are problems, it could prove necessary to detect late submissions and complete such transfers as EXDEV witout HW involvemnet. Yuck. Maybe it won't come to that.

Regards,
Michal

This seems reasonable to me. Drivers should not expect timing information with finer granularity than the endpoint service interval.

For the first URB in a stream, I think rounding makes sense if the requested start frame falls within the valid scheduling window, while requests outside that window should be rejected.

For subsequent URBs submitted as continuations (and therefore able to rely on -EXDEV to detect discontinuities), I think the requested start frame can be ignored.

This sounds good.

For an established stream, I agree with taking the optimistic approach of attempting to append transfers that are only marginally past the IST.

For a new stream with a requested start frame, however, I think the HCD should, to the extent practical, validate that the request falls within the valid scheduling window.

I am happy to help test in whatever way I can.

I would be interested to learn more about how this is handled in reality too.

I think drivers should, whenever possible, avoid submitting URBs too close to the IST. A motivation for having a requested start frame is that it allows transfers to be enqueued well in advance of when the hardware will actually need them.

In our case, URBs are often submitted tens of milliseconds ahead of time, while the transfer buffers themselves are serviced just before the data is consumed by the host controller. For audio streaming, this still allows low latency while avoiding the need to make submissions close to the scheduling threshold.

In practice, we have found it less disruptive to occasionally miss copying audio data in time than to lose continuity of the isochronous stream itself and have to restart the endpoint.

Dylan

https://bugzilla.kernel.org/show_bug.cgi?id=220748

Mathias Nyman ([email protected]) changed:

What |Removed |Added
----------------------------------------------------------------------------

                 CC|                            |[email protected]
                   |                            |om

--- Comment #28 from Mathias Nyman ([email protected]) --- Created attachment 310090
--> https://bugzilla.kernel.org/attachment.cgi?id=310090&action=edit
testpatch set and check xhci frame id correcly

-Aligns frame id to full frame or esit on first urb,
-Always sets urb->frame_id
-Calculates valid frame window as in spec
-checks if next frame id mid stream is within frame window, print message if not. Does not alter frame id even if outside frame window.
- allows next URB to be queued mid transfer continueing mid frame (endpoint interval must be shorter than a full frame as well)

Commit message and some comments needs cleaning up, but code should be testable.

xhci tracing shows the frame_id of each queued urb, enable with:

echo 1 > /sys/kernel/tracing/events/xhci-hcd/xhci_urb_enqueue/enable echo 1 > /sys/kernel/tracing/events/xhci-hcd/xhci_queue_trb/enable <transfer some non SIA isoc transfers >
cat /sys/kernel/tracing/trace

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Always calculate estimated start frame, and set urb->start_frame

Fix valid frame window start and end calculation to match xhci spec 4.11.2.5

Don't inclease frame id with 1 if a URB mod transfer is queued late. Queue it with next expected frame ID but print a message if URB is next expected frame id does not fir valid frame window range (URB mid transfer is queued late)

This patch doesn't switch to URB_ISO_ASAP /SIA scheduling if URB is queued late, not sure if that would help as it only moves the frame id glitch problem forward, unless _every_ URB queued after a late URB is forced to use SIA, which again would defeat the point of Frame ID use.

get rid of the annoying 'xep' and 'xdev' variables, xhci driver uses ep and vdev naming everywhere else

Contains some FIXMEs, patch for initial testing purposes

---
 drivers/usb/host/xhci-mem.c  |   1 +
 drivers/usb/host/xhci-ring.c | 189 ++++++++++++++---------------------
 drivers/usb/host/xhci.h      |   7 +-
 3 files changed, 80 insertions(+), 117 deletions(-)

index 997fe90f54e5..58714e10773f 100644
@@ -1499,6 +1499,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 		return -ENOMEM;
 
 	virt_dev->eps[ep_index].skip = false;
+	virt_dev->eps[ep_index].next_uframe = -1;
 	ep_ring = virt_dev->eps[ep_index].new_ring;
 	xhci_ring_init(xhci, ep_ring);
 

index aeecd301f207..bcb7b8c877db 100644
@@ -287,6 +287,11 @@ struct xhci_run_regs {
 	struct xhci_intr_reg	ir_set[1024];
 };
 
+/* Bits [13:3] of the microframe index equals the 1ms frame index */
+#define MFINDEX_TO_FRAME(p)	(((p) >> 3) & 0x7ff)
+#define MAX_FRAMES		2048
+#define MAX_UFRAMES		(MAX_FRAMES * 8)
+
 /**
  * struct doorbell_array
  *

@@ -696,7 +701,7 @@ struct xhci_virt_ep {
 	struct list_head	bw_endpoint_list;
 	unsigned long		stop_time;
 	/* Isoch Frame ID checking storage */
-	int			next_frame_id;
+	int			next_uframe;
 	/* Use new Isoch TRB layout needed for extended TBC support */
 	bool			use_extended_tbc;
 	/* set if this endpoint is controlled via sideband access*/

2.43.0

Butchered the commit message but code should be testable

First URB is aligned to full frame, or to interval start if interval is longer than a frame

For urbs queued mid isoch transfer we check if the next expected frame id is withinh the accepted frame window, but only print a message if not. It doesn't alter the expected start frame id, or switch to URB_ISO_ASAP/SIA scheduling if the check fails.

Always calculate and set urb->start_frame.

URBs with intrval less than a frame do not need to be aligned to entire frame, next urb can continue mid frame.

-Mathias

This may cause driver developers lots of grey hair when running on old (non-CFC, xHCI 1.0, USB 3.0, "gen 1") HCs where nothing is done to enforce frame number correctness so far.

Writing obviously bogus urb->start_frame at least was obvious :)

Failing to meet the IST is not the same as violating the 895ms limit. The former may (or may not) cause the TD to complete with MSE instead of Success in its interval. The latter is apparently supposed to cause MSE completion when the HC reads the TD, even though the IST was met.

A particularly fascinating case is period > 895ms; it seems that xHCI only allows submitting shortly before interval boundary, or "SIA".

Another nice one is ESIT >= 4096ms - exceeding MFINDEX limit.

That being said, I found that validating end_frame_id before enqueuing doesn't take more effort than generating these warnings, and I think it would be more productive - drivers know that their submission is lost cause, instead of getting bizarre completions later.

As for ignoring the IST, my ASMedia chips seem to take it well. And they specify absurdly long IST of 2ms, while actually running with 3x0.5ms URBs or less. The "less" tends to be glitchy, but I'm not yet sure if it's xHCI problem or snd-usb-audio / other software problem.

For example, I noticed that snd-usb-audio running with 2 URBs sometimes only resubmits when both are completed, an obviously bad idea, while at other times it runs well for a long time. Still don't know why.

Regards,
Michal

v2 writes the TRB frame id field of the first TD of the first URB for older non-CFC hosts as well. So urb->start_frame should be quite accurate. Its probably more off if URB_ISO_ASAP is set.

Added last frame of urb < +895ms to v2

But only for urbs mid stream, and only print a message if check fails. queue it anyways

I think it makes sense to only enforce IST for the very first URB when starting the endpoint.

Otherwise just print a debug message so that class driver developers can notice they use more URBs, queue them sooner, or both.

My v2 is a bit cleaner, fixed a missing "% MAX_UFRAME", and is split into 2 patches.
Can be found in my fix_frame_id branch:

git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git fix_frame_id
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=fix_frame_id

-Mathias

Check if the expected frame IDs for a isochronous URB submitted mid stream is within the valid frame time window that xHC controller is capable of queuing TDs.

The range only needs to be checked once per URB as the isoc TDs of an URB are queued in one go with spinlock held and interrupts disabled.

Calculate the valid frame window start and end frame id in frames instead of microframes to better match how xhci specification section 4.11.2.5 does it.

Don't add frame id gaps or change scheduling to SIA mid stream if the start frame is outside the valid frame winow.
Only print a debug message.
Some devices can't handle gaps in isochronous transfers.

Calculate a valid start frame for the first URB of a stream, and align it to a full frame, or to interval start if interval is longer than a frame

Set urb->start_frame value for every URB

---
 drivers/usb/host/xhci-mem.c  |   1 +
 drivers/usb/host/xhci-ring.c | 180 +++++++++++++++--------------------
 drivers/usb/host/xhci.h      |   7 +-
 3 files changed, 84 insertions(+), 104 deletions(-)

index 997fe90f54e5..58714e10773f 100644
@@ -1499,6 +1499,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 		return -ENOMEM;
 
 	virt_dev->eps[ep_index].skip = false;
+	virt_dev->eps[ep_index].next_uframe = -1;
 	ep_ring = virt_dev->eps[ep_index].new_ring;
 	xhci_ring_init(xhci, ep_ring);
 

index aeecd301f207..bcb7b8c877db 100644
@@ -287,6 +287,11 @@ struct xhci_run_regs {
 	struct xhci_intr_reg	ir_set[1024];
 };
 
+/* Bits [13:3] of the microframe index equals the 1ms frame index */
+#define MFINDEX_TO_FRAME(p)	(((p) >> 3) & 0x7ff)
+#define MAX_FRAMES		2048
+#define MAX_UFRAMES		(MAX_FRAMES * 8)
+
 /**
  * struct doorbell_array
  *

@@ -696,7 +701,7 @@ struct xhci_virt_ep {
 	struct list_head	bw_endpoint_list;
 	unsigned long		stop_time;
 	/* Isoch Frame ID checking storage */
-	int			next_frame_id;
+	int			next_uframe;
 	/* Use new Isoch TRB layout needed for extended TBC support */
 	bool			use_extended_tbc;
 	/* set if this endpoint is controlled via sideband access*/

2.43.0

The frame id field can be set for the first TD of the first isoc URB to schedule the start of an isoc stream even in host doesn't support CFC (Contiguous Frame ID Capability)

Set the frame ID TRB field of the first isoc TD unless URB has the schedule immediately 'URB_ISO_ASAP' transfer flag set.

---
 drivers/usb/host/xhci-ring.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

index b1318515ae58..55a10d1e70cb 100644
@@ -3976,6 +3976,23 @@ static int xhci_ist_microframes(struct xhci_hcd *xhci)
 	return ist;
 }
 
+
+static bool xhci_isoc_td_uses_frame_id(struct xhci_hcd *xhci, struct urb *urb,
+				       struct xhci_virt_ep *ep, int i)
+{
+	if (urb->transfer_flags & URB_ISO_ASAP)
+		return false;
+
+	if (xhci->hcc_params & HCC_CFC)
+		return true;
+
+	/* set frame id for first TD of first URB in stream */
+	if (ep->next_uframe == -1 && i == 0)
+		return true;
+
+	return false;
+}
+
 /*
  * Check if frame is in the valid frame window, including start and end.
  * If start > end then assume window wrapped around at a limit the frame

@@ -4089,7 +4106,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 	int i, j;
 	bool more_trbs_coming;
 	struct xhci_virt_ep *xep;
-	int frame_id;
 	int uinterval = urb->interval;
 	int start_uframe;
 

@@ -4145,12 +4161,13 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		}
 		td = &urb_priv->td[i];
 
-		/* use SIA as default, if frame id is used overwrite it */
-		sia_frame_id = TRB_SIA;
-		if (!(urb->transfer_flags & URB_ISO_ASAP) && (xhci->hcc_params & HCC_CFC)) {
-			frame_id = (start_uframe + i * uinterval) / 8;
-			frame_id %= MAX_FRAMES;
-			sia_frame_id = TRB_FRAME_ID(frame_id);
+
+		/* Choose SIA of frame ID based scheduling for this TD */
+		if (xhci_isoc_td_uses_frame_id(xhci, urb, xep, i)) {
+			sia_frame_id = (start_uframe + i * uinterval) / 8;
+			sia_frame_id = TRB_FRAME_ID(sia_frame_id % MAX_FRAMES);
+		} else {
+			sia_frame_id = TRB_SIA;
 		}
 
 		/*

2.43.0

Testing this patch on a non-CFC host with a bInterval = 1 device, I can confirm that urb->start_frame is now accurately reported when URB_ISO_ASAP is not set.

When URB_ISO_ASAP is set, urb->start_frame is slightly off as expected. For example, I observed urb->start_frame reporting 1645.0 (frame.uframe), while the stream actually began at 1643.2.

This seems good.

Regarding the valid window check for mid-stream URBs, what is the concern with an URB extending beyond the current scheduling window?

Since the controller only evaluates TDs as it advances through the transfer ring, isn't it only important that a TD's frame ID falls within the valid scheduling window at the time the host controller reaches it, rather than when the URB is submitted?

Additionally, due to wrapping, and depending on the values of urb->number_of_packets and uinterval, I don't think this check will reliably detect when urb_end extends too far into the future. That said, I'm not sure this check is actually meaningful.

For endpoints with ESIT greater than 895ms, wouldn't SIA become necessary to avoid ring underrun/overrun?

Best Regards,
Dylan

That email was about v2 of the patch. Just realizing I responded on the v1 thread.

Did you try with short enough playback buffer to get occasional xHCI ring underruns?

Is Frame ID still correct then?

MFINDEX overflows every 2 seconds and the HC may consider such URB 1.1s in the past and drop it with Missed Service Error.

That's true, but the HC is allowed to look at the URB as soon as it is submitted. The spec states that SW shall never do such things, so HW is allowed to assume that SW doesn't and make the dumbest assumptions, if this saves them implementation complexity somehow.

FWIW I have an implementation which checks it reliably, though it doesn't support ESIT > 512ms to save a few lines of code.

Not sure, and it's hard to test when most likely no such HW exists. It's also unclear how much HW would support ESIT > MFINDEX period.

Regards,
Michal

Good point, no, it does not remain correct.

I repeated the test using 1-microframe-long URBs in order to induce ring underruns/overruns.

When the URB_ISO_ASAP flag is not set, urb->start_frame is initially accurate, but drifts further and further off following each ring underrun/overrun event. In the completion handler, urb->start_frame appears to remain continuous with the previous URB even after underrun/overrun events occur. The URB completion status and the isochronous frame descriptor statuses always report success.

When the URB_ISO_ASAP flag is set, the behavior is essentially the same, except that urb->start_frame begins slightly offset from the actual start frame.

Fair enough, that makes sense. In any case, the 895 ms window is more than enough for what I'm trying to do.

-Dylan

Underruns are the most important thing to test here, both with CFC and without. Under normal conditions these patches have no effect (besides possibility of blatant bugs), but when the ring underruns we want to know how the HW and SW will react to it.

Yes, this is very annoying because it's undetectable (without CFC). It seems we need to handle Ring Underrun/Overrun events, somehow.

So far the only successful handling I found was to delay completion of the next URB by 1ms, which caused snd-usb-audio to underrun its buffer and restart the stream :)

I also tried:

1. Violate the 1.0 spec and write Frame IDs instead of SIA to all TDs. HCs respond oddly, some skip more than they should, some get stuck when Frame ID is in the past (probably wait for MFINDEX overflow).

2. Queue initial TDs of the next URB as No-Op TRBs and use Frame ID in the first TD which meets IST. But HCs handle isoc No-Op TDs oddly too:
- Renesas ignores Frame ID in an Isoch TD following a No-Op TD
- Fresco executes only one No-Op per interval
- VIA re-runs the No-Os sequence each interval until Frame ID is met in the subsequent Isoch TD

3. Don't queue inital TDs at all. This turns out to violate assumptions made in various parts of the driver. PITA, WiP, kernel panics etc.

Regards,
Michal

Agreed.

I think handling ring underrun and overrun events is probably the right approach here. I started putting together some tests, but ran out of time. I hope to spend some more time on it this weekend.

My current thinking is that for an URB submitted after an underrun or overrun has actually occurred, but before the xrun event is reported or handled, there will be no way to determine its true start frame, since it will have been submitted with SIA set.

However, because completions are processed in order, the HCD should still be able to determine by completion time whether an xrun occurred before completion of the URB, and could therefore indicate to the completion handler that the reported start_frame is uncertain.

Hopefully, USB drivers could then detect the discontinuity either:

- after completion of the first affected URB, assuming the HCD can indicate that start_frame is uncertain (for example via a flag or sentinel value), or
- at submission time for the first URB submitted after the HCD observes the xrun event.

Not to repeat myself too much, but I think this discussion also helps illustrate the value of allowing drivers to supply a specified start_frame, since xrun events become much less likely when scheduling sufficiently far into the future. That said, I think it is worth sorting out these underlying frame id calculation issues first.

Dylan

Hi

I've been working on a solution like this

If URB is submitted after a overrun/underrun then re-calculate its urb->start_frame based on running microframe index

If xhci driver handles the overrun/underrun event after a new URB is already submitted then kick the urb->start_frame forward by one ESIT

If xHCI CFC is used then don't touch urb->start_frame

I promised to focus on xHCI DbC issues next week so I'll share v3 now even if it again is not tested at all, and has some 'FIXME' cases left. First two patches are the same as v2.

Code should work and be testable

It does change a bit how MSE (+ skip flag) + overrun/underrun cases are handled. I haven't fully yet figured out all those cases, hopefully this doesn't make it much worse.

Would be good to get Michal's comment on that

I

Maybe we could add something like a 'delay_start_by_intervals' to the URB? telling the host that class driver wishes stream to start n intervals after usb was submitted.

Thanks
Mathias

Check if the expected frame IDs for a isochronous URB submitted mid stream is within the valid frame time window that xHC controller is capable of queuing TDs.

The range only needs to be checked once per URB as the isoc TDs of an URB are queued in one go with spinlock held and interrupts disabled.

Calculate the valid frame window start and end frame id in frames instead of microframes to better match how xhci specification section 4.11.2.5 does it.

Don't add frame id gaps or change scheduling to SIA mid stream if the start frame is outside the valid frame winow.
Only print a debug message.
Some devices can't handle gaps in isochronous transfers.

Calculate a valid start frame for the first URB of a stream, and align it to a full frame, or to interval start if interval is longer than a frame

Set urb->start_frame value for every URB

---
 drivers/usb/host/xhci-mem.c  |   1 +
 drivers/usb/host/xhci-ring.c | 180 +++++++++++++++--------------------
 drivers/usb/host/xhci.h      |   7 +-
 3 files changed, 84 insertions(+), 104 deletions(-)

index 997fe90f54e5..58714e10773f 100644
@@ -1499,6 +1499,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 		return -ENOMEM;
 
 	virt_dev->eps[ep_index].skip = false;
+	virt_dev->eps[ep_index].next_uframe = -1;
 	ep_ring = virt_dev->eps[ep_index].new_ring;
 	xhci_ring_init(xhci, ep_ring);
 

index aeecd301f207..bcb7b8c877db 100644
@@ -287,6 +287,11 @@ struct xhci_run_regs {
 	struct xhci_intr_reg	ir_set[1024];
 };
 
+/* Bits [13:3] of the microframe index equals the 1ms frame index */
+#define MFINDEX_TO_FRAME(p)	(((p) >> 3) & 0x7ff)
+#define MAX_FRAMES		2048
+#define MAX_UFRAMES		(MAX_FRAMES * 8)
+
 /**
  * struct doorbell_array
  *

@@ -696,7 +701,7 @@ struct xhci_virt_ep {
 	struct list_head	bw_endpoint_list;
 	unsigned long		stop_time;
 	/* Isoch Frame ID checking storage */
-	int			next_frame_id;
+	int			next_uframe;
 	/* Use new Isoch TRB layout needed for extended TBC support */
 	bool			use_extended_tbc;
 	/* set if this endpoint is controlled via sideband access*/

2.43.0

The frame id field can be set for the first TD of the first isoc URB to schedule the start of an isoc stream even in host doesn't support CFC (Contiguous Frame ID Capability)

Set the frame ID TRB field of the first isoc TD unless URB has the schedule immediately 'URB_ISO_ASAP' transfer flag set.

---
 drivers/usb/host/xhci-ring.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

index b1318515ae58..55a10d1e70cb 100644
@@ -3976,6 +3976,23 @@ static int xhci_ist_microframes(struct xhci_hcd *xhci)
 	return ist;
 }
 
+
+static bool xhci_isoc_td_uses_frame_id(struct xhci_hcd *xhci, struct urb *urb,
+				       struct xhci_virt_ep *ep, int i)
+{
+	if (urb->transfer_flags & URB_ISO_ASAP)
+		return false;
+
+	if (xhci->hcc_params & HCC_CFC)
+		return true;
+
+	/* set frame id for first TD of first URB in stream */
+	if (ep->next_uframe == -1 && i == 0)
+		return true;
+
+	return false;
+}
+
 /*
  * Check if frame is in the valid frame window, including start and end.
  * If start > end then assume window wrapped around at a limit the frame

@@ -4089,7 +4106,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 	int i, j;
 	bool more_trbs_coming;
 	struct xhci_virt_ep *xep;
-	int frame_id;
 	int uinterval = urb->interval;
 	int start_uframe;
 

@@ -4145,12 +4161,13 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		}
 		td = &urb_priv->td[i];
 
-		/* use SIA as default, if frame id is used overwrite it */
-		sia_frame_id = TRB_SIA;
-		if (!(urb->transfer_flags & URB_ISO_ASAP) && (xhci->hcc_params & HCC_CFC)) {
-			frame_id = (start_uframe + i * uinterval) / 8;
-			frame_id %= MAX_FRAMES;
-			sia_frame_id = TRB_FRAME_ID(frame_id);
+
+		/* Choose SIA of frame ID based scheduling for this TD */
+		if (xhci_isoc_td_uses_frame_id(xhci, urb, xep, i)) {
+			sia_frame_id = (start_uframe + i * uinterval) / 8;
+			sia_frame_id = TRB_FRAME_ID(sia_frame_id % MAX_FRAMES);
+		} else {
+			sia_frame_id = TRB_SIA;
 		}
 
 		/*

2.43.0

Make sure a new urb->start_frame is calculated for isoc URBs submitted late mid stream, i.e. isoc URB is submitted to an empty ring after a ring underrun or overrun event.

If URB was queud late, but before xhci driver handles the ring underrun/overrun event then kick the urb->start_frame forward by one ESIT for that delayed URB.

the gap in urb->start_frame allows class drivers to detect the glitch in isochronous transfers.

if xHCI uses Contiguous Frame ID Capability (CFC) then xHC will synchronize each TD to its frame ID, and the late TDs of the URB will be skipped instead of lagging the transfer, so don't touch urb->start_frame or ep->next_uframe in CFC cases

---
 drivers/usb/host/xhci-ring.c | 76 ++++++++++++++++++++++--------------
 drivers/usb/host/xhci.h      |  1 +
 2 files changed, 48 insertions(+), 29 deletions(-)

index 55a10d1e70cb..fa431640861d 100644
@@ -2624,6 +2624,40 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci,
 		return false;
 	}
 }
+/* called for isoc enpoints to handle overrun and underrun transfer events */
+static int handle_tx_event_overunderrun(struct xhci_virt_ep *ep)
+{
+	struct xhci_td *td;
+	struct urb *urb;
+	u32 uinterval;
+
+	/* don't touch ep->next_uframe and urb->start_frame if CFC frame sync is used */
+	if (!ep->last_td_used_sia)
+		return 0;   // what about skip??
+
+	/* re-calculate urb->start_frame on next urb enqueue */
+	if (list_empty(&ep->ring->td_list)) {
+		ep->next_uframe = -2; // FIXME define something, or use suitable -EXXX
+		ep->skip = false;  // maybe, or is it the wrong place?
+		return 0;
+	}
+
+	/* ep has tds, but queued late for its ESIT, adjust start_frame and next_uframe */
+	/* FIXME may have several URBs queued (unlikely) */
+	td = list_first_entry(&ep->ring->td_list, struct xhci_td, td_list);
+	urb = td->urb;
+	uinterval = urb->interval;
+
+	if (urb->dev->speed == USB_SPEED_LOW || urb->dev->speed == USB_SPEED_FULL)
+		uinterval *= 8;
+
+	ep->next_uframe += uinterval;
+	urb->start_frame += urb->interval;
+
+	return 0;
+	/* FIXME: ep_ring->old_trb_comp_code = trb_comp_code; */
+	/* FIXME: if (ep->skip) */
+}
 
 /*
  * If this function returns an error condition, it means it got a Transfer

@@ -2644,7 +2678,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	int status = -EINPROGRESS;
 	struct xhci_ep_ctx *ep_ctx;
 	u32 trb_comp_code;
-	bool ring_xrun_event = false;
 
 	slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
 	ep_index = TRB_TO_EP_ID(le32_to_cpu(event->flags)) - 1;

@@ -2754,11 +2787,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		 * Underrun Event for OUT Isoch endpoint.
 		 */
 		xhci_dbg(xhci, "Underrun event on slot %u ep %u\n", slot_id, ep_index);
-		ring_xrun_event = true;
 		break;
 	case COMP_RING_OVERRUN:
 		xhci_dbg(xhci, "Overrun event on slot %u ep %u\n", slot_id, ep_index);
-		ring_xrun_event = true;
 		break;
 	case COMP_MISSED_SERVICE_ERROR:
 		/*

@@ -2823,6 +2854,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb_dma)
 		return 0;
 
+	if (trb_comp_code == COMP_RING_UNDERRUN || trb_comp_code == COMP_RING_OVERRUN)
+		return handle_tx_event_overunderrun(ep);
+
 	if (list_empty(&ep_ring->td_list)) {
 		/*
 		 * Don't print wanings if ring is empty due to a stopped endpoint generating an

@@ -2832,7 +2866,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		 */
 		if (trb_comp_code != COMP_STOPPED &&
 		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
-		    !ring_xrun_event &&
 		    !xhci_spurious_success_tx_event(xhci, ep_ring)) {
 			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
 				  slot_id, ep_index);

@@ -2856,20 +2889,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 				skip_isoc_td(xhci, td, ep, status);
 
-				if (!list_empty(&ep_ring->td_list)) {
-					if (ring_xrun_event) {
-						/*
-						 * If we are here, we are on xHCI 1.0 host with no
-						 * idea how many TDs were missed or where the xrun
-						 * occurred. New TDs may have been added after the
-						 * xrun, so skip only one TD to be safe.
-						 */
-						xhci_dbg(xhci, "Skipped one TD for slot %u ep %u",
-								slot_id, ep_index);
-						return 0;
-					}
+				if (!list_empty(&ep_ring->td_list))
 					continue;
-				}
 
 				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
 					 slot_id, ep_index);

@@ -2878,10 +2899,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				goto check_endpoint_halted;
 			}
 
-			/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
-			if (ring_xrun_event)
-				return 0;
-
 			/*
 			 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
 			 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue

@@ -2926,10 +2943,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 	ep_ring->old_trb_comp_code = trb_comp_code;
 
-	/* Get out if a TD was queued at enqueue after the xrun occurred */
-	if (ring_xrun_event)
-		return 0;
-
 	trace_xhci_handle_transfer(ep_ring, (struct xhci_generic_trb *) ep_trb, ep_trb_dma);
 
 	/*

@@ -4042,11 +4055,14 @@ static int xhci_get_isoc_start_frame(struct xhci_hcd *xhci, struct urb *urb,
 
 	// FIXME, used to check if !list_empty(&ep_ring->td_list)), is that reliable
 
-	/* Is this the first URB starting the whole isoc transfer */
+	/*  first URB starting the whole isoc transfer, or restart after over/under-run */
 	if (ep->next_uframe < 0) {
-		/* align first URB to next interval boundary, or at last to full frame */
-		start_uframe = mfindex + ist + XHCI_CFC_DELAY;
-		start_uframe = roundup(start_uframe, 8);
+		start_uframe = mfindex + ist;
+		/* if first URB then add additinal delay and align to full frame */
+		if (ep->next_uframe == -1) {
+			start_uframe += XHCI_CFC_DELAY;
+			start_uframe = roundup(start_uframe, 8);
+		}
 		start_uframe = roundup(start_uframe, uinterval) % MAX_UFRAMES;
 	} else {
 		/* URB is mid stream and expected to handle the next frame */

@@ -4166,8 +4182,10 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		if (xhci_isoc_td_uses_frame_id(xhci, urb, xep, i)) {
 			sia_frame_id = (start_uframe + i * uinterval) / 8;
 			sia_frame_id = TRB_FRAME_ID(sia_frame_id % MAX_FRAMES);
+			xep->last_td_used_sia = false;
 		} else {
 			sia_frame_id = TRB_SIA;
+			xep->last_td_used_sia = true;
 		}
 
 		/*

index bcb7b8c877db..19d6f3104a15 100644
@@ -702,6 +702,7 @@ struct xhci_virt_ep {
 	unsigned long		stop_time;
 	/* Isoch Frame ID checking storage */
 	int			next_uframe;
+	bool			last_td_used_sia;
 	/* Use new Isoch TRB layout needed for extended TBC support */
 	bool			use_extended_tbc;
 	/* set if this endpoint is controlled via sideband access*/

2.43.0

Hi Mathias,

I had a chance to test these patches.

This does appear to create a detectable gap, but urb->start_frame still drifts relative to the actual frame id. After each underrun/overrun, urb->start_frame becomes increasingly offset.

This would definitely help, though specified start_frame behavior would still be preferable so that the same driver architectures can be used across Linux, macOS, and Windows.

Best Regards,
Dylan

That's not surprising, the exact delay is unpredictable, which will affect drivers that tolarate the gap and continue their stream.

Yes, they have already been written to the ring as SIA. (And if they weren't SIA, the HW is empirically known to do weird things. The 1.0 spec was weird and so is the HW which implements it).

These URBs will very likely execute in a wrong interval, unless we stop the endpoint to remove them (which we may fail to do quickly enough) or completely prevent submissions violating the IST in the first place.

If we want to gamble that maybe a slight IST violation will still succeed anyway, this seems to be the only recovery when we lose.

Either way, we need to fail certain IST-violating submissions with -EXDEV or complete them immediately with -EXDEV status. Which is what I was working on and finding various issues with (the driver doesn't expect queued URBs which aren't written to the ring). Unfortunately, it still isn't finished as I have little free time currently.

By the way, do you know what Windows does in such cases?

Regards,
Michal

Yeah, if this mid-stream recovery concern can be avoided by disallowing IST violations in the first place, that seems like the better approach. It took me a moment to understand what you were getting at, but if an IST-violating transfer cannot be submitted in the first place, then I think this problem mostly goes away. If a driver wants to try violating the IST, it could just use the ASAP flag instead, right?

This makes sense, and I am starting to wrap my head around the various requirements, so I may try putting together a patch as well, although finding the time may be difficult for me too.

I wasn't sure, so I ran some tests. Windows behaves somewhat unexpectedly with IST-violating submissions. The observable behavior appears to be mostly the same regardless of CFC.

Windows appears to always complete isochronous URBs with a success status, regardless of whether the transfers actually occur on the bus (yikes). When a transfer does occur, it appears on the exact frame requested by the submission, whereas transfers that do not occur, presumably because they violated the IST, may complete immediately upon submission. Consequently, I saw URB completions occurring out of submission order.

Best Regards,
Dylan

Hi,

I'm back after a few months of having no time whatsoever to look at this project.

I noticed that some variant of this patch has appeared in the xhci/for-usb-next branch. There are problems with it.

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

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

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.

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

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.

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.

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.

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.

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.

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

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.

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

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

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:

* 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. Class driver is aware of this and expected to consider the submission a new stream. If it wishes to continue the stream that underrun, its last chance is to submit from completion, when the queue is "active":

* If the flag is not set and the queue is active then the URB is
* always assigned to the next slot in the schedule following the end
* of the endpoint's previous URB, even if that slot is in the past.

These rules were straightforward with synchronous giveback in IRQ. To deal with BH, the following was done for ehci-hcd:

c7ccde6eac6d USB: see if URB comes from a completion handler 46c73d1d3ebc USB: EHCI: handle isochronous underruns with tasklets

Dylan Robinson correctly noted that this doesn't cover submissions from other code while the completion is still waiting in the BH queue. But that's a USB subsystem bug affecting ehci-hcd too, and IDK if any in-tree driver cares. (It might be that no one knows that they care).

It was my impression while testing this that snd-usb-audio handles playback underrun during synchronized duplex operation without cycling through altsetting zero or unlinking playback URBs (none are left), so it could possibly be affected.

It's also possible that the only effect would be wasting a few ms to resubmit URBs until it catches up with MFINDEX. But then, another concern of users was to recover from glitches as fast as possible.

Regards,
Michal

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.

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

This quote needs some context; it applies when the new URB is submitted while the queue is still active, for example, by a completion handler.

Michal is correct; that's what it's supposed to mean.

... useless in that situation. That's right. The flag is used only while the queue is still active.

I don't know about likelihood, but the fact is that "queue is idle" does mean all completion handlers have returned. Of course, this includes the "not yet started" case, but it includes other cases as well.

Do any existing class drivers actually rely on documented behavior of isochronous queues? I have no idea. In fact, I would expect that if a class driver wanted to stop a queue and restart it later, it would switch to altsetting 0 in the meantime, which would eliminate all ambiguities.

The point under discussion is really about what happens when an underrun occurs by accident -- for example, if the system is temporarily overloaded. In this situation there are two logical choices of action if the class driver wants to keep the queue running: Keep the alignment of iso_packet_descriptors and (micro)frames as it was, which means assigning some new packets to times in the past, or forget about the old alignment and start fresh, which means assigning the next packet to the next available future time slot. The URB_ISO_ASAP flag is how the driver communicates its choice to the HCD.

On the other hand, if the class driver doesn't respond right away and lets the queue dry out totally, the first choice becomes invalid. After all, if the driver cared about keeping the slots aligned, it wouldn't have waited so long that the queue became idle. And besides, there's no time limit on how a queue can remain idle before the driver decides to start using it again; we can't expect an HCD to keep track of the queue's state indefinitely while it's not being used.

For these reasons the URB_ISO_ASAP flag is ignored if the queue is idle -- meaning that all completion handlers have returned (because submission of new URBs in an iso stream typically occurs within a completion handler).

Alan Stern

I want to make sure I'm following along correctly. This is only true for host controllers that support CFC. On controllers without CFC, continuations are always submitted with SIA, so the controller can't reject expired TDs based on their Frame ID.

How is the first choice realized when the host controller doesn't support CFC?

Dylan Robinson

Yes, we are talking about the patch fixing CFC logic. You are right, and you could only be more right with "because" in place of "so"; I tried treating old chips as if they had CFC, outcomes were weird.

Currently it isn't, everything goes "ASAP" on xHCI 1.0 hardware and underrun creates a gap. Another patch that Mathias prepared for 7.3 would align the first URB of each new stream, but nothing more.

We get the Ring Underrun event, so we know it happened. Drivers don't know and they seem to have no trivial and robust way to detect it. To them, URBs are completing, submissions are succeeding, all looks fine.

Potential solutions I could think of:

1. Somehow let drivers know that a fresh start is their only choice (some completion status, bogus start_frame, usb_submit_urb() error, callback, ...) and have them ready to deal with this (hard part).

2. Try to resync in xhci-hcd: quickly return newly submitted frames as -EXDEV until one is far enough into the future to reliably meet the IST and start a new "isoc data flow" from HW's point of view.

Complicated by the fact that new URBs may be submitted after the underrun occurs but before our IRQ handler learns about it. Those are doomed to execute with some lag (unless unlinked). We could mark them -EXDEV or some such so that drivers don't trust them.

Not sure if this is desired by drivers, particularly the potential inefficiency of the "complicated" part. I wouldn't be shocked if audio (even specifically pro audio) was the only application in the world that truly cares about exact scheduling.

I wonder if it should be recommended that drivers which don't care always set URB_ISO_ASAP to opt out of any such insanity just in case? I know that uvcvideo uses ASAP, for example.

Regards,
Michal

Tune urb->start_frame to be one step closer to reality could be a first step. Give class drivers a chance to detect a difference in expected and reported urb->start_frame

If there are already queued TDs when we receive an underrun event then add urb->start_frame += ESIT to every queued URB

If there are no pending URBs then set flag to calculate new urb->start_frame for the next URB when enqueued.

This is of course only for the ASAP case.
It won't be perfect, might drift especially if URBs are enqueued before we handle the underrun event, but not too difficult to implement and better than current situation

Thanks
Mathias

IMO, it would be fine for the URBs that are submitted after the underrun but before the IRQ handler learns about it to have their transfers happen discontinuously, as long as they are marked -EXDEV or similar upon completion. This only applies to URBs submitted from their own endpoint's completion handler, right?

Any driver establishing a stream must submit more than one URB at startup. I've often seen discontinuities with the class audio driver happening with these initial URBs. These would not be detectable from the completion status because they are not submitted from completion context. However, perhaps once the non-CFC implementation starts streams without SIA, a class driver could instead use start_frame continuity across completions to detect these cases.

Well, there are HW queues and SW queues, and HW and SW underruns.

If the HW queue underruns but the SW queue still has pending URBs (not completed yet) then we do care about URB_ISO_ASAP. If the SW queue is completely empty (all completed), we aren't expected to.

This does make a difference with snd-usb-audio. If I run

jackd -d alsa -d hw:... -p 24 -n 2

Ring Underrun and Missed Service are reported every now and then, sometimes repeatedly, and it doesn't take long to enter this loop:

1. playback Ring Underrun (xHCI EP state is Running)
2. capture URB unlink (xHCI EP state is Stopped)
3. multiple capture URBs scheduled to the same start_frame before EP state becomes Running
4. new playback URB is scheduled into a distant past due Running EP
5. 2 uframes later OUT endpoint reports Missed Service for all those misscheduled TDs
6. goto 1

Changing the condition from "running endpoint" back to "empty list" breaks this pathological cycle. Experimental patch below.

index 1ff84ab9955a..8e380d1c066e 100644
@@ -4303,6 +4303,12 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 	return ret;
 }
 
+static int _ep(int ep_index)
+{
+	ep_index++;
+	return ep_index / 2 + 0x80 * (ep_index & 1);
+}
+
 /*
  * Check transfer ring to guarantee there is enough room for the urb.
  * Update ISO URB start_frame and interval.

@@ -4348,7 +4354,22 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
 	 * Check if this starts the isoc data flow. Relies on hw setting ep ctx
 	 * state after doorbell ring. Consider adding list_empty(td_list) check
 	 */
-	if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
+	bool running = GET_EP_CTX_STATE(ep_ctx) == EP_STATE_RUNNING;
+
+	bool pending = !list_empty(&ep_ring->td_list) ||
+		       hcd_periodic_completion_in_progress(xhci_to_hcd(xhci), urb->ep);
+
+	if (running && !pending)
+		xhci_info(xhci, "ep %.2x running without pending URBs\n", _ep(ep_index));
+	if (pending && !running)
+		xhci_info(xhci, "ep %.2x pending URBs but not running\n", _ep(ep_index));
+
+	//if (!running)
+	if (!pending)
 		xep->next_uframe = -1;
 
 	return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);

agree, and this needs to be fixed with something like:

- if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)
+ if (list_empty(&ep_ring->td_list) &&
+ GET_EP_CTX_STATE(ep_ctx) != EP_STATE_RUNNING)

But this is different from incorrectly assuming a new stream started mid stream just because list_empty(&ring->td_list) is true and the urb completion workqueue isn't at this instance processing a work item belonging to this endpoint.

Example:

AFAICT, notify_xrun means userspace didn't submit new playback samples before the last OUT URB completed. No OUT URBs are left, endpoint state is Running-Idle, the driver only unlinks IN URBs.

[300299.231743] usb 10-1: notify_xrun 01 from 577
[300299.231749] usb 10-1: Stopping data EP 0x81 (running 1)
[300299.231815] usb 10-1: 2:2 Stop Capture PCM
[300299.231817] usb 10-1: Stopping data EP 0x1 (running 1)
[300299.231819] usb 10-1: 1:3 Stop Playback PCM

Some time later, IN is restarted and scheduled 0x11 uframes ahead. On second submission we see the "ep state not yet running" race.

[300299.233588] usb 10-1: Starting data EP 0x81 (running 0)
[300299.233616] xhci_hcd 0000:08:00.0: xdebug ep 81 uframe 0f47: scheduling new stream for 0f58
[300299.233628] xhci_hcd 0000:08:00.0: xdebug ep 81 uframe 0f47: pending URBs but not running
[300299.233705] usb 10-1: 12 URBs submitted for EP 0x81
[300299.233710] usb 10-1: 2:2 Start Capture PCM

Then OUT is restarted and mis-scheduled to a random uframe 1018, far away from IN URBs, because xHCI endpoint state is Running.

[300299.233713] usb 10-1: Starting data EP 0x1 (running 0)
[300299.233722] xhci_hcd 0000:08:00.0: xdebug ep 01 uframe 0f48: running without pending URBs
[300299.233726] xhci_hcd 0000:08:00.0: xdebug ep 01 uframe 0f48: mis-scheduling new stream for 1018
[300299.233739] usb 10-1: 2 URBs submitted for EP 0x1
[300299.233741] usb 10-1: 1:3 Start Playback PCM

And this smells like regression, because the original condition was:

if (list_empty || EP_STATE != RUNNING)
start new stream

so the driver would never *fail* to start a new stream when no URBs are pending at all, it could only attempt to start a new stream while URBs are pending, due to the pointless EP_STATE test or BH giveback.

Yes, this behavior is inconsistent with documentiation and disliked by one out-of-tree driver developer (probably rightly so), but it's been this way for over a decade, both in xhci-hcd and ehci-hcd.

And it doesn't affect drivers that resubmit from completion, which is apparently good enough that people stopped complaining when Alan added the "or completion is running now on this CPU" hack.

If somebody is very bothered by this, it seems it could be fixed, for example by counting submitted URBs and having the BH worker decrement the count after each finished completion.

In fact, I wrote a hack which does this for xhci-hcd (by replacing the complete callback with a wrapper that performs counting). So far I found no case where this check makes a difference, but finding one could become an argument for fixing this in core for real.

Regards,
Michal