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);