From 08e971f181f90a7c5c3cc0f5eaccf598bd49af0e Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:09:58 +0800 Subject: [PATCH 01/21] drivers/usbhost: Attach the xHCI interrupt after the controller starts. 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 --- drivers/usbhost/usbhost_xhci.c | 37 ++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index ff628cf31fd10..efea0fb89ddcc 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -4545,14 +4545,6 @@ static int xhci_hw_initialize(FAR struct usbhost_xhci_s *priv) goto errout; } - /* Configure interrupts */ - - ret = xhci_irq_initialize(priv); - if (ret < 0) - { - goto errout; - } - /* Halt controller */ ret = xhci_ctrl_halt(priv); @@ -4697,6 +4689,7 @@ xhci_initialize(FAR const char *name, uintptr_t base, { FAR struct usbhost_conn_xhci_s *conn = NULL; FAR struct usbhost_xhci_s *priv = NULL; + uint32_t regval; int ret; DEBUGASSERT(name != NULL && base != 0 && ops != NULL && @@ -4756,6 +4749,34 @@ xhci_initialize(FAR const char *name, uintptr_t base, goto errout; } + /* Take the interrupt only now. + * + * The handler defers to a worker that walks the event ring, and the ring + * does not exist until the controller has been started. A controller + * left running by a boot loader can have an interrupt pending the moment + * the line is enabled, so attaching any earlier is a race with nothing + * to answer it. + */ + + ret = xhci_irq_initialize(priv); + if (ret < 0) + { + uerr("failed to attach interrupt: %d\n", ret); + goto errout; + } + + /* Acknowledge anything the controller raised before the handler was + * attached. A message is sent once, on the transition, so a bit set in + * that window would never produce another. Clear them, so the next + * event is a fresh assertion. + */ + + regval = xhci_oper_getreg(priv, XHCI_USBSTS); + xhci_oper_putreg(priv, XHCI_USBSTS, regval); + + regval = xhci_runt_getreg(priv, XHCI_IMAN(0)); + xhci_runt_putreg(priv, XHCI_IMAN(0), regval | XHCI_IMAN_IP); + #ifdef CONFIG_DEBUG_USB_INFO xhci_dump_mem(priv, "after init"); #endif From ca6502a108ba8cb07a8e6827f5d4a6254f7e6668 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:09:58 +0800 Subject: [PATCH 02/21] drivers/usbhost: Do not disable an xHCI port while probing it. 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 --- drivers/usbhost/usbhost_xhci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index efea0fb89ddcc..50ee8f0f76077 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -1291,8 +1291,13 @@ static void xhci_probe_ports(FAR struct usbhost_xhci_s *priv) portsc = xhci_oper_getreg(priv, XHCI_PORTSC(i)); priv->rhport[i].connected = ((portsc & XHCI_PORTSC_CCS) != 0); - /* Clear status change */ + /* Clear status change, but not PED. Port Enabled/Disabled is + * write-one-to-clear, so writing back what was read disables any + * port that came up enabled, which is what a device attached at + * power up does. + */ + portsc &= ~XHCI_PORTSC_PED; xhci_oper_putreg(priv, XHCI_PORTSC(i), portsc); } } From 7c27394136e43ec70a30f21785161ba79eeb8d3d Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:10:27 +0800 Subject: [PATCH 03/21] drivers/usbhost: Acknowledge xHCI events before walking the ring. 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 --- drivers/usbhost/usbhost_xhci.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 50ee8f0f76077..423090e840dcf 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -2907,6 +2907,20 @@ static void xhci_interrupt_work(FAR void *arg) FAR struct usbhost_xhci_s *priv = arg; uint32_t iman; + /* Acknowledge before walking the ring, not after. An event arriving + * during the walk sets the pending bit again, and clearing after the + * walk discards it. Transfers have no timeout, so the one it belonged + * to would wait forever. + */ + + xhci_oper_putreg(priv, XHCI_USBSTS, priv->pending); + + iman = xhci_runt_getreg(priv, XHCI_IMAN(0)); + if (iman & XHCI_IMAN_IP) + { + xhci_runt_putreg(priv, XHCI_IMAN(0), iman); + } + xhci_events_poll(priv); /* Port Change Detect */ @@ -2939,18 +2953,6 @@ static void xhci_interrupt_work(FAR void *arg) uinfo("Host Controller Error\n"); } - /* ACK interrupts */ - - xhci_oper_putreg(priv, XHCI_USBSTS, priv->pending); - - /* Clear interrupter pending bit */ - - iman = xhci_runt_getreg(priv, XHCI_IMAN(0)); - if (iman & XHCI_IMAN_IP) - { - xhci_runt_putreg(priv, XHCI_IMAN(0), iman); - } - /* Clear pending bits */ priv->pending = 0; From 1d1d21c0b5970d04d3fd21d5c3e7cc2bfd4aa6cb Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:10:44 +0800 Subject: [PATCH 04/21] drivers/usbhost: Silence the xHCI interrupter until its worker has run. 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 --- drivers/usbhost/usbhost_xhci.c | 39 +++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 423090e840dcf..ed417a92a528f 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -2804,6 +2804,7 @@ static int xhci_events_poll(FAR struct usbhost_xhci_s *priv) uintptr_t addr; uint8_t type; uint32_t d2; + int count = 0; /* Invalidate event ring */ @@ -2875,6 +2876,7 @@ static int xhci_events_poll(FAR struct usbhost_xhci_s *priv) /* Next event */ + count++; priv->evnt.i++; /* Handle ring wrap */ @@ -2891,7 +2893,7 @@ static int xhci_events_poll(FAR struct usbhost_xhci_s *priv) addr |= XHCI_ERDP_EHB; xhci_runt_putreg_8b(priv, XHCI_ERDP(0), addr); - return OK; + return count; } /**************************************************************************** @@ -2956,6 +2958,29 @@ static void xhci_interrupt_work(FAR void *arg) /* Clear pending bits */ priv->pending = 0; + + /* Let interrupts back in, which the handler masked on its way out, and + * clear the pending flag in the same write. + * + * A message signalled interrupt is sent on the flag's clear to set + * transition; a wire stays asserted while it is set. Events that + * arrived while this interrupter was masked have already set the flag, + * so enabling without clearing leaves a message with nothing to + * transition on, and transfers have no timeout. + * + * Clearing opens its own window: an event delivered between the ring + * going empty and this write is discarded. So drain again, and repeat + * if that drain found anything. A drain that finds nothing is the only + * state in which no event can have been lost. + */ + + do + { + iman = xhci_runt_getreg(priv, XHCI_IMAN(0)); + xhci_runt_putreg(priv, XHCI_IMAN(0), + iman | XHCI_IMAN_IE | XHCI_IMAN_IP); + } + while (xhci_events_poll(priv) > 0); } /**************************************************************************** @@ -2969,11 +2994,23 @@ static void xhci_interrupt_work(FAR void *arg) static int xhci_interrupt(int irq, FAR void *context, FAR void *arg) { FAR struct usbhost_xhci_s *priv = arg; + uint32_t iman; /* Get pending interrupts */ priv->pending = xhci_oper_getreg(priv, XHCI_USBSTS); + /* Silence the interrupter before returning. + * + * Nothing here clears the condition that raised the interrupt; the work + * runs later on a work queue. On a level triggered line the source is + * still asserted on return, so the interrupt re-raises immediately and + * the worker never runs. The worker clears the status and unmasks. + */ + + iman = xhci_runt_getreg(priv, XHCI_IMAN(0)); + xhci_runt_putreg(priv, XHCI_IMAN(0), iman & ~XHCI_IMAN_IE); + /* Handle interrupts in worker */ if (work_available(&priv->work)) From e6d1f6c3508c97c8143942b1e8c1aba1013008b8 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:10:58 +0800 Subject: [PATCH 05/21] drivers/usbhost: Set the xHCI interrupter moderation interval. 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 --- drivers/usbhost/usbhost_xhci.c | 4 ++++ drivers/usbhost/usbhost_xhci.h | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index ed417a92a528f..2c3d3ad559ae0 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -1141,6 +1141,10 @@ static int xhci_ctrl_start(FAR struct usbhost_xhci_s *priv) xhci_oper_putreg_8b(priv, XHCI_CRCR, up_addrenv_va_to_pa(priv->cmd.ring) | XHCI_CRCR_RCS); + /* Do not sit on completions; see XHCI_IMOD_INTERVAL */ + + xhci_runt_putreg(priv, XHCI_IMOD(0), XHCI_IMOD_DEFAULT); + /* Enable interrupts */ regval = xhci_runt_getreg(priv, XHCI_IMAN(0)); diff --git a/drivers/usbhost/usbhost_xhci.h b/drivers/usbhost/usbhost_xhci.h index 8f940f8a82c42..2fc2aa207aded 100644 --- a/drivers/usbhost/usbhost_xhci.h +++ b/drivers/usbhost/usbhost_xhci.h @@ -315,6 +315,17 @@ #define XHCI_IMOD_IMODI_SHIFT (0) /* Bits 0-15: Interrupt Moderation Interval */ #define XHCI_IMOD_IMODC_SHIFT (16) /* Bits 16-31: Interrupt Moderation Counter */ +/* What to set the moderation interval to, in 250ns units. + * + * The reset default is 4000, a millisecond, which is far too long to wait + * to be told a transfer finished. Zero is too short: it puts no bound on + * how often a controller may interrupt, and a polled device such as a + * keyboard on an interrupt endpoint will then occupy a processor. 160 is + * 40us, which is what Linux uses. + */ + +#define XHCI_IMOD_DEFAULT (160) + /* Event Ring Segment Table Size */ #define XHCI_ERSTS_MASK (0xffff) /* Bit 0-15: Event Ring Segment Table Size */ From 8c427cba8429fb55c2156c9928911583e5e89ec4 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:12:25 +0800 Subject: [PATCH 06/21] drivers/usbhost: Flush the xHCI rings and structures by address. 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 --- drivers/usbhost/usbhost_xhci.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 2c3d3ad559ae0..7b076098a971c 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -808,9 +808,16 @@ static int xhci_ring_init(FAR struct xhci_ring_s *ring, size_t len) ring->len = len; } - /* Reset data in ring */ + /* Reset data in ring. + * + * Clearing dirties every line, and the controller writes into this + * memory itself. Flush now, or a later writeback lands on top of an + * event somebody is waiting for. + */ memset(ring->ring, 0, ring->len * sizeof(struct xhci_trb_s)); + up_flush_dcache((uintptr_t)ring->ring, + (uintptr_t)(ring->ring + ring->len)); /* Fill Link TRB */ @@ -1112,9 +1119,22 @@ static int xhci_ctrl_start(FAR struct usbhost_xhci_s *priv) evnt->size = XHCI_EVENT_MAX; evnt->res = 0; - /* Flush all memory before write to ERDP so xhci sees correct data */ + /* Push the structures the controller is about to be pointed at. + * + * Flush by address: up_flush_dcache_all() is a no-op on architectures + * whose cache can only be maintained by address. + */ - up_flush_dcache_all(); + up_flush_dcache((uintptr_t)priv->pg_erst, + (uintptr_t)priv->pg_erst + + sizeof(struct xhci_event_ring_s) * priv->no_erst); + up_flush_dcache((uintptr_t)priv->pg_ctx, + (uintptr_t)(priv->pg_ctx + priv->no_slots + 1)); + if (priv->pg_sb != NULL) + { + up_flush_dcache((uintptr_t)priv->pg_sb, + (uintptr_t)(priv->pg_sb + priv->no_scratch)); + } xhci_runt_putreg_8b(priv, XHCI_ERDP(0), up_addrenv_va_to_pa(priv->evnt.ring)); @@ -1151,9 +1171,12 @@ static int xhci_ctrl_start(FAR struct usbhost_xhci_s *priv) regval |= XHCI_IMAN_IE; xhci_runt_putreg(priv, XHCI_IMAN(0), regval); - /* Flush all memory once again */ + /* And the command ring, whose last entry was just made to point back at + * its own beginning. + */ - up_flush_dcache_all(); + up_flush_dcache((uintptr_t)priv->cmd.ring, + (uintptr_t)(priv->cmd.ring + XHCI_CMD_MAX)); /* Turn the host controller ON, enable interrupts and system errors */ From 3827a0cacf75d02118954b2da1fda7655ca05711 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:12:59 +0800 Subject: [PATCH 07/21] drivers/usbhost: Chain xHCI TRBs across a 64K boundary. 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 --- drivers/usbhost/usbhost_xhci.c | 53 ++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 7b076098a971c..0c649ce8fc592 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -2365,15 +2365,56 @@ static int xhci_normal_setup(FAR struct xhci_rhport_s *rhport, FAR struct usbhost_xhci_s *priv = XHCI_PRIV_FROM_RHPORT(rhport); struct xhci_trb_s trb; - /* Prepare TRB */ + size_t left; + size_t chunk; + uintptr_t pa; + int n = 0; + + /* One TRB describes one run of memory, and that run may not cross a 64K + * boundary. A longer transfer, or one starting near the wrong side of a + * boundary, becomes several TRBs chained into a single transfer, with + * the interrupt asked for only on the last so that one completion + * arrives for the whole of it. + */ - trb.d0 = up_addrenv_va_to_pa(buffer); - trb.d1 = XHCI_TRB_D1_IRQ_SET(0) | XHCI_TRB_D1_TXLEN_SET(buflen); - trb.d2 = XHCI_TRB_D2_IOC | XHCI_TRB_D2_TYPE_SET(XHCI_TRB_TYPE_NORMAL); + pa = up_addrenv_va_to_pa(buffer); + left = buflen; - /* Add TRBs to ring */ + while (left > 0) + { + chunk = XHCI_TD_LEN_MAX - (pa & (XHCI_TD_LEN_MAX - 1)); + if (chunk > left) + { + chunk = left; + } - xhci_add_trb(priv, &epinfo->td, &trb, 1); + if (++n >= XHCI_TD_MAX) + { + uerr("transfer of %zu needs more TRBs than the ring holds\n", + buflen); + return -EINVAL; + } + + trb.d0 = pa; + trb.d1 = XHCI_TRB_D1_IRQ_SET(0) | XHCI_TRB_D1_TXLEN_SET(chunk); + trb.d2 = XHCI_TRB_D2_TYPE_SET(XHCI_TRB_TYPE_NORMAL); + + left -= chunk; + pa += chunk; + + /* Chain everything but the last, and interrupt only on the last */ + + if (left > 0) + { + trb.d2 |= XHCI_TRB_D2_CH; + } + else + { + trb.d2 |= XHCI_TRB_D2_IOC; + } + + xhci_add_trb(priv, &epinfo->td, &trb, 1); + } /* Trigger transfer */ From 2c52f90980114271b92bbe851873904ce36b0fe3 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:13:14 +0800 Subject: [PATCH 08/21] drivers/usbhost: Maintain the cache over xHCI data buffers. 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 --- drivers/usbhost/usbhost_xhci.c | 226 ++++++++++++++++++++++++++++++++- include/nuttx/usb/xhci.h | 7 + 2 files changed, 231 insertions(+), 2 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 0c649ce8fc592..738c1563e1887 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -27,6 +27,7 @@ #include #include #include +#include #include @@ -34,6 +35,7 @@ #include #include #include +#include #include #include @@ -142,6 +144,9 @@ struct xhci_epinfo_s int result; /* The result of the transfer */ size_t xfrd; /* On completion, will hold the number of bytes transferred */ size_t buflen; /* Buffer length used for transfer */ + FAR uint8_t *buffer; /* The caller's buffer, for cache maintenance */ + FAR uint8_t *bounce; /* Aligned stand-in for it, or NULL */ + bool dmain; /* Direction this buffer was prepared for */ sem_t iocsem; /* Semaphore used to wait for transfer completion */ #ifdef CONFIG_USBHOST_ASYNCH usbhost_asynch_t callback; /* Transfer complete callback */ @@ -411,6 +416,12 @@ static int xhci_isoc_setup(FAR struct xhci_rhport_s *rhport, #endif static ssize_t xhci_transfer_wait(FAR struct usbhost_xhci_s *priv, FAR struct xhci_epinfo_s *epinfo); +static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, + FAR uint8_t *buffer, size_t buflen); +static FAR uint8_t *xhci_dma_prepare(FAR struct xhci_epinfo_s *epinfo, + FAR uint8_t *buffer, size_t buflen, + bool dirin); +static void xhci_dma_finish(FAR struct xhci_epinfo_s *epinfo); /* Interrupt handling *******************************************************/ @@ -2298,6 +2309,13 @@ static int xhci_control_setup(FAR struct xhci_rhport_s *rhport, if (buffer) { + buffer = xhci_dma_prepare(epinfo, buffer, buflen, + (req->type & USB_REQ_DIR_IN) != 0); + if (buffer == NULL) + { + return -ENOMEM; + } + trb[i].d0 = up_addrenv_va_to_pa(buffer); trb[i].d1 = XHCI_TRB_D1_TXLEN_SET(buflen); trb[i].d2 = XHCI_TRB_D2_TYPE_SET(XHCI_TRB_TYPE_DATA_STAGE); @@ -2370,6 +2388,14 @@ static int xhci_normal_setup(FAR struct xhci_rhport_s *rhport, uintptr_t pa; int n = 0; + /* Make the buffer safe for the controller to reach */ + + buffer = xhci_dma_prepare(epinfo, buffer, buflen, epinfo->dirin != 0); + if (buffer == NULL) + { + return -ENOMEM; + } + /* One TRB describes one run of memory, and that run may not cross a 64K * boundary. A longer transfer, or one starting near the wrong side of a * boundary, becomes several TRBs chained into a single transfer, with @@ -2446,6 +2472,14 @@ static int xhci_isoc_setup(FAR struct xhci_rhport_s *rhport, FAR struct usbhost_xhci_s *priv = XHCI_PRIV_FROM_RHPORT(rhport); struct xhci_trb_s trb; + /* Make the buffer safe for the controller to reach */ + + buffer = xhci_dma_prepare(epinfo, buffer, buflen, epinfo->dirin != 0); + if (buffer == NULL) + { + return -ENOMEM; + } + /* Prepare TRB */ trb.d0 = up_addrenv_va_to_pa(buffer); @@ -2739,6 +2773,152 @@ static void xhci_portsc_work(FAR void *arg) } } +/**************************************************************************** + * Name: xhci_dmacapable + * + * Description: + * Whether the controller may be pointed at this buffer. + * + * The driver has no way to know this on its own. Whether an address can + * be turned into one the device will reach, and whether what lies behind + * it is contiguous, is a property of the system the controller was fitted + * into, so the answer comes from there. A platform that says nothing is + * taken to mean every address works, which is what a flat address space + * gives. + * + ****************************************************************************/ + +static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, + FAR uint8_t *buffer, size_t buflen) +{ + if (priv->ops->dmacapable == NULL) + { + return true; + } + + return priv->ops->dmacapable(priv->arg, buffer, buflen); +} + +/**************************************************************************** + * Name: xhci_dma_prepare + * + * Description: + * Make a caller's buffer safe for the controller to reach, and say which + * address to hand it. + * + * The controller writes memory behind the processor's back, so on a + * machine whose caches are not coherent every buffer it touches must be + * flushed before the controller reads and invalidated before the + * processor does. + * + * Both act a whole cache line at a time, which is unsafe for a buffer + * that does not own its lines: invalidating drops whatever shares the + * line, and a writeback lands on top of what the controller just put + * there. Class drivers pass their own structure members, a 31 byte + * command block or a 13 byte status, which share lines. + * + * Such a buffer gets an aligned stand-in and is copied at the ends. + * Anything large enough to matter comes from a filesystem or from + * xhci_ioalloc() and is already aligned. + * + * Returned Value: + * The address to give the controller, or NULL if a stand-in was needed + * and could not be allocated. + * + ****************************************************************************/ + +static FAR uint8_t *xhci_dma_prepare(FAR struct xhci_epinfo_s *epinfo, + FAR uint8_t *buffer, size_t buflen, + bool dirin) +{ + size_t line = up_get_dcache_linesize(); + + epinfo->buffer = buffer; + epinfo->bounce = NULL; + epinfo->dmain = dirin; + + /* No cache to maintain, so nothing to arrange */ + + if (line == 0) + { + return buffer; + } + + if (((uintptr_t)buffer & (line - 1)) != 0 || (buflen & (line - 1)) != 0) + { + /* The buffer shares a line with something else. Work in a stand-in + * that does not. + */ + + epinfo->bounce = kmm_memalign(line, (buflen + line - 1) & ~(line - 1)); + if (epinfo->bounce == NULL) + { + return NULL; + } + + if (!dirin) + { + memcpy(epinfo->bounce, buffer, buflen); + } + + buffer = epinfo->bounce; + } + + /* Push what we are sending; drop what we are about to be sent, so that + * nothing the processor is still holding can be written back over it + * while the transfer is in flight. + */ + + if (dirin) + { + up_invalidate_dcache((uintptr_t)buffer, (uintptr_t)buffer + buflen); + } + else + { + up_clean_dcache((uintptr_t)buffer, (uintptr_t)buffer + buflen); + } + + return buffer; +} + +/**************************************************************************** + * Name: xhci_dma_finish + * + * Description: + * Read back what the controller wrote, and give up any stand-in buffer. + * Called on completion, before whoever is waiting is woken. + * + ****************************************************************************/ + +static void xhci_dma_finish(FAR struct xhci_epinfo_s *epinfo) +{ + FAR uint8_t *dma = epinfo->bounce ? epinfo->bounce : epinfo->buffer; + bool dirin = epinfo->dmain; + + if (dma == NULL) + { + return; + } + + if (dirin) + { + up_invalidate_dcache((uintptr_t)dma, (uintptr_t)dma + epinfo->buflen); + + if (epinfo->bounce != NULL && epinfo->buffer != NULL) + { + memcpy(epinfo->buffer, epinfo->bounce, epinfo->buflen); + } + } + + if (epinfo->bounce != NULL) + { + kmm_free(epinfo->bounce); + epinfo->bounce = NULL; + } + + epinfo->buffer = NULL; +} + /**************************************************************************** * Name: xhci_transfer_complete * @@ -2762,6 +2942,10 @@ static void xhci_transfer_complete(FAR struct usbhost_xhci_s *priv, epinfo = priv->devs[slot - 1].epinfo[ep - 1]; DEBUGASSERT(epinfo != NULL); + /* Read back what the controller wrote before anyone looks at it */ + + xhci_dma_finish(epinfo); + flags = spin_lock_irqsave(&priv->spinlock); /* Get transferred length */ @@ -3737,7 +3921,8 @@ static int xhci_free(FAR struct usbhost_driver_s *drvr, FAR uint8_t *buffer) static int xhci_ioalloc(FAR struct usbhost_driver_s *drvr, FAR uint8_t **buffer, size_t buflen) { - int ret = -ENOMEM; + size_t line; + int ret = -ENOMEM; DEBUGASSERT(drvr && buffer && buflen > 0); @@ -3748,7 +3933,18 @@ static int xhci_ioalloc(FAR struct usbhost_driver_s *drvr, return -ENOMEM; } - /* Allocated buffer must not cross page boundaries */ + /* Allocated buffer must not cross page boundaries. + * + * Round to whole cache lines as well as aligning the start, so that the + * buffer owns every line it touches and can be invalidated without + * disturbing whatever would otherwise share the last one. + */ + + line = up_get_dcache_linesize(); + if (line > 1) + { + buflen = (buflen + line - 1) & ~(line - 1); + } *buffer = (FAR uint8_t *)kmm_memalign((XHCI_PAGE_SIZE / 2) , buflen); if (*buffer) @@ -3847,6 +4043,13 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, len = xhci_getle16(req->len); + /* Refuse a buffer the controller cannot reach, as for bulk transfers */ + + if (buffer != NULL && len > 0 && !xhci_dmacapable(priv, buffer, len)) + { + return -EFAULT; + } + /* Terse output only if we are tracing */ #ifdef CONFIG_USBHOST_TRACE @@ -3870,6 +4073,15 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, ret = xhci_address_set(priv, rhport, true); if (ret == OK) { + /* The controller chose this address and wrote it into the + * output context. Invalidate before reading, or the stale + * copy is used. + */ + + up_invalidate_dcache((uintptr_t)rhport->dev->ctx, + (uintptr_t)rhport->dev->ctx + + sizeof(struct xhci_dev_ctx_s)); + /* Store USB Device Address assigned by xHCI */ ep0info->devaddr = @@ -4010,6 +4222,16 @@ static ssize_t xhci_transfer(FAR struct usbhost_driver_s *drvr, DEBUGASSERT(priv && rhport && epinfo && buffer && buflen > 0); + /* Refuse a buffer the controller cannot reach rather than pointing it at + * the wrong memory. A caller that has somewhere better to put the data + * will try again with it; the FAT filesystem does exactly that. + */ + + if (!xhci_dmacapable(priv, buffer, buflen)) + { + return -EFAULT; + } + /* We must have exclusive access to the xHCI hardware and data * structures. */ diff --git a/include/nuttx/usb/xhci.h b/include/nuttx/usb/xhci.h index 2368a5a9efe23..1010811244ba6 100644 --- a/include/nuttx/usb/xhci.h +++ b/include/nuttx/usb/xhci.h @@ -62,6 +62,13 @@ struct xhci_bus_ops_s /* Undo it, and release anything the bus allocated to make it work */ CODE void (*irq_detach)(FAR void *arg); + + /* Whether the controller may be pointed at a given buffer, which is a + * property of the platform. Leave NULL where every address a caller can + * produce is reachable, as a flat address space gives. + */ + + CODE bool (*dmacapable)(FAR void *arg, FAR uint8_t *buffer, size_t buflen); }; /**************************************************************************** From 042f20ce4bf0cba2625e0748c9f04f6709e84c5f Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:14:53 +0800 Subject: [PATCH 09/21] drivers/usbhost: Read xHCI HCIVERSION with an aligned access. 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 --- drivers/usbhost/usbhost_xhci.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 738c1563e1887..c92fec198bed2 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -729,8 +729,17 @@ static void xhci_dump_mem(FAR struct usbhost_xhci_s *priv, uinfo("Dump xHCI registers: %s\n", msg); uinfo("=== Host Controller Capability Registers ===\n"); - xhci_dump_capa_reg(priv, "CAPLENGTH ", XHCI_CAPLENGTH); - xhci_dump_capa_reg(priv, "HCIVERSION ", XHCI_HCIVERSION); + + /* CAPLENGTH and HCIVERSION share one word, and a register block reached + * over a bus that only answers aligned accesses cannot be read at the + * odd offset the second one has. Read the word once and take both from + * it. + */ + + uinfo("\tCAPLENGTH :\t\t0x%" PRIx32 "\n", + xhci_capa_getreg(priv, XHCI_CAPLENGTH) & 0xff); + uinfo("\tHCIVERSION :\t\t0x%" PRIx32 "\n", + xhci_capa_getreg(priv, XHCI_CAPLENGTH) >> 16); xhci_dump_capa_reg(priv, "HCSPARAMS1 ", XHCI_HCSPARAMS1); xhci_dump_capa_reg(priv, "HCSPARAMS2 ", XHCI_HCSPARAMS2); xhci_dump_capa_reg(priv, "HCSPARAMS3 ", XHCI_HCSPARAMS3); From 84653a3460925830f0ef8f9e447a358f86fe1299 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:14:53 +0800 Subject: [PATCH 10/21] drivers/usbhost: Report which xHCI command was rejected. 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 --- drivers/usbhost/usbhost_xhci.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index c92fec198bed2..b1f05a3385ba5 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -1915,7 +1915,8 @@ static void xhci_context_ctrl(FAR struct usbhost_xhci_s *priv, static int xhci_command(FAR struct usbhost_xhci_s *priv, FAR struct xhci_trb_s *trb, uint16_t timeout_ms) { - int ret; + uint32_t cmdtype; + int ret; /* Lock bus */ @@ -1925,6 +1926,10 @@ static int xhci_command(FAR struct usbhost_xhci_s *priv, return ret; } + /* Remember what this was before the result overwrites it */ + + cmdtype = XHCI_TRB_D2_TYPE_GET(trb->d2); + /* Add command to ring */ xhci_add_trb(priv, &priv->cmd, trb, 1); @@ -1960,7 +1965,8 @@ static int xhci_command(FAR struct usbhost_xhci_s *priv, } else { - uerr("event CC = %d\n", XHCI_TRB_D1_CC_GET(trb->d1)); + uerr("command type %d failed, CC = %d\n", cmdtype, + XHCI_TRB_D1_CC_GET(trb->d1)); ret = -EIO; } From 66dfbf49acbdcc933419324939046dbff55c4add Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:15:08 +0800 Subject: [PATCH 11/21] drivers/usbhost: Compute the event ring segment count at full width. 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 --- drivers/usbhost/usbhost_xhci.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index b1f05a3385ba5..144a4b66faaca 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -4611,6 +4611,7 @@ static void xhci_disconnect(FAR struct usbhost_driver_s *drvr, static int xhci_hw_getparams(FAR struct usbhost_xhci_s *priv) { uint32_t regval; + uint32_t erst; /* Get data form Host Controller Capability 1 Parameters */ @@ -4651,16 +4652,21 @@ static int xhci_hw_getparams(FAR struct usbhost_xhci_s *priv) uinfo("no scratch = %d\n", priv->no_scratch); - priv->no_erst = 1 << XHCI_HCSPARAMS2_ERST(regval); + /* How many event ring segments the controller will allow, which is a + * power of two and can reach 32768, so it is worked out at full width + * and only then narrowed to what this driver actually uses. Computed + * into the field directly it would wrap to zero on any controller + * offering more than 128 segments, and a table declared to hold no + * entries gives a controller with nowhere to report anything. + */ + + erst = 1ul << XHCI_HCSPARAMS2_ERST(regval); - uinfo("no_erst = %d\n", priv->no_erst); + uinfo("erst max = %" PRIu32 "\n", erst); /* Limit event ring segment table to 1 */ - if (priv->no_erst > XHCI_MAX_ERST) - { - priv->no_erst = XHCI_MAX_ERST; - } + priv->no_erst = (erst > XHCI_MAX_ERST) ? XHCI_MAX_ERST : erst; uinfo("no erst = %d\n", priv->no_erst); From 4246bb3ec51c7aac772407fe9a80e082efa5be1c Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:15:27 +0800 Subject: [PATCH 12/21] drivers/usbhost: Describe devices to an xHCI controller correctly. 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 --- drivers/usbhost/usbhost_xhci.c | 281 +++++++++++++++++++++++++-------- drivers/usbhost/usbhost_xhci.h | 16 ++ 2 files changed, 233 insertions(+), 64 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 144a4b66faaca..377791bce5cea 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -81,6 +82,14 @@ */ #define XHCI_PORT_RESET_MS (500) + +/* How much memory a context occupies, which depends on the stride the + * controller asked for. One entry for the slot and one per endpoint, and + * the input context carries its control entry in front of both. + */ + +#define XHCI_DEVCTX_SIZE(priv) ((1 + XHCI_MAX_ENDPOINTS) * (priv)->ctxsize) +#define XHCI_INCTX_SIZE(priv) ((2 + XHCI_MAX_ENDPOINTS) * (priv)->ctxsize) #define XHCI_BUFSIZE (512) /* Port numbers macros */ @@ -146,6 +155,8 @@ struct xhci_epinfo_s size_t buflen; /* Buffer length used for transfer */ FAR uint8_t *buffer; /* The caller's buffer, for cache maintenance */ FAR uint8_t *bounce; /* Aligned stand-in for it, or NULL */ + size_t dmalen; /* Length the cache is maintained over */ + size_t dmacopy; /* Length to copy back out of a stand-in */ bool dmain; /* Direction this buffer was prepared for */ sem_t iocsem; /* Semaphore used to wait for transfer completion */ #ifdef CONFIG_USBHOST_ASYNCH @@ -251,6 +262,7 @@ struct usbhost_xhci_s FAR const struct xhci_bus_ops_s *ops; /* Bus operations */ FAR void *arg; /* Bus private data */ FAR const char *name; /* What to call this controller */ + uint8_t ctxsize; /* Context stride, 32 or 64 bytes */ uint32_t pending; /* IRQ pending status */ struct work_s work; /* IRQ work */ struct work_s pscwork; /* Port status change work */ @@ -418,7 +430,17 @@ static ssize_t xhci_transfer_wait(FAR struct usbhost_xhci_s *priv, FAR struct xhci_epinfo_s *epinfo); static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, FAR uint8_t *buffer, size_t buflen); -static FAR uint8_t *xhci_dma_prepare(FAR struct xhci_epinfo_s *epinfo, +static uint32_t xhci_speed_id(uint8_t speed); +static inline FAR struct xhci_slot_ctx_s * +xhci_in_slot(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_input_dev_ctx_s *input); +static inline FAR struct xhci_ep_ctx_s * +xhci_in_ep(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_input_dev_ctx_s *input, int epidx); +static inline FAR struct xhci_slot_ctx_s * +xhci_out_slot(FAR struct xhci_dev_ctx_s *ctx); +static FAR uint8_t *xhci_dma_prepare(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_epinfo_s *epinfo, FAR uint8_t *buffer, size_t buflen, bool dirin); static void xhci_dma_finish(FAR struct xhci_epinfo_s *epinfo); @@ -1614,7 +1636,7 @@ static int xhci_slot_init(FAR struct usbhost_xhci_s *priv, * Initialize all fields to 0. */ - memset(dev->input, 0, sizeof(struct xhci_input_dev_ctx_s)); + memset(dev->input, 0, XHCI_INCTX_SIZE(priv)); /* Step 2. Initialize the Input Control Context by setting the A0 and * A1 flags to 1 (Slot flag and EP0 flag). @@ -1624,9 +1646,16 @@ static int xhci_slot_init(FAR struct usbhost_xhci_s *priv, XHCI_IN_CTX1_A(XHCI_EP0_FLAG); xhci_context_ctrl(priv, dev, 0, regval); - /* Step 3. Initialize the Input Slot Context */ + /* Step 3. Initialize the Input Slot Context. + * + * The speed field has no valid zero. This is the only place the + * controller learns the device's speed, and one that checks refuses + * Address Device with a parameter error without it. + */ - regval = XHCI_ST_CTX0_CTXENT_SET(1); + regval = XHCI_ST_CTX0_CTXENT_SET(1) | + XHCI_ST_CTX0_SPEED_SET( + xhci_speed_id(dev->rhport->hport.hport.speed)); #ifdef CONFIG_USBHOST_HUB /* TODO: @@ -1638,7 +1667,7 @@ static int xhci_slot_init(FAR struct usbhost_xhci_s *priv, # warning missing logic #endif - dev->input->slot.ctx[0] = htole32(regval); + xhci_in_slot(priv, dev->input)->ctx[0] = htole32(regval); /* Configure Root Hub Port Number (starts from 1) */ @@ -1647,7 +1676,7 @@ static int xhci_slot_init(FAR struct usbhost_xhci_s *priv, /* TODO: configure number of ports */ regval |= XHCI_ST_CTX1_PORTS_SET(0); - dev->input->slot.ctx[1] = htole32(regval); + xhci_in_slot(priv, dev->input)->ctx[1] = htole32(regval); /* Step 4. the Transfer Ring for the Default Control Endpoint is already * allocated. @@ -1673,7 +1702,7 @@ static int xhci_slot_init(FAR struct usbhost_xhci_s *priv, DEBUGASSERT(drdp != 0); xhci_ep_configure(priv, - &dev->input->ep[0], + xhci_in_ep(priv, dev->input, 0), XHCI_EPTYPE_CTRL, maxpkt, 0, drdp, 0, 0); @@ -1682,13 +1711,22 @@ static int xhci_slot_init(FAR struct usbhost_xhci_s *priv, * Initialize all fields to 0. */ - memset(dev->ctx, 0, sizeof(struct xhci_dev_ctx_s)); + memset(dev->ctx, 0, XHCI_DEVCTX_SIZE(priv)); - /* Flush Device input context */ + /* Flush both contexts. + * + * The output context is the controller's to write, so clearing it must + * reach memory: the dirty zeros left in cache are written back later, on + * top of what the controller has put there. The slot state lives in + * that context, and losing it fails the next command against the slot. + */ + + up_flush_dcache((uintptr_t)dev->ctx, + (uintptr_t)dev->ctx + XHCI_DEVCTX_SIZE(priv)); up_flush_dcache((uintptr_t)dev->input, (uintptr_t)dev->input + - sizeof(struct xhci_input_dev_ctx_s)); + XHCI_INCTX_SIZE(priv)); /* Step 7. Load the appropriate (Device Slot ID) entry in the Device * Context Base Address Array with a pointer to the Output Device @@ -1826,8 +1864,15 @@ static int xhci_device_deinit(FAR struct usbhost_xhci_s *priv, rhport->dev->state = XHCI_SLOT_DISABLED; - memset(rhport->dev->ctx, 0, sizeof(struct xhci_dev_ctx_s)); - memset(rhport->dev->input, 0, sizeof(struct xhci_input_dev_ctx_s)); + memset(rhport->dev->ctx, 0, XHCI_DEVCTX_SIZE(priv)); + memset(rhport->dev->input, 0, XHCI_INCTX_SIZE(priv)); + + /* And push both, so nothing is left to be written back later */ + + up_flush_dcache((uintptr_t)rhport->dev->ctx, + (uintptr_t)rhport->dev->ctx + XHCI_DEVCTX_SIZE(priv)); + up_flush_dcache((uintptr_t)rhport->dev->input, + (uintptr_t)rhport->dev->input + XHCI_INCTX_SIZE(priv)); /* Remove reference to a device slot */ @@ -1896,8 +1941,8 @@ static void xhci_context_ctrl(FAR struct usbhost_xhci_s *priv, } } - dev->input->slot.ctx[0] &= ~XHCI_ST_CTX0_CTXENT_MASK; - dev->input->slot.ctx[0] |= XHCI_ST_CTX0_CTXENT_SET(i); + xhci_in_slot(priv, dev->input)->ctx[0] &= ~XHCI_ST_CTX0_CTXENT_MASK; + xhci_in_slot(priv, dev->input)->ctx[0] |= XHCI_ST_CTX0_CTXENT_SET(i); } /**************************************************************************** @@ -2324,7 +2369,7 @@ static int xhci_control_setup(FAR struct xhci_rhport_s *rhport, if (buffer) { - buffer = xhci_dma_prepare(epinfo, buffer, buflen, + buffer = xhci_dma_prepare(priv, epinfo, buffer, buflen, (req->type & USB_REQ_DIR_IN) != 0); if (buffer == NULL) { @@ -2405,7 +2450,8 @@ static int xhci_normal_setup(FAR struct xhci_rhport_s *rhport, /* Make the buffer safe for the controller to reach */ - buffer = xhci_dma_prepare(epinfo, buffer, buflen, epinfo->dirin != 0); + buffer = xhci_dma_prepare(priv, epinfo, buffer, buflen, + epinfo->dirin != 0); if (buffer == NULL) { return -ENOMEM; @@ -2431,8 +2477,8 @@ static int xhci_normal_setup(FAR struct xhci_rhport_s *rhport, if (++n >= XHCI_TD_MAX) { - uerr("transfer of %zu needs more TRBs than the ring holds\n", - buflen); + uerr("transfer of %zu from pa %" PRIxPTR " needs more than %d " + "TRBs\n", buflen, pa, XHCI_TD_MAX); return -EINVAL; } @@ -2489,7 +2535,8 @@ static int xhci_isoc_setup(FAR struct xhci_rhport_s *rhport, /* Make the buffer safe for the controller to reach */ - buffer = xhci_dma_prepare(epinfo, buffer, buflen, epinfo->dirin != 0); + buffer = xhci_dma_prepare(priv, epinfo, buffer, buflen, + epinfo->dirin != 0); if (buffer == NULL) { return -ENOMEM; @@ -2788,6 +2835,82 @@ static void xhci_portsc_work(FAR void *arg) } } +/**************************************************************************** + * Name: xhci_in_slot / xhci_in_ep / xhci_out_slot + * + * Description: + * Reach into a device context. + * + * A context is an array of equally sized entries, and how big they are is + * a property of the controller rather than of the specification: it + * reports either thirty-two or sixty-four bytes, and the wider form is + * the same fields with reserved space after them. So these are the same + * structures at a different stride, and only the arithmetic to find the + * n'th one has to know which. + * + * Output context: slot, then endpoints 1 upward. + * Input context: input control, then slot, then endpoints. + * + * The first entry of either is at offset zero, so only the ones after it + * need this. + * + ****************************************************************************/ + +static inline FAR struct xhci_slot_ctx_s * +xhci_in_slot(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_input_dev_ctx_s *input) +{ + return (FAR struct xhci_slot_ctx_s *)((uintptr_t)input + priv->ctxsize); +} + +static inline FAR struct xhci_ep_ctx_s * +xhci_in_ep(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_input_dev_ctx_s *input, int epidx) +{ + return (FAR struct xhci_ep_ctx_s *)((uintptr_t)input + + (epidx + 2) * priv->ctxsize); +} + +static inline FAR struct xhci_slot_ctx_s * +xhci_out_slot(FAR struct xhci_dev_ctx_s *ctx) +{ + return (FAR struct xhci_slot_ctx_s *)ctx; +} + +/**************************************************************************** + * Name: xhci_speed_id + * + * Description: + * Turn the speed the USB host stack uses into the one a slot context + * wants, which is a different numbering with no relation to it. + * + ****************************************************************************/ + +static uint32_t xhci_speed_id(uint8_t speed) +{ + switch (speed) + { + case USB_SPEED_LOW: + return XHCI_SPEED_LOW; + case USB_SPEED_FULL: + return XHCI_SPEED_FULL; + case USB_SPEED_HIGH: + return XHCI_SPEED_HIGH; + case USB_SPEED_SUPER: + return XHCI_SPEED_SUPER; + case USB_SPEED_SUPER_PLUS: + return XHCI_SPEED_SUPER_PLUS; + default: + + /* Nothing else can be described to a controller, and full speed + * is the safe answer. + */ + + uwarn("no speed ID for USB speed %d\n", speed); + return XHCI_SPEED_FULL; + } +} + /**************************************************************************** * Name: xhci_dmacapable * @@ -2842,30 +2965,53 @@ static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, * ****************************************************************************/ -static FAR uint8_t *xhci_dma_prepare(FAR struct xhci_epinfo_s *epinfo, +static FAR uint8_t *xhci_dma_prepare(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_epinfo_s *epinfo, FAR uint8_t *buffer, size_t buflen, bool dirin) { - size_t line = up_get_dcache_linesize(); + size_t line = up_get_dcache_linesize(); + bool reachable = xhci_dmacapable(priv, buffer, buflen); + + epinfo->buffer = buffer; + epinfo->bounce = NULL; + epinfo->dmalen = buflen; + + /* How much to bring back afterwards. This cannot be taken from buflen + * at completion time: that field means the length of a data transfer and + * control transfers deliberately leave it zero, so a descriptor read + * would copy nothing back and the caller would see whatever its buffer + * held before. + */ - epinfo->buffer = buffer; - epinfo->bounce = NULL; - epinfo->dmain = dirin; + epinfo->dmacopy = buflen; + epinfo->dmain = dirin; - /* No cache to maintain, so nothing to arrange */ + /* Nothing to arrange: no cache to maintain, and an address the + * controller can be pointed at as it stands. + */ - if (line == 0) + if (line == 0 && reachable) { return buffer; } - if (((uintptr_t)buffer & (line - 1)) != 0 || (buflen & (line - 1)) != 0) + if (!reachable || + ((uintptr_t)buffer & (line - 1)) != 0 || (buflen & (line - 1)) != 0) { /* The buffer shares a line with something else. Work in a stand-in * that does not. */ - epinfo->bounce = kmm_memalign(line, (buflen + line - 1) & ~(line - 1)); + /* Maintain the whole stand-in, not just the part in use: cache + * operations work a line at a time and this chip rejects a partial + * range. + */ + + epinfo->dmalen = line ? ((buflen + line - 1) & ~(line - 1)) : buflen; + + epinfo->bounce = kmm_memalign(line ? line : sizeof(uintptr_t), + epinfo->dmalen); if (epinfo->bounce == NULL) { return NULL; @@ -2886,11 +3032,13 @@ static FAR uint8_t *xhci_dma_prepare(FAR struct xhci_epinfo_s *epinfo, if (dirin) { - up_invalidate_dcache((uintptr_t)buffer, (uintptr_t)buffer + buflen); + up_invalidate_dcache((uintptr_t)buffer, + (uintptr_t)buffer + epinfo->dmalen); } else { - up_clean_dcache((uintptr_t)buffer, (uintptr_t)buffer + buflen); + up_clean_dcache((uintptr_t)buffer, + (uintptr_t)buffer + epinfo->dmalen); } return buffer; @@ -2917,11 +3065,12 @@ static void xhci_dma_finish(FAR struct xhci_epinfo_s *epinfo) if (dirin) { - up_invalidate_dcache((uintptr_t)dma, (uintptr_t)dma + epinfo->buflen); + up_invalidate_dcache((uintptr_t)dma, + (uintptr_t)dma + epinfo->dmalen); if (epinfo->bounce != NULL && epinfo->buffer != NULL) { - memcpy(epinfo->buffer, epinfo->bounce, epinfo->buflen); + memcpy(epinfo->buffer, epinfo->bounce, epinfo->dmacopy); } } @@ -3557,8 +3706,11 @@ static int xhci_ep0configure(FAR struct usbhost_driver_s *drvr, { /* Update max packet size */ - rhport->dev->input->ep[0].ctx1 &= ~XHCI_EP_CTX1_MAXPKT_MASK; - rhport->dev->input->ep[0].ctx1 |= XHCI_EP_CTX1_MAXPKT(maxpacketsize); + FAR struct xhci_ep_ctx_s *ep0ctx = + xhci_in_ep(priv, rhport->dev->input, 0); + + ep0ctx->ctx1 &= ~XHCI_EP_CTX1_MAXPKT_MASK; + ep0ctx->ctx1 |= XHCI_EP_CTX1_MAXPKT(maxpacketsize); /* Add Slot Context and EP0 Context */ @@ -3570,13 +3722,17 @@ static int xhci_ep0configure(FAR struct usbhost_driver_s *drvr, up_flush_dcache((uintptr_t)rhport->dev->input, (uintptr_t)rhport->dev->input + - sizeof(struct xhci_input_dev_ctx_s)); + XHCI_INCTX_SIZE(priv)); /* Free mutex before command execution */ nxmutex_unlock(&priv->lock); ctx = up_addrenv_va_to_pa(rhport->dev->input); + + uinfo("slot %d funcaddr %d speed %d maxpacket %d\n", + epinfo->slot, funcaddr, speed, maxpacketsize); + ret = xhci_cmd_evalctx(priv, epinfo->slot, ctx); } @@ -3627,6 +3783,12 @@ static int xhci_epalloc(FAR struct usbhost_driver_s *drvr, && ep != NULL); hport = epdesc->hport; + /* Only the tracing alternative below and the hub logic further down use + * this, and a configuration may have neither. + */ + + UNUSED(hport); + /* Terse output only if we are tracing */ #ifdef CONFIG_USBHOST_TRACE @@ -3747,7 +3909,7 @@ static int xhci_epalloc(FAR struct usbhost_driver_s *drvr, * Max Burst Size set for 0 for now (USB3.0 specific) */ - xhci_ep_configure(priv, &dev->input->ep[idx - 1], + xhci_ep_configure(priv, xhci_in_ep(priv, dev->input, idx - 1), eptype, epdesc->mxpacketsize, 0, up_addrenv_va_to_pa(epinfo->td.ring), 0, epinfo->interval); @@ -3758,7 +3920,7 @@ static int xhci_epalloc(FAR struct usbhost_driver_s *drvr, up_flush_dcache((uintptr_t)dev->input, (uintptr_t)dev->input + - sizeof(struct xhci_input_dev_ctx_s)); + XHCI_INCTX_SIZE(priv)); /* Configure EP */ @@ -4058,13 +4220,6 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, len = xhci_getle16(req->len); - /* Refuse a buffer the controller cannot reach, as for bulk transfers */ - - if (buffer != NULL && len > 0 && !xhci_dmacapable(priv, buffer, len)) - { - return -EFAULT; - } - /* Terse output only if we are tracing */ #ifdef CONFIG_USBHOST_TRACE @@ -4095,13 +4250,14 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, up_invalidate_dcache((uintptr_t)rhport->dev->ctx, (uintptr_t)rhport->dev->ctx + - sizeof(struct xhci_dev_ctx_s)); + XHCI_DEVCTX_SIZE(priv)); /* Store USB Device Address assigned by xHCI */ ep0info->devaddr = - XHCI_ST_CTX3_ADDR_GET(rhport->dev->ctx->slot.ctx[3]); - rhport->dev->input->slot.ctx[3] = rhport->dev->ctx->slot.ctx[3]; + XHCI_ST_CTX3_ADDR_GET(xhci_out_slot(rhport->dev->ctx)->ctx[3]); + xhci_in_slot(priv, rhport->dev->input)->ctx[3] = + xhci_out_slot(rhport->dev->ctx)->ctx[3]; } return OK; @@ -4237,16 +4393,6 @@ static ssize_t xhci_transfer(FAR struct usbhost_driver_s *drvr, DEBUGASSERT(priv && rhport && epinfo && buffer && buflen > 0); - /* Refuse a buffer the controller cannot reach rather than pointing it at - * the wrong memory. A caller that has somewhere better to put the data - * will try again with it; the FAT filesystem does exactly that. - */ - - if (!xhci_dmacapable(priv, buffer, buflen)) - { - return -EFAULT; - } - /* We must have exclusive access to the xHCI hardware and data * structures. */ @@ -4615,12 +4761,14 @@ static int xhci_hw_getparams(FAR struct usbhost_xhci_s *priv) /* Get data form Host Controller Capability 1 Parameters */ + /* Context entry stride, 32 or 64 bytes as the controller reports. The + * wider form is the same fields with padding. + */ + regval = xhci_capa_getreg(priv, XHCI_HCCPARAMS1); - if (regval & XHCI_HCCPARAMS1_CSZ) - { - uerr("Only 32 byte Context data structures supported!\n"); - return -EIO; - } + priv->ctxsize = (regval & XHCI_HCCPARAMS1_CSZ) ? 64 : 32; + + uinfo("context size = %d\n", priv->ctxsize); /* Get data from Structural Parameters 1 register */ @@ -4787,7 +4935,12 @@ static int xhci_mem_alloc(FAR struct usbhost_xhci_s *priv) { /* Allocate Device Context */ - priv->devs[i].ctx = kmm_zalloc(sizeof(struct xhci_dev_ctx_s)); + /* The base address array holds these, and every entry in it must be + * 64 byte aligned, so the allocation has to be too. + */ + + priv->devs[i].ctx = kmm_memalign(XHCI_CTX_ALIGN, + XHCI_DEVCTX_SIZE(priv)); if (!priv->devs[i].ctx) { uerr("dev ctx zalloc failed!\n"); @@ -4799,7 +4952,7 @@ static int xhci_mem_alloc(FAR struct usbhost_xhci_s *priv) */ priv->devs[i].input = kmm_memalign((XHCI_PAGE_SIZE / 2), - sizeof(struct xhci_input_dev_ctx_s)); + XHCI_INCTX_SIZE(priv)); if (!priv->devs[i].input) { uerr("dev input zalloc failed!\n"); diff --git a/drivers/usbhost/usbhost_xhci.h b/drivers/usbhost/usbhost_xhci.h index 2fc2aa207aded..3339721895d95 100644 --- a/drivers/usbhost/usbhost_xhci.h +++ b/drivers/usbhost/usbhost_xhci.h @@ -506,6 +506,22 @@ #define XHCI_ST_CTX0_RTSTR_MASK (0xfffff << XHCI_ST_CTX0_RTSTR_SHIFT) #define XHCI_ST_CTX0_SPEED_SHIFT (20) /* Bits 20:23: Speed */ #define XHCI_ST_CTX0_SPEED_MASK (0xf << XHCI_ST_CTX0_SPEED_SHIFT) +#define XHCI_ST_CTX0_SPEED_SET(x) (((x) << XHCI_ST_CTX0_SPEED_SHIFT) & \ + XHCI_ST_CTX0_SPEED_MASK) + +/* Port Speed IDs, which xHCI numbers its own way rather than USB's. These + * are the values every controller reports in PORTSC and expects back in a + * slot context; a device is described to the controller with one of them + * and with nothing else, so zero is not a default but an invalid context. + * + * Reference: Table 7-13: Default USB Speed ID Mapping + */ + +#define XHCI_SPEED_FULL (1) +#define XHCI_SPEED_LOW (2) +#define XHCI_SPEED_HIGH (3) +#define XHCI_SPEED_SUPER (4) +#define XHCI_SPEED_SUPER_PLUS (5) #define XHCI_ST_CTX0_MTT (1 << 25) /* Bit 25: Multi-TT */ /* Bit 24: Reserved */ #define XHCI_ST_CTX0_HUB (1 << 26) /* Bit 26: Hub */ From 28858db5a07d0d8fb257b99313304958a129e7f4 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:16:12 +0800 Subject: [PATCH 13/21] drivers/usbhost: Carry the xHCI transfer chain across the ring join. 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 --- drivers/usbhost/usbhost_xhci.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 377791bce5cea..e1461ccfbd23c 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -990,6 +990,19 @@ static void xhci_add_trb(FAR struct usbhost_xhci_s *priv, XHCI_TRB_D2_TYPE_SET(XHCI_TRB_TYPE_LINK); } + /* Carry the chain forward across the join. + * + * A multi-TRB transfer can reach the end of the ring part way + * through, putting the link inside it. A link without the + * chain bit ends the transfer where it stands, and the TRB that + * asked for the completion interrupt is never reached. + */ + + if ((trb[i].d2 & XHCI_TRB_D2_CH) != 0) + { + d2 |= XHCI_TRB_D2_CH; + } + /* Other parameters are already correct for this TRB */ ring->ring[ring->i].d2 = htole32(d2); From ddabfb1e6dc380ec1948356b0e0b9c1dbebbc54b Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:16:41 +0800 Subject: [PATCH 14/21] drivers/usbhost: Announce what an xHCI port has attached. 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 --- drivers/usbhost/usbhost_xhci.c | 45 ++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index e1461ccfbd23c..b8068948d68f3 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -358,6 +359,7 @@ static int xhci_ctrl_reset(FAR struct usbhost_xhci_s *priv); /* Port management **********************************************************/ static void xhci_probe_ports(FAR struct usbhost_xhci_s *priv); +static FAR const char *xhci_speed_str(uint32_t portsc); static int xhci_port_enable(FAR struct usbhost_xhci_s *priv, FAR struct usbhost_hubport_s *hport); @@ -1496,6 +1498,16 @@ static int xhci_port_enable(FAR struct usbhost_xhci_s *priv, } } + /* Say what turned up, now that the port can answer. + * + * The speed field only means anything once the port has been reset and + * enabled. A USB2 port reports the reset default, full speed, until + * then. + */ + + syslog(LOG_INFO, "%s: port %d: device attached at %s\n", + priv->name, rhpndx + 1, xhci_speed_str(regval)); + return OK; } @@ -2732,6 +2744,36 @@ static void xhci_asynch_completion(FAR struct xhci_epinfo_s *epinfo) } #endif +/**************************************************************************** + * Name: xhci_speed_str + * + * Description: + * What a port negotiated, in words. PORTSC reports a speed ID, not a + * speed. + * + ****************************************************************************/ + +static FAR const char *xhci_speed_str(uint32_t portsc) +{ + switch (XHCI_PORTSC_PS(portsc)) + { + case XHCI_PORTSC_PS_FULL: + return "full speed, 12Mbps"; + case XHCI_PORTSC_PS_LOW: + return "low speed, 1.5Mbps"; + case XHCI_PORTSC_PS_HIGH: + return "high speed, 480Mbps"; + case XHCI_PORTSC_PS_SUPPER11: + return "SuperSpeed, 5Gbps"; + case XHCI_PORTSC_PS_SUPPER21: + case XHCI_PORTSC_PS_SUPPER12: + case XHCI_PORTSC_PS_SUPPER22: + return "SuperSpeed+, 10Gbps"; + default: + return "an unknown speed"; + } +} + /**************************************************************************** * Name: xhci_portsc_work * @@ -2811,6 +2853,9 @@ static void xhci_portsc_work(FAR void *arg) usbhost_vtrace2(XHCI_VTRACE2_PORTSC_DISCONND, rhpndx + 1, priv->pscwait); + syslog(LOG_INFO, "%s: port %d: device removed\n", + priv->name, rhpndx + 1); + rhport->connected = false; /* Are we bound to a class instance? */ From 843d5acc708fe245a3a4e6a59eef43af90a2dc6e Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:16:51 +0800 Subject: [PATCH 15/21] drivers/usbhost: Copy an xHCI stand-in buffer in the caller's context. 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 --- drivers/usbhost/usbhost_xhci.c | 69 +++++++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index b8068948d68f3..64886b5454859 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -433,6 +433,10 @@ static ssize_t xhci_transfer_wait(FAR struct usbhost_xhci_s *priv, static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, 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, + FAR uint8_t *buffer, size_t buflen); +#endif static inline FAR struct xhci_slot_ctx_s * xhci_in_slot(FAR struct usbhost_xhci_s *priv, FAR struct xhci_input_dev_ctx_s *input); @@ -2995,6 +2999,32 @@ static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, return priv->ops->dmacapable(priv->arg, buffer, buflen); } +#ifdef CONFIG_USBHOST_ASYNCH +/**************************************************************************** + * Name: xhci_dma_direct + * + * Description: + * Whether the controller can be pointed straight at this buffer, with no + * stand-in needed: an address it can reach, owning whole cache lines. + * + ****************************************************************************/ + +static bool xhci_dma_direct(FAR struct usbhost_xhci_s *priv, + FAR uint8_t *buffer, size_t buflen) +{ + size_t line = up_get_dcache_linesize(); + + if (!xhci_dmacapable(priv, buffer, buflen)) + { + return false; + } + + return line == 0 || + (((uintptr_t)buffer & (line - 1)) == 0 && + (buflen & (line - 1)) == 0); +} +#endif + /**************************************************************************** * Name: xhci_dma_prepare * @@ -3057,6 +3087,8 @@ static FAR uint8_t *xhci_dma_prepare(FAR struct usbhost_xhci_s *priv, if (!reachable || ((uintptr_t)buffer & (line - 1)) != 0 || (buflen & (line - 1)) != 0) { + /* A stand-in is needed; see xhci_dma_direct() for the same test */ + /* The buffer shares a line with something else. Work in a stand-in * that does not. */ @@ -3107,7 +3139,13 @@ static FAR uint8_t *xhci_dma_prepare(FAR struct usbhost_xhci_s *priv, * * Description: * Read back what the controller wrote, and give up any stand-in buffer. - * Called on completion, before whoever is waiting is woken. + * + * This must run in the context of whoever asked for the transfer, not in + * the completion handler. The buffer being copied back into may belong + * to a user process, and its address means nothing in the work queue + * thread that handles the completion event, where the write would fault + * or corrupt another process. The caller is blocked until the transfer + * finishes anyway. * ****************************************************************************/ @@ -3164,10 +3202,6 @@ static void xhci_transfer_complete(FAR struct usbhost_xhci_s *priv, epinfo = priv->devs[slot - 1].epinfo[ep - 1]; DEBUGASSERT(epinfo != NULL); - /* Read back what the controller wrote before anyone looks at it */ - - xhci_dma_finish(epinfo); - flags = spin_lock_irqsave(&priv->spinlock); /* Get transferred length */ @@ -4353,6 +4387,11 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, /* And wait for the transfer to complete */ nbytes = xhci_transfer_wait(priv, ep0info); + + /* As for bulk: the copy back belongs in the caller's context */ + + xhci_dma_finish(ep0info); + return nbytes >= 0 ? OK : (int)nbytes; errout_with_iocwait: @@ -4512,6 +4551,13 @@ static ssize_t xhci_transfer(FAR struct usbhost_driver_s *drvr, /* Then wait for the transfer to complete */ nbytes = xhci_transfer_wait(priv, epinfo); + + /* And bring back what it produced, here rather than in the completion, + * because this is the context the caller's buffer belongs to. + */ + + xhci_dma_finish(epinfo); + return nbytes; errout_with_iocwait: @@ -4569,6 +4615,19 @@ static int xhci_asynch(FAR struct usbhost_driver_s *drvr, usbhost_ep_t ep, DEBUGASSERT(priv && rhport && epinfo && buffer && buflen > 0); + /* An asynchronous transfer has no caller to come back to, so a buffer + * needing a stand-in cannot be used: the copy back out of it would have + * to happen in the completion handler, which runs in a work queue thread + * where a caller's address means nothing. The callers of this are class + * drivers using kernel memory, which needs no stand-in. + */ + + if (!xhci_dma_direct(priv, buffer, buflen)) + { + uerr("ERROR: asynchronous transfer needs a directly usable buffer\n"); + return -EFAULT; + } + /* We must have exclusive access to the xHCI hardware and data * structures. */ From 67d796deb4e9533fde5e6ac5f6d7891a7b2b4448 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:17:15 +0800 Subject: [PATCH 16/21] drivers/usbhost: Convert the xHCI endpoint interval from the descriptor. 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 --- drivers/usbhost/usbhost_xhci.c | 62 ++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 64886b5454859..b8b29784ac5ca 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -3831,6 +3831,68 @@ static int xhci_ep0configure(FAR struct usbhost_driver_s *drvr, return ret; } +/**************************************************************************** + * Name: xhci_interval + * + * Description: + * Work out the Interval an endpoint context wants. + * + * The field 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 simply be copied across. + * + * A low or full speed interrupt endpoint counts in frames, so its period + * is bInterval milliseconds, or bInterval * 8 microframes, and the + * exponent is the position of the highest bit of that. Everything else + * that is 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. + * + ****************************************************************************/ + +static uint8_t xhci_interval(uint8_t speed, uint8_t xfrtype, + uint8_t interval) +{ + unsigned int exp; + + if (xfrtype != USB_EP_ATTR_XFER_INT && xfrtype != USB_EP_ATTR_XFER_ISOC) + { + return 0; + } + + if ((speed == USB_SPEED_LOW || speed == USB_SPEED_FULL) && + xfrtype == USB_EP_ATTR_XFER_INT) + { + /* Frames. Round down to a power of two, and keep it inside what the + * specification allows for this kind of endpoint: 2^3 microframes is + * one frame, 2^10 is 128 of them. + */ + + if (interval == 0) + { + interval = 1; + } + + for (exp = 0; (1u << (exp + 1)) <= interval * 8u; exp++); + + if (exp < 3) + { + exp = 3; + } + else if (exp > 10) + { + exp = 10; + } + + return exp; + } + + /* Already an exponent, counted from one */ + + return interval > 0 ? interval - 1 : 0; +} + /**************************************************************************** * Name: xhci_epalloc * From 6682b21960c24e0442d0a5fbf71bda6b92ca12e4 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sun, 16 Aug 2026 17:17:15 +0800 Subject: [PATCH 17/21] drivers/usbhost: Check for the device before allocating an endpoint. 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 --- drivers/usbhost/usbhost_xhci.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index b8b29784ac5ca..b81bd0e35fe35 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -3971,16 +3971,31 @@ static int xhci_epalloc(FAR struct usbhost_driver_s *drvr, epinfo->epno = epdesc->addr; #ifndef CONFIG_USBHOST_INT_DISABLE - epinfo->interval = epdesc->interval; + epinfo->interval = xhci_interval(hport->speed, epdesc->xfrtype, + epdesc->interval); #endif epinfo->xfrtype = epdesc->xfrtype; nxsem_init(&epinfo->iocsem, 0, 0); /* xhci_epno_get() returns Device Context Index (DCI) */ - idx = xhci_epno_get(epinfo); - mask = XHCI_IN_CTX1_A(XHCI_EP_FLAG(idx)); - dev = rhport->dev; + idx = xhci_epno_get(epinfo); + mask = XHCI_IN_CTX1_A(XHCI_EP_FLAG(idx)); + dev = rhport->dev; + + /* There has to be a device to hang the endpoint off. A port whose + * enumeration failed is retried after its slot has been given back, so + * this can run for a root hub port with nothing behind it. + */ + + if (dev == NULL) + { + uerr("no device on port %d\n", RHPNDX(rhport)); + nxsem_destroy(&epinfo->iocsem); + kmm_free(epinfo); + return -ENODEV; + } + dev->epinfo[idx - 1] = epinfo; /* TD rings already allocated but not connected yet. */ From b0d9bb9938aa41323e88ea41edf4b7f5fa2e0fc3 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Wed, 5 Aug 2026 23:31:26 +0800 Subject: [PATCH 18/21] drivers/usbhost: Make xHCI asynchronous transfers deliver their data. 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 --- drivers/usbhost/usbhost_xhci.c | 124 ++++++++++++++++++--------------- 1 file changed, 66 insertions(+), 58 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index b81bd0e35fe35..c8b4d542eab38 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -412,9 +412,11 @@ static int xhci_ioc_wait(FAR struct xhci_epinfo_s *epinfo); #ifdef CONFIG_USBHOST_ASYNCH static inline int xhci_ioc_async_setup(FAR struct xhci_rhport_s *rhport, FAR struct xhci_epinfo_s *epinfo, + size_t buflen, usbhost_asynch_t callback, FAR void *arg); -static void xhci_asynch_completion(FAR struct xhci_epinfo_s *epinfo); +static void xhci_asynch_completion(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_epinfo_s *epinfo); #endif static int xhci_control_setup(FAR struct xhci_rhport_s *rhport, FAR struct xhci_epinfo_s *epinfo, @@ -433,10 +435,6 @@ static ssize_t xhci_transfer_wait(FAR struct usbhost_xhci_s *priv, static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, 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, - FAR uint8_t *buffer, size_t buflen); -#endif static inline FAR struct xhci_slot_ctx_s * xhci_in_slot(FAR struct usbhost_xhci_s *priv, FAR struct xhci_input_dev_ctx_s *input); @@ -2649,6 +2647,8 @@ static ssize_t xhci_transfer_wait(FAR struct usbhost_xhci_s *priv, * Input Parameters: * epinfo - The IN or OUT endpoint descriptor for the device endpoint on * which the transfer will be performed. + * buflen - The length of the transfer, from which the completion works + * out how much was transferred. * callback - The function to be called when the transfer completes * arg - An arbitrary argument that will be provided with the callback. * @@ -2662,6 +2662,7 @@ static ssize_t xhci_transfer_wait(FAR struct usbhost_xhci_s *priv, static inline int xhci_ioc_async_setup(FAR struct xhci_rhport_s *rhport, FAR struct xhci_epinfo_s *epinfo, + size_t buflen, usbhost_asynch_t callback, FAR void *arg) { @@ -2684,6 +2685,7 @@ static inline int xhci_ioc_async_setup(FAR struct xhci_rhport_s *rhport, epinfo->iocwait = false; /* No synchronous wakeup */ epinfo->status = 0; /* No status yet */ epinfo->xfrd = 0; /* Nothing transferred yet */ + epinfo->buflen = buflen; /* Buffer length */ epinfo->result = -EBUSY; /* Transfer in progress */ epinfo->callback = callback; /* Asynchronous callback */ epinfo->arg = arg; /* Argument that accompanies the callback */ @@ -2698,10 +2700,11 @@ static inline int xhci_ioc_async_setup(FAR struct xhci_rhport_s *rhport, * Name: xhci_asynch_completion * * Description: - * This function is called at the interrupt level when an asynchronous - * transfer completes. It performs the pending callback. + * This function is called from the interrupt work queue when an + * asynchronous transfer completes. It performs the pending callback. * * Input Parameters: + * priv - xHCI private state * epinfo - The IN or OUT endpoint descriptor for the device endpoint on * which the transfer was performed. * @@ -2709,21 +2712,26 @@ static inline int xhci_ioc_async_setup(FAR struct xhci_rhport_s *rhport, * None * * Assumptions: - * - Called from the interrupt level + * - Called from the work queue, without the spinlock held * ****************************************************************************/ -static void xhci_asynch_completion(FAR struct xhci_epinfo_s *epinfo) +static void xhci_asynch_completion(FAR struct usbhost_xhci_s *priv, + FAR struct xhci_epinfo_s *epinfo) { usbhost_asynch_t callback; ssize_t nbytes; FAR void *arg; + irqstate_t flags; int result; - DEBUGASSERT(epinfo != NULL && epinfo->iocwait == false && - epinfo->callback != NULL); + DEBUGASSERT(epinfo != NULL && epinfo->iocwait == false); + + /* Extract and reset the callback info, atomically against a concurrent + * cancellation. + */ - /* Extract and reset the callback info */ + flags = spin_lock_irqsave(&priv->spinlock); callback = epinfo->callback; arg = epinfo->arg; @@ -2735,6 +2743,23 @@ static void xhci_asynch_completion(FAR struct xhci_epinfo_s *epinfo) epinfo->result = OK; epinfo->iocwait = false; + spin_unlock_irqrestore(&priv->spinlock, flags); + + /* A cancellation that got in first has already done the callback */ + + if (callback == NULL) + { + return; + } + + /* Bring back what the controller wrote before anyone reads it. The + * addresses are usable here: a transfer given to DRVR_ASYNCH must use + * memory from DRVR_ALLOC, and that is kernel memory, which this work + * queue thread can reach. + */ + + xhci_dma_finish(epinfo); + /* Then perform the callback. Provide the number of bytes successfully * transferred or the negated errno value in the event of a failure. */ @@ -2999,32 +3024,6 @@ static bool xhci_dmacapable(FAR struct usbhost_xhci_s *priv, return priv->ops->dmacapable(priv->arg, buffer, buflen); } -#ifdef CONFIG_USBHOST_ASYNCH -/**************************************************************************** - * Name: xhci_dma_direct - * - * Description: - * Whether the controller can be pointed straight at this buffer, with no - * stand-in needed: an address it can reach, owning whole cache lines. - * - ****************************************************************************/ - -static bool xhci_dma_direct(FAR struct usbhost_xhci_s *priv, - FAR uint8_t *buffer, size_t buflen) -{ - size_t line = up_get_dcache_linesize(); - - if (!xhci_dmacapable(priv, buffer, buflen)) - { - return false; - } - - return line == 0 || - (((uintptr_t)buffer & (line - 1)) == 0 && - (buflen & (line - 1)) == 0); -} -#endif - /**************************************************************************** * Name: xhci_dma_prepare * @@ -3196,6 +3195,9 @@ static void xhci_transfer_complete(FAR struct usbhost_xhci_s *priv, uint8_t ep = XHCI_TRB_D2_EP_GET(evt->d2); uint8_t ret = XHCI_TRB_D1_CC_GET(evt->d1); irqstate_t flags; +#ifdef CONFIG_USBHOST_ASYNCH + bool asynch = false; +#endif /* Get EP associated with this transfer */ @@ -3257,17 +3259,32 @@ static void xhci_transfer_complete(FAR struct usbhost_xhci_s *priv, } #ifdef CONFIG_USBHOST_ASYNCH - /* No.. Is there a pending asynchronous transfer? */ + /* No.. Is there a pending asynchronous transfer instead? Decide while + * still holding the lock: the moment the waiter above is posted, the + * endpoint may be given a new transfer, and that one is not complete. + */ - else if (epinfo->callback != NULL) + else { - /* Yes.. perform the callback */ - - xhci_asynch_completion(epinfo); + asynch = epinfo->callback != NULL; } #endif spin_unlock_irqrestore(&priv->spinlock, flags); + +#ifdef CONFIG_USBHOST_ASYNCH + /* The callback runs outside the spinlock: it is class driver code, and + * what it does (queue work, take its own locks) has no business running + * with interrupts masked. + */ + + if (asynch) + { + /* Perform the callback */ + + xhci_asynch_completion(priv, epinfo); + } +#endif } /**************************************************************************** @@ -4692,19 +4709,6 @@ static int xhci_asynch(FAR struct usbhost_driver_s *drvr, usbhost_ep_t ep, DEBUGASSERT(priv && rhport && epinfo && buffer && buflen > 0); - /* An asynchronous transfer has no caller to come back to, so a buffer - * needing a stand-in cannot be used: the copy back out of it would have - * to happen in the completion handler, which runs in a work queue thread - * where a caller's address means nothing. The callers of this are class - * drivers using kernel memory, which needs no stand-in. - */ - - if (!xhci_dma_direct(priv, buffer, buflen)) - { - uerr("ERROR: asynchronous transfer needs a directly usable buffer\n"); - return -EFAULT; - } - /* We must have exclusive access to the xHCI hardware and data * structures. */ @@ -4717,7 +4721,7 @@ static int xhci_asynch(FAR struct usbhost_driver_s *drvr, usbhost_ep_t ep, /* Set the request for the callback well BEFORE initiating the transfer. */ - ret = xhci_ioc_async_setup(rhport, epinfo, callback, arg); + ret = xhci_ioc_async_setup(rhport, epinfo, buflen, callback, arg); if (ret != OK) { goto errout_with_lock; @@ -4857,9 +4861,13 @@ static int xhci_cancel(FAR struct usbhost_driver_s *drvr, usbhost_ep_t ep) else { - /* Yes.. perform the callback */ + /* Yes.. give back any stand-in buffer, then perform the callback. + * The endpoint has been stopped, so the controller is no longer + * writing into it. + */ DEBUGASSERT(callback != NULL); + xhci_dma_finish(epinfo); callback(arg, -ESHUTDOWN); } #endif From 1e65485562be0df7815821c5d361de51195e6a24 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sat, 8 Aug 2026 11:55:27 +0800 Subject: [PATCH 19/21] drivers/usbhost: Serialise xHCI transfers per endpoint. 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 --- drivers/usbhost/usbhost_xhci.c | 43 ++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index c8b4d542eab38..23e19e7a7301a 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -160,6 +160,16 @@ struct xhci_epinfo_s size_t dmacopy; /* Length to copy back out of a stand-in */ 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 + * released while a transfer is in flight, so it cannot serve this: two + * threads would each set up a transfer on the same endpoint and the + * second would find iocwait already set. A device's default control + * endpoint is the one that meets this, since every interface driver on + * a composite device speaks through it. + */ + + mutex_t exclsem; /* Serialises transfers on this endpoint */ #ifdef CONFIG_USBHOST_ASYNCH usbhost_asynch_t callback; /* Transfer complete callback */ FAR void *arg; /* Argument that accompanies the callback */ @@ -3993,6 +4003,7 @@ static int xhci_epalloc(FAR struct usbhost_driver_s *drvr, #endif epinfo->xfrtype = epdesc->xfrtype; nxsem_init(&epinfo->iocsem, 0, 0); + nxmutex_init(&epinfo->exclsem); /* xhci_epno_get() returns Device Context Index (DCI) */ @@ -4008,6 +4019,7 @@ static int xhci_epalloc(FAR struct usbhost_driver_s *drvr, if (dev == NULL) { uerr("no device on port %d\n", RHPNDX(rhport)); + nxmutex_destroy(&epinfo->exclsem); nxsem_destroy(&epinfo->iocsem); kmm_free(epinfo); return -ENODEV; @@ -4160,6 +4172,8 @@ static int xhci_epfree(FAR struct usbhost_driver_s *drvr, usbhost_ep_t ep) /* Free the container */ + nxmutex_destroy(&epinfo->exclsem); + nxsem_destroy(&epinfo->iocsem); kmm_free(epinfo); return OK; } @@ -4404,6 +4418,17 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, DEBUGASSERT(rhport != NULL && ep0info != NULL && req != NULL); + /* One request at a time on this endpoint. Taken before the controller + * lock and held across the wait, so the ordering is always endpoint then + * controller and never the reverse. + */ + + ret = nxmutex_lock(&ep0info->exclsem); + if (ret < 0) + { + return ret; + } + len = xhci_getle16(req->len); /* Terse output only if we are tracing */ @@ -4446,6 +4471,7 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, xhci_out_slot(rhport->dev->ctx)->ctx[3]; } + nxmutex_unlock(&ep0info->exclsem); return OK; } @@ -4456,6 +4482,7 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, ret = nxmutex_lock(&priv->lock); if (ret < 0) { + nxmutex_unlock(&ep0info->exclsem); return ret; } @@ -4486,12 +4513,14 @@ static int xhci_ctrl_xfer(FAR struct usbhost_driver_s *drvr, xhci_dma_finish(ep0info); + nxmutex_unlock(&ep0info->exclsem); return nbytes >= 0 ? OK : (int)nbytes; errout_with_iocwait: ep0info->iocwait = false; errout_with_lock: nxmutex_unlock(&priv->lock); + nxmutex_unlock(&ep0info->exclsem); return ret; } @@ -4584,6 +4613,16 @@ static ssize_t xhci_transfer(FAR struct usbhost_driver_s *drvr, DEBUGASSERT(priv && rhport && epinfo && buffer && buflen > 0); + /* One transfer at a time on this endpoint, taken before the controller + * lock and held across the wait. See the note beside exclsem. + */ + + ret = nxmutex_lock(&epinfo->exclsem); + if (ret < 0) + { + return (ssize_t)ret; + } + /* We must have exclusive access to the xHCI hardware and data * structures. */ @@ -4591,6 +4630,7 @@ static ssize_t xhci_transfer(FAR struct usbhost_driver_s *drvr, ret = nxmutex_lock(&priv->lock); if (ret < 0) { + nxmutex_unlock(&epinfo->exclsem); return (ssize_t)ret; } @@ -4652,12 +4692,14 @@ static ssize_t xhci_transfer(FAR struct usbhost_driver_s *drvr, xhci_dma_finish(epinfo); + nxmutex_unlock(&epinfo->exclsem); return nbytes; errout_with_iocwait: epinfo->iocwait = false; errout_with_lock: nxmutex_unlock(&priv->lock); + nxmutex_unlock(&epinfo->exclsem); return (ssize_t)ret; } @@ -5351,6 +5393,7 @@ static inline int xhci_sw_initialize(FAR struct usbhost_xhci_s *priv) rhport->ep0.epno = 0; rhport->ep0.devaddr = 0; nxsem_init(&rhport->ep0.iocsem, 0, 0); + nxmutex_init(&rhport->ep0.exclsem); /* Initialize the public port representation */ From 471a04c1a3b3ab6278995add5d8061f500254389 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sat, 8 Aug 2026 12:37:50 +0800 Subject: [PATCH 20/21] drivers/usbhost: Release the xHCI slot when enumeration fails. 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 --- drivers/usbhost/usbhost_xhci.c | 42 ++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 23e19e7a7301a..25603cae07dee 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -1810,7 +1810,15 @@ static int xhci_device_init(FAR struct usbhost_xhci_s *priv, ret = xhci_cmd_sloten(priv, &slot); if (ret < 0 || slot > priv->no_slots) { - /* Something goes wrong ! */ + /* A slot the controller cannot address is no more usable than no + * slot at all, and the command itself succeeds in that case, so the + * caller needs an error either way. + */ + + if (ret >= 0) + { + ret = -EINVAL; + } usbhost_vtrace1(XHCI_TRACE1_SLOTEN_FAILED, ret); return ret; @@ -1834,7 +1842,7 @@ static int xhci_device_init(FAR struct usbhost_xhci_s *priv, if (ret < 0) { uerr("ep0 ring init failed\n"); - return ret; + goto errout_with_slot; } rhport->ep0.slot = slot; @@ -1845,7 +1853,7 @@ static int xhci_device_init(FAR struct usbhost_xhci_s *priv, ret = xhci_slot_init(priv, dev); if (ret < 0) { - return ret; + goto errout_with_slot; } /* Step 6: Assign and address to the device and enable its Default @@ -1860,12 +1868,21 @@ static int xhci_device_init(FAR struct usbhost_xhci_s *priv, if (ret < 0) { uerr("failed to set address %d\n", ret); - return ret; + goto errout_with_slot; } /* Steps 7-12 don't belong here! */ return OK; + +errout_with_slot: + + /* Nothing else gives the slot back, and the controller has a fixed + * number of them. + */ + + xhci_device_deinit(priv, rhport); + return ret; } /**************************************************************************** @@ -3770,6 +3787,23 @@ static int xhci_enumerate(FAR struct usbhost_connection_s *conn, { /* Failed to enumerate */ + /* The device is addressed by now, so it holds a slot, and the retry + * below asks for another. + */ + +#ifdef CONFIG_USBHOST_HUB + if (ROOTHUB(hport)) +#endif + { + FAR struct usbhost_xhci_s *priv = XHCI_PRIV_FROM_CONN(conn); + FAR struct xhci_rhport_s *rhport = &priv->rhport[hport->port]; + + if (rhport->dev != NULL) + { + xhci_device_deinit(priv, rhport); + } + } + /* If this is a root hub port, then marking the hub port not connected * will cause xhci_wait() to return and we will try the connection * again. From 5e77cd2efa572882914a173af45df53c9174f134 Mon Sep 17 00:00:00 2001 From: Justin Hammond Date: Sat, 8 Aug 2026 12:55:35 +0800 Subject: [PATCH 21/21] drivers/usbhost: Stop retrying an xHCI port that will not enumerate. 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 --- drivers/usbhost/Kconfig | 16 ++++++++++++++++ drivers/usbhost/usbhost_xhci.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/drivers/usbhost/Kconfig b/drivers/usbhost/Kconfig index ccc6e63022d33..fbde6e51f59e6 100644 --- a/drivers/usbhost/Kconfig +++ b/drivers/usbhost/Kconfig @@ -832,6 +832,22 @@ config USBHOST_XHCI_MAX_DEVS ---help--- How many USB devices will be supported by xHCI driver. +config USBHOST_XHCI_ENUM_RETRIES + int "xHCI enumeration attempts per port" + default 3 + range 1 255 + ---help--- + How many times to attempt enumeration of a newly connected device + before leaving the port alone until the device is unplugged. + + A device whose descriptors cannot be read, or that no class driver + claims, fails enumeration every time. Each failure marks the port + disconnected so the attempt repeats, so without a limit such a + device is retried for as long as it stays plugged in, logging and + taking a device slot on every pass. + + The count is per root hub port and is cleared by a new connection. + endif # USBHOST_XHCI menuconfig USBHOST_XHCI_PCI diff --git a/drivers/usbhost/usbhost_xhci.c b/drivers/usbhost/usbhost_xhci.c index 25603cae07dee..261ece9ce047c 100644 --- a/drivers/usbhost/usbhost_xhci.c +++ b/drivers/usbhost/usbhost_xhci.c @@ -199,6 +199,7 @@ struct xhci_rhport_s /* Root hub port status */ bool connected; /* Connected to device */ + uint8_t enumfail; /* Consecutive failed enumerations */ int8_t slot; /* Slot ID associated with this port */ struct xhci_epinfo_s ep0; /* EP0 endpoint info */ struct usbhost_roothubport_s hport; /* This is the hub port description understood @@ -2882,6 +2883,12 @@ static void xhci_portsc_work(FAR void *arg) rhport->connected = true; + /* A new device gets the full allowance of attempts, + * whatever the last one that sat here managed. + */ + + rhport->enumfail = 0; + usbhost_vtrace2(XHCI_VTRACE2_PORTSC_CONNECTED, rhpndx + 1, priv->pscwait); @@ -3802,6 +3809,18 @@ static int xhci_enumerate(FAR struct usbhost_connection_s *conn, { xhci_device_deinit(priv, rhport); } + + /* Clearing connected below is what makes xhci_wait() return, + * so it is also what repeats the attempt. Leave the port alone + * past the limit; a new connection clears the count. + */ + + if (++rhport->enumfail >= CONFIG_USBHOST_XHCI_ENUM_RETRIES) + { + syslog(LOG_ERR, "%s: port %d: giving up after %d attempts\n", + priv->name, hport->port + 1, rhport->enumfail); + return ret; + } } /* If this is a root hub port, then marking the hub port not connected @@ -3811,6 +3830,17 @@ static int xhci_enumerate(FAR struct usbhost_connection_s *conn, hport->connected = false; } + else + { +#ifdef CONFIG_USBHOST_HUB + if (ROOTHUB(hport)) +#endif + { + FAR struct usbhost_xhci_s *priv = XHCI_PRIV_FROM_CONN(conn); + + priv->rhport[hport->port].enumfail = 0; + } + } return ret; }