Conversation
OpenIPC#440 gave the RTL8733B a busy_window_reset() in Stop() and argued the other Realtek backends shared the hole. They did. This measures it and closes it, and adds the on-air arm that proves both halves. THE BUG. An armed window survives Stop(), and the next retune's note then stamps it Retuned — a spoil reason earned by a hardware session that no longer exists. The mechanism differs by backend, and the version in OpenIPC#440's commit message was only right for one of them: on the RTL8733B SetMonitorChannel re-runs bring_up_to_phy(), which sets the flag with_ccx gates on back to true. On Jaguar1/2/3 the retune does no bring-up at all — there the window stays reachable because nothing ever clears _brought_up. MEASURED, tests/busy_window_probe.sh --mode revive (arm, Stop(), retune, read), each die with its own reset removed and then restored: RTL8812AU (jaguar1) spoil=retuned -> none RTL8822BU (jaguar2) spoil=retuned -> none RTL8812CU (jaguar3) spoil=retuned -> none RTL8733BU spoil=retuned -> none THE ASSERTION IS ON THE REASON, NOT THE READING, and the Jaguar2 is why. A first cut asserted the post-Stop read was invalid; that is true on three dies and FALSE on the Jaguar2, whose Stop() only joins its runtime threads and leaves the chip live, so its sampled path answers with a 2 ms window. That assertion would have failed a correct backend. What holds everywhere is that no spoil reason survives the session that ended. Verified through the harness on the Jaguar2 specifically: with the reset removed the arm fails on the spoil check alone (3/3 not spoil=none), which is the only signal available on that die. WHAT THE RESETS DO NOT CLOSE, on the Jaguars. Unlike the RTL8733B — whose Stop() holds its recursive register lock across the whole body, which with_ccx takes first — there is no such span on Jaguar1/2/3, so a concurrent ArmChannelBusy can still land after the reset and during teardown. ArmChannelBusy is single-control-thread by contract (IRadio.h). Closing it properly means clearing _brought_up in Stop(), which gates other paths and is a behaviour change of its own; src/rtl8733b/CLAUDE.md records it as the half that stays open. LOCKING. Every reset takes the CCX mutex alone in a scope that acquires no other lock and calls no device method. Jaguar3's sits OUTSIDE _reg_mu rather than inside it as the RTL8733B's does, and deliberately: Stop() joins the coex thread, which takes _reg_mu, so holding it across the join would deadlock — the same shape as an earlier round's self-inflicted self-deadlock. Jaguar1 has no family-wide register lock (its _port0_mu is narrower and never taken under the CCX lock). No path takes the CCX mutex and then a register lock. THE ARM REFUSES WHERE IT CANNOT RUN. MediaTek's Stop() closes the device and nulls the handle, leaving no retune path (measured: the probe wedges). With SENSOR_RX=1 the Realtek Init runs on a detached thread that Stop() would be torn down underneath — the use-after-free this probe's own cleanup path exists to avoid, and the sequence RtlJaguar3Device::InitWrite refuses outright. Both are gated on the sensor VID and the RX flag rather than a capability, because there is no cap for "Stop() is survivable"; that is a lifecycle fact, not a feature one. Also in this change: expect_all accepted a partial record set, so a probe that died after one rep passed every arm — it now requires REPS records, as expect_spoil does. The harness header listed ten arms and claimed nine; it now lists and counts eleven, and says the list is by role rather than run order. The MediaTek skip total is 8 (7 at the early exit plus the revive arm), correcting the 7 stated in OpenIPC#440. Verified: ctest 69/69, ASan/UBSan clean on both selftests, and on air — full harness PASS with an RTL8733BU sensor against an RTL8822BU flooder (63% under load) and with the Jaguar2 as sensor against the RTL8733BU (61%); the revive arm passing on all three dies present; the RX guard refusing rather than wedging. Pre-existing and unrelated, verified at fc66aa9 without these changes: the txsess arm hangs on a Jaguar1 sensor, and an RTL8812AU txdemo exits after ~482 frames so that die cannot serve as the harness flooder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j
PR Summary by QodoReset Jaguar busy windows when hardware sessions stop
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Busy contract is copied into three guides
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Review round on OpenIPC#442. The rule this PR adds was written into one backend's chip guide and three copies of a code comment; none of that is where a reader looks for it. THE CONTRACT NOW HAS A DECLARATION. IRadio::ArmChannelBusy lists the three spoilers and said nothing about the session ending underneath a window; IRtlRadio::busy_window_reset described itself as a bring-up concern. Both now carry the lifetime rule, which is the only place a fifth backend would find it. The review that prompted this asked only for the Jaguar text to move — it would have moved out of the RTL8733B guide into three others and still had no home above them. PER-GENERATION FACTS MOVED TO THE PER-GENERATION GUIDES, worded per die rather than copied: Jaguar1's reset sits above the teardown_power_down=0 early return and takes the CCX lock alone (no family-wide register lock exists on that generation); Jaguar2's Stop() only joins its runtime threads, so the chip stays live and its sampled path still answers; Jaguar3's reset is outside _reg_mu because Stop() joins a thread that takes it. The measured four-die matrix moves to docs/rx-spectrum-sensing.md, which the root guide designates for measured numbers and the generation matrix. The three Jaguar Stop() comments each asserted RTL8733B internals to explain their own behaviour; each is now self-contained. TWO ERRORS OF MINE THAT THIS ROUND CAUGHT, both worth naming because the first was at the declaration site: - The new IRadio.h text said a surviving window "stays reachable across the next bring-up". That is the RTL8733B's mechanism and only its own — on Jaguar1/2/3 the retune runs no bring-up at all and the window survives because nothing clears _brought_up. This PR's own commit message says so two paragraphs down. Corrected in both the header and the doc. - The new doc section was inserted between the spoiler table and the two paragraphs that reference its rows by position ("The third row...", "The first row..."), silently re-pointing both at the wrong table. Moved below them. Also: the RTL8733B guide read as contradicting itself, saying in consecutive paragraphs that clearing _phy_ready does not protect a window and that it does — true of a window armed before a Stop and an arm issued after one respectively, now hinged. It also said the Jaguars lack "both" halves of the protection; they lack either. And the RTL8733BU row of the new matrix implied its "no reading" came from teardown depth, when that die has no sampled path at all. Repaired three mangled comment lines (a blank ' *' welded onto the end of the preceding line) introduced by 2f95498 in the Jaguar Stop() blocks. Comments and docs only; no behaviour change. Verified: ctest 69/69, ASan/UBSan clean on both selftests, and the on-air harness PASS on an RTL8733BU sensor against an RTL8822BU flooder, 63% under load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j
|
Addressed in The placement finding is correct — Jaguar1/2/3 lifecycle facts were in But moving it alone would have left the rule homeless. Also fixed while in there: the three Jaguar Two errors of mine that this round caught, the first at the declaration site:
And three smaller ones: the RTL8733B guide read as contradicting itself about Comments and docs only, no behaviour change. ctest 69/69, ASan/UBSan clean, and the on-air harness still PASSes on an RTL8733BU sensor against an RTL8822BU flooder at 63% under load. |
|
/review |
| **`Stop()` forgets any armed busy window, and the hazard it leaves is not | ||
| fully closed.** `with_ccx` gates on `_brought_up`, which `Stop()` does not | ||
| clear, so an `ArmChannelBusy()` issued *after* a `Stop()` still succeeds | ||
| against a chip that has been torn down. The reset in `Stop()` only handles a |
There was a problem hiding this comment.
1. Busy contract is copied into three guides 📘 Rule violation ⚙ Maintainability
src/jaguar1/CLAUDE.md, src/jaguar2/CLAUDE.md, and src/jaguar3/CLAUDE.md restate the Stop() and ArmChannelBusy() behavior already documented on IRadio::ArmChannelBusy instead of only directing readers to that declaration. Because all three copies describe the same reset and post-stop gating contract, future API changes can leave generation guidance inconsistent with the authoritative header.
Agent Prompt
## Issue description
Three per-generation `CLAUDE.md` guides copy the common busy-window lifecycle contract that is authoritative on the `IRadio::ArmChannelBusy` declaration, creating multiple copies that can drift.
## Fix Focus Areas
- src/jaguar1/CLAUDE.md[42-48]
- src/jaguar2/CLAUDE.md[75-81]
- src/jaguar3/CLAUDE.md[109-115]
## Recommended Fix
Replace the repeated common contract text with concise references to `IRadio::ArmChannelBusy`. Retain only generation-specific teardown, locking, measured-hardware, and remaining-hazard details in each guide.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 200a880 |
Review round on OpenIPC#442. The harness gated its new arms on "the sensor VID is not 0x0e8d", which is wrong in both directions and is the discriminator this whole series argues against. WRONG IN BOTH DIRECTIONS, verified against the ID tables: - Kestrel ships under 0x0bda, 0x0586 and 0x0b05. All pass a "not MediaTek" test, and Kestrel overrides neither ArmChannelBusy nor with_ccx, so it inherits the default returning 0: the probe emits only busy.skip and the harness reports failures for hardware that was never in scope. - MediaTek ships under nine OEM VIDs beyond 0x0e8d. Those pass the same test, so an OEM-VID MediaTek did not merely fail the new arm — it ran the seven LOADED arms the early exit exists to prevent, whose whole premise is that this backend cannot bring up on a saturated channel. 0x0b05, 0x2c4e and 0x7392 appear in BOTH the Kestrel and MediaTek tables, so no VID test can separate those two vendors at all. Every per-backend decision now reads the probe's own caps record. The first arm runs on every backend and needs no flooder, so generation and busy_airtime_ok are available before any decision. A backend with no busy-airtime engine — Kestrel is the only one — exits 77 as a clean SKIP rather than emitting a failure per arm. busy_airtime_ok maps exactly to "an arm is implemented" across all six backends; it does not promise the arm succeeds right now, and the block says so. The stale arm's guard turned out to be a tautology: the MediaTek early exit above it is unconditional, so every path reaching it can arm. Removed, rather than left as accounting that can never fire. DOCS. The session-lifetime paragraph was byte-identical in all three Jaguar guides, which re-created one level up the duplication this PR had just reduced in the Stop() comments. Replaced with a pointer to the declaration, keeping each guide's per-die measured facts, which do differ. That pointer promised "the rule and what it does not cover", and IRadio::ArmChannelBusy carried only the rule. Rather than weaken the pointer, the residual is now stated where it belongs: a reset forgets a window armed BEFORE the teardown, and where nothing clears the flag the engine's accessor gates on, an arm issued AFTER Stop() still succeeds against a torn-down chip — true of Jaguar1/2/3, not of the RTL8733B. The three Stop() comments said "this generation's guide records where it stands", which the same change made false; they point at the declaration now. Three claims of mine this round corrected: the revive gate's comment still described the VID gate it no longer was; "a dozen" OEM VIDs is nine; and 5/5 spoil=none reads as five confirmations when the arm sits inside the rep loop, so reps 2..N arm a chip the previous rep already tore down and only rep 1 exercises the intended sequence. The negative control is what discriminates, and that is now in the probe comment. Comments, docs and harness gating; no behaviour change to the library. Verified: ctest 69/69, ASan/UBSan clean on both selftests, and on air — Jaguar3 sensor against an MT7612U flooder PASS with no skips, MediaTek sensor against a Jaguar3 flooder PASS with 8 arms skipped (revive by its own branch, then seven at the early exit; three ran; 11 total). Pre-existing and unrelated, unchanged by this round: the txsess arm fails on a Jaguar1 sensor, verified at fc66aa9 without these changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j
|
Both findings addressed in Other supported sensors fail this probe — real, and the blast radius was bigger. The gate was The part the review did not reach: that same gate also guards the MediaTek loaded-arms early exit, so an OEM-VID MediaTek did not merely fail the new arm — it ran the seven loaded arms that branch exists to prevent, whose premise is that this backend cannot bring up on a saturated channel. That was the larger hole. Every per-backend decision now reads the probe's own caps record. The first arm runs on every backend and needs no flooder, so Busy contract copied into three guides — correct. The paragraph was byte-identical in all three; replaced with a pointer to the declaration, keeping each guide's per-die measured facts, which do differ. That pointer promised "the rule and what it does not cover", and Three of my own claims corrected this round: the revive gate's comment still described the VID gate it no longer was; "a dozen" OEM VIDs is nine; and ctest 69/69, ASan/UBSan clean, and on air: Jaguar3 sensor vs MT7612U flooder PASS with no skips; MediaTek sensor vs Jaguar3 flooder PASS with 8 arms skipped (1 + 7, three ran, 11 total). |
1 participant
Follow-up to #440. That PR gave the RTL8733B a
busy_window_reset()inStop()and argued the other Realtek backends shared the hole. They did.This measures it on each die, closes it, and adds the on-air arm that proves
both halves.
The bug
An armed window survives
Stop(), and the next retune's note then stamps itRetuned— a spoil reason earned by a hardware session that no longerexists. The reading is invalid either way, so nothing reports a wrong
number; what it reports is a wrong reason, which is the one thing the
spoilfield exists to get right.The mechanism differs by backend, and the version in #440's commit message
was only right for one of them:
SetMonitorChannelre-runsbring_up_to_phy(), which setsthe flag
with_ccxgates on back to true.reachable because nothing ever clears
_brought_up.Measured, per die
tests/busy_window_probe.sh --mode revive(arm →Stop()→ retune → read),each backend with its own reset removed and then restored:
spoil=retunedspoil=nonespoil=retunedspoil=nonespoil=retunedspoil=nonespoil=retunedspoil=noneThe assertion is on the reason, not the reading — and the Jaguar2 is why
My first cut asserted the post-
Stop()read was invalid. That is true onthree dies and false on the Jaguar2, whose
Stop()only joins its runtimethreads and leaves the chip live, so its sampled path answers with a valid
2 ms window. That assertion would have failed a correct backend.
What holds everywhere is that no spoil reason survives the session that ended.
Verified through the harness on the Jaguar2 specifically: with the reset
removed the arm fails on the spoil check alone (
3/3 samples not spoil=none), which is the only signal available on that die.What the resets do not close
On the Jaguars, unlike the RTL8733B — whose
Stop()holds its recursiveregister lock across the whole body, which
with_ccxtakes first — there isno such span, so a concurrent
ArmChannelBusycan still land after the resetand during teardown.
ArmChannelBusyis single-control-thread by contract(
IRadio.h). Closing it properly means clearing_brought_upinStop(),which gates other paths and is a behaviour change of its own;
src/rtl8733b/CLAUDE.mdrecords it as the half that stays open.Locking
Every reset takes the CCX mutex alone, in a scope that acquires no other lock
and calls no device method. Jaguar3's sits outside
_reg_murather thaninside it as the RTL8733B's does, deliberately:
Stop()joins the coexthread, which takes
_reg_mu, so holding it across the join would deadlock —the same shape as an earlier self-inflicted deadlock in this feature's
history. Jaguar1 has no family-wide register lock (its
_port0_muis narrowerand never taken under the CCX lock). No path takes the CCX mutex and then a
register lock.
The arm refuses where it cannot run
Stop()closes the device and nulls the handle, leaving noretune path. Measured: the probe wedges.
SENSOR_RX=1— the RealtekInitruns on a detached thread thatStop()would be torn down underneath. That is the use-after-free thisprobe's own cleanup path exists to avoid, and the sequence
RtlJaguar3Device::InitWriterefuses outright.Both gate on the sensor VID and the RX flag rather than a capability, because
there is no cap for "
Stop()is survivable" — that is a lifecycle fact, not afeature one.
Also in this change
expect_allaccepted a partial record set, so a probe that died after onerep passed every arm. It now requires
REPSrecords, asexpect_spoildoes.eleven, and says the list is by role rather than run order.
correcting the 7 stated in sensing: port the CCX CLM busy window to the RTL8733B #440.
Verification
ctest 69/69, ASan/UBSan clean on both selftests, and on air:
validity split
Pre-existing and unrelated, verified at
fc66aa9without these changes:the
txsessarm hangs on a Jaguar1 sensor, and an RTL8812AUtxdemoexitsafter ~482 frames, so that die cannot serve as the harness flooder. Worth
knowing because
SENSOR_PIDdefaults to the 8812AU.🤖 Generated with Claude Code
https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j