From cbef64ebf9de98f053fc73bc43b61147cb077247 Mon Sep 17 00:00:00 2001 From: Ricard Rosson Date: Sat, 18 Jul 2026 13:42:12 +0100 Subject: [PATCH] drivers/usbdev/cdcncm: fix TX corruption/wedge under write buffers Two related defects corrupt CDC-NCM transmit once TCP write buffers make TX bursty (a single txavail poll drains many queued segments back-to-back through cdcncm_send): 1. Buffer-reuse race. cdcncm coalesces datagrams into the single pre-allocated wrreq->buf that the USB controller transmits directly from, but cdcncm_send formatted a new NTB batch into it (cdcncm_transmit_format) without first waiting for the previous transfer to complete -- the wrreq_idle wait happened only later, in cdcncm_transmit_work. A new batch started while the previous NTB was still in flight overwrote the in-flight buffer, so the host dropped the corrupted NTB and TX could wedge (wrreq_idle never reposted). Fix: acquire wrreq_idle in cdcncm_send when starting a new batch (dgramcount == 0), before formatting; drop the now-redundant wait in cdcncm_transmit_work (a second wait on the init-to-1 semaphore would deadlock). 2. Concurrent transmit_work. cdcncm_send runs under the recursive netdev_lock and calls cdcncm_transmit_work() synchronously in the buffer-full branch, while a scheduled delaywork instance runs cdcncm_transmit_work() on ETHWORK -- two different threads. Two EP_SUBMITs of the one wrreq corrupt the IN request queue and leave the IN buffer prepared-but-unarmed (controller idle, wrreq_idle never reposted). Fix: wrap cdcncm_transmit_work in netdev_lock (the synchronous caller already holds this recursive nxrmutex; a delaywork instance blocks until the drain releases it), and add an empty-batch guard (dgramcount == 0 -> return) so a delaywork that runs after a synchronous flush emptied the batch does not seal an empty NTB and double-submit the in-flight wrreq. Validated on RP2350 (Pico 2 W) with CONFIG_NET_TCP_WRITE_BUFFERS=y as part of the complete fix set: 144 dense/concurrent HTTP downloads, zero wedges, ~486 KB/s (previously transmit hung within a few requests). On RP2350 full stability under maximal TX density additionally requires a memory barrier between the BUFF_STATUS clear and the AVAILABLE re-arm in the Cortex-M33 USB device driver (a separate change); these cdcncm defects are real and the fixes correct independent of it. Signed-off-by: Ricard Rosson Assisted-by: Claude (Anthropic Claude Code) Signed-off-by: Ricard Rosson --- drivers/usbdev/cdcncm.c | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/drivers/usbdev/cdcncm.c b/drivers/usbdev/cdcncm.c index 5407c8ff72f..d9fa738c600 100644 --- a/drivers/usbdev/cdcncm.c +++ b/drivers/usbdev/cdcncm.c @@ -918,14 +918,26 @@ static void cdcncm_transmit_work(FAR void *arg) int ndpindex; int totallen; - /* Wait until the USB device request for Ethernet frame transmissions - * becomes available. + /* Serialise against cdcncm_send() and any other transmit_work: they share + * the single wrreq buffer and run under the recursive netdev_lock. + * Without it, delay-0 scheduling can submit the same wrreq twice and + * wedge TX. */ - while (nxsem_wait(&self->wrreq_idle) != OK) + netdev_lock(&self->dev.netdev); + + /* Empty batch: a previous flush already submitted it. Don't resubmit. */ + + if (self->dgramcount == 0) { + netdev_unlock(&self->dev.netdev); + return; } + /* cdcncm_send() already holds the wrreq_idle token for this batch, so we + * must not wait for it again here (wrcomplete reposts it after EP_SUBMIT). + */ + ncblen = opts->nthsize; ndpindex = NCM_ALIGN(ncblen, ndpalign); @@ -950,6 +962,8 @@ static void cdcncm_transmit_work(FAR void *arg) self->wrreq->len = totallen; EP_SUBMIT(self->epbulkin, self->wrreq); + + netdev_unlock(&self->dev.netdev); } /**************************************************************************** @@ -1291,6 +1305,21 @@ static int cdcncm_send(FAR struct netdev_lowerhalf_s *dev, FAR netpkt_t *pkt) FAR struct cdcncm_driver_s *self; self = container_of(dev, struct cdcncm_driver_s, dev); + + /* At the start of a new NTB batch, wait for the previous transfer to + * finish before reusing wrreq->buf (the USB controller transmits straight + * out of it). With TCP write buffers, cdcncm_send() drains many segments + * back-to-back, so batches overlap; waiting only just before EP_SUBMIT let + * the in-flight buffer be overwritten and wedged TX. + */ + + if (self->dgramcount == 0) + { + while (nxsem_wait(&self->wrreq_idle) != OK) + { + } + } + cdcncm_transmit_format(self, pkt); netpkt_free(dev, pkt, NETPKT_TX);