Repository navigation
fix(devourer): prevent deadlock when StopRxLoop is called during Init - #503
floppyhammer wants to merge 2 commits into
Conversation
Fix a deadlock where calling StopRxLoop() during device initialization (inside Init()) was ignored and overwritten. Root cause: StartRxLoop() unconditionally cleared the stop flag (should_stop / _rx_stop) upon entry. If StopRxLoop() was called while Init() was executing (e.g. during IQK calibration or channel setup), StartRxLoop() wiped out the pending stop request and launched bulk_read_async_loop indefinitely, deadlocking the caller waiting on the worker thread. Changes: - Clear stop flag at entry of Init() to mark the start of initialization. - In StartRxLoop(), check whether stop was requested (should_stop / _rx_stop) before resetting the flag. Abort StartRxLoop() immediately if true.
PR Summary by QodoPrevent RX initialization from discarding stop requests
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Reviewed with a hardware reproducer (Init on a thread, StopRxLoop() at N ms, join with a 20 s timeout), both libraries built from clean worktrees — master f46c28a and this PR's head — run as root with no kernel driver bound.
| Build | Adapter | Stop at 100 / 400 / 1000 / 2500 ms | Stop+join, then standalone StartRxLoop |
StopRxLoop before Init |
|---|---|---|---|---|
| master | RTL8822BU (Jaguar2, ch6) | DEADLOCK / DEADLOCK / DEADLOCK / ok | ok | ok |
| master | RTL8812EU (Jaguar3, ch36) | DEADLOCK / DEADLOCK / ok / ok | ok | ok |
| PR | RTL8822BU | ok / ok / ok / ok | ok, frames flow | ok, frames flow |
| PR | RTL8812EU | ok / ok / ok / ok | ok, frames flow | ok, frames flow |
DEADLOCK = the Init thread never returned (frames kept arriving the whole 20 s). "ok" on the PR = Init returned 0.5–1.4 s after the stop, i.e. bring-up completed and StartRxLoop refused to start. The last two columns are the scenarios the Qodo thread raised; both behave correctly. Jaguar1 and Kestrel were not on the bench, so those two are compile-verified only.
The fix is real and the mechanism works. Before merge I'd like these addressed, ranked:
- Coverage.
Rtl8733bDevice::StartRxLoopandMt7612uRadio::StartRxLoophave the same unconditional_rx_stop = falseand bothInits callStartRxLoopthe same way, so the identical deadlock remains on those two backends. The title says the library; it currently fixes four of six. - The
InitWrite+StartRxLoop-on-a-thread shape (DEVOURER_TX_WITH_RX=thread) keeps the same race: a stop issued before that thread reachesStartRxLooptakes the "stale, clear it" branch. Fine to leave out of scope, but say so in theIRadio.hStopRxLoopcomment so callers know which window is covered. - Simpler mechanism (inline comments): a private
start_rx_loop(proc, bool clear_stale)thatInitcalls withfalseand the public override withtrueneeds no atomic, no scope guard, and has no window at all. - The clear in
Initshould be its first statement (inline comment). - Indentation: 4-space blocks in 2-space files, and the Jaguar3
StartRxLoopprologue re-indented, which churns untouched comment lines. - The red
qodo-gateis only because resolving the thread doesn't re-trigger it — a reply inside that thread re-runs the check.
| _init_in_progress.store(false, std::memory_order_release); | ||
| }; | ||
| struct ScopeGuard { | ||
| std::function<void()> fn; |
There was a problem hiding this comment.
This is a class definition, and it's pasted into three files with a std::function it doesn't need. The whole flag + guard pair goes away if Init calls a private start_rx_loop(std::move(packetProcessor), /*clear_stale=*/false) and the public StartRxLoop override calls it with true: the only thing the atomic encodes is "who called me", which the call site already knows. No shared state, no release/acquire, nothing to roll back on an exception.
| _station_ready = false; | ||
| _station.retire(_device, _logger, "Jaguar2"); | ||
| } | ||
| _rx_stop = false; |
There was a problem hiding this comment.
A StopRxLoop() that lands between Init being entered and this line is dropped. The window is microseconds, but it's avoidable: make the clear the first statement of Init (same on Jaguar1, where it sits after the beacon/sounder rollback registration, and on Jaguar3).
| * - If this is a standalone restart after initialization has completed, clear | ||
| * the stale stop request. | ||
| */ | ||
| if (_init_in_progress.load(std::memory_order_acquire)) { |
There was a problem hiding this comment.
The _init_in_progress name over-promises: it stays true for the whole blocking RX loop that Init runs, not just the bring-up. If the flag stays, call it what it is (_rx_loop_from_init or similar) — or drop it for the clear_stale parameter above.
| * - If this is a standalone restart after initialization has completed, clear | ||
| * the stale stop request. | ||
| */ | ||
| if (_init_in_progress.load(std::memory_order_acquire)) { |
There was a problem hiding this comment.
4-space indentation in a 2-space file, and the existing "Take over bulk-IN" comment below got re-indented with it. Please match the surrounding style so the diff is only the behaviour change.
| * rpkt_type=7 releases) so the two never read the endpoint concurrently. */ | ||
| stop_wp_drain(); | ||
| _rx_stop = false; | ||
| if (g_devourer_should_stop) |
There was a problem hiding this comment.
The g_devourer_should_stop early return is new behaviour on every backend and unrelated to the fix: the loop's own stop predicate already checks the same flag on its first pass. Harmless, but drop it (or justify it in the commit) so the change stays about the Init-time stop. Here specifically, stop_wp_drain() has already run by this point, so an early return leaves no bulk-IN reader at all — same as before, but worth not adding a second path to it.
| _logger->error("Jaguar1: post-arm Init rollback was not fully verified; " | ||
| "see the register-specific error above"); | ||
| }); | ||
| should_stop = false; |
There was a problem hiding this comment.
Same as Jaguar2: this clear belongs at the top of Init, before the busy-window reset and the rollback guard, so the covered window starts the moment Init is entered.
Fix a deadlock where calling StopRxLoop() during device initialization (inside Init()) was ignored and overwritten.
Root cause:
StartRxLoop() unconditionally cleared the stop flag (should_stop / _rx_stop) upon entry. If StopRxLoop() was called while Init() was executing (e.g. during IQK calibration or channel setup), StartRxLoop() wiped out the pending stop request and launched bulk_read_async_loop indefinitely, deadlocking the caller waiting on the worker thread.
Changes: