Skip to content

usb: device_next: cdc_acm: bound poll_out backpressure wait - #2

Closed
Mikefly123 wants to merge 1 commit into
fix/cdc-acm-tx-fifo-drain-on-disablefrom
fix/cdc-acm-poll-out-bound
Closed

usb: device_next: cdc_acm: bound poll_out backpressure wait#2
Mikefly123 wants to merge 1 commit into
fix/cdc-acm-tx-fifo-drain-on-disablefrom
fix/cdc-acm-poll-out-bound

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, it targets a new v4.4.1-base branch pushed to point at
the upstream v4.4.1 tag commit (1f6485eca25431b5ff27ce9a754218c9e559bbbb).

What

Bounds the sleep-retry loop in cdc_acm_poll_out() to ~20 ms (20 retries of the
existing 1 ms sleep) when the TX ring buffer is full and flow control is active, and
falls back to the existing discard-and-warn behavior once the budget is exhausted.

Why

cdc_acm_poll_out() previously sleep-retried in an unbounded loop while the TX
ring buffer stayed full under active flow control. This is fine for a host session
that is genuinely draining the buffer, but an attached-but-stalled host (e.g. a
host-side USB driver that stops issuing IN tokens for an extended period without
disconnecting) turns this into a block for the full duration of the stall on every
single console byte written. Any additional thread that tries to log while blocked
this way backs up behind the stalled writer, and shared downstream queues can
cascade the stall into a system-wide livelock rather than staying contained to the
console path.

Capping the wait at ~20 ms and then discarding — the same fallback already used for
the detached (!flow_ctrl) case — keeps the console best-effort under sustained
backpressure without changing behavior in the normal case, since the loop still
exits immediately once the ring buffer drains.

This has been carried and HWIL-proven as a downstream patch on PROVES FCB v5e. It
was captured during an HWIL soak where a macOS host ceased IN polling for extended
periods; the unbounded retry loop cascaded into a com-stack livelock, later
root-caused to this unbounded wait. Companion HWIL/CI verification:
Open-Source-Space-Foundation/proves-core-reference#439

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

@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: 8599742c-05d1-4915-addf-7950d170c74c

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.

Problem: cdc_acm_poll_out() sleep-retries in an unbounded loop while
the TX ring buffer is full and flow control is active. When a host
session is attached but stalled (e.g. a host-side USB driver that
stops issuing IN tokens for an extended period without disconnecting),
this loop blocks for the full duration of the stall on every console
byte written. Any thread that logs while this is happening backs up
behind the stalled writer, and if enough producers share the same
downstream queues, the backpressure cascades into a system-wide
livelock rather than staying contained to the console path.

Fix: cap the retry loop at 20 iterations of the existing 1 ms sleep
(~20 ms total). Once the retry budget is exhausted, treat the
still-attached-but-unresponsive session the same as the already-handled
detached case: log once and discard the pending byte instead of
continuing to block. This keeps the console best-effort under sustained
backpressure while leaving the normal (non-stalled) flow-controlled
path unaffected, since the loop still exits immediately once the ring
buffer drains.

Signed-off-by: Michael Pham <phamlongmichael@gmail.com>
@Mikefly123
Mikefly123 force-pushed the fix/cdc-acm-poll-out-bound branch from 3c87e09 to 1ce6608 Compare July 31, 2026 03:02
@Mikefly123
Mikefly123 changed the base branch from v4.4.1-base to fix/cdc-acm-tx-fifo-drain-on-disable July 31, 2026 03:03
@Mikefly123

Copy link
Copy Markdown
Author

Stack position 2/2 (stack zephyrproject-rtos#4, top of stack, targets #1's branch fix/cdc-acm-tx-fifo-drain-on-disable). Rebased onto #1 via git rebase --onto; combined tree verified identical to former integration branch feat/proves-usp-radio@3838a28.

@Mikefly123

Copy link
Copy Markdown
Author

Pruning this PR (changeset blast-radius trim, 2026-08-01)

Closing as part of the pre-merge trim of the USP radio changeset. The reason is
reachability: the code this PR adds cannot execute in the flight image.

The added bound is behind a condition that is always false. The patched
predicate is:

if (k_is_in_isr() || !data->flow_ctrl || retries-- <= 0)

C short-circuit evaluation means retries-- is only ever evaluated when
data->flow_ctrl is true. On the v5e it is false, from both directions:

  • Devicetree: hw-flow-control is a plain boolean
    (dts/bindings/serial/uart-controller.yaml), and it is absent from the
    cdc_acm_uart0 node — the node body is only compatible and label
    (proves_flight_control_board_v5.dtsi:55-58). Confirmed against the
    generated devicetree in the build tree, not just the source. So
    .flow_ctrl = DT_INST_PROP(n, hw_flow_control) initialises false.
  • Runtime: the only uart_configure() caller in the non-vendored tree sets
    .flow_ctrl = UART_CFG_FLOW_CTRL_NONE (ZephyrUartDriver.cpp:48), which
    usbd_cdc_acm.c maps to data->flow_ctrl = false.

Pre-patch, cdc_acm_poll_out() therefore already exits on iteration 1 via the
existing !data->flow_ctrl branch. The unbounded sleep-retry loop described in
the PR never runs in this deployment — this adds a bound to a loop that is
already bounded at zero iterations.

This also revises the root-cause story. git log -S UART_CFG_FLOW_CTRL_NONE
shows the NONE configuration present since "Upgrade to v4.0.0", long predating
the USP work, so the flight image never ran with flow_ctrl true and the
observed macOS console stall cannot have originated in this loop. The actual
containment for a stalled host is being kept and is unaffected: the
ZephyrUartDriver TX staging ring plus dedicated UartTxWriter thread, which
decouples the F' consumer thread from poll_out entirely.

A secondary reason not to carry it: uart_poll_out here is not a debug
console — CONFIG_CONSOLE/UART_CONSOLE/PRINTK/LOG are all disabled and F'
drives cdc_acm_uart0 directly as the downlink. If flow control were ever
enabled, this change would convert "block until the host drains" into "silently
discard TM bytes after ~20 ms", corrupting a CCSDS frame mid-stream rather than
backpressuring. That is the right trade for a console and the wrong one for a
binary telemetry link.

Nothing is stacked on this PR, and no test or CI job asserts the bound.
PR #1 (TX-FIFO drain on disable) is explicitly NOT being pruned — that one
is reachable and load-bearing wherever a USB host is enumerated, which includes
the self-hosted integration-uart / integration-radio CI runners and their
Korad power-cycle / re-enumeration steps.

Branch fix/cdc-acm-poll-out-bound is left in place. If a component ever enables
RTS/CTS flow control on a CDC instance this becomes live again — the cheaper
guard in the meantime is a note in ZephyrUartDriver::configure() recording that
FLOW_CTRL_NONE is load-bearing for non-blocking poll_out.

@Mikefly123

Copy link
Copy Markdown
Author

Closed as pruned — see the rationale comment above. The added retry bound sits behind data->flow_ctrl, which is false on the flight image from both devicetree and the explicit UART_CFG_FLOW_CTRL_NONE configure, so the code is unreachable. PR #1 is unaffected and stays. Branch retained for recoverability.

@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