Repository navigation
kestrel: hardware ACK responder on the 8852C - #489
Open
agustinglez wants to merge 1 commit into
Open
agustinglez wants to merge 1 commit into
agustinglez wants to merge 1 commit into
Conversation
On the 8852C, SetAckResponder refused: the firmware ignores the addr_cam H2C altogether. After it neither the address CAM entry nor the BSSID CAM entry it carries is written, and nothing changed that: a byte-for-byte replay of the vendor Windows driver's association H2Cs, a join_info alone, a 64-byte addr_cam. The same replay commits on the 8852B. The RMAC still answers from the CAM, so on this die the arm writes the station entry from the host (HalKestrel::write_addr_cam_sta), the way the vendor's mac_upd_addr_cam does when the firmware is not running: the same short entry the 8852B firmware commits, VALID cleared first (a retarget rewrites a live entry), then the body, then VALID. No H2C is sent, and the clear is the host clear alone (no join_info disconnect for a station the firmware never registered). - The responder's slot is per die (ack_slot()): 127 on the 8852B (unchanged) and 63 on the 8852C, its last entry: the CAM reads 0xdeadbeef from entry 64 up. RegisterPeerSta/RegisterBeamformee keep refusing it. - The response-abort gate clear is mandatory on this die: its EDCCA reads its own floor as busy, and with the gates armed every ACK was dropped. - The station entry encoding moves to KestrelAddrCam.h and is shared by the 8852B's addr_cam H2C (byte-identical payload) and the 8852C's host write; tests/kestrel_addr_cam_selftest.cpp pins it against hardware read-backs (an entry the 8852B firmware committed, one the host wrote on an 8852C). - ack_responder_ok is true on both dies; the SetAckResponder contract and the flag's comment say that on the 8852C a true return does not guarantee every bring-up answers (see below). Measured, TX50UH (RTL8832CU) responder, TX20U Nano (RTL8852BU) soliciting and counting the ACKs it receives, ch161, MCS1, one attempt per frame, 10 s per cell, 3 runs per row: 13831/14041 armed at Init, 14239/14408 retargeted, armed at InitWrite 98.9% and 97.0% (third run a dead session), 0/5993 at the old address, 0/6062 never armed, 0/5965 after a live disarm; arm/retarget/clear/re-arm correct in the live cycle; injection delivery unchanged armed vs unarmed. The RX descriptor shows every frame to the responder matching the host-written entry. 8852B regression on this code: 99.5-99.8% armed and retargeted, 0 after a disarm, cycle correct, slot 127. Not solved: some 8852C bring-ups answer no ACK for the whole session while the entry reads back and the adapter airs its own frames. It tracks run time (0 of 12 sessions straight after a long power-off, 3 of the next 12; up to three in four after a day of back-to-back runs). The MAC configuration registers are identical to a good session's; the difference is that the MAC classifies the soliciting frames as its own BSS's (WMAC_NAV_UP_INFO, PS_RXINFO). Response NAV checks, the BSSID CAM, the injector, host RX cost and arm timing are ruled out; the doc lists what was measured. Full MSVC build and ctest pass (79 tests with the new selftest, 4 skipped as on master).
|
/review |
Code Review by Qodo
1. 8852C advertises ACKs it cannot deliver
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| * with ~90% witness capture of a 9-copy truth — obedient, no wedge). */ | ||
| c.tx_retry_limit_ok = true; | ||
| c.ack_responder_ok = _variant == kestrel::ChipVariant::C8852B; | ||
| c.ack_responder_ok = true; |
There was a problem hiding this comment.
1. 8852c advertises acks it cannot deliver 📘 Rule violation ☼ Reliability
GetAdapterCaps() sets ack_responder_ok to true for every Kestrel variant, although SetAckResponder() treats CAM read-back as success without confirming that the 8852C emits ACKs. On 8852C bring-ups where the entry reads back but the responder sends no ACK for the entire session, callers relying on that boolean cannot distinguish a working responder from a dead one.
Agent Prompt
## Issue description
On the 8852C, `ack_responder_ok` is unconditionally true even though some sessions successfully arm and read back the responder entry but never emit an ACK. Callers using that flag cannot tell whether ACK response is actually available.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[1159-1170]
- src/kestrel/RtlKestrelDevice.h[99-111]
- src/AdapterCaps.h[199-211]
- docs/kestrel-ack-responder.md[90-106]
## Recommended Fix
Do not make CAM-only arming appear to confirm reliable ACK support on the 8852C. Return the documented not-ported or unsupported capability result unless runtime health can verify that ACKs are emitted; alternatively, add a distinct best-effort capability field that is true on the 8852C and false on the 8852B, so callers requiring a reliably working responder need not rely on `ack_responder_ok` alone. Document the resulting per-die capability behavior alongside the existing explanation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+343
to
+346
| write_mac_mem32(base, 0u); | ||
| for (int i = 7; i >= 1; --i) | ||
| write_mac_mem32(base + 4 * i, w[i]); | ||
| write_mac_mem32(base, w[0]); |
There was a problem hiding this comment.
2. Failed writes appear to arm the responder 🐞 Bug ☼ Reliability
write_addr_cam_sta() ignores register-write failures, while addr_cam_holds() checks only the valid bit and the responder address rather than the complete station entry. On the new 8852C host-write path, a failed write to another field, such as the station identifier, can leave an incomplete entry that SetAckResponder() reports as armed.
Agent Prompt
## Issue description
The new 8852C host-write path can report success after a partial address-CAM write because it discards transfer failures and verifies only the valid bit and responder address.
## Fix Focus Areas
- src/kestrel/HalKestrel.h[316-319]
- src/kestrel/HalKestrel.h[335-372]
- src/kestrel/RtlKestrelDevice.cpp[578-593]
## Recommended Fix
Propagate failures from both register transfers in each indirect write, stop before restoring VALID if a body write fails, and require read-back of the complete expected station entry before recording a successful arm. Route failures through the existing rollback path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| bool RtlKestrelDevice::peer_slot_reserved(const char *who, uint8_t macid, | ||
| uint8_t addr_cam_idx) const { | ||
| if (macid != kAckMacid && addr_cam_idx != kAckAddrCamIdx) | ||
| if (macid != ack_slot() && addr_cam_idx != ack_slot()) |
There was a problem hiding this comment.
3. Peer registrations can silently claim an out-of-range cam slot 🐞 Bug ☼ Reliability
peer_slot_reserved only refuses macid/addr_cam_idx equal to ack_slot(), which is now 63 on the 8852C instead of 127; this frees slot 127 for RegisterPeerSta/RegisterBeamformee to use on that die. The 8852C's address CAM only has 64 real entries (the PR states entries 64 and above read back 0xdeadbeef), so a peer registered at index 127 goes through the same addr_cam H2C the firmware ignores and fails silently rather than being refused with a clear error.
Agent Prompt
## Issue description
`peer_slot_reserved` only checks equality against `ack_slot()` (63 on the 8852C), leaving indices 64-127 free for `RegisterPeerSta`/`RegisterBeamformee` even though the PR documents that the 8852C's address CAM only has 64 real entries and reads back `0xdeadbeef` from entry 64 onward. A caller registering a peer in that range gets no error but the entry never actually commits.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[1098-1107]
- src/kestrel/RtlKestrelDevice.h[324-332]
## Recommended Fix
Add a per-die CAM-capacity check (e.g. refuse any `addr_cam_idx >= 64` when `_variant == ChipVariant::C8852C`) in `peer_slot_reserved` or in the registration path, alongside the existing exact-match check against `ack_slot()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
The problem
On the 8852C,
SetAckResponderrefused: the firmware ignores the addr_cam H2C altogether. After it, neither the address CAM entry nor the BSSID CAM entry it carries is written, and nothing changed that: a byte-for-byte replay of the vendor Windows driver's association H2Cs (CMAC table, DMAC table, join_info, addr_cam), a join_info alone, and a 64-byte addr_cam. The same replay commits on the 8852B.The change
HalKestrel::write_addr_cam_sta), the way the vendor'smac_upd_addr_camdoes when the firmware is not running: VALID cleared first (a retarget rewrites a live entry), then the body, then VALID. No H2C is sent, and the clear is the host clear alone. The 8852B path is unchanged.ack_slot()): 127 on the 8852B, as before, and 63 on the 8852C, its last entry (the CAM reads back0xdeadbeeffrom entry 64 up).RegisterPeerSta/RegisterBeamformeekeep refusing it.KestrelAddrCam.h, shared by the 8852B's addr_cam H2C (the payload is byte-identical to before) and the 8852C's host write. A new headless selftest,kestrel_addr_cam, pins it against hardware read-backs: an entry the 8852B firmware committed, and one the host wrote on an 8852C.ack_responder_okis true on both dies. TheSetAckRespondercontract and the flag's comment say that on the 8852C a true return does not guarantee every bring-up answers (below).AdapterCaps.handdocs/kestrel-ack-responder.mdare updated.Measured
TX50UH (RTL8832CU) as responder, TX20U Nano (RTL8852BU) soliciting and counting the ACKs it receives, ch161, MCS1, one attempt per frame, 10 s per cell, 3 runs per row:
Arm A, retarget to B, clear, re-arm A in one session: correct in the live run (A 98.3%, then A 0% / B 99.0%, both 0% after the clear, A 99.0% after the re-arm); the other two runs were dead sessions. Injection delivery to a witness is unchanged armed vs unarmed. The long RX descriptor shows every frame addressed to the responder matching the host-written entry (macid 63).
8852B regression on this code: 99.5-99.8% armed and retargeted, 0 after a live disarm, the arm/retarget/clear/re-arm cycle correct, still on slot 127.
Full MSVC build and
ctestpass (79 tests with the new selftest, 4 skipped as on master).What this does not solve
Some 8852C bring-ups answer no ACK at all: the responder arms, the entry reads back, the adapter airs its own frames, and it answers nothing for the whole session. It tracks how long the adapter has been running: 0 of 12 sessions straight after a long power-off, 3 of the next 12 (the matrix above), up to three in four after a day of back-to-back runs. The RF thermal meter is too coarse to separate temperature from uptime.
In failed sessions the MAC configuration registers are identical to a good session's, and the response abort counters stay at 0 (the response is never attempted). What does differ is that the MAC classifies the soliciting frames as its own BSS's traffic (
R_AX_WMAC_NAV_UP_INFOrecords intra-BSS NAV updates,R_AX_PS_RXINFOflags port-0 data). Ruled out by measurement: the response NAV checks, the BSSID CAM, the injector, the host's per-frame RX cost, and arm timing. The doc lists what was measured. I don't have the cause yet.One bench, one pair of dongles, near field.