drivers/usbhost: Make the xHCI driver work, and support hubs. - #19745
Open
Fishwaldo wants to merge 15 commits into
Open
drivers/usbhost: Make the xHCI driver work, and support hubs.#19745Fishwaldo wants to merge 15 commits into
Fishwaldo wants to merge 15 commits into
Conversation
Four faults that between them kept this driver from reaching a device on any controller that enforces the specification rather than tolerating the driver's assumptions. The width of a register access is part of the register interface. xHCI asks for aligned accesses of the register's own size, and a controller may answer anything narrower with nothing at all; QEMU's does. A volatile load does not pin the width: when only one bit of the value is used, GCC 16.1.0 at -Os narrows "load 32 bits, test bit zero" into a one byte testb, the read comes back zero, and a poll of USBSTS for the halted bit never sees it. The binary happened to work only because a diagnostic consumed all 32 bits of the polled value and pinned that one load full width. Proven through QEMU's gdbstub against an unmodified binary: with the diagnostics compiled out the halt poll times out while the controller's true state, read 32 bits at a time from outside, is USBSTS=0x9, halted throughout; a 4-byte read of the register returns 0x9 and a 1-byte read of the same address is refused. Every accessor now forces the value through a register with an empty asm, loads and stores both, so the access can only be the full-width one the source wrote. A controller is entitled to want no scratchpad buffers, and QEMU's reports zero. The driver worked that count into a size and asked the allocator for it, and a zero-byte allocation returns NULL, indistinguishable from being out of memory, so a controller asking for no scratch space was refused for lack of it before it was ever started. Skip the allocation when none is wanted and leave the first device context base address array entry zero, which is what it means. Halting waited for something that had already happened. The driver wrote the whole of USBCMD zero and waited a second for the halted bit, but a controller that was never started is already halted and says so, so there is no transition to wait for. Writing the whole register zero also cleared the interrupt and host system error enables along with Run/Stop. Look first, stop it only if it is running, and clear the one bit that was meant. Resetting a port disabled it. Eight bits of PORTSC are write-one-to-clear, so writing back what was just read acts on every one that happened to be set: setting Port Reset that way also cleared Port Enabled and discarded every change the port was reporting. Mask them out first, and name the set in the header for the next read-modify-write of this register. The wait afterwards judged itself by its own counter rather than by the port, so a port coming up on the last attempt was reported as a timeout, and only when the timing landed that way, which reads as intermittent rather than wrong. Decide on Port Enabled, and say what the register held when it does fail. Finally, a command whose completion the fallback poll did find, after a late or missed interrupt, still returned the timeout, so a command that had demonstrably succeeded was reported unanswered and its caller unwound work the controller had done. The result is what the completion event said. This does not make a missing interrupt harmless, but it stops the driver contradicting the evidence in front of it. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The controller driver and the PCI bus it happened to be found on were one file. Nothing in the driver is PCI-specific beyond finding the registers and the interrupt, so an SoC that wires an xHCI controller directly could not use any of it. Move the driver to usbhost_xhci.c and leave usbhost_xhci_pci.c as the PCI attachment: the ID table, the BAR mapping and the MSI-X vector. What passes between them is in include/nuttx/usb/xhci.h: a bus supplies the register base, a way to attach the interrupt, and a name to report the controller by, since a system may have more than one and "port 1" alone does not say which. The interrupt belongs to the bus entirely: the bus attaches it, the bus detaches it, and the controller driver never holds an interrupt number, so there is no number for the two sides to disagree about. USBHOST_XHCI is the driver and is not selectable on its own; USBHOST_XHCI_PCI selects it. Another bus adds its own symbol beside it. No functional change intended. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
This driver has only ever run behind QEMU's message signalled interrupt,
and it holds assumptions that are safe only there, and some not even
there.
It never silences the source. The handler reads the status, queues the
work that will answer it, and returns with everything still asserted.
On a level triggered line the interrupt controller sees the condition
still true and raises it again at once, forever, and the work that would
have cleared it never runs. Mask the interrupter on the way out and let
the worker unmask when it is done.
It acknowledges after walking the event ring rather than before. An
event arriving during the walk sets the pending bit again, and clearing
the bit afterwards discards it. That event is the last this controller
will raise until something else happens, so whatever was waiting on it
waits forever: transfers have no timeout, only commands do, and a lost
transfer event is therefore a hang rather than an error. Acknowledge
first; a spurious second pass over an empty ring costs nothing.
It attaches the interrupt before there is anything to answer it with.
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 that a boot loader left running has an interrupt pending
the instant the line is enabled. Attach after the start instead.
Attaching late then loses the first interrupt behind a message, because
a message is sent once, on the pending flag's transition from clear to
set, and a flag raised while nobody was attached has already spent it.
A wire is still asserted when the handler finally arrives, so it costs a
wire nothing. Clear the status and pending flags once the handler is in
place, so the next event is a fresh transition.
The same rule governs the worker's unmask. Events that arrived while
the interrupter was masked have left the pending flag set, and enabling
with it still set gives a message nothing to transition on. Clear it in
the same write, then drain the ring again: clearing can discard an event
that arrived a moment earlier, and repeating until a drain comes back
empty is the only state in which none was lost.
And while reading ports, do not disable them. xhci_probe_ports() writes
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 once the controller settles, is switched
off by the act of looking at it. The port status worker already gets
this right and says so in a comment; this path did not.
Once the interrupts arrive at all, they arrive too late. The
interrupter moderation interval is how long a controller waits after an
event before reporting it, in 250ns units, and it resets to 4000, a full
millisecond. The driver never wrote the register. A transfer therefore
cost a millisecond before its completion was even reported, and mass
storage spends three transfers on each request, so a request waited
three milliseconds no matter how little it asked for. Measured on a
Synopsys DWC3 with a USB 2.0 drive, timestamping from the doorbell to
the interrupt: 986-1021us before, 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
Set it to 160, which is 40us, rather than to zero. Zero puts no bound
on how often a controller may interrupt: measured with a keyboard on an
interrupt endpoint, it took interrupts continuously and spent an entire
processor doing it. It is the same value Linux uses, for the same
reason.
Found on two controllers: the level triggered half on a Synopsys DWC3
whose PLIC line re-fired forever, the message signalled half on QEMU,
where enumeration stopped dead after the port reset with no error and no
further interrupts. With both, the same driver serves both.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Justin Hammond <justin@dynam.ac>
The controller moves every byte itself; there is no programmed-I/O path in xHCI to fall back to. On a machine whose caches are not coherent with it, and on one whose addresses are not flat, that makes cache maintenance and address validation part of getting a transfer right rather than an optimisation. This driver was doing neither, beyond its own descriptors. Its rings were published with up_flush_dcache_all() before the controller was pointed at them. That reads as thorough and is the opposite: an architecture whose cache can only be maintained by address implements the whole-cache variants as a barrier and nothing more, so the event ring segment table, the device context base address array and the scratchpad pointers were never written out at all. The controller then reads whatever those addresses held before, which presents as every command timing out with no events ever arriving, a failure that looks like the interrupt is missing rather than like the ring is unreadable. Flush each structure by name instead. xhci_ring_init() had the same shape of bug from the other direction: it clears the whole ring and then flushes one descriptor, the link entry it writes afterwards, leaving the rest of the clearing in the cache. This is memory the controller writes into itself, so a dirty line written back later lands on top of whatever the controller has put there since. What gets destroyed is an event somebody is waiting for, and the ring then looks permanently empty while the controller believes it has reported everything. Flush the whole ring, which is what the clearing was for. Data buffers got no maintenance whatsoever: nothing pushed before an OUT, nothing dropped after an IN. On a coherent host this cannot be seen, which is presumably why it survived. And the buffers a class driver hands down are not all its own to maintain. Cache operations work a whole line at a time because that is all the hardware offers, so dropping a line that a buffer only partly covers also drops whatever else lives in it, and writing one back over memory the controller has just filled destroys the transfer. Mass storage passes a thirty-one byte command block and a thirteen byte status straight out of its instance structure, sharing lines with everything around them. So a buffer that does not own its lines is copied through one that does, and only the small transfers ever need it: anything large comes from a filesystem or from xhci_ioalloc(), already aligned. While here, make xhci_ioalloc() round its length up as well as aligning its start, so what it returns owns its last line too. Also drop the device output context before reading the address out of it. The controller chose that address and wrote it there; reading without invalidating returns whatever the processor had cached, and the driver then addresses the device by a number it was never assigned. Separately, two ways a transfer could be programmed that the controller will not honour. A buffer that cannot be reached. The driver turns a caller's address into a physical one and hands it to the controller, and on a system with an address environment that translation is only meaningful for some addresses. A userspace buffer under CONFIG_BUILD_KERNEL is neither mapped address-for-address nor physically contiguous, so what the controller gets is a number that names the wrong memory. The transfer then completes successfully, having read or written somewhere else entirely. That is the worst shape a fault can take, and it is indistinguishable from a cache problem when the data comes back wrong. Whether an address can be used this way is a property of the system the controller was fitted into, not of the controller, so it is asked rather than assumed: a platform may supply dmacapable, and one that does not is taken to mean every address works, which is what every existing user has. A buffer that is refused gives -EFAULT, which is a redirection rather than a failure: the FAT filesystem answers it by retrying through its own DMA-safe sector buffer, and the read succeeds by DMA either way. And transfers longer than one descriptor can describe. A Normal TRB carries one run of memory that may not cross a 64K boundary, while the block layer above hands down whole multi-sector reads whose length is bounded by nothing here. A single TRB was being programmed regardless, so a long enough read, or merely one starting near the wrong side of a boundary, produced a descriptor the controller is entitled to reject or to satisfy partly. Chain as many as the run needs instead, asking for the completion interrupt only on the last so that one event still arrives for the whole transfer. The cache half compiles to nothing on an architecture with 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>
Everything a controller is told about a device before it will accept it, found by bringing this driver up on a Synopsys DWC3 core, which checks what QEMU's controller does not. Contexts came in one size only. A controller says in HCCPARAMS1 whether its context structures are thirty-two or sixty-four bytes, and this driver refused the wider form outright with -EIO, so it worked on exactly the controllers reporting the narrower one. QEMU's is one; DWC3 cores are not, and the EIC7700X reports HCCPARAMS1 = 0x0220fe45 on both of its controllers, so this driver could not have driven either. The difference is smaller than the refusal suggests: a wide context is the same fields with reserved space after them, so nothing about the layout changes, only the distance from one entry to the next. Read the size at start up and let the three places that walk a context array use it. Contexts also have to be allocated aligned; every entry of the device context base address array points at one and must be 64 byte aligned, and the output context was coming from kmm_zalloc(), which promises nothing of the sort. The event ring segment table came out sized zero. How many segments a controller allows is a power of two reported as its exponent, and the exponent can be 15, so computing 1 << exponent into the uint8_t that holds it wraps to zero on any controller offering more than 128. A controller told its event ring table holds no entries has nowhere to report anything: every command times out, and the first thing to notice is a host controller error with no explanation. Work it out at full width and narrow it afterwards. The slot context never carried the device speed. It is the only place a controller is told how fast the device it is about to address runs, and the field has no meaningful zero, so the context described nothing and a controller that validates it answers Address Device with a parameter error rather than guessing. The speed was known: the endpoint context built next to it had the right maximum packet size all along. The numbering is xHCI's own and unrelated to the values the USB host stack uses, hence the mapping. The output device context was cleared and never flushed. That context is the controller's to write, which is exactly why clearing it has to reach memory: what stays behind is a dirty line of zeros that the processor writes back whenever it next needs the line, on top of whatever the controller has put there since. The slot state lives in that context, so what is destroyed is the record of the device having been addressed at all, and the next command against the slot is refused with a context state error. The symptom is enumeration reaching SET_ADDRESS and stopping. A buffer copied through an aligned stand-in was copied back using the wrong length. buflen means the length of a data transfer and control transfers deliberately leave it zero, so a descriptor read copied nothing back and the caller was handed whatever its buffer held before. That reads as a device returning nonsense, and is followed by the endpoint being configured with a garbage maximum packet size. Keep the requested length separately. Cache maintenance on such a buffer is also rounded to the whole of it rather than the part in use, since the operation works a line at a time and an architecture may reasonably refuse a range stopping part way through one. A buffer the controller cannot reach is now copied rather than refused. Answering -EFAULT works for a caller with somewhere better to put the data, which the FAT filesystem has, and fails outright for one without: reading a block device directly from a user program returned an error where the transfer could simply have gone through a stand-in. Two diagnostics while here. 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. And a rejected command now says which command it was: the difference between a refused Address Device and a refused Evaluate Context is most of the diagnosis, and the completion code alone does not give it. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
Four things, all found by driving a USB drive and a keyboard on a DWC3
controller, and all invisible on a coherent host with a flat address
space.
A link TRB inside a transfer was not chained. A transfer described by
more than one TRB can reach the end of the ring part way through, and
the link that sends the controller back to the beginning is then 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
descriptor that asked for the completion interrupt is on the far side of
the join and is never reached. Nothing waiting is woken, and transfers
have no timeout, so the symptom is a read that never returns. It only
appears once transfers need more than one descriptor.
The effect is not subtle. Reading a megabyte from a USB drive:
512 byte blocks 166 KB/s
4 KiB blocks 1333 KB/s
32 KiB blocks 10666 KB/s
64 KiB blocks 15515 KB/s
Before the fix the last two did not complete at all. The same board
reads its SD card at 16000 KB/s and its eMMC at 42666 KB/s, so a USB 2.0
drive now sits where it ought to between them.
A stand-in buffer was copied back in the wrong context. That work was
being 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 anyway, so
the copy belongs there instead.
An asynchronous transfer cannot use a stand-in at all, for the same
reason: there is no caller to come back to, and the copy would have to
happen in the completion. Refuse a buffer that would need one. The
callers of that path are class drivers using kernel memory, which do
not.
And say what is attached. A controller now reports each device once,
by name, as it comes up, and from the end of the port enable rather than
at connect, because the speed field in PORTSC only means anything
once the port has been reset: before that a USB2 port reports its reset
default, which reads as full speed, and every device would be announced
at 12Mbps regardless of what it negotiates a moment later. Verified
with both at once: a high speed drive on one controller announces
480Mbps while a low speed keyboard on the other announces 1.5Mbps.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Justin Hammond <justin@dynam.ac>
Two faults a low speed keyboard meets in succession, the second only reachable because of the first. The Interval field of an endpoint context is an exponent: the controller services the endpoint every 2^Interval microframes. An endpoint descriptor does not say it that way, and what it does say depends on how fast the device is, so the number cannot be copied across, which is what this did. A low or full speed interrupt endpoint counts in frames, so a keyboard asking to be polled every 10ms was programmed as 2^10 microframes, an interval it never agreed to and one the controller would not accept: the Configure Endpoint command went unanswered, endpoint allocation failed with -EIO, and the keyboard never enumerated. Convert instead. Low and full speed interrupt endpoints state a period in frames, so the exponent is the highest bit of that period in microframes, kept inside the range the specification allows. Everything else periodic already states an exponent, one greater than the one wanted here. Control and bulk endpoints are not periodic and the field means nothing to them. A root hub port whose enumeration failed that way is then enumerated again, and the slot the failed attempt was using has been given back before that happens, so the port has no device context behind it any more. xhci_epalloc() took that pointer and wrote the new endpoint through it without looking. Storing through NULL costs the whole system, and it does it in answer to a device that merely failed to come up: the first attempt reports the error correctly and the retry then panics the kernel. Check for the device, and give back the endpoint that has no home rather than leaking it. With both, a low speed keyboard configures its interrupt endpoint and enumerates, where before it failed every time and took the system down on the retry. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
github-actions
Bot
added
Size: XL
|
An asynchronous transfer never completed. Submitting one refused any buffer that would need a cache line stand-in, and the test for that also refuses every buffer whose length is not a whole number of cache lines, which an interrupt transfer's essentially never is: a HID keyboard reads eight bytes at a time. Every submission came straight back with -EFAULT before a descriptor was written. The refusal is silent in a normal build, because the only report of it is error logging that is usually compiled out, and a class driver in its interrupt-driven mode resubmits from the completion callback, so the first refusal is also the last word. A keyboard enumerated, registered its devices, and never produced a byte. The refusal was there because the completion side had nowhere to bring the data back: the copy out of a stand-in is done by the blocked caller, and an asynchronous transfer has no blocked caller. But the work queue thread that handles the completion is a fine place for it. A buffer given to DRVR_ASYNCH must come from DRVR_ALLOC, which is kernel memory reachable from any thread, so the addresses mean the same thing there as in the submitting context. Let the submission use the same stand-in machinery as every other transfer, and finish the DMA in the completion, just before the callback: invalidate, copy back, give up the stand-in. A transfer that is cancelled instead gives its stand-in back on cancellation. The callback also moves outside the spinlock. It is class driver code: it queues work and takes locks of its own, and the completion must now also be free to return a stand-in to the heap, none of which has any business happening with interrupts masked. Whether a completion is synchronous or asynchronous is still decided under the lock, because the moment a synchronous waiter is posted the endpoint may be carrying a new transfer, and that one is not complete. The byte count went the same way. The completion callback is handed the number of bytes transferred, worked out from the residue in the transfer event and the length that was asked for, and only the synchronous setup recorded that length; the asynchronous one left whatever was there from before, which is nothing on an endpoint that has only ever carried asynchronous transfers. The HID keyboard class gets away with a zero, because it treats any non-negative count as a report worth parsing and reads the buffer regardless, but the count is part of what DRVR_ASYNCH promises. Record the length in the asynchronous setup exactly as the synchronous setup does. Verified on QEMU with a keyboard and a drive on the same bus: keys injected from the monitor arrive through /dev/kbda while the drive mounts and reads back its file. 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 calling 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 therefore both reach xhci_ioc_setup(), and the second one trips the DEBUGASSERT(!epinfo->iocwait) that guards it. Where the assertion is compiled out the second thread overwrites the first thread's completion state instead. A device's 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 that both issue GET_REPORT there. Every other host controller driver in the tree holds its controller lock across the wait. Doing that here would serialise the whole controller and give up the per endpoint rings that xHCI provides, so add a mutex to struct xhci_epinfo_s instead and hold it across the wait. It is taken before the controller lock on both paths, so the order is always endpoint then controller and never the reverse. xhci_epfree() also freed the endpoint container without destroying iocsem. Destroy both objects there. The fault predates the preceding commits and is reachable on any xHCI controller. It was found on an EIC7700X board, where a two interface USB keyboard tripped the assertion on every boot and does not with this change applied. 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 a slot and returned without giving it back. xhci_device_init() enables a slot before it initialises the transfer ring, the slot context and the device address. Each of those three can fail, and each returned directly. The same function also treated a slot number larger than the controller supports as success, because Enable Slot itself had succeeded, and returned a slot the driver cannot address. The larger leak is in xhci_enumerate(). The device is addressed by the time usbhost_enumerate() runs, so a failure there, a device whose descriptor cannot be read or one no class driver claims, leaves the slot held. That path then clears hport->connected so the port is retried, which asks for another slot, and the retry never stops on a device that cannot be enumerated. Release the slot on both paths with xhci_device_deinit(), which already issues Disable Slot, clears the DCBAA entry and resets the context. The endpoint ring is deliberately left allocated: xhci_ring_init() reuses an existing ring and only allocates when there is none. Tested on an EIC7700X board with a USB hub, which no class driver claims because this driver does not yet support hubs, so the port retries indefinitely. Before, the eighth attempt failed with Enable Slot completion code 9 (No Slots Available) and the controller enumerated nothing further, including on its other port. After, 1104 consecutive attempts produced no slot failure and a keyboard on the second port enumerated normally throughout. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
xhci_enumerate() reports a failure by marking the hub port disconnected, which is what makes xhci_wait() return and the attempt repeat. The root port itself is still connected, so the two disagree again immediately and the attempt repeats for as long as the device stays plugged in. Nothing bounded that. A device that fails every time, one whose descriptors cannot be read or that no class driver claims, is retried forever. Measured on an EIC7700X board with a USB hub, which no class driver claims because this driver does not support hubs yet: 1055 attempts in 90 seconds, enough console traffic to stop the board being usable at all. 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 that needs a second attempt still gets one. The default of three rides out a slow device or a marginal reset without spinning. The same board with the same hub now makes three attempts, reports that it has given up, and falls silent; a keyboard on the other port enumerates throughout and the shell responds in 1.5 s. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
A root hub port and a device were the same thing in this driver. The slot, the default control endpoint and the device context all lived in struct xhci_rhport_s, and anything needing a device reached it as rhport->dev. That holds only while every device is plugged straight into the controller. A hub puts several behind one root port, each with its own slot and context, so the port cannot go on being the identity. Two identities replace it, because there are two questions. An endpoint records the slot it was opened on, so xhci_dev_from_ep() answers "which device does this transfer belong to". A hub port belongs to one device wherever it sits, so xhci_dev_from_hport() answers "which device is on this port" for the case with no endpoint to ask yet: the first one, whose allocation is what needs the device in the first place. The functions converted here were using both keys at once. xhci_ep0configure() issued Evaluate Context for epinfo->slot while filling in the context belonging to rhport->dev, and xhci_ctrl_xfer() reached the endpoint ring as rhport->dev->rhport->ep0.td, a round trip through the port back to the endpoint the caller had supplied. xhci_slot_init() read the device's speed and control ring through the port as well, which is the one that would have failed quietly: the slot context speed field has no valid zero, and a low speed device behind a high speed hub does not have its hub's speed. No functional change for a directly attached device: its port's slot and its endpoint's slot are the same one, and its hub port is the root port's own. Tested on an EIC7700X board, where a two interface USB keyboard enumerates through the full Address Device path and both of its interfaces still work. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
Some host controllers have to be told about the hubs in a topology, not only about the device at the end of it. xHCI is one: a hub's slot context carries a hub flag, its downstream port count and the think time of its transaction translator, and the controller uses them to route to anything behind that hub. The hub class driver already reads all of this from the hub descriptor and keeps it privately. Publish the two values a controller can act on, on the hub's own hub port, beside the speed and function address that already describe the device attached there rather than the port itself. A driver setting up a device behind a hub finds them on that device's parent. They are written before the hub activates any downstream port, so they are in place before there is anything behind the hub to set up, and a port with no hub attached reports zero ports because the hub class clears each child before use. Nothing is required to read them, so a controller that does not need them is unaffected. Fields rather than a new driver method: a method would need a null check at the call site and would define an order in which it must be called, and neither can be got wrong here. Both are inside CONFIG_USBHOST_HUB, as struct usbhost_hubport_s's parent pointer already is, so a build without hub support is unchanged. Multi-TT is not included. It comes from the hub's interface protocol rather than the hub descriptor, and treating a multi-TT hub as single-TT costs bandwidth behind that hub but is correct. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
A controller reaches a device by the path to it and, for a slow device, through the hub that translates for it. Neither was described, so a device behind a hub was addressed as though it were on the root port. The route string is that path: each hub between the device and the root contributes a nibble holding the port the next thing down is plugged into, tier nearest the root in the lowest nibble. Walking up from the device reaches the deepest tier first, so shifting what is already there left by a nibble each time leaves them in the order the field wants. The walk stops after five, which is both what the field holds and what USB allows, and a port above fifteen is clamped rather than carrying into the tier below it. Slot context dword 2 names the transaction translator that carries a low or full speed device behind a high speed hub. It reports the hub by slot, where EHCI reports it by USB address, and it names the nearest high speed ancestor rather than the immediate parent: a full speed hub below a high speed one is itself carried by the translator above it, so the device's own hub is not always the one doing the work. The think time comes from the hub descriptor by way of the hub class driver, and both count in the same units, so it carries across unchanged. What was there instead came from EHCI. xhci_epalloc() carried a copy of sam_ehci.c's block, writing epinfo->hubaddr and epinfo->hubport, which are how EHCI describes a split transaction in its queue head. This driver never read either field, so the work was thrown away, and the place xHCI wants it is the slot context rather than the endpoint. Both fields and the code setting them are gone. Multi-TT is not set. It comes from the hub's interface protocol rather than its descriptor, and driving a multi-TT hub as single-TT costs bandwidth behind that hub but is correct. No functional change: hubs cannot be enabled yet, and a device on a root port has neither hubs above it nor a translator, so both fields are zero as before. Tested on an EIC7700X board with a directly attached low speed keyboard, the case that would use a translator if a hub were in the way. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
The driver refused CONFIG_USBHOST_HUB outright. Everything needed to describe a device behind a hub is now in place, so implement the rest and let the configuration build. A device is created wherever it sits. xhci_device_init() took a root hub port and read the slot, the control endpoint and the device out of it, all of which belong to the device. It now takes the hub port and the control endpoint too, and records the device on the root port only when that is where it is: the device a root port names, once a hub is plugged in, is the hub. xhci_address_set() and xhci_device_deinit() likewise work on a device, and xhci_disconnect() finds the device by the port going away rather than assuming the root port's. The hub asks for a port's control endpoint before it reports the connection, so xhci_epalloc() has nothing to attach one to. It returns an endpoint with no slot in that case, and xhci_connect() gives it one when it creates the device, which is the hub's next action. A hub also has to be described to the controller as a hub before anything behind it can be reached, and nothing knows it is one when its slot is created: it is addressed and configured like any other device, and only then does its class driver read the descriptor saying how many ports it has. xhci_hub_update() corrects the slot context with a Configure Endpoint command the first time something appears behind it. A hub reports each port whose state changed one after another without waiting for any to be dealt with, so the connect method queues them. Holding one pointer, as it first did, meant the second report overwrote the first and the device on it was never enumerated, silently. No more can be outstanding than the controller has slots to put devices in. Report the geometry the hardware describes rather than leaving it to a debug build: the root port and slot counts from HCSPARAMS1, and the port count a hub gives in its descriptor. Tested on an EIC7700X board with a hub on one controller and a keyboard on the other. Behind the hub, a 59 GB mass storage device mounts and reads a file back correctly, a composite CDC device gives four ttyACM nodes, and a Realtek adapter with no driver in this tree is enumerated and reported as unclaimed. Both interfaces of the keyboard keep working throughout. Assisted-by: Claude:claude-opus-5 Signed-off-by: Justin Hammond <justin@dynam.ac>
Fishwaldo
force-pushed
the
upstream-usbhost-xhci
branch
from
August 8, 2026 09:30
b3903d2 to
5a140b6
Compare
This was referenced Aug 8, 2026
acassis
approved these changes
Aug 8, 2026
| /**************************************************************************** | ||
| * Included Files | ||
| ****************************************************************************/ | ||
|
|
||
| #include <nuttx/config.h> | ||
|
|
||
| #include <assert.h> | ||
| #include <nuttx/debug.h> | ||
| #include <debug.h> |
|
|
||
| __asm__ __volatile__("" : "+r"(value)); | ||
| *((FAR volatile uint64_t *)addr) = value; | ||
| pci_release_irq(pcix->dev, &pcix->irq, 1); |
Contributor
There was a problem hiding this comment.
do you need up_disable_irq
| interval = 1; | ||
| } | ||
|
|
||
| for (exp = 0; (1u << (exp + 1)) <= (unsigned int)interval * 8; exp++); |
| exp = 10; | ||
| } | ||
|
|
||
| return (uint8_t)exp; |
Contributor
There was a problem hiding this comment.
remove the cast too
|
|
||
| uinfo("no slots = %d, no ports = %d\n", | ||
| priv->no_slots, priv->no_ports); | ||
| syslog(LOG_INFO, "%s: %d root port%s, %d device slot%s\n", |
|
|
||
| dev->ishub = true; | ||
|
|
||
| syslog(LOG_INFO, "%s: port %d: hub with %d port%s\n", |
Contributor
There was a problem hiding this comment.
why use syslog, not uxxx
Member
|
@acassis did you even look through this massive PR or did you blindly approved? this PR should be tested on real intel64 HW before merge |
raiden00pl
requested changes
Aug 9, 2026
raiden00pl
left a comment
raiden00pl
left a comment
Member
There was a problem hiding this comment.
319aadf touches too many things. Please break this down into smaller parts because right now it's unclear what fixes are for gcc16 and what fixes qemu. Your description is misleading because this driver works with gcc version lower than 16. It looks like AI couldn't find a real problem and fixed everything one by one and put it in one commit. The real issue here is gcc16 and memory access.
Was this PR completely generated by AI or did you verify its output? Some of the claims in it seem like BS, like this:
The driver worked that count into a size and asked the
allocator for it, and a zero-byte allocation returns NULL,
indistinguishable from being out of memory, so a controller asking for
no scratch space was refused for lack of it before it was ever started.
raiden00pl
reviewed
Aug 9, 2026
| @@ -0,0 +1,6041 @@ | |||
| /**************************************************************************** | |||
Member
There was a problem hiding this comment.
this way you've lost all git history of original file. You should use git mv and then add your changes on top of it
Contributor
@raiden00pl I look the diff code and for some files that github didn't accepted to show the diff I saw the final file, nothing too suspect here. He also provided the QEMU test results. Are you planing to do real test on real HW? |
linguini1
requested changes
Aug 9, 2026
linguini1
left a comment
linguini1
left a comment
Contributor
There was a problem hiding this comment.
Please split this patch into multiple PRs, this is a massive code change for just one.
Member
|
@acassis I'll test it on intel HW probably tomorrow |
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
5 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 xHCI driver does not work on a conforming controller. On the one in-tree configuration that has one,
qemu-intel64:jumbowithqemu-xhci, the controller fails to come up at all:No root ports, no devices, nothing on the bus. This series makes it work, separates the driver from the PCI bus so that any hardware carrying an xHCI controller can use it rather than only PCI-attached ones, and adds hub support.
Fifteen commits, in four groups.
Making it work at all (1-8): register access widths that a conforming controller ignores when narrowed; an interrupt path that assumed a level-triggered wire and so never re-armed behind a message-signalled one; cache maintenance around DMA and transfer-length limits; device and endpoint context fields the specification requires and the driver left zero; TRB chaining across the ring wrap; endpoint allocation for interrupt endpoints; and asynchronous transfers that set up correctly but delivered no data.
Commit 2 is the separation from PCI. The controller is the same part wherever it is fitted, but the driver was written as a PCI device, so an SoC that wires one directly could not use it. The bus-specific work moves behind a small
struct xhci_bus_ops_s(probe, interrupt attach, DMA address translation), leaving the controller logic bus-agnostic. It is a move rather than a rewrite, which is why it is kept as its own commit and why the diff is large.Resource handling (9-11): a per-endpoint lock, because the controller lock is released across a transfer and two threads on one endpoint corrupted each other's completion state; releasing the device slot when enumeration fails, which otherwise leaks one per attempt until the controller has none left; and bounding the retries, since a device that cannot enumerate is otherwise retried for as long as it stays plugged in.
Preparing for hubs (12-14): stop using the root port as a device's identity, describe a device to the controller from the device rather than from the port, and fill in the route string and transaction translator that a device behind a hub needs.
Hub support (15):
CONFIG_USBHOST_HUBwas refused outright by an#error. It now works.Impact
qemu-intel64:jumbois the one in-tree configuration that selects xHCI (CONFIG_USBHOST_XHCI_PCI=y, with MSC, HIDKBD, HIDMOUSE and COMPOSITE). Its behaviour changes: the controller initialises where it previously did not, and USB devices enumerate where previously none did. That is the purpose of the series, but it is a behaviour change and not merely an addition. Both that configuration and the EIC7700X port were used to test it, and no other in-tree configuration selects xHCI.CONFIG_USBHOST_XHCI_ENUM_RETRIES, default 3, bounding enumeration attempts per port.xhci_initialize()takes a bus number so an SoC driver can identify its controller. The PCI caller passes 0.Testing
Host: macOS 15.5 (Apple Silicon). qemu: 10.1.5 with KVM on Fedora 43 x86_64. Board: ESWIN EIC7700X (RISC-V, 4 cores SMP), whose port will be upstreamed later.
qemu - reproducible in tree
Before:
pci_xhci_probe: failed to initialize HW!, and/devholds nosda.After: the controller initialises and the drive enumerates:
With a hub in the topology, three devices enumerate together and the data path works through it:
Hardware - the non-PCI path
The EIC7700X attaches its xHCI controllers directly, not over PCI, so it exercises the separation in commit 2. Both controllers drive real devices concurrently: a low-speed keyboard on one, and on the other a hub carrying a 59 GB mass storage device (mounted, directory listed, file read back correctly), a composite CDC device presenting four
ttyACMnodes, and a Realtek Ethernet adapter with no driver in this tree, which is enumerated and reported as unclaimed rather than wedging the bus.Every commit builds and links individually against
qemu-intel64:jumbo.