From 475681874424979149c4441f78f33ef02df6caff Mon Sep 17 00:00:00 2001 From: Ricard Rosson Date: Wed, 15 Jul 2026 21:01:19 +0100 Subject: [PATCH] usbmsc: fix composite-mode class requests and Hi>Di stall behavior Three defects that together prevented macOS from ever mounting a composite USBMSC function (Linux was mostly unaffected because its probe sequence and recovery timing never exercised these paths): 1. usbmsc_setup() compared the class-request wIndex against the compile-time constant USBMSC_INTERFACEID (= CONFIG_USBMSC_IFNOBASE, i.e. 0) instead of the composite-assigned priv->devinfo.ifnobase. In composite mode the MSC interface number is nonzero, so GET MAX LUN, Bulk-Only Mass Storage Reset, and GET/SET INTERFACE all failed the index check and stalled EP0. Standalone MSC is unaffected (ifnobase == 0), which is why this went unnoticed. 2. usbmsc_deferredresponse() has its entire body inside #ifndef CONFIG_USBMSC_COMPOSITE, so the deferred EP0 status stage for MSRESET/SETINTERFACE was never sent in composite mode and the host's Bulk-Only reset timed out. (Unreachable before fix 1 -- MSRESET used to stall at the wrong-interface check.) Compile the body in composite mode too, but suppress the worker's deferred response for SETCONFIGURATION there: the composite driver answers that request itself, and a duplicate zero-length packet corrupts the EP0 state. 3. usbmsc_cmdfinishstate() stalled the bulk IN endpoint whenever a device-to-host command left a residue, even when the response had already been sent and terminated by a short packet (or ZLP). The stall is BOT-legal (USB MSC BOT 6.7.2) but gratuitous: the short packet already ended the data phase and the residue is reported in dCSWDataResidue. Hosts such as macOS answer any bulk-IN halt during device probing with a full Bulk-Only reset sequence, which costs seconds per command or aborts the probe entirely (macOS probes MODE SENSE(6) with allocation lengths that exceed the response; Linux's probe does not). Only halt the endpoint when nothing terminated the data phase. Root-cause analysis and host traces in apache/nuttx#19435. Validated on RP2350 silicon (Raspberry Pi Pico 2 W, composite CDC-ACM + CDC-NCM + USBMSC): GET MAX LUN answers 1 LUN (previously EP0 stall and a garbage LUN count on macOS), MSRESET completes 10/10 (previously ETIMEDOUT), MODE SENSE(6) alloc=0xC0 returns short data plus a CSW with dCSWDataResidue and zero bulk-IN stalls across the exact-length suite, and macOS now mounts the volume (together with the companion DCD fixes). Co-Authored-By: Claude Fable 5 Signed-off-by: Ricard Rosson --- drivers/usbdev/usbmsc.c | 10 ++++----- drivers/usbdev/usbmsc_scsi.c | 42 ++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/drivers/usbdev/usbmsc.c b/drivers/usbdev/usbmsc.c index bfac6ff03f9..f668f702da6 100644 --- a/drivers/usbdev/usbmsc.c +++ b/drivers/usbdev/usbmsc.c @@ -741,7 +741,7 @@ static int usbmsc_setup(FAR struct usbdevclass_driver_s *driver, if (ctrl->type == USB_REQ_RECIPIENT_INTERFACE) { if (priv->config == USBMSC_CONFIGID && - index == USBMSC_INTERFACEID && + index == priv->devinfo.ifnobase && value == USBMSC_ALTINTERFACEID) { /* Signal to instantiate the interface change */ @@ -764,7 +764,7 @@ static int usbmsc_setup(FAR struct usbdevclass_driver_s *driver, if (ctrl->type == (USB_DIR_IN | USB_REQ_RECIPIENT_INTERFACE) && priv->config == USBMSC_CONFIGIDNONE) { - if (index != USBMSC_INTERFACEID) + if (index != priv->devinfo.ifnobase) { ret = -EDOM; } @@ -805,7 +805,7 @@ static int usbmsc_setup(FAR struct usbdevclass_driver_s *driver, { /* Only one interface is supported */ - if (index != USBMSC_INTERFACEID) + if (index != priv->devinfo.ifnobase) { usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_MSRESETNDX), index); @@ -836,7 +836,7 @@ static int usbmsc_setup(FAR struct usbdevclass_driver_s *driver, { /* Only one interface is supported */ - if (index != USBMSC_INTERFACEID) + if (index != priv->devinfo.ifnobase) { usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_GETMAXLUNNDX), index); @@ -1280,7 +1280,6 @@ void usbmsc_rdcomplete(FAR struct usbdev_ep_s *ep, void usbmsc_deferredresponse(FAR struct usbmsc_dev_s *priv, bool failed) { -#ifndef CONFIG_USBMSC_COMPOSITE FAR struct usbdev_s *dev; FAR struct usbdev_req_s *ctrlreq; int ret; @@ -1322,7 +1321,6 @@ void usbmsc_deferredresponse(FAR struct usbmsc_dev_s *priv, bool failed) usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_DEFERREDRESPSTALLED), 0); EP_STALL(dev->ep0); } -#endif } /**************************************************************************** diff --git a/drivers/usbdev/usbmsc_scsi.c b/drivers/usbdev/usbmsc_scsi.c index d63478be224..0b2f217d77b 100644 --- a/drivers/usbdev/usbmsc_scsi.c +++ b/drivers/usbdev/usbmsc_scsi.c @@ -2614,6 +2614,8 @@ static int usbmsc_cmdfinishstate(FAR struct usbmsc_dev_s *priv) case USBMSC_FLAGS_DIRDEVICE2HOST: if (priv->cbwlen > 0) { + bool terminated = false; + /* On most commands (the exception is outgoing, write commands), * the data has not yet been sent. */ @@ -2645,11 +2647,35 @@ static int usbmsc_cmdfinishstate(FAR struct usbmsc_dev_s *priv) usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_CMDFINISHSUBMIT), (uint16_t)-ret); } + else + { + /* The request was submitted with USBDEV_REQFLAGS_NULLPKT, + * so the transfer ends with a short packet (or a ZLP if + * the response length is an exact multiple of the packet + * size). Either way the host's Data-In phase terminates + * cleanly when this request completes. + */ + + terminated = true; + } } - /* Stall the BULK In endpoint if there is a residue */ + /* If there is a residue, the host expected more data than we + * sent. If the data phase was already terminated by a short + * packet (or ZLP), nothing more is needed: the host's transfer + * has completed and the residue is reported in the CSW + * (dCSWDataResidue). Stalling here as well is permitted by BOT + * (USB MSC BOT 6.7.2), but it is gratuitous and some hosts + * (macOS) respond with a full Bulk-Only reset sequence to any + * bulk-IN halt during device probing, which costs seconds per + * command or aborts the probe entirely. + * + * Only halt the endpoint when nothing terminated the data + * phase - otherwise the host would mistake the following CSW + * for transfer data. + */ - if (priv->residue > 0) + if (priv->residue > 0 && !terminated) { #ifndef CONFIG_USBMSC_NOT_STALL_BULKEP usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_CMDFINISHRESIDUE), @@ -2975,8 +3001,20 @@ int usbmsc_scsi_main(int argc, FAR char *argv[]) * response */ +#ifdef CONFIG_USBMSC_COMPOSITE + /* In composite mode the composite driver itself responds to + * SETCONFIGURATION (see composite_ep0submit); only the deferred + * responses to MSRESET and SETINTERFACE are owed by this class. + * Submitting a second response for SETCONFIGURATION would corrupt + * the EP0 state. + */ + + if ((eventset & (USBMSC_EVENT_RESET | + USBMSC_EVENT_IFCHANGE)) != 0) +#else if ((eventset & (USBMSC_EVENT_RESET | USBMSC_EVENT_CFGCHANGE | USBMSC_EVENT_IFCHANGE)) != 0) +#endif { usbmsc_deferredresponse(priv, false); }