drivers/usbhost: Make xHCI transfers work with real devices. - #19861
Open
Fishwaldo wants to merge 21 commits into
Open
drivers/usbhost: Make xHCI transfers work with real devices.#19861Fishwaldo wants to merge 21 commits into
Fishwaldo wants to merge 21 commits into
Conversation
github-actions
Bot
added
Size: XL
|
🔗 Cross-repo PR dependenciesThe read-only Build run reported the following dependent PR(s) and fetched head SHA(s): CI run: https://github.com/apache/nuttx/actions/runs/31939575272 |
Member
|
@Fishwaldo Where did you get the idea that the driver was only run from QEMU? That's not true. This driver worked with real Intel64 hw. |
Contributor
Author
|
@raiden00pl The only thing in this tree that selects this driver is boards/x86_64/qemu/qemu-intel64/configs/jumbo/defconfig |
Contributor
Author
|
@jerpelea can you give some guidance how to do Stacked PR's here ? I looked at the Depends On Infrastructure and rebasing but can't figure out how to do it while passing CI tests. (I was asked to split this into three PR's.... so... ) |
Fishwaldo
force-pushed
the
upstream-xhci-datapath
branch
from
August 17, 2026 13:17
8817abb to
6d6b83e
Compare
Contributor
Author
|
Rebased after #19745 landed. |
Fishwaldo
marked this pull request as draft
August 17, 2026 15:16
🔗 Cross-repo PR dependenciesThe read-only Build run reported the following dependent PR(s) and fetched head SHA(s): CI run: https://github.com/apache/nuttx/actions/runs/32034276870 |
Contributor
|
You have to wait until the first PRs are reviewed and merged then rebase |
The handler defers to a worker that walks the event ring, and the ring is not allocated until the controller is started, several steps later. A controller left running by a boot loader has an interrupt pending as soon as the line is enabled, so attaching earlier is a race with nothing able to answer it. Attach after the start, and clear USBSTS and the interrupter pending flag once the handler is in place: a message signalled interrupt is sent on the flag's clear to set transition, so a flag raised before the handler existed would never produce another. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
xhci_probe_ports() wrote PORTSC back to clear the change bits, including PED, which is write-one-to-clear. A port that came up enabled, which is what a device attached at power up produces, was switched off by the act of reading it. Mask PED out of the value written back. The port status worker already does this. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The event ring was acknowledged after being walked. An event arriving during the walk sets the pending bit again, and clearing the bit afterwards discards it. Transfers have no timeout, so the transfer that event belonged to waits forever. Acknowledge first. A spurious second pass over an empty ring costs nothing. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The handler read the status, queued the work that would answer it, and returned with the source still asserted. On a level triggered line the interrupt controller sees the condition still true and raises it again at once, so the work that would have cleared it never runs. Mask the interrupter in the handler and let the worker unmask when it is done. The unmask clears the pending flag in the same write, because a message is sent on that flag's clear to set transition and events that arrived while the interrupter was masked have already set it. Clearing opens its own window, so the worker drains the ring again after unmasking and repeats while a drain finds anything; xhci_events_poll() returns how many events it handled for that purpose. A drain that finds nothing is the only state in which no event can have been lost. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The interval was left at its reset value of 4000, a millisecond, which is
how long the controller waits after an event before reporting it. Every
completion paid that, and mass storage spends three transfers on a
request.
Set it to 160, which is 40us, as Linux does. Zero puts no bound on how
often a controller may interrupt: a keyboard on an interrupt endpoint then
takes them continuously and occupies a processor.
Measured on a DWC3 with a USB 2.0 drive, doorbell to interrupt 986-1021us
before and 13-56us after:
reading 1MiB before after
512 byte blocks 166 KB/s 775 KB/s
32 KiB blocks 10666 KB/s 18618 KB/s
mounting a FAT32 volume: 92.7s before, 21.1s after
Assisted-by: Claude:claude-opus-5
Signed-off-by: Justin Hammond <justin@dynam.ac>
xhci_ctrl_start() published the event ring segment table, the device context base address array and the scratchpad pointers with up_flush_dcache_all(), which an architecture whose cache can only be maintained by address implements as a barrier and nothing more, so none of them reached memory. The controller then reads whatever those addresses held before, which presents as every command timing out with no events arriving. Flush each structure by address. xhci_ring_init() has the same fault from the other direction: it clears a whole ring and flushes only the link entry it writes afterwards, leaving the rest of the clearing in the cache. The controller writes into that memory itself, so a line written back later lands on top of an event somebody is waiting for. Flush the whole ring. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
A Normal TRB describes one run of memory that may not cross a 64K boundary, and the block layer hands down whole multi-sector reads whose length is bounded by nothing here. One TRB was programmed regardless, so a long enough transfer, or merely one starting near the wrong side of a boundary, produced a descriptor the controller is entitled to reject or to satisfy in part. Program as many as the run needs, chained, asking for the completion interrupt only on the last so one event still arrives for the transfer. A transfer needing more TRBs than the ring holds is refused. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The controller moves every byte itself, so on a machine whose caches are not coherent with it the driver must flush before the controller reads and invalidate before the processor does. Data buffers got no maintenance at all: nothing pushed before an OUT, nothing dropped after an IN. Cache operations act a whole line at a time, which is unsafe for a buffer that does not own its lines: invalidating drops whatever else shares the line, and a writeback lands on top of what the controller has just put there. Mass storage passes a 31 byte command block and a 13 byte status out of its instance structure. Such a buffer is copied through an aligned stand-in; anything large comes from a filesystem or from xhci_ioalloc(), which now rounds its length up as well as aligning its start, so what it returns owns its last line. Whether the controller can reach a buffer at all is asked of the platform through a new dmacapable operation, since it is a property of the system the controller was fitted into rather than of the controller. A platform that does not supply it is taken to accept every address, which is what existing users have. A refused buffer gives -EFAULT, which the FAT filesystem answers by retrying through its own DMA-safe sector buffer. The device output context is also invalidated before the assigned address is read out of it; the controller wrote that address, and reading without invalidating returns whatever the processor had cached. Compiles to nothing where there is no cache to maintain, and dmacapable is NULL on PCI, so the existing user is unaffected. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The register dump read HCIVERSION with a 32-bit access at offset two. It is a 16-bit register sharing a word with CAPLENGTH, so that is an unaligned read of a device register: harmless where the bus permits it and a fault where it does not. Read the word once and take both fields from it. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
A failed command logged only its completion code. The difference between a refused Address Device and a refused Evaluate Context is most of the diagnosis, and the completion code does not give it. Keep the command type before the result overwrites the TRB, and name it in the message. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The number of event ring segments a controller allows is a power of two reported as its exponent, and the exponent can reach 15. Computing 1 << exponent into the uint8_t that holds it wraps to zero on any controller offering more than 128 segments, and a controller told its event ring table holds no entries has nowhere to report anything: every command times out. Work it out at full width and narrow afterwards. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
What a controller is told about a device before it will accept it. A DWC3 core validates these where QEMU's controller does not. - HCCPARAMS1 says whether context structures are 32 or 64 bytes, and the wider form was refused outright with -EIO; the EIC7700X reports 0x0220fe45 on both of its controllers, so this driver could not have driven either. A wide context is the same fields with reserved space after them, so only the stride changes. Read it at start up and use it wherever a context array is walked. - Contexts must be 64 byte aligned, since every device context base address array entry points at one, and the output context came from kmm_zalloc(). - The slot context never carried the device speed, which has no valid zero, so a validating controller answers Address Device with a parameter error. The speed was already implied by the endpoint context's maximum packet size. The numbering is xHCI's own, hence the mapping. - The output device context was cleared and never flushed. That context is the controller's to write, so what stays behind is a dirty line of zeros written back over the slot state, and the next command against the slot is refused with a context state error. Enumeration reached SET_ADDRESS and stopped. - A buffer copied through an aligned stand-in was copied back using buflen, which control transfers deliberately leave zero, so a descriptor read copied nothing back and the caller was handed whatever its buffer held before. Keep the requested length separately, and maintain the cache over the whole stand-in rather than the part in use. - A buffer the controller cannot reach is now copied through a stand-in rather than refused. -EFAULT works for a caller with somewhere better to put the data, and fails outright for one without: reading a block device directly from a user program returned an error where the transfer could have gone through a stand-in. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
A transfer described by more than one TRB can reach the end of the ring
part way through, so the link that sends the controller back to the
beginning falls inside the transfer rather than between two of them.
Written without the chain bit, that link ends the transfer where it
stands: the controller follows it, considers the work finished, and
reports nothing, because the TRB that asked for the completion interrupt
is on the far side of the join. Nothing waiting is woken, and transfers
have no timeout, so the symptom is a read that never returns.
Carry the chain bit onto the link when the TRB it follows has it.
Reading 1MiB from a USB drive, where the last two sizes did not complete
at all before:
512 byte blocks 166 KB/s
4 KiB blocks 1333 KB/s
32 KiB blocks 10666 KB/s
64 KiB blocks 15515 KB/s
Assisted-by: Claude:claude-opus-5
Signed-off-by: Justin Hammond <justin@dynam.ac>
Report each device as it comes up, and report it going away. The announcement is made at the end of the port enable rather than at connect, because the PORTSC speed field means nothing until the port has been reset: a USB2 port reports its reset default, full speed, until then, so every device would be announced at 12Mbps regardless of what it negotiates a moment later. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The copy out of a stand-in was done in the completion handler, which runs on a work queue, while the buffer it copies into may belong to a user process whose addresses mean nothing there. Reading a block device directly from a user program faulted. The caller is blocked until the transfer finishes, so the copy belongs there. An asynchronous transfer has no blocked caller to come back to, so a buffer that would need a stand-in is refused for that path. Its callers are class drivers using kernel memory, which do not need one. The refusal is lifted once the completion path can do the copy itself. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The Interval field of an endpoint context is an exponent: the controller services the endpoint every 2^Interval microframes. An endpoint descriptor states its period differently depending on device speed, so the number cannot be copied across, which is what this did. A low speed keyboard asking to be polled every 10ms was programmed as 2^10 microframes, which the controller would not accept: Configure Endpoint went unanswered and allocation failed with -EIO. Low and full speed interrupt endpoints state a period in frames, so the exponent is the highest bit of that period in microframes, clamped to the range the specification allows. Other periodic endpoints already state an exponent, one greater than the one wanted here. Control and bulk endpoints are not periodic and the field means nothing to them. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
A root hub port whose enumeration failed is enumerated again, and the slot the failed attempt used has been given back by then, so the port has no device context behind it. xhci_epalloc() took that pointer and wrote the new endpoint through it without looking, so the retry stored through NULL and took the system down in answer to a device that had merely failed to come up. Check for the device, and free the endpoint that has no home rather than leaking it. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
Submitting an asynchronous transfer refused any buffer needing a cache line stand-in, and that test also refuses every buffer whose length is not a whole number of cache lines, which an interrupt transfer's rarely is: a HID keyboard reads eight bytes. Every submission returned -EFAULT before a descriptor was written, and a class driver resubmitting from its completion callback never sees a second chance. The refusal existed because the copy out of a stand-in is done by the blocked caller, and an asynchronous transfer has none. The work queue thread handling the completion will do: a buffer given to DRVR_ASYNCH comes from DRVR_ALLOC, so it is kernel memory reachable from any thread. Use the same stand-in machinery as every other transfer and finish the DMA in the completion, just before the callback. A cancelled transfer returns its stand-in on cancellation. The callback also moves outside the spinlock. It is class driver code that queues work and takes its own locks, and it may now free a stand-in. Whether a completion is synchronous is still decided under the lock, since a posted waiter may be carrying a new transfer immediately. The asynchronous setup now records the requested length, as the synchronous setup does. The byte count handed to the callback is worked out from it and the residue, and was previously whatever the endpoint held from an earlier transfer. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
xhci_ctrl_xfer() and xhci_transfer() release the controller lock before xhci_transfer_wait(), so the lock does not cover the interval in which a transfer is outstanding. Two threads issuing requests on the same endpoint both reach xhci_ioc_setup(), and the second trips the DEBUGASSERT(!epinfo->iocwait) that guards it, or overwrites the first thread's completion state where assertions are compiled out. A default control endpoint reaches this readily: every interface driver on a composite device speaks through endpoint 0, so a two interface HID keyboard runs two poll threads both issuing GET_REPORT. Other host controller drivers hold the controller lock across the wait, which here would serialise the whole controller and give up the per endpoint rings xHCI provides. Add a mutex to struct xhci_epinfo_s and hold that instead. It is taken before the controller lock on both paths, so the order is endpoint then controller. xhci_epfree() also freed the endpoint container without destroying iocsem. Destroy both. Reachable on any xHCI controller, independently of the preceding commits. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
A device slot is a finite controller resource: HCSPARAMS1 reports how many exist and Enable Slot fails with No Slots Available once they are gone. Two paths took one and returned without giving it back. xhci_device_init() enables a slot before initialising the transfer ring, the slot context and the device address, and each of those returned directly on failure. It also treated a slot number larger than the controller supports as success, since Enable Slot itself had succeeded. xhci_enumerate() is the larger leak: the device is addressed by the time usbhost_enumerate() runs, so a device whose descriptor cannot be read, or that no class driver claims, leaves the slot held. That path clears hport->connected so the port is retried, taking another slot each time. Release the slot on both paths with xhci_device_deinit(), which issues Disable Slot, clears the DCBAA entry and resets the context. The endpoint ring is left allocated; xhci_ring_init() reuses an existing one. Tested on an EIC7700X board with a device no class driver claims, so the port retries indefinitely: previously the eighth attempt failed with completion code 9 and the controller enumerated nothing further on either port; now 1104 consecutive attempts produced no slot failure. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
xhci_enumerate() reports failure by marking the hub port disconnected, which is what makes xhci_wait() return and the attempt repeat. The root port is still connected, so the two disagree again immediately and the attempt repeats for as long as the device stays plugged in. A device that fails every time is retried forever: 1055 attempts in 90 seconds on an EIC7700X board, enough console traffic to make the board unusable. Count consecutive failures per root port and stop at CONFIG_USBHOST_XHCI_ENUM_RETRIES, leaving the port as it is so xhci_wait() blocks until something physically changes. A new connection clears the count, as does a successful enumeration, so a device needing a second attempt still gets one. The default of three rides out a slow device or a marginal reset. The same board now makes three attempts, reports that it has given up and falls silent, while a keyboard on the other port enumerates throughout. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
Fishwaldo
force-pushed
the
upstream-xhci-datapath
branch
from
August 18, 2026 13:32
6d6b83e to
5e77cd2
Compare
Fishwaldo
marked this pull request as ready for review
August 18, 2026 13:45
Contributor
Author
|
Rebased :) |
❌ Cross-repo dependency could not be appliedThe Build report says the declared dependency PR(s) could not be applied, so CI did not run against the combined code: Reason: cherry-pick failed (if your PR has merge commits, rebase instead) CI run: https://github.com/apache/nuttx/actions/runs/32142995920 |
| usbhost_vtrace2(XHCI_VTRACE2_PORTSC_DISCONND, | ||
| rhpndx + 1, priv->pscwait); | ||
|
|
||
| syslog(LOG_INFO, "%s: port %d: device removed\n", |
Contributor
There was a problem hiding this comment.
let's add debug macro in include/debug.h like others
| * then. | ||
| */ | ||
|
|
||
| syslog(LOG_INFO, "%s: port %d: device attached at %s\n", |
| * drivers using kernel memory, which needs no stand-in. | ||
| */ | ||
|
|
||
| if (!xhci_dma_direct(priv, buffer, buflen)) |
Contributor
There was a problem hiding this comment.
do you need change other similar check to xhci_dma_direct
| * | ||
| ****************************************************************************/ | ||
|
|
||
| static uint8_t xhci_interval(uint8_t speed, uint8_t xfrtype, |
Contributor
There was a problem hiding this comment.
where you call this function
|
|
||
| #ifndef CONFIG_USBHOST_INT_DISABLE | ||
| epinfo->interval = epdesc->interval; | ||
| epinfo->interval = xhci_interval(hport->speed, epdesc->xfrtype, |
Contributor
There was a problem hiding this comment.
squash into previous patch
| FAR uint8_t *buffer, size_t buflen); | ||
| static uint32_t xhci_speed_id(uint8_t speed); | ||
| #ifdef CONFIG_USBHOST_ASYNCH | ||
| static bool xhci_dma_direct(FAR struct usbhost_xhci_s *priv, |
Contributor
There was a problem hiding this comment.
why add in the previous patch, but remove here? please review AI generated patch carefully and remove the intermediate change
| bool dmain; /* Direction this buffer was prepared for */ | ||
| sem_t iocsem; /* Semaphore used to wait for transfer completion */ | ||
|
|
||
| /* One transfer at a time on an endpoint. The controller lock below is |
Contributor
There was a problem hiding this comment.
why the second transfer can't append the transfer to the hardware link list
jerpelea
requested review from
acassis,
cederom,
jerpelea,
linguini1 and
xiaoxiang781216
August 19, 2026 13:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The first seven commits belong to #19745 and #19860 and are not part of this
change. This PR adds the remaining twenty-one, one fault each.
The driver had only ever run behind QEMU's controller and its message
signalled interrupt, on a host with coherent caches and a flat address space.
These are the faults that reach it once any of that stops being true.
Interrupts. The handler returned with the source still asserted, so a level
triggered line re-fired forever and the worker never ran. The event ring was
acknowledged after being walked, discarding anything that arrived during the
walk. The interrupt was attached before the event ring existed, which races a
controller a boot loader left running. The moderation interval was left at its
reset value of 1ms, so every completion paid that. A port was disabled by the
act of probing it, since PORTSC change bits are write-one-to-clear.
Cache and addressing. Rings were published with
up_flush_dcache_all(),which an architecture that can only maintain by address implements as a barrier
and nothing more, so the controller read stale memory. Data buffers got no
maintenance at all. A buffer that does not own its cache lines now goes through
an aligned stand-in, since maintaining a partial line disturbs whatever shares
it. Whether the controller can reach a buffer is asked of the platform through
a new optional
dmacapableoperation.Transfer descriptors. A Normal TRB describes one run of memory that may not
cross a 64K boundary, and one TRB was programmed regardless of length. A link
TRB reached part way through a multi-TRB transfer was written without the chain
bit, which ends the transfer at the link, so nothing is woken and the read
never returns.
Device description. Contexts came in one size only and the wider form was
refused with
-EIO; the EIC7700X reports it on both of its controllers. Theevent ring segment count wrapped to zero above 128 segments. The slot context
never carried the device speed, which has no valid zero.
Endpoints and slots. The endpoint interval is an exponent and the
descriptor's period was copied across unconverted, so a low speed keyboard
never enumerated. Asynchronous transfers refused every buffer whose length was
not a whole number of cache lines, which an eight byte HID report never is.
Transfers on one endpoint were not serialised, which a composite device's two
poll threads reach through endpoint 0. A failed enumeration leaked its device
slot and then retried forever.
Two commits cover more than one point and say why in their messages: the
interrupter mask cannot be separated from the unmask that answers it, and the
context stride change touches every context walk, so splitting it further would
produce commits that do not build.
Impact
USBHOST_XHCIusers. Required for any controller that is not QEMU's. Noconfiguration change;
dmacapableis optional, so a platform that does notsupply it is unaffected.
Testing
QEMU with
-device qemu-xhci, built forqemu-intel64:nshwith the drivercompiled and linked:
usb-storagedevice enumerates as/dev/sda, mounts vfat and reads backqemu-xhci-regression-okusb-hub, ausb-storageand ausb-kbdboth enumerate, giving/dev/sdaand/dev/kbda, with the file read back and no assertionsEIC7700 EVB, Synopsys DWC3 in host mode, with a real 6-port hub:
/dev/sdamounts as a 59 GB vfat volume anddd if=/dev/sda bs=512 count=256reads at 703 KB/s.
Throughput on that board, doorbell to interrupt 986-1021us before and 13-56us
after the moderation fix:
One note for anyone reproducing the keyboard tests:
CONFIG_HIDKBD_STACKSIZEdefaults to 1024, which is not enough for the polling thread on a 64-bit
target. It overflows and presents as an assertion at the top of
kbdpoll,which reports the damage rather than the cause. 3072 is enough; the EIC7700
EVB configuration already uses that. This is unrelated to the changes here.