Skip to content

drivers/usbdev/cdcncm: fix TX corruption/wedge under write buffers#19470

Open
ricardgb wants to merge 1 commit into
apache:masterfrom
ricardgb:cdcncm-tx-buffer-reuse
Open

drivers/usbdev/cdcncm: fix TX corruption/wedge under write buffers#19470
ricardgb wants to merge 1 commit into
apache:masterfrom
ricardgb:cdcncm-tx-buffer-reuse

Conversation

@ricardgb

@ricardgb ricardgb commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

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). This PR fixes both.

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 via 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), plus an empty-batch guard (dgramcount == 0 → return) so a
delaywork that runs after a synchronous flush emptied the batch doesn't seal an
empty NTB and double-submit the in-flight wrreq.

Testing

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. 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_STATUS clear and the AVAILABLE re-arm in the
Cortex-M33 USB device driver (a separate change, not included here). The two
cdcncm defects fixed here are real and the fixes correct independent of that
barrier.


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.

@ricardgb
ricardgb requested a review from Donny9 as a code owner July 18, 2026 12:44
@github-actions github-actions Bot added Size: S The size of the change in this PR is small Area: USB labels Jul 18, 2026
@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

@linguini1 linguini1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commits cannot be co-authored by AI. Please use the Assisted-by field as per the contribution guide.

@ricardgb
ricardgb marked this pull request as draft July 18, 2026 13:07
@ricardgb

Copy link
Copy Markdown
Contributor Author

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.

@ricardgb
ricardgb force-pushed the cdcncm-tx-buffer-reuse branch from a46a336 to 6954c46 Compare July 18, 2026 13:10
@ricardgb

Copy link
Copy Markdown
Contributor Author

Commits cannot be co-authored by AI. Please use the Assisted-by field as per the contribution guide.

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)
@ricardgb ricardgb changed the title drivers/usbdev/cdcncm: fix TX buffer-reuse race that corrupts NTBs and wedges transmit drivers/usbdev/cdcncm: fix TX corruption/wedge under write buffers Jul 18, 2026
@ricardgb
ricardgb force-pushed the cdcncm-tx-buffer-reuse branch from 6954c46 to 8650000 Compare July 18, 2026 14:15
@ricardgb
ricardgb marked this pull request as ready for review July 18, 2026 14:15
@ricardgb

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: USB Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants