From 86500002e5de0439507aeeff2367bc29f0ff5819 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) --- drivers/usbdev/cdcncm.c | 70 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/drivers/usbdev/cdcncm.c b/drivers/usbdev/cdcncm.c index d528d8f0da4c2..338a1a06fd9a5 100644 --- a/drivers/usbdev/cdcncm.c +++ b/drivers/usbdev/cdcncm.c @@ -918,14 +918,51 @@ 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 this transmit against cdcncm_send() and against any other + * cdcncm_transmit_work() invocation. cdcncm_send() runs under netdev_lock + * (the recursive per-device d_lock) for the whole tx drain; re-taking the + * same lock here guarantees that: + * + * - a delaywork instance running on ETHWORK cannot seal and EP_SUBMIT the + * NTB while cdcncm_send() is still formatting datagrams into wrreq->buf + * on the net thread (dgramcount / dgramaddr / the buffer would tear), + * and + * - the synchronous buffer-full flush (called from cdcncm_send() under + * this very lock -- a recursive re-acquire) can never overlap a + * delaywork instance. + * + * Without this, delay-0 scheduling (the low-latency RTT path) lets two + * cdcncm_transmit_work() bodies run on different threads, EP_SUBMIT the + * single wrreq twice, corrupt the USB driver's IN request queue and leave + * the IN buffer prepared-but-unarmed (buffer-control shows LEN/PID but + * AVAILABLE clear) -- TX then wedges permanently (wrreq_idle is never + * reposted). netdev_lock is a recursive nxrmutex, so the synchronous + * (already-locked) caller re-enters it safely. + */ + + netdev_lock(&self->dev.netdev); + + /* Nothing to send means a previous flush (typically the synchronous + * buffer-full path in cdcncm_send) already emptied and submitted this + * batch. Bail out rather than seal an empty NTB and EP_SUBMIT the still + * in-flight wrreq a second time. */ - while (nxsem_wait(&self->wrreq_idle) != OK) + if (self->dgramcount == 0) { + netdev_unlock(&self->dev.netdev); + return; } + /* Ownership of the single wrreq->buf (the wrreq_idle token) has already + * been taken by cdcncm_send() when this NTB batch was started + * (self->dgramcount transitioned from 0), guaranteeing the previous + * transmission has completed. Do not wait again here: the token is a + * counting semaphore initialised to 1, so a second wait would block + * forever (it is only reposted by cdcncm_wrcomplete after the EP_SUBMIT + * below). + */ + ncblen = opts->nthsize; ndpindex = NCM_ALIGN(ncblen, ndpalign); @@ -950,6 +987,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 +1330,31 @@ 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); + + /* Starting a new NTB batch? The driver coalesces several datagrams into + * the single pre-allocated wrreq->buf. That buffer may still be in-flight + * from the previous transmission (EP_SUBMIT'ed, wrcomplete not yet run): + * the USB controller transmits directly out of wrreq->buf, so we must not + * begin formatting new data into it until the hardware has finished with + * it. Take ownership of the buffer here (waiting for the previous + * transfer to complete) instead of only just before EP_SUBMIT. + * + * This matters for TCP write buffers: a single txavail poll drains many + * queued segments back-to-back through cdcncm_send(), so a full NTB can be + * submitted and the next batch started while the first is still on the + * wire. Without this guard the in-flight buffer gets overwritten, the + * host drops the corrupted NTB, and the transfer can wedge (wrreq_idle is + * never reposted), permanently stalling all TX. The unbuffered send path + * transmits one segment per blocking send() and so never overlaps. + */ + + if (self->dgramcount == 0) + { + while (nxsem_wait(&self->wrreq_idle) != OK) + { + } + } + cdcncm_transmit_format(self, pkt); netpkt_free(dev, pkt, NETPKT_TX);