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.
|
![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
Collaborator
Author
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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