jaguar1: restore port identity when disarming 8812 ACKs - #411
Conversation
PR Summary by QodoRestore RTL8812 port identity when disarming Jaguar1 ACKs
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. Unverified beacon claim release
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
3d85cca to
716d1b2
Compare
|
/review |
|
Code review by qodo was updated up to the latest commit 716d1b2 |
716d1b2 to
0941ec0
Compare
|
/review |
|
Code review by qodo was updated up to the latest commit 0941ec0 |
0941ec0 to
401dbf9
Compare
|
/review\n\nPlease review the current 401dbf9 head. In particular, please re-check delayed-disarm/rollback token ownership and failed-clear semantics; the exact stale-timer replacement race and normal matching-timer disarm were both rerun on RTL8812AU hardware before this update. |
|
/review |
|
Code review by qodo was updated up to the latest commit 401dbf9 |
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Reviewed at 401dbf9. No correctness findings; the three earlier Qodo items (unverified beacon release, ownership only on CHIP_8812, stale timer clearing a new arm) are all closed at this head by the verified stop, the all-die claim flags and the arm-generation token. Two doc nits inline, neither blocking.
What I checked:
- ASan+UBSan build of the branch, ctest 55/55.
- Token/rollback ordering in
Init: the disarmjthreadis declared after the scope-exit guard, so it is stopped and joined before the rollback runs, and a fired timer has already invalidated the token the rollback checks. A failed re-arm invalidates the previous token before its first write, so no stale timer can reach the replacement. Claim flags are retained on every unverified path. - Hardware regression on the non-8812 Jaguar1 path, since the PR touches every die's arm path. No RTL8812AU was on the bench, so the 8812 cell itself is not reproduced here; an RTL8821AU responder (
0bda:a81a) against an RTL8822CU solicitor, channel 36 / MCS3 / retry limit 12:
| phase | reports | ACK rate | mean retries |
|---|---|---|---|
| on | 1569 | 1.00 | 0.05 |
| retarget | 1549 | 1.00 | 0.04 |
| off (never armed) | 1526 | 0.00 | 12.0 (pin rate 1.00) |
The disarmed phase reports SKIP on the 8821A as designed.
One behaviour change worth a line in the PR body: on CHIP_8812 an adapter whose MACID reads zero or group at arm time is now refused outright (has_safe_restore_mac), where previously the arm went ahead. Correct, since restoring zero stops TX scheduling, but it is a new failure mode a caller can hit.
| * airing and contaminated the next test's witness) — so any beaconing | ||
| * session that ends without a device power-cycle must call this. Idempotent; | ||
| * returns false when no beacon was active. */ | ||
| * returns false when no beacon was active or shutdown could not be verified. |
There was a problem hiding this comment.
This base-contract sentence now promises a verified stop, but only Jaguar1 verifies: RtlJaguar2Device::StopBeacon and RtlJaguar3Device::StopBeacon still return true unconditionally after their writes. Either scope the sentence to Jaguar1 ("Jaguar1 additionally reads back the three stop controls and returns false if any is still active") or leave the base contract as it was.
| * the live-disarm claim for the responder used in that run. On the reference | ||
| * RTL8812AU, that cell found that the old gate-only clear left 1946/1946 | ||
| * soliciting reports ACKed. Restoring the captured pre-arm MACID produced | ||
| * 0/1052 ACKed with retries pinned at 12; the never-armed counterpart was |
There was a problem hiding this comment.
Three different count sets now describe the same claim: this header says 0/1052 and 0/1033, the PR body says 0/997 and 0/1077, and docs/scheduled-mac.md points here as the owner of "the exact counts". They are different runs and all agree, but a caps header comment is turning into a measurement log. Keep one set here (or point at the doc that carries the table) so the next edit does not have to reconcile them.
| "the current port identity could not be read"); | ||
| return false; | ||
| } | ||
| if (!devourer::ack::has_safe_restore_mac(identity)) { |
There was a problem hiding this comment.
Behaviour change to state in the PR body: before this PR an 8812 with a zero/group MACID at arm time still armed; now the arm is refused. The refusal is right (restoring zero stops TX scheduling), but it is a new failure mode for a caller with an unprogrammed EFUSE MAC.
2 participants
Problem
Jaguar1's existing
ClearAckResponder()only wrotenet_type=NoLink. On thereference RTL8812AU (
0bda:8812) that write read back successfully, but theport continued sending SIFS ACKs for the old responder MAC: 1946/1946
soliciting CCX reports were still ACKed.
This is not the RTL8733B gate model: a never-armed RTL8812AU did not answer on
its own MAC (0/1074), but arm-to-own-MAC followed by the old gate-only clear
did (1120/1120). The clear therefore needs both NoLink and a move away from the
armed MAC.
Change
CHIP_8812, snapshot the live port-0 MACID/BSSID before the first arm andrestore/readback-verify that exact identity during clear and failed-arm
rollback.
MACID; either case would make a later retarget unsafe or ineffective.
later
Init/InitWritestep throws.Init/InitWriterollback to the exact completedACK arm. Clearing or replacing that arm invalidates its token, so stale
cleanup cannot disarm a newer responder.
its first register mutation, including a partially failed setup, and ACK
arming is refused until
StopBeacon()verifies the beacon-function,TX-beacon, and net-type stop controls and releases it. An unverifiable stop
retains ownership and recovery state for retry.
measured
0bda:8812responder.Hardware evidence
Final tree, RTL8812AU
0bda:8812responder,0bda:c812solicitor, channel 36,MCS3, retry limit 12, 12 seconds per phase:
The zero-ms countdown is created by the Jaguar1 backend only after completed
bring-up and verified arm; it cannot win a race against
Init(). A separate2000-ms run also produced 0/1077 ACKed with retries pinned at 12 after clear.
The final own-MAC negative test refuses the arm before the responder becomes
active.
The stale-timer race was also exercised directly on this tree: arm A scheduled
an 8000-ms disarm, A was explicitly cleared and replaced by arm B after four
seconds, and A's old timer reported a stale/no-op at its deadline. Arm B then
ACKed 377/377 reports (ACK rate 1.00, mean retries 0.04, coverage 0.883) before
its own verified clear. The normal matching-timer path is covered by the
0/997 same-process disarm cell above.
The ACK-rate result establishes MACID's behavioral role. BSSID is restored and
read back as defensive port-state cleanup; this PR does not claim BSSID gates
ACK behavior.
The all-die port-ownership guard was separately exercised on the RTL8812AU:
StartBeacon()refused while the ACK responder owned port 0, succeeded afterverified ACK clear, and a subsequent
ClearAckResponder()while the beaconowned the port left it airing. An independent RTL8822CU witness captured 48
valid hardware beacons after that clear. No RTL8821AU/RTL8814AU was connected
for this rerun; their ownership path is the same backend state and lock, while
their existing gate-only responder clear remains unchanged.
The final verified-stop tree was then exercised in the opposite direction.
The RTL8812AU aired 55 valid beacons before
StopBeacon()returned true; theRTL8822CU witness saw zero beacons after a 250-ms drain window. In the same
RTL8812AU process, ACK ownership was acquired immediately afterward and the
RTL8822CU solicitor measured 372/372 successful reports (ACK rate 1.00, mean
retries 0.02, coverage 0.882). The responder then completed verified
MACID/BSSID restoration. This hardware result covers the normal stop path;
the transport-failure branches are register-model failure-injection tests.
Scope and validation
CHIP_8812path (including the RTL8811AU cut), but RTL8811AU was notseparately measured.
RTL8733B behavior is unchanged.
claim.
Jaguar3-only builds pass; ACK analyzer selftests,
AckResponderSelftest,ShellCheck, bash syntax, and
git diff --checkpass.zero/group/equal rollback rejection, false-return and thrown-write failure
injection, all-four-register restore attempts, independent attempts of all
three beacon stop operations, fail-closed readback, and false-but-landed
transfer handling.