arch/rp2040,rp23xx: fix CDC-NCM bulk/notify endpoint handling#19378
arch/rp2040,rp23xx: fix CDC-NCM bulk/notify endpoint handling#19378ricardgb wants to merge 4 commits into
Conversation
rp2040_epread() armed the DPSRAM buffer-control register with the full usbdev request length. That length is only correct for requests no larger than the buffer-control LEN field, which is 10 bits wide (max 1023 bytes). Class drivers that post larger read requests -- e.g. cdcncm allocates a 16 KiB NTB read buffer -- overflow LEN: 16384 & 0x3ff is 0, and the high bits corrupt the neighbouring control flags. The controller then sees a zero-length available buffer and completes the transfer immediately with zero bytes, over and over, so no OUT data is ever received (cdcncm floods "Wrong NTH SIGN, skblen 0"). The receive path already accumulates a request across multiple packets: rp2040_rxcomplete() copies each packet, advances xfrd and re-arms via rp2040_rdrequest() until the request is satisfied or a short packet arrives. So the buffer only ever needs to be armed for a single maximum-size packet. Clamp nbytes accordingly. This matches the transmit path, which already chunks to ep.maxpacket in rp2040_wrrequest. Bulk classes with small reads (cdcacm, usbmsc) were unaffected because their request lengths already fit in LEN, which is why the defect only showed up on cdcncm. Validated on raspberrypi-pico (RP2040): a CONFIG_NET_CDCNCM device that previously received nothing now passes traffic in both directions with 0% packet loss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AHJRvWeBMTHwzpwjaUg4HW Signed-off-by: Ricard Rosson <ricard@groundbits.com>
…llback rp2040_txcomplete() and rp2040_rxcomplete() unconditionally called rp2040_wrrequest()/rp2040_rdrequest() to start the next transfer after invoking a request's completion callback. When that callback resubmits a request on the same, now-idle endpoint -- which cdcncm and rndis do from their interrupt/notify completion handlers -- rp2040_epsubmit() already arms the hardware buffer for it. The unconditional re-arm in the completion path then arms the same buffer a second time, toggling the DATA0/DATA1 PID twice. The host sees a stale PID and silently discards the packet as a retransmission, so e.g. the cdcncm NETWORK_CONNECTION / SPEED_CHANGE notifications never reach the host and the interface stays NO-CARRIER. Track whether a request's hardware buffer has already been armed with a per-request flag (set in rp2040_wrrequest/rp2040_rdrequest, cleared in rp2040_epsubmit) and skip the redundant re-arm when the completion callback has already resubmitted. The in-progress multi-packet case (transfer not yet complete) still continues normally. Validated on raspberrypi-pico (RP2040): the cdcncm interrupt-IN notification is now delivered (confirmed with usbmon) and the host brings the link up; previously it never was. This is also the likely cause of the long-standing rndis control-response timeout on this controller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AHJRvWeBMTHwzpwjaUg4HW Signed-off-by: Ricard Rosson <ricard@groundbits.com>
Per the RP2040 datasheet section 4.1.2.5.1, when handing a buffer to the USB controller the AVAILABLE bit must be written after the rest of the buffer-control register (length and data PID) and after a short delay, because buffer control crosses from the system clock domain into the USB clock domain. Writing everything in a single store risks the controller acting on a stale length or PID. rp2040_update_buffer_control() wrote the whole word, AVAILABLE included, in one access. Follow the sequence the datasheet (and the Pico SDK) use: write the control word with AVAILABLE cleared, wait ~12 CPU cycles, then set AVAILABLE. The delay covers system clocks up to 12x the 48 MHz USB clock. Validated on raspberrypi-pico (RP2040) as part of bringing up cdcncm; no regression on cdcacm/usbmsc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AHJRvWeBMTHwzpwjaUg4HW Signed-off-by: Ricard Rosson <ricard@groundbits.com>
rp23xx_usbdev.c is a line-for-line copy of the rp2040 USB device driver
and shares all three endpoint-handling defects fixed in the preceding
commits:
- bulk OUT reads armed with the full request length, overflowing the
10-bit buffer-control LEN field for large reads (e.g. cdcncm);
- endpoint requests resubmitted from their own completion callback
being armed twice, corrupting the data PID;
- the buffer AVAILABLE bit written together with length/PID instead of
afterwards.
Port the identical fixes to the RP2350 driver. The affected functions
are byte-identical to their rp2040 counterparts, so the changes match
verbatim. These were validated on RP2040 hardware; RP2350 shares the
same USB controller IP and driver, but was not re-tested on silicon.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AHJRvWeBMTHwzpwjaUg4HW
Signed-off-by: Ricard Rosson <ricard@groundbits.com>
18b2a57 to
f982711
Compare
|
Thanks for reviewing. Here are before/after logs. Setup: Before the patchesThe host binds Separately, the interrupt-IN After the patchesThe The interrupt-IN notification now completes and reaches the host —
No regression on Disclosure: these logs, the patch, and its validation were produced by an AI agent |
Port the three RP2040 USB device fixes (PR apache#19378) to the RP2350 controller, without which CDC-NCM RX returns zero-length reads and the data-PID toggle desynchronises under load: - Clamp the bulk-OUT read length to the endpoint maxpacket size before arming the hardware buffer. The buffer-control LEN field is 10 bits; a larger request overflows it and the host sees zero-byte reads. - Track whether a request already armed the hardware buffer (new rp23xx_req_s.armed) so a request resubmitted from its own completion callback is not armed a second time. A double arm toggles the data PID and the host drops the packet. - Set the AVAILABLE bit in a separate write after the LEN/PID word, per RP2040 datasheet section 4.1.2.5.1, instead of in the same store. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The failing |
please rebase your patch to the latest master and push again. |
Port the three RP2040 USB device fixes (PR apache#19378) to the RP2350 controller, without which CDC-NCM RX returns zero-length reads and the data-PID toggle desynchronises under load: - Clamp the bulk-OUT read length to the endpoint maxpacket size before arming the hardware buffer. The buffer-control LEN field is 10 bits; a larger request overflows it and the host sees zero-byte reads. - Track whether a request already armed the hardware buffer (new rp23xx_req_s.armed) so a request resubmitted from its own completion callback is not armed a second time. A double arm toggles the data PID and the host drops the packet. - Set the AVAILABLE bit in a separate write after the LEN/PID word, per RP2040 datasheet section 4.1.2.5.1, instead of in the same store. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 0ef4f53)
Port the three RP2040 USB device fixes (PR apache#19378) to the RP2350 controller, without which CDC-NCM RX returns zero-length reads and the data-PID toggle desynchronises under load: - Clamp the bulk-OUT read length to the endpoint maxpacket size before arming the hardware buffer. The buffer-control LEN field is 10 bits; a larger request overflows it and the host sees zero-byte reads. - Track whether a request already armed the hardware buffer (new rp23xx_req_s.armed) so a request resubmitted from its own completion callback is not armed a second time. A double arm toggles the data PID and the host drops the packet. - Set the AVAILABLE bit in a separate write after the LEN/PID word, per RP2040 datasheet section 4.1.2.5.1, instead of in the same store. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Ricard Rosson <ricard@groundbits.com>
Summary
Fixes three endpoint-handling bugs in the RP2040/RP2350 USB device driver
(
rp2040_usbdev.c, and its line-for-line copyrp23xx_usbdev.c). Together theyprevent CDC-NCM (and, for the second one, likely RNDIS) from working on these
controllers; with all three applied, a
CONFIG_NET_CDCNCMgadget on aRaspberry Pi Pico passes traffic in both directions with 0% packet loss.
The bugs were found by comparing the driver against the Pico SDK's TinyUSB
device controller driver (
dcd_rp2040.c/rp2040_usb.c).1. Bulk OUT read length not clamped to the endpoint max packet size.
rp2040_epread()armed the DPSRAM buffer-control register with the full usbdevrequest length. The buffer-control
LENfield is only 10 bits (max 1023), so aclass driver that posts a larger read request — cdcncm allocates a 16 KiB NTB read
buffer — overflows
LEN(16384 & 0x3ff == 0) and corrupts the neighbouringcontrol bits. The controller then completes the transfer immediately with zero
bytes, forever, and no OUT data is ever received (cdcncm floods
Wrong NTH SIGN, skblen 0). The receive path already accumulates a request acrossmultiple packets, so the buffer only ever needs to be armed for one max-size
packet. This matches the transmit path, which already chunks to
ep.maxpacket.Bulk classes with small reads (cdcacm, usbmsc) were unaffected, which is why the
defect only surfaced on cdcncm.
2. An endpoint request resubmitted from its own completion callback is armed
twice.
rp2040_txcomplete()/rp2040_rxcomplete()unconditionally started thenext transfer after invoking a request's completion callback. When that callback
resubmits a request on the same, now-idle endpoint — which cdcncm and rndis do from
their notify/interrupt completion handlers —
rp2040_epsubmit()already armed thehardware buffer, and the unconditional re-arm toggles the DATA0/DATA1 PID a second
time. The host discards the packet as a stale retransmission, so e.g. the cdcncm
NETWORK_CONNECTION/SPEED_CHANGEnotifications never arrive and the interfacestays
NO-CARRIER. A per-requestarmedflag now guards the redundant re-arm.This is also the likely cause of the long-standing rndis control-response
-110timeout on this controller.
3. Buffer
AVAILABLEbit written together with length/PID. The RP2040 datasheet§4.1.2.5.1 requires the
AVAILABLEbit to be set after the rest of buffer controland after a short settle, because buffer control crosses into the USB clock domain.
rp2040_update_buffer_control()wrote the whole word at once. Now it writes thecontrol word with
AVAILABLEcleared, waits ~12 CPU cycles, then setsAVAILABLE— the sequence the datasheet and the Pico SDK use.
Impact
data). Likely also fixes RNDIS (bug 2).
read lengths already fit in
LENand they don't resubmit from completion callbacks.rp2040_usbdev.candrp23xx_usbdev.c(RP2350), which is abyte-identical copy of the affected functions.
Testing
Built and run on a Raspberry Pi Pico (RP2040), Linux host:
ping20/20 host→board and 4/4 board→host, 0% loss,ARP resolves, board
REACHABLE. Before the fixes the board received nothing(
Wrong NTH SIGN, skblen 0flood).usbmon: the cdcncm interrupt-INSPEED_CHANGEnotification now completes and reaches the host(
C Ii:3:NN:1 0:4 16 = a12a...); previously it never did.RP2350: the affected functions in
rp23xx_usbdev.care byte-identical to theirRP2040 counterparts and the change compiles cleanly for
raspberrypi-pico-2, but itwas not separately re-tested on RP2350 silicon.
Host-side note for reviewers reproducing this: NuttX gadgets ship with the
Linux-gadget VID/PID
0525:a4a2, which is also claimed by the hostcdc_subsetdriver; if
cdc_subsetbinds the data interface first you'll seecdc_ncm: failed to claim data intf. Force NCM withecho 3-1:1.1 > /sys/bus/usb/drivers/cdc_subset/unbind; echo 3-1:1.0 > /sys/bus/usb/drivers/cdc_ncm/bind.Disclosure: this change — the code, its validation, and this description — was
produced by an AI agent (Claude Code), operated and directed by me, and reviewed by
me before submission.