Skip to content

usb: device_next: cdc_acm: clear TX_FIFO_BUSY on disable; retry when stuck - #1

Closed
Mikefly123 wants to merge 1 commit into
v4.4.1-basefrom
fix/cdc-acm-tx-fifo-drain-on-disable
Closed

usb: device_next: cdc_acm: clear TX_FIFO_BUSY on disable; retry when stuck#1
Mikefly123 wants to merge 1 commit into
v4.4.1-basefrom
fix/cdc-acm-tx-fifo-drain-on-disable

Conversation

@Mikefly123

Copy link
Copy Markdown

Base branch note

This fork's main tracks upstream zephyrproject-rtos/zephyr main, which has diverged
substantially from the v4.4.1 release this organization currently pins. To keep this PR's
diff scoped to the actual change (rather than the full main/v4.4.1 delta), it targets a new
v4.4.1-base branch pushed to point at the upstream v4.4.1 tag commit
(1f6485eca25431b5ff27ce9a754218c9e559bbbb).

What

Clears CDC_ACM_TX_FIFO_BUSY in usbd_cdc_acm_disable(), and makes
cdc_acm_tx_fifo_handler() self-reschedule with a 10 ms delay when it finds BUSY
already set and the TX ring buffer still has data pending.

Why

If a USB host stops issuing IN tokens while a CDC-ACM TX transfer is in flight (a
passive stall, not a disconnect), CDC_ACM_TX_FIFO_BUSY is left set indefinitely. The
existing error-completion path only clears the flag on disconnect/cancel
(ECONNABORTED), so a stall that never fires a completion leaves the flag stuck.
Once stuck, uart_poll_out() keeps scheduling the TX FIFO work handler, but the
handler returns immediately every time because of the busy short-circuit — the TX
path stops draining permanently, even after the host resumes polling, until an
explicit disable/enable cycle (e.g. a USB replug).

The fix re-arms the drain: usbd_cdc_acm_disable() clears the busy flag so it starts
clean on the next enable/reconnect, and the handler retries every 10 ms while busy
and non-empty, so the TX path recovers on its own as soon as the host resumes
acknowledging the IN endpoint.

This has been carried and HWIL-proven as a downstream patch on PROVES FCB v5e
(RP2350, macOS USB host), where the observed failure mode was secondary silence on
the USB CDC-ACM TX path after an initial burst of frames: the host driver paused
issuing IN tokens while a downstream consumer processed the burst, TX_FIFO_BUSY
stuck, and the console/telemetry path never recovered without a physical replug.
Companion HWIL/CI verification: Open-Source-Space-Foundation/proves-core-reference#439

Not intended for merge into upstream zephyrproject-rtos/zephyr from this fork.

…stuck

Problem: if the USB host stops issuing IN tokens while a CDC-ACM TX
transfer is in flight (e.g. a host-side driver that pauses polling
without disconnecting), CDC_ACM_TX_FIFO_BUSY is left set indefinitely.
The existing error-completion path clears the flag on disconnect or
transfer cancellation, but a passive stall that never fires a
completion leaves it stuck. Once stuck, uart_poll_out() keeps
scheduling cdc_acm_tx_fifo_handler(), but the handler returns
immediately on every invocation because the busy check short-circuits
it, so the TX path stops draining permanently even after the host
resumes polling.

Mechanism: usbd_cdc_acm_disable() now clears CDC_ACM_TX_FIFO_BUSY,
guaranteeing the flag starts clean on the next enable/reconnect cycle.
Additionally, when cdc_acm_tx_fifo_handler() finds BUSY already set
and the ring buffer still has data pending, it reschedules itself
after a 10 ms delay instead of returning permanently. If BUSY clears
normally when the in-flight transfer completes, the retry fires once
more and is a no-op (the ring buffer is already drained). If BUSY is
stuck because the host stopped acknowledging the IN endpoint, the
handler keeps retrying every 10 ms at negligible CPU cost until the
host resumes polling and the transfer completes.

