rtl8733b: a live ACK disarm must move the identity, not just close the gate - #410
Conversation
…e gate AckResponder.h documents net_type (0x0102[1:0]) as the gate: arm by programming MACID and flipping the field, disarm by clearing the field, "so the MACID may stay". That holds for ARMING on every generation tested. It is not reliable in reverse. Measured on an RTL8733B (0bda:f72b) against a peer soliciting unicast QoS-Data at ~32 frames/s, scoring the SOLICITING side's CCX tx.report (this backend emits none of its own, so its reporting gap is not in the loop): fresh process, never armed 100.00 % unanswered (969/969) armed 0.42 % disarmed, gate only 0.53 % <-- still answering fresh process, never armed 100.00 % unanswered (reversibility control) The gate write landed every time — is_disabled() read 0x0102[1:0] back as 0 on each disarm and nothing was logged. The die kept auto-ACKing anyway, and only re-initialising the chip stopped it. A gate-only disarm on this die is therefore a responder that reports passive while still transmitting: the inverse of the 8821AU episode in AdapterCaps.h, and the worse direction, since turning a passive monitor into an active transmitter is opt-in by contract and this left no way to take it back. Rtl8733bDevice::ClearAckResponder now composes the existing disable() with a new ack::retarget() that moves the ACK-match identity off the responder address and back to the adapter's own MAC. Restoring a real MAC rather than zeroing REG_MACID is load-bearing, not tidiness. Many Realtek MAC TX paths refuse to schedule a frame when the MAC ID is zero — the T1 canary bug that programming REG_MACID was introduced to fix (src/jaguar1/HalModule.cpp, EepromManager.h) — and a radio being disarmed live may still be injecting; a downstream consumer that exposes a runtime disarm lands it on the injecting adapter while video is flowing. Zero would not even remove the match, only move it: 00:00:00:00:00:00 has the I/G bit clear, so is_unicast() accepts it. The restore value cannot be zero: initialize() REFUSES to bring the MAC up unless mac_valid() (rejects all-zero, all-0xFF, I/G-set), and _mac_ready — which ClearAckResponder checks first — is set only after that succeeded. What the retarget restores is exactly the register state a NEVER-ARMED monitor session already ships in: program_mac writes this same MACID at init and net_type stays 0. So the disarm returns the port to the status quo ante rather than to silence — on a die that ignores the gate, a port matching its own MAC may still answer a frame addressed to it, precisely as an unarmed monitor radio would. It cannot create an exposure monitor mode does not already have. enable()'s BSSID writes (0x0618/0x061c) are not restored because program_mac never wrote them; the on-air result below, taken with that residue standing, is the evidence the engine does not match on it. SCOPE. disable() and is_disabled() are UNCHANGED, so Jaguar1/2/3 and Kestrel keep exactly the behaviour they were measured with. The evidence here is one die; jaguar3's own SetAckResponder comment says adopting a shared readback "wants a bench cell per die", and the same restraint applies to this. Halmac8733bMac::stop() is deliberately left alone. Its RCR and CR writes take the MAC down two lines later, so nothing answers after a teardown either way; making it symmetric would cost every teardown two register writes, including sessions that never armed, to fix a path that was never broken. CORRECTED CLAIMS. AdapterCaps.h certified the 8733B "disarm-proof" on the strength of tests/ack_txreport_matrix.sh, and src/rtl8733b/CLAUDE.md read the same cell as "disarmed, 0/1723". That cell is NEVER-ARMED, not disarmed: run_phase() starts no responder process at all when the responder MAC is empty. Same fresh-process fallacy the 8821AU line in that very paragraph already narrates. CLAUDE.md's post-teardown "ack_rate 0.00" is likewise not evidence for the net_type clear, because stop()'s RCR/CR disable makes that silence unavoidable. Both are fixed here, along with the SetAckResponder comment that called the gate "the whole gate" and IRtlDevice.h's Clear contract. Hardware verification with the fix, same rig: fresh process, never armed 100.00 % armed 0.63 % disarmed 100.00 % <-- the fix armed again 1.15 % Both directions now work live, with no re-init. tests/ack_responder_selftest.cpp models the register file and pins the arm, the gate-only disarm staying gate-only, the composed disarm landing on a NON-ZERO restore address, the re-arm round trip, and that zero is unicast. Neither on-air harness can cover this: every cell in ack_responder_check.sh and ack_txreport_matrix.sh is a fresh process, so their responder-off arm always starts from a chip that was never armed, and the behaviour only appears when one process arms and then disarms. Mutation-checked — a retarget that writes zero fails, one that short-circuits on `&&` fails, wrong offsets fail, a self-consistent endian flip of both helpers fails, and re-introducing the MACID clear into the shared disable() fails. ctest 55/55.
PR Summary by QodoFix RTL8733B live ACK disarm by restoring MACID
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
…ders Both from the Qodo review on the PR. 1. Own-MAC arm produced an un-disarmable responder (Bug, correctness). SetAckResponder's only address restriction was is_unicast, so the adapter's own EFUSE MAC was accepted. ClearAckResponder disarms by retargeting the identity BACK to that same address, so an arm on it could not be moved off: both MACID writes and the readback succeed without moving the match, and the success line is logged while the port keeps answering. That is precisely the silent-live-responder failure this backend's disarm exists to prevent, reintroduced through the one address the disarm cannot help with. Refused at arm time rather than papered over at disarm time. The alternative — deriving some other restore address — trades a corner case for a port left on an address that is neither the responder's nor the adapter's, and the honest end of the trade is simply not to offer an arm that cannot be taken back. Other generations disarm through the net_type gate alone and carry no such restriction. The self-test uses distinct responder and restore addresses, which is what left this case uncovered; its "does not cover" block now names it, since the refusal is device-layer and not reachable from the register-file model. 2. CLAUDE.md duplicated the capability evidence (Rule violation). The RTL8733B guide restated the matrix result, the never-armed caveat and the live-disarm finding that src/AdapterCaps.h and src/AckResponder.h already own. Replaced with a pointer to both, plus the harness name. Parallel copies are what let the "disarm-proof" mislabel this PR corrects survive in two places at once. ctest 55/55.
The refusal added in the previous commit was device-layer only: nothing in
the register-file model could reach it, and the self-test's "does not
cover" block said so. Both halves are now covered.
The decision moves into ack::disarmable_by_retarget(responder, restore) —
a predicate rather than a check inside retarget(), because retarget()
cannot know what the port was armed to; the caller owns both addresses
and is the only place the question can be asked. Rtl8733bDevice calls it
where it previously open-coded a memcmp.
The self-test now pins the predicate AND the reason it exists,
demonstrating the failure on the register file: arm on `own`, disarm,
retarget to `own` — every write and the readback succeed while the
identity never moves, so retargeted() reports success for a port still
matching the address it was armed to. Mutation-checked: a predicate
hardcoded to true fails the suite.
DEVICE-VERIFIED with devourer's own harness on an RTL8733B (0bda:f72b),
since DEVOURER_ACK_RESPONDER reaches the same SetAckResponder path, and
GetPermanentMacAddress returns exactly the _efuse.mac the guard compares
against:
DEVOURER_ACK_RESPONDER=<adapter's own EFUSE MAC>
-> "ACK responder cannot be armed to the adapter's OWN MAC"
then OpenIPC#406's init throw: "configured ACK responder could not be
armed" (rxdemo exit 134)
DEVOURER_ACK_RESPONDER=02:12:34:56:78:9a (control)
-> "hardware ACK responder armed for 02:12:34:56:78:9a (net_type=AP)"
and the demo ran its full window
The control is what makes it evidence: the refusal is specific to the own
MAC, not a broken build.
Worth noting for the demos rather than fixing here: rxdemo has no
try/catch around bring-up, so a refused configured responder surfaces as
std::terminate rather than a clean exit. examples/tx/main.cpp gained that
guard in OpenIPC#402; rxdemo did not. Pre-existing, and this refusal is simply a
new way to reach it.
ctest 55/55.
josephnef
left a comment
There was a problem hiding this comment.
The fix direction is right and the selftest builds and passes here, but the same file keeps two paths that leave the 8733B in exactly the state this PR exists to remove, and the composed disarm cannot be reached or re-measured from anything in the repo. Inline comments cover those; three things sit on lines outside the diff:
Stale claims that survive this PR. docs/rtl8733b.md:382 still cites off 0/1723 with retries pinned at 12 as the SetAckResponder off evidence — the cell AdapterCaps.h now declares never-armed. docs/aggregation.md:194 still says "The identity+net_type pair is the whole gate" for every generation, which contradicts the new AckResponder.h header ("NOT reliable in reverse"). docs/scheduled-mac.md:244 and AdapterCaps.h:174 still call the 8821A "disarm-proof-verified" from a responder-powered-down cell, i.e. the same never-armed shape this PR names as a fallacy four lines below. Either retract that or drop the "same fallacy as the 8821AU line" assertion.
The measurement is unreproducible from the tree. git grep ClearAckResponder at 586c1c1 finds only definitions — no demo, env knob, signal path or tests/*.sh calls it — and the on-air numbers live in two comments with different values (0.42/0.53 % in ClearAckResponder, 0.63/1.15 % in the selftest header). src/rtl8733b/CLAUDE.md then points at tests/ack_txreport_matrix.sh as "the harness", the script the CMake comment says cannot cover the disarm. Please add a mid-run disarm trigger to rxdemo (or an armed-then-disarmed phase in the matrix script), keep one number set, and point CLAUDE.md at that.
Pre-existing, not caused here, but worth knowing: examples/chanmig/main.cpp:574 arms the responder on 57:42:75:xx:xx:01. 0x57 has the I/G bit set, so the is_unicast() refusal from #406 makes chanmig --role drone exit 1 on every generation today — and this PR's selftest uses 0x57 as its group-address example.
…ot silence Maintainer review on the PR asked for the cell that separates "net_type is ignored on disarm" from "net_type is inert on this die". It is the second, and that changes the story rather than the fix. Measured, single-shot ACK rate (retry limit 0) at MCS3, Jaguar1 soliciting, RTL8733B responding, scoring the soliciting side's CCX tx.report: never-armed port, peer solicits the adapter's own EFUSE MAC 85.2 %, 82.5 % never-armed port, peer solicits an address nobody holds 0.0 % (control) port armed to X, peer solicits X 83.3 % A never-armed monitor answers on its own MAC at the same rate as a deliberately armed responder, and the negative control is clean. So the engine matches MACID and 0x0102[1:0] decides nothing on this die: program_mac is what makes it answer, arming merely repoints it. The gate is real on the generations the AP-mode work covered; this is a per-die exception, not a correction to the recipe. Two consequences, both now stated where they belong rather than argued: every never-armed RTL8733B monitor session already auto-ACKs unicast to its own EFUSE MAC — pre-existing, not created by an arm — and a disarm on this die cannot make the port silent, only take it off the responder address. The log line says that instead of claiming a disarm it cannot deliver. Review findings, in order: - Arm-failure rollback was still gate-only. enable() writes MACID before its final gate write, so a failed write or a failed verify() left the identity ON the responder address — a live responder by this PR's own measurement, with nothing else to restore it. Both rollback branches now run the same disarm ClearAckResponder uses. - The retarget was skipped whenever the gate half failed. A single timed-out read of 0x0102 made disable_verified() return false and the early return skipped the identity writes — the half that actually decides anything, gated on the half that decides nothing. Both halves are now attempted and neither gates the other, the same doctrine retarget() already applies internally. - The own-MAC arm refusal is REMOVED. It guarded MACID = own MAC with the gate closed, which the cells above show is exactly the state every never-armed session ships in: a per-chip API asymmetry with no safety payoff. Its predicate goes with it. - retarget()/retargeted() were a 4th and 5th copy of the MAC -> le32/le16 packing in a header whose verify() promises to keep the map in one place. Factored into macid_lo()/macid_hi()/macid_is(); enable(), verify() and retarget() all route through them, and retargeted() is gone — it was macid_is() under another name. - AdapterCaps.h: changelog framing replaced with the timeless fact, and the 8821A row no longer says "disarm-proof-verified" — same never-armed shape, same non-claim. - Self-test: the duplicated assertion (two comments, one check) is replaced by distinct raw-register checks, the unused macid_clear() is gone, and a new cell pins that verify() and macid_is() agree with the bytes in the file, which is the property the factoring exists for. Hardware, after the rework: fresh process, never armed 100.00 % armed 0.53 % disarmed 100.00 % armed again 0.63 % ctest 55/55.
…ff-cell claims Review follow-up: the measurement could not be reproduced from this repository, and three docs still cited a never-armed cell as disarm evidence. REACHABILITY. `git grep ClearAckResponder` found only definitions — no demo, env knob or script could disarm, so nothing in the tree could arm and then disarm inside one session, which is the only shape in which a disarm can be measured or regress. rxdemo gains DEVOURER_ACK_DISARM_AFTER_MS=<n>: disarm from a side thread <n> ms after bring-up while RX keeps running (Init() runs the loop and does not return, hence the thread; ignored without DEVOURER_ACK_RESPONDER). tests/ack_txreport_matrix.sh gains a `disarmed` phase that drives it — arm on MAC1, disarm at 2 s, solicit MAC1 — which is the only cell in that matrix measuring a disarm rather than a chip that was never armed. Measured with it, single-shot ACK rate, MCS3, Jaguar1 soliciting an RTL8733B responder: armed, no disarm 72.2 % ACKed armed, disarmed at 2000 ms 0.0 % ACKed First time the disarm has been scored with this repository's own tooling rather than a downstream consumer's counters. ONE NUMBER SET. The disarm-cycle percentages that disagreed between two comments are gone; what remains is the three-cell block (85.2/82.5 % never-armed on own MAC, 0.0 % control, 83.3 % armed) quoted identically in AckResponder.h, AdapterCaps.h, Rtl8733bDevice.cpp and the selftest header, plus the two rows above. RETRACTED CLAIMS. docs/rtl8733b.md cited `off 0/1723 with retries pinned at 12` as SetAckResponder off evidence — that cell is never-armed, and it now says so and points at the `disarmed` phase. docs/aggregation.md said "the identity+net_type pair is the whole gate" for every generation; which half is the gate is per-die, and on this one net_type is inert. docs/scheduled-mac.md called the 8821A "disarm-proof-verified" from the same responder-powered-down shape; retracted, as AdapterCaps.h:174 already was. src/rtl8733b/CLAUDE.md points at the `disarmed` phase and states the exposure plainly: a never-armed session on this die auto-ACKs unicast to its own EFUSE MAC, so it is not passive. NOT ADDRESSED HERE, deliberately: a parking MACID that would make an 8733B monitor genuinely silent. Zero is out (the MAC refuses to schedule TX with MACID 0), so any parking value needs its own on-air cell — a separate change, not a rider on this one. ctest 55/55.
|
Pushed Reachability. Scored with it, single-shot ACK rate, MCS3, Jaguar1 soliciting an RTL8733B responder:
First time the disarm has been measured with this repository’s own tooling rather than a downstream consumer’s counters. One number set. The disarm-cycle percentages that disagreed between two comments are gone. What remains is the three-cell block (85.2/82.5 % never-armed on own MAC, 0.0 % control, 83.3 % armed) quoted identically in Retracted. Deliberately not here: the parking MACID. Zero is out for the reason you give, so any parking value needs its own on-air cell — a separate change rather than a rider on this one. On I have not resolved your threads — they are yours to close once you are satisfied. |
josephnef
left a comment
There was a problem hiding this comment.
Re-verified ee0d36e here: selftest passes, and on the bench (RTL8733B 0bda:b733 responder, RTL8822CU soliciting MCS3 on ch36, ~1700 frames/cell, scored from the solicitor's CCX tx.report):
| cell | ack_rate | retries_mean |
|---|---|---|
| armed, no disarm | 1.00 | 0.04 |
armed, DEVOURER_ACK_DISARM_AFTER_MS=2000 |
0.00 | 12.0 |
armed, DEVOURER_ACK_DISARM_AFTER_MS=0 |
0.00 | 12.0 |
Every finding is addressed. One thing to follow up, not blocking: the rxdemo disarm timer counts from before Init(), while the arm happens inside bring-up on every generation. At 0 ms the log shows "disarming" printed before "armed"; the disarm still landed after the arm only because Rtl8733bDevice::Init holds _reg_mu across the whole bring-up, so ClearAckResponder blocked until the RX ring was up. Jaguar1/2/3 have no bring-up-wide lock, so a timer shorter than their bring-up would disarm before the arm and the disarmed phase would read a false "disarm broken". Anchoring the countdown on the arm (first RX callback, or a loop-entered flag) would make the phase portable. Two nits alongside: disarm_ack_responder() returns the gate result after logging "disarmed", so an inert-gate failure makes the callers log "hardware state UNKNOWN" although the identity read back; and the four-number block is now quoted verbatim in four files, where AdapterCaps.h could own it.
Head branch was pushed to by a user without write access
|
Pushed
Final rerun on the connected The review also uncovered a real but separate Jaguar1/RTL8812AU defect: its gate-only clear logs disarmed while the adapter continues ACKing the old responder MAC. I have kept that out of this die-scoped change and will send it as a separately tested follow-up. I am not claiming BlockAck disarm from the normal-ACK evidence above. |
|
One more fail-closed issue found in the pre-follow-up adversarial review and fixed in The verdict now requires 80% coverage by default ( |
|
The second independent adversarial pass found another verdict hole, fixed in The existing hardware rows are genuinely pinned ( |
|
One final harness-safety fix is in |
|
The Windows CI failure was a real portability issue in the new timer: |
josephnef
left a comment
There was a problem hiding this comment.
Re-approving at b5f3e00 (the previous approval was auto-dismissed by the push of 9a03e75, which also dropped the armed auto-merge).
The five commits since ee0d36e address the timer-ordering follow-up from the last review: the countdown now lives in Rtl8733bDevice::Init and starts after the locked bring-up, rxdemo refuses the knob on any other generation, and disarm_ack_responder() returns true on a verified identity move instead of the inert gate's status. The matrix waits on log lines instead of fixed sleeps and rejects sparse / unpinned verdicts.
Re-verified on hardware from a build of b5f3e00, tests/ack_txreport_matrix.sh with the RTL8733B (0bda:b733) as responder and an RTL8822CU (0bda:c812) transmitter, ch36, MCS3, ~1.7k frames per phase:
| phase | ack_rate | retries_mean | retry_pin_rate | coverage | verdict |
|---|---|---|---|---|---|
| on | 1.00 | 0.02 | – | 0.971 | ok |
| retarget | 1.00 | 0.03 | – | 0.972 | ok |
| off | 0.00 | 12.0 | 1.00 | 0.917 | ok |
| disarmed | 0.00 | 12.0 | 1.00 | 0.931 | ok |
Responder log for the disarmed phase shows the intended order: armed → RX ring submitted → "disarming ... 2000 ms after completed bring-up" → disarmed. AckResponderSelftest passes.
Problem
On RTL8733B, clearing port-0
net_typedoes not stop immediate responses. Anever-armed port already answers unicast frames addressed to the MACID written
during bring-up, even while
net_typereads NoLink. Consequently the previousgate-only
ClearAckResponder()could report success while the port continuedanswering for the configured responder address.
This also exposed a test gap: the matrix's old
offrow starts a fresh processwithout ever arming a responder. It was a useful never-armed control, but could
not prove a live arm -> disarm transition.
Change
ack::disable()gate-only for generations where that is themeasured recipe.
retarget to the adapter's valid EFUSE MAC.
reports failure; log the inconsistent transport result without returning the
contradictory "state unknown" after identity is verified.
bring-up, arming, channel setup, and monitor-RX configuration, so a zero-ms
run cannot clear a cold port and then be undone by later initialization.
disarmedmatrix row for the RTL8733B USB IDs. The harnessnow waits for exact arm/RX/disarm evidence, checks process liveness, requires
minimum submitted-frame and 80% report-coverage counts, requires at least
90% of an OFF cell's reports to be retry-pinned, validates numeric controls
before touching hardware, propagates analyzer failures, and aborts on early
TX exit instead of producing a green result from empty, sparse, or
outlier-driven data.
necessarily passive or
net_typeas a universal reverse gate.Restoring the initialization MAC is intentional. Zero is still a unicast
address by the I/G test and can inhibit Realtek TX scheduling; an arbitrary
parking address has not been qualified. This operation restores the pre-arm
identity, not radio silence: on RTL8733B the port may still answer for that
unadvertised initialization address, exactly as it did before arming. Arming
on that same address is therefore a behaviorally idempotent corner case.
On-air verification
All verdicts are scored on the soliciting adapter's CCX
tx.report, with retrylimit 12 and high report coverage.
f72b/ RTL8812AU, current treef72b/ RTL8812AU, current treef72b/ RTL8812AU, current treef72b/ RTL8812AU, current tree, zero-ms post-bring-up disarmb733/ RTL8822CUack_rate=1.00, retries mean 0.04b733/ RTL8822CU, 0-ms and 2000-ms disarmack_rate=0.00, retries mean 12.0f72b/ RTL8812AUf72b/ RTL8812AUThe final local matrix completed with 99.9-100% report coverage. A deliberately
occupied responder adapter also made the hardened harness exit nonzero before
transmission, confirming that the old zero-frame false-pass path is closed.
Other verification
ctest: 55/55git diff --checkScope boundary
This proves single-frame ACK behavior for the tested RTL8733B samples and
solicitors. It does not claim that a live disarm suppresses BlockAck on this
die, that restoring MACID is required on other generations, or that
net_typeis inert elsewhere. The independently observed RTL8812AU/Jaguar1gate-only disarm defect is deliberately left for a separate hardware-tested
follow-up.