From a943ee5918a89cda5cddf9d70810de540e33b281 Mon Sep 17 00:00:00 2001 From: Ricard Rosson Date: Wed, 15 Jul 2026 21:01:19 +0100 Subject: [PATCH] rp2040/rp23xx: implement stall queueing and GET STATUS responses Two device-controller defects that break standard host error recovery, found while root-causing why macOS never mounts a NuttX mass-storage function (full analysis and host traces in apache/nuttx#19435): 1. epsubmit() aborted (-EBUSY) any IN request submitted while the endpoint was halted. The mass storage class halts bulk-IN for a failed device-to-host command (BOT-legal) and then submits the CSW; the CSW was dropped on the floor, so after the host's Clear-Halt the endpoint NAKed forever and the host timed out (macOS: 30 s, then a Bulk-Only-reset/device-reset spiral until it disables the port). This is exactly the race documented for years in the usbmsc_scsi.c header (David Hewson's analysis); the USBMSC_STALL_RACEWAR sleep workaround only survives hosts that clear the halt within 100 ms -- Linux does, macOS does not, which is why Linux testing never saw it. Implement the stall-queueing contract instead: requests submitted while an endpoint is halted are queued without arming the hardware (arming rewrites the buffer control word and would silently clear the STALL bit); a halt terminates any in-flight IN transfer (its hardware buffer is disarmed by the STALL write and would never complete); clearing the halt resets the data toggle and re-arms the head of the queue; stale buffer completions latched for transfers aborted by a halt are ignored. RP2040/RP23XX now select ARCH_USBDEV_STALLQUEUE, which also retires the RACEWAR's two 100 ms sleeps per failed command. 2. The USB_REQ_GETSTATUS handler in ep0setup() validated the request but never queued the two-byte response, for all three recipients (device/interface/endpoint), so EP0 NAKed the host's data stage forever and every GET STATUS timed out. macOS issues GetPipeStatus = GET STATUS(endpoint) on a halted pipe before running Bulk-Only reset recovery and hit this on every probe; lsusb -v's device-status query hangs on it as well. Send the response: endpoint recipient reports the halt bit, device recipient reports self-powered, interface reports zeros; the status stage is armed by handle_zlp() exactly as for class-dispatched IN transfers. Validated on RP2350 silicon (Raspberry Pi Pico 2 W, composite CDC-ACM + CDC-NCM + USBMSC): pre-fix, a raw-usbfs replay of macOS's sequence and timing reproduced both defects deterministically (CSW read ETIMEDOUT after a delayed clear-halt; all GET STATUS variants ETIMEDOUT). Post-fix: the CSW survives the halt and is delivered after Clear-Halt with correct tag/status/residue across a post-stall delay sweep of 0-1000 ms; all GET STATUS variants answer immediately with correct halt reporting; no regressions in the exact-length SCSI suite, Bulk-Only reset, FAT mount and full reads, CDC-NCM/ACM, or warm reboots; and macOS now mounts the volume (together with the companion usbmsc fixes). The rp2040 driver shares the code and receives the identical fix (build-tested). Assisted-by: Claude (Anthropic Claude Code) Signed-off-by: Ricard Rosson --- arch/arm/Kconfig | 2 + arch/arm/src/rp2040/rp2040_usbdev.c | 131 +++++++++++++++++++--------- arch/arm/src/rp23xx/rp23xx_usbdev.c | 131 +++++++++++++++++++--------- 3 files changed, 180 insertions(+), 84 deletions(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index b107f0ab91d2f..0c7f2b6d81c9e 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -381,6 +381,7 @@ config ARCH_CHIP_RP2040 select ARM_HAVE_WFE_SEV select ARCH_BOARD_COMMON select ARCH_HAVE_CUSTOM_TESTSET + select ARCH_USBDEV_STALLQUEUE if USBDEV ---help--- Raspberry Pi RP2040 architectures (ARM dual Cortex-M0+). @@ -395,6 +396,7 @@ config ARCH_CHIP_RP23XX select ARCH_HAVE_FPU select ARCH_HAVE_CUSTOM_TESTSET select ARCH_BOARD_COMMON + select ARCH_USBDEV_STALLQUEUE if USBDEV ---help--- Raspberry Pi RP23XX architectures (ARM dual Cortex-M33 or RISC-V). diff --git a/arch/arm/src/rp2040/rp2040_usbdev.c b/arch/arm/src/rp2040/rp2040_usbdev.c index 470f32567ed39..1c96211544212 100644 --- a/arch/arm/src/rp2040/rp2040_usbdev.c +++ b/arch/arm/src/rp2040/rp2040_usbdev.c @@ -318,9 +318,6 @@ static void rp2040_update_buffer_control(struct rp2040_ep_s *privep, static int rp2040_epwrite(struct rp2040_ep_s *privep, uint8_t *buf, uint16_t nbytes); static int rp2040_epread(struct rp2040_ep_s *privep, uint16_t nbytes); -static void rp2040_abortrequest(struct rp2040_ep_s *privep, - struct rp2040_req_s *privreq, - int16_t result); static void rp2040_reqcomplete(struct rp2040_ep_s *privep, int16_t result); static void rp2040_txcomplete(struct rp2040_ep_s *privep); static int rp2040_wrrequest(struct rp2040_ep_s *privep); @@ -586,30 +583,6 @@ static int rp2040_epread(struct rp2040_ep_s *privep, uint16_t nbytes) return OK; } -/**************************************************************************** - * Name: rp2040_abortrequest - * - * Description: - * Discard a request - * - ****************************************************************************/ - -static void rp2040_abortrequest(struct rp2040_ep_s *privep, - struct rp2040_req_s *privreq, - int16_t result) -{ - usbtrace(TRACE_DEVERROR(RP2040_TRACEERR_REQABORTED), - (uint16_t)privep->epphy); - - /* Save the result in the request structure */ - - privreq->req.result = result; - - /* Callback to the request completion handler */ - - privreq->req.callback(&privep->ep, &privreq->req); -} - /**************************************************************************** * Name: rp2040_reqcomplete * @@ -1052,6 +1025,11 @@ static void rp2040_ep0setup(struct rp2040_usbdev_s *priv) } else { + uint8_t response[2]; + + response[0] = 0; + response[1] = 0; + switch (priv->ctrl.type & USB_REQ_RECIPIENT_MASK) { case USB_REQ_RECIPIENT_ENDPOINT: @@ -1068,10 +1046,23 @@ static void rp2040_ep0setup(struct rp2040_usbdev_s *priv) priv->ctrl.type); priv->stalled = true; } + else if (privep->stalled) + { + response[0] = 1; /* Endpoint HALTed */ + } } break; case USB_REQ_RECIPIENT_DEVICE: + usbtrace(TRACE_INTDECODE( + RP2040_TRACEINTID_GETIFDEV), + 0); + if (priv->selfpowered) + { + response[0] = 1 << USB_FEATURE_SELFPOWERED; + } + break; + case USB_REQ_RECIPIENT_INTERFACE: usbtrace(TRACE_INTDECODE( RP2040_TRACEINTID_GETIFDEV), @@ -1087,6 +1078,17 @@ static void rp2040_ep0setup(struct rp2040_usbdev_s *priv) } break; } + + /* Send the two-byte endpoint status. Without a data + * stage EP0 NAKs the host's IN forever; macOS queries + * GET_STATUS on a halted bulk pipe during Bulk-Only reset + * recovery, so this is required for MSC error recovery. + */ + + if (!priv->stalled) + { + rp2040_epwrite(ep0, response, 2); + } } } break; @@ -1394,7 +1396,15 @@ static bool rp2040_usbintr_buffstat(struct rp2040_usbdev_s *priv) if (privep->in) { - if (!rp2040_rqempty(privep)) + if (privep->epphy != 0 && privep->stalled) + { + /* Completion latched for a transfer the halt already + * aborted (canceled by rp2040_epstall); don't + * attribute the stale buffer event to post-halt + * requests. + */ + } + else if (!rp2040_rqempty(privep)) { rp2040_txcomplete(privep); } @@ -1694,24 +1704,22 @@ static int rp2040_epsubmit(struct usbdev_ep_s *ep, flags = enter_critical_section(); - if (privep->stalled && privep->in) - { - rp2040_abortrequest(privep, privreq, -EBUSY); - ret = -EBUSY; - } - /* Handle IN (device-to-host) requests */ - else if (privep->in) + if (privep->in) { - /* Add the new request to the request queue for the IN endpoint */ + /* Queue the request on the IN endpoint. If halted, only queue it; + * it is armed when the halt clears (rp2040_epstall). MSC relies on + * this: the CSW is submitted while the bulk IN is still halted + * (ARCH_USBDEV_STALLQUEUE). + */ bool empty = rp2040_rqempty(privep); rp2040_rqenqueue(privep, privreq); usbtrace(TRACE_INREQQUEUED(privep->epphy), privreq->req.len); - if (empty) + if (empty && !privep->stalled) { rp2040_wrrequest(privep); } @@ -1721,7 +1729,9 @@ static int rp2040_epsubmit(struct usbdev_ep_s *ep, else { - /* Add the new request to the request queue for the OUT endpoint */ + /* Queue on the OUT endpoint. As for IN, don't arm while halted: + * arming rewrites the buffer control word and clears the STALL bit. + */ bool empty = rp2040_rqempty(privep); @@ -1731,7 +1741,7 @@ static int rp2040_epsubmit(struct usbdev_ep_s *ep, /* This there a incoming data pending the availability of a request? */ - if (empty) + if (empty && !privep->stalled) { ret = rp2040_rdrequest(privep); } @@ -1829,14 +1839,17 @@ static int rp2040_epstall(struct usbdev_ep_s *ep, bool resume) { struct rp2040_ep_s *privep = (struct rp2040_ep_s *)ep; struct rp2040_usbdev_s *priv = privep->dev; + irqstate_t irqflags; irqstate_t flags; + irqflags = enter_critical_section(); flags = spin_lock_irqsave(&priv->lock); if (resume) { usbtrace(TRACE_EPRESUME, privep->epphy); privep->stalled = false; + privep->pending_stall = false; if (privep->epphy == 0) { clrbits_reg32(privep->in ? @@ -1849,8 +1862,29 @@ static int rp2040_epstall(struct usbdev_ep_s *ep, bool resume) ~(RP2040_USBCTRL_DPSRAM_EP_BUFF_CTRL_STALL), 0); + /* Halt clearing resets the data toggle to DATA0 (USB 2.0 9.4.5) */ + privep->next_pid = 0; priv->zlp_stat = RP2040_ZLP_NONE; + + spin_unlock_irqrestore(&priv->lock, flags); + + /* Restart any requests that were queued (but not armed) while the + * endpoint was halted -- e.g. the mass storage CSW that concludes + * a failed command (ARCH_USBDEV_STALLQUEUE). + */ + + if (privep->epphy != 0 && !rp2040_rqempty(privep)) + { + if (privep->in) + { + rp2040_wrrequest(privep); + } + else + { + rp2040_rdrequest(privep); + } + } } else { @@ -1861,18 +1895,31 @@ static int rp2040_epstall(struct usbdev_ep_s *ep, bool resume) /* EP0 IN Transfer ongoing : postpone the stall until the end */ privep->pending_stall = true; + priv->zlp_stat = RP2040_ZLP_NONE; + spin_unlock_irqrestore(&priv->lock, flags); } else { /* Stall immediately */ rp2040_epstall_exec_nolock(ep); - } + priv->zlp_stat = RP2040_ZLP_NONE; + spin_unlock_irqrestore(&priv->lock, flags); - priv->zlp_stat = RP2040_ZLP_NONE; + /* Terminate the IN transfer the halt aborted: its buffer was + * disarmed by the STALL write above and would never complete. + * Anything the class resubmits stays queued until the halt + * clears. + */ + + if (privep->epphy != 0 && privep->in && !rp2040_rqempty(privep)) + { + rp2040_cancelrequests(privep); + } + } } - spin_unlock_irqrestore(&priv->lock, flags); + leave_critical_section(irqflags); return OK; } diff --git a/arch/arm/src/rp23xx/rp23xx_usbdev.c b/arch/arm/src/rp23xx/rp23xx_usbdev.c index 357b96eb8d8d9..af7550e754d18 100644 --- a/arch/arm/src/rp23xx/rp23xx_usbdev.c +++ b/arch/arm/src/rp23xx/rp23xx_usbdev.c @@ -318,9 +318,6 @@ static void rp23xx_update_buffer_control(struct rp23xx_ep_s *privep, static int rp23xx_epwrite(struct rp23xx_ep_s *privep, uint8_t *buf, uint16_t nbytes); static int rp23xx_epread(struct rp23xx_ep_s *privep, uint16_t nbytes); -static void rp23xx_abortrequest(struct rp23xx_ep_s *privep, - struct rp23xx_req_s *privreq, - int16_t result); static void rp23xx_reqcomplete(struct rp23xx_ep_s *privep, int16_t result); static void rp23xx_txcomplete(struct rp23xx_ep_s *privep); static int rp23xx_wrrequest(struct rp23xx_ep_s *privep); @@ -585,30 +582,6 @@ static int rp23xx_epread(struct rp23xx_ep_s *privep, uint16_t nbytes) return OK; } -/**************************************************************************** - * Name: rp23xx_abortrequest - * - * Description: - * Discard a request - * - ****************************************************************************/ - -static void rp23xx_abortrequest(struct rp23xx_ep_s *privep, - struct rp23xx_req_s *privreq, - int16_t result) -{ - usbtrace(TRACE_DEVERROR(RP23XX_TRACEERR_REQABORTED), - (uint16_t)privep->epphy); - - /* Save the result in the request structure */ - - privreq->req.result = result; - - /* Callback to the request completion handler */ - - privreq->req.callback(&privep->ep, &privreq->req); -} - /**************************************************************************** * Name: rp23xx_reqcomplete * @@ -1051,6 +1024,11 @@ static void rp23xx_ep0setup(struct rp23xx_usbdev_s *priv) } else { + uint8_t response[2]; + + response[0] = 0; + response[1] = 0; + switch (priv->ctrl.type & USB_REQ_RECIPIENT_MASK) { case USB_REQ_RECIPIENT_ENDPOINT: @@ -1067,10 +1045,23 @@ static void rp23xx_ep0setup(struct rp23xx_usbdev_s *priv) priv->ctrl.type); priv->stalled = true; } + else if (privep->stalled) + { + response[0] = 1; /* Endpoint HALTed */ + } } break; case USB_REQ_RECIPIENT_DEVICE: + usbtrace(TRACE_INTDECODE( + RP23XX_TRACEINTID_GETIFDEV), + 0); + if (priv->selfpowered) + { + response[0] = 1 << USB_FEATURE_SELFPOWERED; + } + break; + case USB_REQ_RECIPIENT_INTERFACE: usbtrace(TRACE_INTDECODE( RP23XX_TRACEINTID_GETIFDEV), @@ -1086,6 +1077,17 @@ static void rp23xx_ep0setup(struct rp23xx_usbdev_s *priv) } break; } + + /* Send the two-byte endpoint status. Without a data + * stage EP0 NAKs the host's IN forever; macOS queries + * GET_STATUS on a halted bulk pipe during Bulk-Only reset + * recovery, so this is required for MSC error recovery. + */ + + if (!priv->stalled) + { + rp23xx_epwrite(ep0, response, 2); + } } } break; @@ -1394,7 +1396,15 @@ static bool rp23xx_usbintr_buffstat(struct rp23xx_usbdev_s *priv) if (privep->in) { - if (!rp23xx_rqempty(privep)) + if (privep->epphy != 0 && privep->stalled) + { + /* Completion latched for a transfer the halt already + * aborted (canceled by rp23xx_epstall); don't + * attribute the stale buffer event to post-halt + * requests. + */ + } + else if (!rp23xx_rqempty(privep)) { rp23xx_txcomplete(privep); } @@ -1696,24 +1706,22 @@ static int rp23xx_epsubmit(struct usbdev_ep_s *ep, flags = enter_critical_section(); - if (privep->stalled && privep->in) - { - rp23xx_abortrequest(privep, privreq, -EBUSY); - ret = -EBUSY; - } - /* Handle IN (device-to-host) requests */ - else if (privep->in) + if (privep->in) { - /* Add the new request to the request queue for the IN endpoint */ + /* Queue the request on the IN endpoint. If halted, only queue it; + * it is armed when the halt clears (rp23xx_epstall). MSC relies on + * this: the CSW is submitted while the bulk IN is still halted + * (ARCH_USBDEV_STALLQUEUE). + */ bool empty = rp23xx_rqempty(privep); rp23xx_rqenqueue(privep, privreq); usbtrace(TRACE_INREQQUEUED(privep->epphy), privreq->req.len); - if (empty) + if (empty && !privep->stalled) { rp23xx_wrrequest(privep); } @@ -1723,7 +1731,9 @@ static int rp23xx_epsubmit(struct usbdev_ep_s *ep, else { - /* Add the new request to the request queue for the OUT endpoint */ + /* Queue on the OUT endpoint. As for IN, don't arm while halted: + * arming rewrites the buffer control word and clears the STALL bit. + */ bool empty = rp23xx_rqempty(privep); @@ -1733,7 +1743,7 @@ static int rp23xx_epsubmit(struct usbdev_ep_s *ep, /* This there a incoming data pending the availability of a request? */ - if (empty) + if (empty && !privep->stalled) { ret = rp23xx_rdrequest(privep); } @@ -1818,14 +1828,17 @@ static int rp23xx_epstall(struct usbdev_ep_s *ep, bool resume) { struct rp23xx_ep_s *privep = (struct rp23xx_ep_s *)ep; struct rp23xx_usbdev_s *priv = privep->dev; + irqstate_t irqflags; irqstate_t flags; + irqflags = enter_critical_section(); flags = spin_lock_irqsave(&g_usbdev.lock); if (resume) { usbtrace(TRACE_EPRESUME, privep->epphy); privep->stalled = false; + privep->pending_stall = false; if (privep->epphy == 0) { clrbits_reg32(privep->in ? @@ -1838,8 +1851,29 @@ static int rp23xx_epstall(struct usbdev_ep_s *ep, bool resume) ~(RP23XX_USBCTRL_DPSRAM_EP_BUFF_CTRL_STALL), 0); + /* Halt clearing resets the data toggle to DATA0 (USB 2.0 9.4.5) */ + privep->next_pid = 0; priv->zlp_stat = RP23XX_ZLP_NONE; + + spin_unlock_irqrestore(&g_usbdev.lock, flags); + + /* Restart any requests that were queued (but not armed) while the + * endpoint was halted -- e.g. the mass storage CSW that concludes + * a failed command (ARCH_USBDEV_STALLQUEUE). + */ + + if (privep->epphy != 0 && !rp23xx_rqempty(privep)) + { + if (privep->in) + { + rp23xx_wrrequest(privep); + } + else + { + rp23xx_rdrequest(privep); + } + } } else { @@ -1850,18 +1884,31 @@ static int rp23xx_epstall(struct usbdev_ep_s *ep, bool resume) /* EP0 IN Transfer ongoing : postpone the stall until the end */ privep->pending_stall = true; + priv->zlp_stat = RP23XX_ZLP_NONE; + spin_unlock_irqrestore(&g_usbdev.lock, flags); } else { /* Stall immediately */ rp23xx_epstall_exec(ep); - } + priv->zlp_stat = RP23XX_ZLP_NONE; + spin_unlock_irqrestore(&g_usbdev.lock, flags); - priv->zlp_stat = RP23XX_ZLP_NONE; + /* Terminate the IN transfer the halt aborted: its buffer was + * disarmed by the STALL write above and would never complete. + * Anything the class resubmits stays queued until the halt + * clears. + */ + + if (privep->epphy != 0 && privep->in && !rp23xx_rqempty(privep)) + { + rp23xx_cancelrequests(privep); + } + } } - spin_unlock_irqrestore(&g_usbdev.lock, flags); + leave_critical_section(irqflags); return OK; }