drivers/usbdev/cdcncm: fix TX corruption/wedge under write buffers#19470
drivers/usbdev/cdcncm: fix TX corruption/wedge under write buffers#19470ricardgb wants to merge 1 commit into
Conversation
linguini1
left a comment
There was a problem hiding this comment.
Commits cannot be co-authored by AI. Please use the Assisted-by field as per the contribution guide.
|
On-hardware follow-up: stress testing on RP2350 (Pico 2 W) shows this fix is necessary but not sufficient. With write buffers on, the first ~5 large transfers run at ~486 KB/s, then TX still wedges and stays dead (25-download stress run: 5 OK, then 000). Converting to draft — the buffer-reuse serialization here is correct, but a residual wedge (likely the bulk-IN/ZLP completion path under dense TX) remains under investigation before write buffers can be enabled. |
a46a336 to
6954c46
Compare
Sorry, I changed the commit accordingly. If it's more convenient I can file bug reports instead of PRs in the future. |
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)
6954c46 to
8650000
Compare
|
Updated and marked ready: expanded to also fix the concurrent cdcncm_transmit_work double-submit (serialized under netdev_lock + empty-batch guard), which was the second half of the write-buffer wedge. On-hardware soak now clean at 144 dense/concurrent requests. See the note re: the companion RP2350 Cortex-M33 memory-barrier fix. |
Summary
Two related defects corrupt CDC-NCM transmit once TCP write buffers make TX bursty
(a single
txavailpoll drains many queued segments back-to-back throughcdcncm_send). This PR fixes both.1. Buffer-reuse race.
cdcncmcoalesces datagrams into the singlepre-allocated
wrreq->bufthat the USB controller transmits directly from, butcdcncm_sendformatted a new NTB batch into it viacdcncm_transmit_format()without first waiting for the previous transfer to complete — the
wrreq_idlewait happened only later, in
cdcncm_transmit_work(). A new batch started whilethe previous NTB was still in flight overwrote the in-flight buffer, so the host
dropped the corrupted NTB and TX could wedge (
wrreq_idlenever reposted).Fix: acquire
wrreq_idleincdcncm_sendwhen starting a new batch(
dgramcount == 0), before formatting; drop the now-redundant wait incdcncm_transmit_work(a second wait on the init-to-1 semaphore would deadlock).2. Concurrent
transmit_work.cdcncm_sendruns under the recursivenetdev_lockand callscdcncm_transmit_work()synchronously in the buffer-fullbranch, while a scheduled
delayworkinstance runscdcncm_transmit_work()onETHWORK — two different threads. Two
EP_SUBMITs of the onewrreqcorrupt theIN request queue and leave the IN buffer prepared-but-unarmed (controller idle,
wrreq_idlenever reposted).Fix: wrap
cdcncm_transmit_workinnetdev_lock(the synchronous calleralready holds this recursive
nxrmutex; adelayworkinstance blocks until thedrain releases it), plus an empty-batch guard (
dgramcount == 0→ return) so adelayworkthat runs after a synchronous flush emptied the batch doesn't seal anempty NTB and double-submit the in-flight
wrreq.Testing
Validated on RP2350 (Pico 2 W) with
CONFIG_NET_TCP_WRITE_BUFFERS=y, as part ofthe complete fix set: 144 dense/concurrent HTTP downloads, zero wedges, ~486
KB/s — previously transmit hung within a few requests. Each fix was confirmed
against the specific frozen-state failure signature captured over SWD (corrupted /
prepared-but-unarmed IN buffer, controller idle).
Note: on RP2350, full stability under maximal TX density additionally requires a
memory barrier between the
BUFF_STATUSclear and theAVAILABLEre-arm in theCortex-M33 USB device driver (a separate change, not included here). The two
cdcncmdefects fixed here are real and the fixes correct independent of thatbarrier.
Disclosure: this change and its analysis were prepared with the assistance of an
AI agent (Anthropic's Claude, via Claude Code), and validated on-hardware by a
human maintainer before submission.