From 2f9549806be57a438a2dac793cbcb29364466883 Mon Sep 17 00:00:00 2001 From: snokvist Date: Sun, 20 Sep 2026 18:45:22 +0200 Subject: [PATCH 1/3] sensing: a busy window must not outlive its hardware session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 #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 #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) Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j --- src/jaguar1/RtlJaguarDevice.cpp | 20 +++++++++ src/jaguar2/RtlJaguar2Device.cpp | 22 ++++++++++ src/jaguar3/RtlJaguar3Device.cpp | 20 +++++++++ src/rtl8733b/CLAUDE.md | 15 ++++--- tests/busy_window_probe.cpp | 50 ++++++++++++++++++++++- tests/busy_window_probe.sh | 69 +++++++++++++++++++++++++++++++- 6 files changed, 187 insertions(+), 9 deletions(-) diff --git a/src/jaguar1/RtlJaguarDevice.cpp b/src/jaguar1/RtlJaguarDevice.cpp index 0b66ee7e..5ce6334e 100644 --- a/src/jaguar1/RtlJaguarDevice.cpp +++ b/src/jaguar1/RtlJaguarDevice.cpp @@ -2395,6 +2395,26 @@ bool RtlJaguarDevice::NetDevOpen(SelectedChannel selectedChannel) { * Best-effort: a chip that already dropped off the bus makes the writes fail, * which is fine on a teardown path. */ void RtlJaguarDevice::Stop() { + /* The armed window dies with the session. Nothing else forgets it: this + * generation's with_ccx gates on _brought_up, which Stop() does not clear, + * so a window armed before a Stop stays visible afterwards and the next + * retune's note hands the caller a spoil reason earned by a session that no + * longer exists. Measured on an RTL8812AU with this reset removed: an + * arm/Stop/retune/read sequence reports spoil=retuned; with it, none. + * Scoped; nothing below takes the CCX lock. This generation has no + * FAMILY-WIDE register lock to order against (it has _port0_mu, a + * narrower one over the port0/TSF block, which is never taken under the + * CCX lock). * + * What this does NOT close: unlike the RTL8733B — whose Stop() holds its + * recursive register lock across the whole body, which with_ccx takes + * first — there is no such span here, so a concurrent ArmChannelBusy can + * still land after this reset and during teardown. ArmChannelBusy is + * single-control-thread by contract (IRadio.h), and closing it properly + * means clearing _brought_up, which gates other paths. */ + { + std::lock_guard ccx(busy_window_mutex()); + busy_window_reset(); + } _device.quiesce_tx(); if (!_cfg.tuning.teardown_power_down) { _logger->info("Jaguar1: Stop() leaving the chip powered " diff --git a/src/jaguar2/RtlJaguar2Device.cpp b/src/jaguar2/RtlJaguar2Device.cpp index bf4f0284..08a55d73 100644 --- a/src/jaguar2/RtlJaguar2Device.cpp +++ b/src/jaguar2/RtlJaguar2Device.cpp @@ -2013,6 +2013,28 @@ bool RtlJaguar2Device::WriteTsf(uint64_t tsf) { } void RtlJaguar2Device::Stop() { + /* The armed window dies with the session. Nothing else forgets it: + * with_ccx gates on _brought_up, which Stop() does not clear, so a window + * armed before a Stop stays visible afterwards and the next retune's note + * hands the caller a spoil reason earned by a session that no longer + * exists. Measured on an RTL8822BU with this reset removed: an + * arm/Stop/retune/read sequence reports spoil=retuned; with it, none. + * + * Note this Stop does NOT tear the chip down — it only joins the runtime + * threads below — so after the reset the sampled path still answers, with + * a live 2 ms window. That is why the on-air `revive` arm asserts the spoil + * REASON rather than the reading's validity. Scoped; neither joined thread + * takes the CCX lock. * + * What this does NOT close: unlike the RTL8733B — whose Stop() holds its + * recursive register lock across the whole body, which with_ccx takes + * first — there is no such span here, so a concurrent ArmChannelBusy can + * still land after this reset and during teardown. ArmChannelBusy is + * single-control-thread by contract (IRadio.h), and closing it properly + * means clearing _brought_up, which gates other paths. */ + { + std::lock_guard ccx(busy_window_mutex()); + busy_window_reset(); + } stop_pwrtrack(); stop_dig(); } diff --git a/src/jaguar3/RtlJaguar3Device.cpp b/src/jaguar3/RtlJaguar3Device.cpp index b4b42f64..93cad1a8 100644 --- a/src/jaguar3/RtlJaguar3Device.cpp +++ b/src/jaguar3/RtlJaguar3Device.cpp @@ -747,6 +747,26 @@ void RtlJaguar3Device::apply_replay_wseq() { /* Clean shutdown — see IRadio::Stop. Best-effort: a chip that already * dropped off the bus will make the de-init writes fail, which is fine. */ void RtlJaguar3Device::Stop() { + /* The armed window dies with the session. Nothing else forgets it: + * with_ccx gates on _brought_up, which Stop() does not clear, so a window + * armed before a Stop stays visible afterwards and the next retune's note + * hands the caller a spoil reason earned by a session that no longer + * exists. Measured on an RTL8812CU with this reset removed: an + * arm/Stop/retune/read sequence reports spoil=retuned; with it, none. + * Scoped, and OUTSIDE _reg_mu rather than inside it as the RTL8733B's is: + * Stop() joins the coex thread below, and that thread takes _reg_mu, so + * holding it across the join would deadlock. The coex loop never takes the + * CCX lock, which is what makes this ordering safe. * + * What this does NOT close: unlike the RTL8733B — whose Stop() holds its + * recursive register lock across the whole body, which with_ccx takes + * first — there is no such span here, so a concurrent ArmChannelBusy can + * still land after this reset and during teardown. ArmChannelBusy is + * single-control-thread by contract (IRadio.h), and closing it properly + * means clearing _brought_up, which gates other paths. */ + { + std::lock_guard ccx(busy_window_mutex()); + busy_window_reset(); + } _coex_stop = true; if (_coex_thread.joinable()) _coex_thread.join(); diff --git a/src/rtl8733b/CLAUDE.md b/src/rtl8733b/CLAUDE.md index e92a825d..e343ab7c 100644 --- a/src/rtl8733b/CLAUDE.md +++ b/src/rtl8733b/CLAUDE.md @@ -480,12 +480,15 @@ and the retune note hands the caller a `Retuned` spoil earned by a hardware session that no longer exists: invalid either way, but the reason would be a lie. The reset is scoped under the `_reg_mu` `Stop()` already holds. -Jaguar1/2/3 share this hole and have it worse — none of their `Stop()` -implementations resets the window either, and unlike this backend none of them -clears the flag their `with_ccx` gates on, so a `GetChannelBusy()` straight -after `Stop()` reads CCX registers on a deinitialised chip with no retune -needed. Not fixed here; noted so the asymmetry is not mistaken for an 8733B -quirk. +Jaguar1/2/3 had the same hole and are fixed in the same change — measured on +each die with the reset removed, an arm/Stop/retune/read reports +`spoil=retuned`. One half of the asymmetry remains on them and is NOT fixed +here: their `with_ccx` gates on `_brought_up`, which no `Stop()` clears, so an +`ArmChannelBusy()` issued *after* a Stop still succeeds against a chip that +has been torn down. This backend does not have that problem because `Stop()` +clears `_phy_ready`, which is what `with_ccx` gates on. Closing it on the +Jaguars means clearing `_brought_up` in their `Stop()`, which gates other +paths and is a behaviour change of its own. Retune notes live in `SetMonitorChannel` and `FastRetune`, both **scoped**: `FastRetune` calls `SetMonitorChannel` on its declined path while already diff --git a/tests/busy_window_probe.cpp b/tests/busy_window_probe.cpp index e3be9ead..729a4457 100644 --- a/tests/busy_window_probe.cpp +++ b/tests/busy_window_probe.cpp @@ -18,6 +18,15 @@ * the counter runs across the channel change. * early arm, read immediately. Must be INVALID (spoil=not-elapsed) * rather than the previous window's latched value. + * revive arm, Stop(), retune, read. The window must not outlive its own + * hardware session. A backend that does not forget it here hands + * back a spoil reason earned by a session that no longer exists — + * measured as spoil=retuned on all four Realtek backends with the + * reset removed. The assertion is on the REASON only: whether the + * read is valid depends on how deeply that backend's Stop tears + * the chip down, and on a Jaguar2 (whose Stop only joins its + * runtime threads) the sampled path answers with a live 2 ms + * window. Not runnable on MediaTek: Stop() closes the device. * txsess arm, TRANSMIT inside the window, read. Must stay valid and be * flagged own_tx: Realtek reads low (the receiver is deaf while * the PA is up), MediaTek reads high (it counts own airtime). @@ -177,7 +186,8 @@ int main(int argc, char **argv) { std::fprintf(stderr, "usage: %s [--vid N --pid N] [--channel N] [--other N] " "[--reps N] [--window-ms N] [--rx] " - "[--mode sampled|window|interrupt|retune|early|txsess]\n", + "[--mode sampled|window|interrupt|quality|race|retune|early|" + "revive|stale|txsess]\n", argv[0]); return 2; } @@ -271,6 +281,18 @@ int main(int argc, char **argv) { nap(200); } + /* `revive` tears the session down and brings it back. With --rx the Realtek + * Init is running on a DETACHED thread (it ends in a blocking StartRxLoop), + * so Stop() and SetMonitorChannel would run underneath it — the same + * use-after-free the cleanup path above exists to avoid, and the sequence + * RtlJaguar3Device::InitWrite refuses outright. Refuse rather than wedge. */ + if (mode == "revive" && rx_on) { + devourer::Ev(g_ev, "busy.skip") + .t() + .f("why", "revive cannot run with the RX loop live"); + return finish(5); + } + const uint32_t window_us = static_cast(window_ms) * 1000u; for (int i = 1; i <= reps; i++) { @@ -326,6 +348,32 @@ int main(int argc, char **argv) { nap(wait_ms / 2); } else if (mode == "early") { nap(5); + } else if (mode == "revive") { + /* The window must not survive its own hardware session. + * + * Stop() ends the session, but that alone does not protect an armed + * window, and the mechanism differs by backend. 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 simply because + * nothing ever clears `_brought_up`. Either way a backend that does + * not reset the window in Stop() hands the next read a spoil reason + * earned by a session that no longer exists, and the retune note below + * is what stamps it. + * + * The harness asserts the REASON only. Whether the read is valid + * depends on how deeply that backend's Stop tears the chip down: + * measured false on the RTL8733B, Jaguar1 and Jaguar3, and TRUE with a + * 2 ms window on a Jaguar2, whose Stop only joins its runtime + * threads. */ + nap(wait_ms / 2); + dev->Stop(); + nap(50); + dev->SetMonitorChannel( + SelectedChannel{.Channel = static_cast(channel), + .ChannelOffset = 0, + .ChannelWidth = CHANNEL_WIDTH_20}); + nap(wait_ms); } else if (mode == "stale") { /* Let this window finish and consume it, so the result register holds a * completed measurement; then arm again and read at once. */ diff --git a/tests/busy_window_probe.sh b/tests/busy_window_probe.sh index 9b7ead1f..c4b0879b 100755 --- a/tests/busy_window_probe.sh +++ b/tests/busy_window_probe.sh @@ -9,7 +9,7 @@ # in 55 of 71 windows on an RTL8822BU while 300-400 frames per window were # decoded). # -# Arms, in order: +# Arms (listed by role, not run order): # quiet sensor alone -- the floor. Must read ~0, not "no reading". # window armed, under load -- the feature. # sampled unarmed, same load -- the spread comparison. @@ -24,9 +24,15 @@ # the window length recorded at arm). # stale re-arm, then read early -- must not return the latched result # of the PREVIOUS window. +# revive arm, Stop(), retune, read -- must carry spoil=NONE: no reason +# may survive from the session that ended. Whether the reading is +# VALID is backend-dependent and deliberately not asserted (a +# Jaguar2 Stop leaves the chip live, so its sampled path answers +# with a 2 ms window; the others tear down and report nothing). +# Realtek only -- see the arm. # txsess sensor transmits -- must stay VALID and be flagged own_tx. # -# Nine arms. The skip counts below are stated against this list, so if you +# Eleven arms. The skip counts below are stated against this list, so if you # add one, add it here too or the verdict's "N arm(s) skipped" stops being # auditable. # @@ -150,6 +156,31 @@ assert_ge() { # label, value, bound else echo "FAIL $1: $2 < $3"; fails=$((fails + 1)); fi } +# Assert only the spoil REASON, ignoring validity. Some arms have a verdict +# that is the same on every backend while the reading's validity is not: a +# post-Stop read is invalid where Stop kills the chip (RTL8733B, Jaguar1/3) +# and a valid 2 ms sampled reading where it does not (Jaguar2 Stop only joins +# its runtime threads — measured). Asserting valid there would encode one +# family's teardown depth as a contract. +expect_spoil() { # mode, spoil + local mode="$1" want="$2" n bad before="$fails" + n=$(jq_field "$OUT/$mode.log" spoil | wc -l); n=${n:-0} + if [ "$n" -eq 0 ]; then + echo "FAIL $mode: no samples"; fails=$((fails + 1)); return 0 + fi + if [ "$n" -ne "$REPS" ]; then + echo "FAIL $mode: $n/$REPS records — the probe stopped early" + fails=$((fails + 1)); return 0 + fi + bad=$(jq_field "$OUT/$mode.log" spoil | grep -vc "^$want$"); bad=${bad:-ERR} + [ "$bad" = "ERR" ] && { echo "FAIL $mode: could not read spoil reasons" + fails=$((fails + 1)); return 0; } + [ "$bad" -ne 0 ] && { echo "FAIL $mode: $bad/$n samples not spoil=$want" + fails=$((fails + 1)); } + [ "$fails" -eq "$before" ] && echo " ok: $n samples spoil=$want" + return 0 +} + # The probe emits JSON Lines (ev=busy.window / busy.caps / busy.skip), so the # assertions below read fields out of the JSON rather than scraping a bespoke # text format. @@ -204,6 +235,13 @@ expect_all() { # mode, valid(0|1), [spoil] if [ "$n" -eq 0 ]; then echo "FAIL $mode: no samples"; fails=$((fails + 1)); return fi + # A probe that died partway leaves fewer records than reps. Without this a + # single surviving sample passes the whole arm — the same hole expect_spoil + # closes, and there is no reason for the two to disagree. + if [ "$n" -ne "$REPS" ]; then + echo "FAIL $mode: $n/$REPS records — the probe stopped early" + fails=$((fails + 1)); return + fi local want_json="False"; [ "$want_valid" = "1" ] && want_json="True" # `grep -vc` on an empty stream prints 0 but a FAILED extractor prints # nothing at all, and `[ "" -ne 0 ]` is a bash error, not false — which @@ -293,6 +331,33 @@ run_arm retune "retune mid-window (no load needed)" expect_all retune 0 retuned run_arm early "read before the window elapsed (no load needed)" expect_all early 0 not-elapsed +# The window must not outlive its own hardware session. Stop() ends the +# session, but a retune re-runs bring-up, so a backend that forgets to reset +# the window there revives it and answers with a spoil reason earned by a +# session that no longer exists. spoil=none is the assertion that matters: +# valid=0 alone passes either way. +# MediaTek is excluded: its Stop() closes the device and nulls the handle, so +# the arm cannot retune afterwards — measured, the probe wedges rather than +# reporting. This is a Realtek lifecycle rule and is checked where it applies. +# Gated on the sensor VID, not on a capability — deliberately, and worth +# saying since this harness otherwise argues against identity gates: there is +# no cap for "Stop() is survivable", and the two exclusions are lifecycle +# facts rather than feature ones. MediaTek's Stop() closes the device and +# nulls the handle, so there is no retune path left (measured: the probe +# wedges). And with SENSOR_RX=1 the Realtek Init runs on a detached thread +# that Stop() would be torn down underneath. +if [ "$realtek_sensor" = "1" ] && [ "$SENSOR_RX" != "1" ]; then + run_arm revive "arm, Stop(), retune, read (no load needed)" + # SPOIL only. Whether the post-Stop read is valid depends on how deeply that + # backend's Stop tears the chip down; what must hold everywhere is that no + # spoil reason survives from the session that ended. + expect_spoil revive none +else + echo "== revive arm skipped: needs a Realtek sensor with the RX loop OFF —" + echo " MediaTek's Stop() closes the device, and under SENSOR_RX=1 the" + echo " Init thread would be torn down underneath." + skip_arms 1 +fi if [ "$realtek_sensor" != "1" ]; then echo "== SKIPPING the loaded arms: a MediaTek sensor must be brought up" From 200a880cc154b51b7072a94963e617ab3783bfc1 Mon Sep 17 00:00:00 2001 From: snokvist Date: Sun, 20 Sep 2026 19:05:17 +0200 Subject: [PATCH 2/3] sensing: file the session-lifetime rule where each half of it belongs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round on #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) Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j --- docs/rx-spectrum-sensing.md | 32 +++++++++++++++++++++++++++++++- src/IRadio.h | 14 +++++++++++++- src/IRtlRadio.h | 12 ++++++++---- src/jaguar1/CLAUDE.md | 16 ++++++++++++++++ src/jaguar1/RtlJaguarDevice.cpp | 16 +++++++++------- src/jaguar2/CLAUDE.md | 16 ++++++++++++++++ src/jaguar2/RtlJaguar2Device.cpp | 16 +++++++++------- src/jaguar3/CLAUDE.md | 15 +++++++++++++++ src/jaguar3/RtlJaguar3Device.cpp | 22 ++++++++++++---------- src/rtl8733b/CLAUDE.md | 24 +++++++++++++++--------- 10 files changed, 144 insertions(+), 39 deletions(-) diff --git a/docs/rx-spectrum-sensing.md b/docs/rx-spectrum-sensing.md index b1bc4311..ee1ffae3 100644 --- a/docs/rx-spectrum-sensing.md +++ b/docs/rx-spectrum-sensing.md @@ -415,7 +415,8 @@ quick-connect decision has. ### A window is only about the dwell if nothing else touched it Three things spoil one, all measured, and each makes the reading come back -invalid with a reason (`ChannelBusy::spoil`) rather than plausible: +invalid with a reason (`ChannelBusy::spoil`) rather than plausible. A fourth — +the session ending under it — is not a spoiler but a lifetime rule, below. | spoiler | Jaguar1 (11AC) | Jaguar3 (JGR3) | RTL8733B (JGR3) | |---|---|---|---| @@ -439,6 +440,35 @@ fails loudly: `GetRxQuality()` calls `GetRxEnergy(with_nhm=true)`, so a consumer polling link quality inside its own survey dwell spoils it without touching the busy API at all. +### A window does not outlive its hardware session + +`Stop()` forgets any armed window. Without that, one armed before a teardown +stays reachable afterwards and the next retune's note stamps it `Retuned` — a +reason earned by a session that no longer exists. The mechanism differs by +die: on the RTL8733B the retune re-runs bring-up, which restores the flag +`with_ccx` gates on; on Jaguar1/2/3 the retune does no bring-up at all and the +window simply stays reachable because nothing ever clears `_brought_up`. The +reading is invalid either way, so what a missing reset costs is the REASON — +session end never produces a spoil value of its own, it forges another's. + +Measured with `tests/busy_window_probe.sh --mode revive` (arm, `Stop()`, +retune, read), each die with its own reset removed and then restored: + +| sensor | without the reset | with it | reading, with it | +|---|---|---|---| +| RTL8812AU (Jaguar1) | `spoil=retuned` | `spoil=none` | no reading — `Stop()` powers the card down | +| RTL8822BU (Jaguar2) | `spoil=retuned` | `spoil=none` | **valid, 2 ms window** — this `Stop()` only joins its runtime threads | +| RTL8812CU (Jaguar3) | `spoil=retuned` | `spoil=none` | no reading — `Stop()` runs `rtw_hal_deinit()` | +| RTL8733BU | `spoil=retuned` | `spoil=none` | no reading — and not because of teardown depth: this die has no sampled path at all, since it does not override `GetRxEnergy` | + +The Jaguar2 column is why the arm asserts the spoil REASON and not the +reading's validity: an "invalid" assertion encodes one family's teardown depth +as a contract and fails a correct backend. What holds everywhere is that no +reason survives the session that ended. + +The contract is at `IRadio::ArmChannelBusy`; how far each generation's +teardown goes is in its own `src//CLAUDE.md`. + ### Own transmission is carried, not corrected CLM counts receive-side deferral only, and a radio is deaf to the channel while diff --git a/src/IRadio.h b/src/IRadio.h index ba4b355c..3868cc5c 100644 --- a/src/IRadio.h +++ b/src/IRadio.h @@ -617,7 +617,19 @@ class IRadio { * of the dwell. A window is spoiled by an NHM read (IRtlRadio::GetRxEnergy * with with_nhm, which re-arms the shared CCX engine), by a retune, and by * reading before it has elapsed; the reading then comes back INVALID rather - * than plausible-but-wrong. A spoiled or completed window is consumed by + * than plausible-but-wrong. + * + * A window also does not outlive its hardware session: Stop() forgets it. + * Without that a window armed before a teardown stays reachable afterwards + * — by different mechanisms on different backends, so clearing whatever + * flag guards the engine is not a substitute — and the next retune's note + * stamps it Retuned, a reason earned by a session that no longer exists. + * Measured on all four Realtek backends. The reading is invalid either + * way, so what a missing reset costs is the REASON, which is the whole + * point of the spoil field. A backend implementing this owes the reset; + * the per-generation guides record how far each teardown goes. + * + * A spoiled or completed window is consumed by * the read; a not-yet-elapsed one stays armed, so the caller reads again * at the end of its dwell instead of re-arming. Single control thread, like * every other control-plane entry point. */ diff --git a/src/IRtlRadio.h b/src/IRtlRadio.h index 3bfe016d..c78f4137 100644 --- a/src/IRtlRadio.h +++ b/src/IRtlRadio.h @@ -245,10 +245,14 @@ class IRtlRadio : public IRadio { void busy_window_note_nhm_read() { _busy_window.note_nhm_read(); } void busy_window_note_retune() { _busy_window.note_retune(); } - /* Forget any armed window. Bring-up paths call it: a window armed before a - * re-Init describes a chip state that no longer exists, and leaving it - * armed would make the next unrelated GetChannelBusy() take the armed - * branch and report a stale period as if it were its own dwell. */ + /* Forget any armed window. Bring-up AND teardown paths call it: a window + * armed before a re-Init — or before a Stop() — describes a chip state that + * no longer exists, and leaving it armed would make the next unrelated + * GetChannelBusy() take the armed branch and report a stale period as if it + * were its own dwell. Stop() is the half that is easy to miss, because + * clearing the flag with_ccx gates on does not stand in for it: on a + * backend whose retune re-runs bring-up, that flag comes straight back. + * See IRadio::ArmChannelBusy for the contract. */ void busy_window_reset() { _busy_window = devourer::ClmWindow{}; } /* Serialises the CCX engine: the armed window's state, the arm/read, and diff --git a/src/jaguar1/CLAUDE.md b/src/jaguar1/CLAUDE.md index 9d778218..3eb7bcd5 100644 --- a/src/jaguar1/CLAUDE.md +++ b/src/jaguar1/CLAUDE.md @@ -39,6 +39,22 @@ methods), `RadioManagementModule` (channel/BW/TX power, up to 4 RF paths), ## Teardown +**`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 +window armed *before* it. Closing the rest means clearing `_brought_up` in +`Stop()`, which gates other paths and is a behaviour change of its own. The +contract is at `IRadio::ArmChannelBusy`. + +Measured on an RTL8812AU with the reset removed: arm, `Stop()`, retune, read +reports `spoil=retuned` — a reason earned by a session that no longer exists; +with it, `spoil=none` and no reading (this die's `Stop()` powers the card +down). The reset sits ABOVE the `teardown_power_down=0` early return, so the +"leave the chip powered" path forgets the window too. It takes the CCX lock +alone: this generation has no family-wide register lock to order against +(`_port0_mu` is narrower and never taken under the CCX lock). + `RtlJaguarDevice::Stop()` and the destructor run `HalModule::rtw_hal_deinit()`: halt the MAC engines (`REG_CR`, `REG_RCR`), then the die's card-disable power sequence via the existing `HalPwrSeqCmdParsing` (`rtl8812_card_disable_flow` / diff --git a/src/jaguar1/RtlJaguarDevice.cpp b/src/jaguar1/RtlJaguarDevice.cpp index 5ce6334e..84a99d7c 100644 --- a/src/jaguar1/RtlJaguarDevice.cpp +++ b/src/jaguar1/RtlJaguarDevice.cpp @@ -2404,13 +2404,15 @@ void RtlJaguarDevice::Stop() { * Scoped; nothing below takes the CCX lock. This generation has no * FAMILY-WIDE register lock to order against (it has _port0_mu, a * narrower one over the port0/TSF block, which is never taken under the - * CCX lock). * - * What this does NOT close: unlike the RTL8733B — whose Stop() holds its - * recursive register lock across the whole body, which with_ccx takes - * first — there is no such span here, so a concurrent ArmChannelBusy can - * still land after this reset and during teardown. ArmChannelBusy is - * single-control-thread by contract (IRadio.h), and closing it properly - * means clearing _brought_up, which gates other paths. */ + * CCX lock). + * + * What this does NOT close: no lock spans this Stop(), so a concurrent + * ArmChannelBusy can still land after the reset and during teardown, and + * with_ccx gates on _brought_up, which nothing here clears — so an arm + * issued AFTER a Stop still succeeds against a torn-down chip. + * ArmChannelBusy is single-control-thread by contract (IRadio.h); closing + * the rest means clearing _brought_up, which gates other paths. This + * generation's guide records where it stands. */ { std::lock_guard ccx(busy_window_mutex()); busy_window_reset(); diff --git a/src/jaguar2/CLAUDE.md b/src/jaguar2/CLAUDE.md index 7952ac58..1bd5b6de 100644 --- a/src/jaguar2/CLAUDE.md +++ b/src/jaguar2/CLAUDE.md @@ -72,6 +72,22 @@ The 8822B/8821C descriptor `TXPWR_OFSET` is a hardware LUT ## CCX energy sensing (`clm` / `nhm_env`) +**`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 +window armed *before* it. Closing the rest means clearing `_brought_up` in +`Stop()`, which gates other paths and is a behaviour change of its own. The +contract is at `IRadio::ArmChannelBusy`. + +Measured on an RTL8822BU with the reset removed: arm, `Stop()`, retune, read +reports `spoil=retuned`; with it, `spoil=none`. Note this `Stop()` does NOT +tear the chip down — it only joins `stop_pwrtrack()`/`stop_dig()` — so after +the reset the sampled path still answers, with a live 2 ms window. That is +why the on-air `revive` arm asserts the spoil REASON rather than the reading's +validity: asserting "invalid" would encode another family's teardown depth as +a contract and fail this one. Neither joined thread takes the CCX lock. + `GetRxEnergy(with_nhm=true)` runs the shared CCX window (`src/NhmReader.h`) on the 11AC register map; on-air validated on an RTL8822BU. diff --git a/src/jaguar2/RtlJaguar2Device.cpp b/src/jaguar2/RtlJaguar2Device.cpp index 08a55d73..dc9d5ec0 100644 --- a/src/jaguar2/RtlJaguar2Device.cpp +++ b/src/jaguar2/RtlJaguar2Device.cpp @@ -2024,13 +2024,15 @@ void RtlJaguar2Device::Stop() { * threads below — so after the reset the sampled path still answers, with * a live 2 ms window. That is why the on-air `revive` arm asserts the spoil * REASON rather than the reading's validity. Scoped; neither joined thread - * takes the CCX lock. * - * What this does NOT close: unlike the RTL8733B — whose Stop() holds its - * recursive register lock across the whole body, which with_ccx takes - * first — there is no such span here, so a concurrent ArmChannelBusy can - * still land after this reset and during teardown. ArmChannelBusy is - * single-control-thread by contract (IRadio.h), and closing it properly - * means clearing _brought_up, which gates other paths. */ + * takes the CCX lock. + * + * What this does NOT close: no lock spans this Stop(), so a concurrent + * ArmChannelBusy can still land after the reset and during teardown, and + * with_ccx gates on _brought_up, which nothing here clears — so an arm + * issued AFTER a Stop still succeeds against a torn-down chip. + * ArmChannelBusy is single-control-thread by contract (IRadio.h); closing + * the rest means clearing _brought_up, which gates other paths. This + * generation's guide records where it stands. */ { std::lock_guard ccx(busy_window_mutex()); busy_window_reset(); diff --git a/src/jaguar3/CLAUDE.md b/src/jaguar3/CLAUDE.md index 278e9301..58dce49a 100644 --- a/src/jaguar3/CLAUDE.md +++ b/src/jaguar3/CLAUDE.md @@ -106,6 +106,21 @@ same TSSI reshape as its offset slope). ## CCX energy sensing (`clm` / `nhm_env`) +**`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 +window armed *before* it. Closing the rest means clearing `_brought_up` in +`Stop()`, which gates other paths and is a behaviour change of its own. The +contract is at `IRadio::ArmChannelBusy`. + +Measured on an RTL8812CU with the reset removed: arm, `Stop()`, retune, read +reports `spoil=retuned`; with it, `spoil=none` and no reading (`Stop()` runs +`rtw_hal_deinit()`). The reset sits OUTSIDE `_reg_mu`, unlike the RTL8733B's, +and deliberately: `Stop()` joins the coex thread, and that thread takes +`_reg_mu`, so holding it across the join would deadlock. The coex loop never +takes the CCX lock, which is what makes this ordering safe. + **An armed busy window (`ArmChannelBusy`) is DESTROYED by an NHM read on this map.** Measured on an RTL8812CU: a clean 240 ms window read 60.4-61.6% under load, while the same window with one `GetRxEnergy(with_nhm=true)` mid-way came diff --git a/src/jaguar3/RtlJaguar3Device.cpp b/src/jaguar3/RtlJaguar3Device.cpp index 93cad1a8..c2e49811 100644 --- a/src/jaguar3/RtlJaguar3Device.cpp +++ b/src/jaguar3/RtlJaguar3Device.cpp @@ -753,16 +753,18 @@ void RtlJaguar3Device::Stop() { * hands the caller a spoil reason earned by a session that no longer * exists. Measured on an RTL8812CU with this reset removed: an * arm/Stop/retune/read sequence reports spoil=retuned; with it, none. - * Scoped, and OUTSIDE _reg_mu rather than inside it as the RTL8733B's is: - * Stop() joins the coex thread below, and that thread takes _reg_mu, so - * holding it across the join would deadlock. The coex loop never takes the - * CCX lock, which is what makes this ordering safe. * - * What this does NOT close: unlike the RTL8733B — whose Stop() holds its - * recursive register lock across the whole body, which with_ccx takes - * first — there is no such span here, so a concurrent ArmChannelBusy can - * still land after this reset and during teardown. ArmChannelBusy is - * single-control-thread by contract (IRadio.h), and closing it properly - * means clearing _brought_up, which gates other paths. */ + * Scoped, and deliberately NOT under _reg_mu: Stop() joins the coex thread + * below, that thread takes _reg_mu, and holding it across the join would + * deadlock. Taking the CCX lock alone is safe here because the coex loop + * never takes it. + * + * What this does NOT close: no lock spans this Stop(), so a concurrent + * ArmChannelBusy can still land after the reset and during teardown, and + * with_ccx gates on _brought_up, which nothing here clears — so an arm + * issued AFTER a Stop still succeeds against a torn-down chip. + * ArmChannelBusy is single-control-thread by contract (IRadio.h); closing + * the rest means clearing _brought_up, which gates other paths. This + * generation's guide records where it stands. */ { std::lock_guard ccx(busy_window_mutex()); busy_window_reset(); diff --git a/src/rtl8733b/CLAUDE.md b/src/rtl8733b/CLAUDE.md index e343ab7c..6d4cc0b0 100644 --- a/src/rtl8733b/CLAUDE.md +++ b/src/rtl8733b/CLAUDE.md @@ -480,15 +480,21 @@ and the retune note hands the caller a `Retuned` spoil earned by a hardware session that no longer exists: invalid either way, but the reason would be a lie. The reset is scoped under the `_reg_mu` `Stop()` already holds. -Jaguar1/2/3 had the same hole and are fixed in the same change — measured on -each die with the reset removed, an arm/Stop/retune/read reports -`spoil=retuned`. One half of the asymmetry remains on them and is NOT fixed -here: their `with_ccx` gates on `_brought_up`, which no `Stop()` clears, so an -`ArmChannelBusy()` issued *after* a Stop still succeeds against a chip that -has been torn down. This backend does not have that problem because `Stop()` -clears `_phy_ready`, which is what `with_ccx` gates on. Closing it on the -Jaguars means clearing `_brought_up` in their `Stop()`, which gates other -paths and is a behaviour change of its own. +All of that is about a window armed BEFORE the Stop. For an arm issued +*after* one, the flag does its job, and this backend closes both sides of the +hazard: SEQUENTIALLY, `Stop()` clears `_phy_ready`, which is what `with_ccx` +gates on, so a later `ArmChannelBusy()` is refused rather than arming a +torn-down chip; CONCURRENTLY, `Stop()` holds the recursive `_reg_mu` across +its whole body and `with_ccx` takes that lock first, so an arm racing the +teardown blocks instead of slipping in after the reset. + +Jaguar1/2/3 reset the window in `Stop()` the same way but stop there — they +have NEITHER half: no `Stop()` clears `_brought_up`, and none holds a +register lock across its teardown (Jaguar1 has no family-wide one at all). +So on those an arm issued after a Stop still succeeds against a torn-down +chip, and one racing the teardown can still slip in. Each Jaguar guide +records its own generation's teardown, the race is in each `Stop()`'s own +comment, and `IRadio::ArmChannelBusy` carries the rule itself. Retune notes live in `SetMonitorChannel` and `FastRetune`, both **scoped**: `FastRetune` calls `SetMonitorChannel` on its declined path while already From 667cb8c46dd4d6d6ef4d0d20d6337746432ba9f7 Mon Sep 17 00:00:00 2001 From: snokvist Date: Mon, 21 Sep 2026 18:58:27 +0200 Subject: [PATCH 3/3] sensing: ask the caps which arms apply, not the USB VID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round on #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) Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j --- src/IRadio.h | 7 ++++ src/jaguar1/CLAUDE.md | 10 ++--- src/jaguar1/RtlJaguarDevice.cpp | 4 +- src/jaguar2/CLAUDE.md | 10 ++--- src/jaguar2/RtlJaguar2Device.cpp | 4 +- src/jaguar3/CLAUDE.md | 10 ++--- src/jaguar3/RtlJaguar3Device.cpp | 4 +- tests/busy_window_probe.cpp | 10 ++++- tests/busy_window_probe.sh | 65 +++++++++++++++++++++++--------- 9 files changed, 78 insertions(+), 46 deletions(-) diff --git a/src/IRadio.h b/src/IRadio.h index 3868cc5c..fad4c766 100644 --- a/src/IRadio.h +++ b/src/IRadio.h @@ -629,6 +629,13 @@ class IRadio { * point of the spoil field. A backend implementing this owes the reset; * the per-generation guides record how far each teardown goes. * + * What the reset does NOT close: it forgets a window armed BEFORE the + * teardown. Where nothing clears the flag the engine's own accessor gates + * on, an arm issued AFTER Stop() still succeeds against a chip that has + * been torn down — true of Jaguar1/2/3, whose gate is never cleared, and + * not of the RTL8733B, whose Stop() clears it. Closing it everywhere is a + * behaviour change on paths that flag also guards. + * * A spoiled or completed window is consumed by * the read; a not-yet-elapsed one stays armed, so the caller reads again * at the end of its dwell instead of re-arming. Single control thread, like diff --git a/src/jaguar1/CLAUDE.md b/src/jaguar1/CLAUDE.md index 3eb7bcd5..0feed14f 100644 --- a/src/jaguar1/CLAUDE.md +++ b/src/jaguar1/CLAUDE.md @@ -39,13 +39,9 @@ methods), `RadioManagementModule` (channel/BW/TX power, up to 4 RF paths), ## Teardown -**`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 -window armed *before* it. Closing the rest means clearing `_brought_up` in -`Stop()`, which gates other paths and is a behaviour change of its own. The -contract is at `IRadio::ArmChannelBusy`. +**`Stop()` forgets any armed busy window** — the rule, and the residual it +does not close, are at `IRadio::ArmChannelBusy`, the one declaration site +where they can be kept true. What is specific to this die: Measured on an RTL8812AU with the reset removed: arm, `Stop()`, retune, read reports `spoil=retuned` — a reason earned by a session that no longer exists; diff --git a/src/jaguar1/RtlJaguarDevice.cpp b/src/jaguar1/RtlJaguarDevice.cpp index 84a99d7c..f5935bc7 100644 --- a/src/jaguar1/RtlJaguarDevice.cpp +++ b/src/jaguar1/RtlJaguarDevice.cpp @@ -2411,8 +2411,8 @@ void RtlJaguarDevice::Stop() { * with_ccx gates on _brought_up, which nothing here clears — so an arm * issued AFTER a Stop still succeeds against a torn-down chip. * ArmChannelBusy is single-control-thread by contract (IRadio.h); closing - * the rest means clearing _brought_up, which gates other paths. This - * generation's guide records where it stands. */ + * the rest means clearing _brought_up, which gates other paths. The + * contract and this residual are both at IRadio::ArmChannelBusy. */ { std::lock_guard ccx(busy_window_mutex()); busy_window_reset(); diff --git a/src/jaguar2/CLAUDE.md b/src/jaguar2/CLAUDE.md index 1bd5b6de..48346d3e 100644 --- a/src/jaguar2/CLAUDE.md +++ b/src/jaguar2/CLAUDE.md @@ -72,13 +72,9 @@ The 8822B/8821C descriptor `TXPWR_OFSET` is a hardware LUT ## CCX energy sensing (`clm` / `nhm_env`) -**`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 -window armed *before* it. Closing the rest means clearing `_brought_up` in -`Stop()`, which gates other paths and is a behaviour change of its own. The -contract is at `IRadio::ArmChannelBusy`. +**`Stop()` forgets any armed busy window** — the rule, and the residual it +does not close, are at `IRadio::ArmChannelBusy`, the one declaration site +where they can be kept true. What is specific to this die: Measured on an RTL8822BU with the reset removed: arm, `Stop()`, retune, read reports `spoil=retuned`; with it, `spoil=none`. Note this `Stop()` does NOT diff --git a/src/jaguar2/RtlJaguar2Device.cpp b/src/jaguar2/RtlJaguar2Device.cpp index dc9d5ec0..48c7c676 100644 --- a/src/jaguar2/RtlJaguar2Device.cpp +++ b/src/jaguar2/RtlJaguar2Device.cpp @@ -2031,8 +2031,8 @@ void RtlJaguar2Device::Stop() { * with_ccx gates on _brought_up, which nothing here clears — so an arm * issued AFTER a Stop still succeeds against a torn-down chip. * ArmChannelBusy is single-control-thread by contract (IRadio.h); closing - * the rest means clearing _brought_up, which gates other paths. This - * generation's guide records where it stands. */ + * the rest means clearing _brought_up, which gates other paths. The + * contract and this residual are both at IRadio::ArmChannelBusy. */ { std::lock_guard ccx(busy_window_mutex()); busy_window_reset(); diff --git a/src/jaguar3/CLAUDE.md b/src/jaguar3/CLAUDE.md index 58dce49a..399bcc7e 100644 --- a/src/jaguar3/CLAUDE.md +++ b/src/jaguar3/CLAUDE.md @@ -106,13 +106,9 @@ same TSSI reshape as its offset slope). ## CCX energy sensing (`clm` / `nhm_env`) -**`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 -window armed *before* it. Closing the rest means clearing `_brought_up` in -`Stop()`, which gates other paths and is a behaviour change of its own. The -contract is at `IRadio::ArmChannelBusy`. +**`Stop()` forgets any armed busy window** — the rule, and the residual it +does not close, are at `IRadio::ArmChannelBusy`, the one declaration site +where they can be kept true. What is specific to this die: Measured on an RTL8812CU with the reset removed: arm, `Stop()`, retune, read reports `spoil=retuned`; with it, `spoil=none` and no reading (`Stop()` runs diff --git a/src/jaguar3/RtlJaguar3Device.cpp b/src/jaguar3/RtlJaguar3Device.cpp index c2e49811..9643b83e 100644 --- a/src/jaguar3/RtlJaguar3Device.cpp +++ b/src/jaguar3/RtlJaguar3Device.cpp @@ -763,8 +763,8 @@ void RtlJaguar3Device::Stop() { * with_ccx gates on _brought_up, which nothing here clears — so an arm * issued AFTER a Stop still succeeds against a torn-down chip. * ArmChannelBusy is single-control-thread by contract (IRadio.h); closing - * the rest means clearing _brought_up, which gates other paths. This - * generation's guide records where it stands. */ + * the rest means clearing _brought_up, which gates other paths. The + * contract and this residual are both at IRadio::ArmChannelBusy. */ { std::lock_guard ccx(busy_window_mutex()); busy_window_reset(); diff --git a/tests/busy_window_probe.cpp b/tests/busy_window_probe.cpp index 729a4457..d47b617a 100644 --- a/tests/busy_window_probe.cpp +++ b/tests/busy_window_probe.cpp @@ -365,7 +365,15 @@ int main(int argc, char **argv) { * depends on how deeply that backend's Stop tears the chip down: * measured false on the RTL8733B, Jaguar1 and Jaguar3, and TRUE with a * 2 ms window on a Jaguar2, whose Stop only joins its runtime - * threads. */ + * threads. + * + * Read `5/5 spoil=none` as ONE confirmation, not five. This arm sits + * inside the rep loop, so reps 2..N arm a chip the previous rep's + * Stop() already tore down — which succeeds only because of the + * residual this very comment is about (nothing clears the gate + * with_ccx reads). Only rep 1 exercises the intended live-session + * sequence. The negative control is what discriminates: with the + * reset removed every rep reports spoil=retuned. */ nap(wait_ms / 2); dev->Stop(); nap(50); diff --git a/tests/busy_window_probe.sh b/tests/busy_window_probe.sh index c4b0879b..6e171d9c 100755 --- a/tests/busy_window_probe.sh +++ b/tests/busy_window_probe.sh @@ -273,17 +273,47 @@ expect_all() { # mode, valid(0|1), [spoil] echo "== sensor $SENSOR_VID:$SENSOR_PID, flooder $FLOOD_VID:$FLOOD_PID, ch$CHANNEL" -# The Realtek-only arms drive IRtlRadio facilities (the NHM read, the CCX -# result latch). A MediaTek sensor runs the rest. -# Compared numerically: a literal string test made SENSOR_VID=0x0E8D (or a -# decimal VID) run the Realtek-only NHM arms against a MediaTek sensor, where -# the cast fails and the window comes back valid. -realtek_sensor=1 -[ "$(printf '%d' "$SENSOR_VID")" -eq "$(printf '%d' 0x0e8d)" ] && realtek_sensor=0 - stop_flood run_arm window "quiet floor (no flooder)" mv -f "$OUT/window.log" "$OUT/quiet.log" + +# EVERY per-backend decision below is read from the probe's own caps record, +# not from the sensor's USB VID. The VID cannot answer any of these questions: +# Kestrel ships under 0x0bda/0x0586/0x0b05, MediaTek ships under nine OEM +# VIDs beyond 0x0e8d, and 0x0b05, 0x2c4e and 0x7392 appear in BOTH tables — +# so no VID test can even separate those two. A "not MediaTek" gate therefore called a Kestrel +# Realtek and ran arms it cannot pass, and called an OEM-VID MediaTek Realtek +# and ran the loaded arms its bring-up cannot survive. +sensor_gen="$(caps_field "$OUT/quiet.log" generation)" +busy_cap="$(caps_field "$OUT/quiet.log" busy_airtime_ok)" + +# busy_airtime_ok says the backend HAS an engine, not that an arm will +# succeed right now — a MediaTek with RX off and a Realtek before bring-up +# both report true and refuse. That is fine here: the flag answers "is this +# harness in scope", and the quiet arm's own assertion catches an engine that +# exists but cannot arm. +# +# No busy-airtime engine at all (Kestrel) means this harness does not apply. +# Say so once and leave, rather than emitting a failure per arm for hardware +# that was never in scope. +if [ "$busy_cap" = "False" ]; then + echo "SKIP: $sensor_gen has no busy-airtime engine (busy_airtime_ok=false)," + echo " so there is no armed window for this harness to measure." + exit 77 +fi +# "Realtek" here means "not the backend whose Stop() closes the device and +# whose bring-up cannot survive a saturated channel" — a generation fact, and +# the caps record carries the generation by name. +realtek_sensor=1 +[ "$sensor_gen" = "mt7612u" ] && realtek_sensor=0 + +if [ "$busy_cap" != "True" ]; then + echo "FAIL caps: busy_airtime_ok not readable from the probe (got '$busy_cap')" + echo " — refusing to decide which arms apply from a caps record this" + echo " harness cannot parse." + fails=$((fails + 1)) +fi + expect_all quiet 1 # a quiet channel is a READING of ~0, never "no reading" quiet_mean="$(stat_of quiet mean)" # The floor must BE a floor. Without this the whole comparison below passes @@ -339,13 +369,11 @@ expect_all early 0 not-elapsed # MediaTek is excluded: its Stop() closes the device and nulls the handle, so # the arm cannot retune afterwards — measured, the probe wedges rather than # reporting. This is a Realtek lifecycle rule and is checked where it applies. -# Gated on the sensor VID, not on a capability — deliberately, and worth -# saying since this harness otherwise argues against identity gates: there is -# no cap for "Stop() is survivable", and the two exclusions are lifecycle -# facts rather than feature ones. MediaTek's Stop() closes the device and -# nulls the handle, so there is no retune path left (measured: the probe -# wedges). And with SENSOR_RX=1 the Realtek Init runs on a detached thread -# that Stop() would be torn down underneath. +# Gated on the reported GENERATION, not on a capability: there is no cap for +# "Stop() is survivable", so this one exclusion stays a lifecycle fact. The +# two cases: MediaTek's Stop() nulls the device handle, leaving no retune +# path (measured: the probe wedges), and with SENSOR_RX=1 the Realtek Init +# runs on a detached thread that Stop() would be torn down underneath. if [ "$realtek_sensor" = "1" ] && [ "$SENSOR_RX" != "1" ]; then run_arm revive "arm, Stop(), retune, read (no load needed)" # SPOIL only. Whether the post-Stop read is valid depends on how deeply that @@ -428,8 +456,10 @@ else skip_arms 3 fi -if [ "$realtek_sensor" = "1" ]; then - run_arm stale "re-arm, then read before the new window elapsed" +# No guard: the MediaTek early exit above is unconditional, so every path +# that reaches here is one that can arm. This arm's skip is already counted +# in that exit's total. +run_arm stale "re-arm, then read before the new window elapsed" # The first read of each pair must be a real measurement and the second must # refuse. If the trigger did NOT clear the ready bit, the second read would # return the first window's latched value and look perfectly valid. @@ -460,7 +490,6 @@ EOF echo "FAIL stale: $bad early reads returned a stale latched value" fails=$((fails + 1)) fi -fi f0="$(flood_frames)" run_arm txsess "sensor transmitting inside its own window"