Fix: the pending retry re-arms the IN endpoint as soon as the host
resumes issuing IN tokens, instead of requiring an explicit
disable/enable cycle (e.g. USB replug) to recover the TX path.

Signed-off-by: Michael Pham <phamlongmichael@gmail.com>
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 48262354-4f75-4c26-884f-9a3b59560714

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Mikefly123

Copy link
Copy Markdown
Author

Stack position 1/2 (stack zephyrproject-rtos#4, base of stack, targets v4.4.1-base). Next: #2.

@Mikefly123

Copy link
Copy Markdown
Author

Pruning this PR — validated by a CI negative control (2026-08-01)

Closing as part of the pre-merge trim of the USP radio changeset. Unlike the
other prunes in this pass, this one was tested rather than argued, because
the case for keeping it initially looked strong and rested on a claim nobody had
checked.

The argument that was made for keeping it

That integration-uart / integration-radio run on a self-hosted runner with a
real USB host attached to the board, so the stuck-TX_FIFO_BUSY path is live in
CI, and that the Korad power-cycle / re-enumeration steps exercise exactly the
usbd_cdc_acm_disable() hunk.

Why that does not hold

The integration runner is Linux, and this is a macOS host quirk.
tools/ci/detect-board-tty.sh walks /sys/bus/usb/devices/*/idVendor and
references /dev/serial/by-id — sysfs, so Linux. The failure this patch
addresses needs a host that stops issuing IN tokens and never resumes until a
physical replug
. On Linux, closing the port kills the IN URBs and reopening
resubmits them: the queued transfer completes, TX_FIFO_BUSY is cleared at
usbd_cdc_acm.c:327, and the drain is rescheduled at :331. It self-recovers.

TX_FIFO_BUSY is harder to latch than the PR body states. The body says the
error path "only clears the flag on disconnect/cancel (ECONNABORTED)". In fact
the clear at :297 sits inside if (err) and fires for any error on the
bulk-IN endpoint — the ECONNABORTED test only selects log severity. Together
with :327 (normal completion) and :672 / :686 (alloc/enqueue failure), the
only uncovered case is "no completion ever fires."

The Korad argument is backwards. The power-cycle cuts power to the board.
The MCU resets and data->state resets with it, so the atomic is clean on the
next boot regardless of the disable() hunk.

The negative control

Phase A/B never ran one — every soak ran with this patch present, which the
review flagged as an evidence gap. So we ran it: pcr PR zephyrproject-rtos#487 pins stock
upstream Zephyr v4.4.1
(1f6485ec), no fork, no carried patches.

Full CI green, run
30711682571:
lint, unit-test, build, yamcs-build, integration-uart, and
integration-radio all pass. Every step the keep-argument predicted would
break succeeded on real hardware: Flash Firmware, Detect Board TTY (twice,
across re-enumeration), Start GDS (twice), Sync Sequence Number, Format
Filesystem, Run UART Integration Tests, two Korad power-cycles, and the YAMCS
round-trip.

Consequence

With this pruned, the fork's delta against v4.4.1 is zero — pcr no longer
depends on an OSSF Zephyr fork at all. That is the single largest blast-radius
reduction in this trim: one entire forked repository leaves the dependency set,
along with the v4.4.1-base synthetic-base machinery this PR needed.

The value that remains is real but local: on the macOS HWIL bench this patch
saves a physical replug when the host driver stalls. That is bench ergonomics, and
if it is wanted there it belongs as a local bench patch, not in the flight pin.
Branch fix/cdc-acm-tx-fifo-drain-on-disable is retained, so it is recoverable.

Credit where due: the prune was proposed by @Mikefly123 pushing back on the
review's KEEP verdict, and the pushback was correct.

@Mikefly123

Copy link
Copy Markdown
Author

Closed as pruned — validated by the CI negative control on pcr zephyrproject-rtos#487 (stock Zephyr v4.4.1, full green including integration-uart and integration-radio on real hardware). See the rationale comment above. Branch retained.

@Mikefly123 Mikefly123 closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant