mirror of
https://github.com/apache/nuttx.git
synced 2026-08-01 20:28:58 +00:00
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 <ricard@groundbits.com> Assisted-by: Claude (Anthropic Claude Code) Signed-off-by: Ricard Rosson <ricard@groundbits.com>
This commit is contained in:
parent
2cb7b7c03e
commit
cbef64ebf9
1 changed files with 32 additions and 3 deletions
|
|
@ -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);
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue