mirror of
https://github.com/apache/nuttx.git
synced 2026-08-01 20:28:58 +00:00
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 <noreply@anthropic.com> Signed-off-by: Ricard Rosson <ricard@groundbits.com>
This commit is contained in:
parent
53c27e155b
commit
4756818744
2 changed files with 44 additions and 8 deletions
|
|
@ -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
|
||||
}
|
||||
|
||||
/****************************************************************************
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue