kestrel: hardware ACK responder on the 8852B

[agustinglez]

The problem

SetAckResponder was not implemented on the AX generation, and ack_responder_ok read false with the note "matrix-measured 0% closure". The Jaguar recipe has nothing to port: the AX MAC has no port identity register to retarget. Its RMAC answers for an address only when the firmware has committed a valid ADDR_CAM entry carrying it as SMA.

Reading the physical CAM back (MAC table SRAM at 0x18850000, through R_AX_FILTER_MODEL_ADDR / R_AX_INDIR_ACCESS_ENTRY) shows why it was 0%: the entry add_self_sta sends for the NO_LINK self role is accepted over H2C and never committed. It reads valid=0.

The change

• What commits the entry. A USBPcap capture of the vendor Windows driver associating the adapter shows the sequence a real association uses. Registering the responder address as an INFRA station that way makes the firmware commit it. Two commands, in the vendor's order: join_info(INFRA, connect), then the station form of the addr_cam (SMA = responder address, TMA = a BSSID, the SHORT entry, BSSID CAM at index 1). New: KestrelFw::fw_join_info, KestrelFw::fw_upd_addr_cam_sta, HalKestrel::register_infra_sta.
• Only what was measured to matter is sent. The vendor sends role CLIENT, a macid unpause, and the CMAC and DMAC tables around those two. Each was dropped on the 8852B with no change in the commit or the ACK rate, and so was setting REC_ACK/DONE_ACK in the H2C header. Without join_info the entry never commits.
• SetAckResponder verifies by reading the CAM back (addr_cam_holds) and returns false when the entry did not commit. The commit is asynchronous: typically read back about 2.2 ms after the H2Cs, once 19.5 ms, and a one-shot read refused healthy arms during development. So it polls for up to 100 ms.
• It clears the response-abort CCA gates (R_AX_TRXPTCL_RESP_0 bits 23-28, new B_AX_RSP_CHK_*). On ch149, shared with a neighbour AP, one of three runs without the clear fell to 82.4% single-shot, while three with it stayed at >= 99.6%. On a quiet ch161 it made no difference.
• It refuses a group address or a call before bring-up. DEVOURER_ACK_RESPONDER arms at the end of Init and of InitWrite and throws if refused, as on Jaguar3.
• ClearAckResponder puts macid 0 back to the bring-up NO_LINK entry and restores the gates, then confirms the CAM no longer holds the address. rxdemo's DEVOURER_ACK_DISARM_AFTER_MS now accepts Kestrel through a ScheduleAckResponderDisarmForTest hook like the RTL8733B's; Stop() cancels a pending one.
• ack_responder_ok is true on the 8852B only. The 8852C firmware accepts the same two commands and leaves the entry invalid, so the arm refuses there. The vendor's 8852C association sends much more before the addr_cam, and which of it the C needs is open.

The change is additive. Nothing that ran before this calls the new code unless SetAckResponder or DEVOURER_ACK_RESPONDER is used.

Measured

There is no CCX tx.report on Kestrel, so the soliciting TX is an RTL8832CU (TP-Link TX50UH) counting the ACK frames addressed to its own TA, one attempt per frame. The responder is an RTL8852BU (TP-Link TX20U Nano) running rxdemo, or txdemo for the InitWrite row. Setup: ch161, MCS1, 20 MHz, ~480 frames/s for 10 s, both dongles on one desk with the injector at -15 dB, Windows 11. Three runs per row:

row	ACKed / sent
armed at Init (rxdemo)	14230 / 14430 (98.6%)
armed, then live disarm before TX	0 / 6603

The 8852C refuses the arm with "the firmware accepted but did not commit the ADDR_CAM entry". Full build and ctest pass on MSVC (78 tests, 4 skipped as on master).

What this does not show:

• Near field only. These numbers are a near-field bench, not a link budget.
• Host-counted ACKs, not the TX's own ARQ verdict. A separate run with the soliciting TMAC retrying and its ACK window narrowed found the responder's timing indistinguishable from a commercial AP's (both pass a 24 us window and fail at 20 us).
• The responder's own transmissions are outside this matrix. With this die's TX carrier-sense default (gates cleared), a responder that also sends unicast loses some ACKs to its own queued frames, and the response scheduler abort counter (0xCC18[23:16]) moves. That is noted in docs/kestrel-ack-responder.md and is not addressed here.

[openipc-ai]

/review

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Failed disarms cannot be retried ✓ Resolved

Description

ClearAckResponder() resets _ack_responder before submitting the NO_LINK rewrite or verifying
that the physical CAM changed. If either firmware command fails or the rewrite does not commit, the
method warns that the MAC may keep answering, but every subsequent clear returns immediately.

Code

src/kestrel/RtlKestrelDevice.cpp[608]

+  _ack_responder.reset();

Evidence

The reset precedes both best-effort firmware calls and the CAM poll; the failure branch explicitly
acknowledges that the MAC may still answer, while the entry guard makes a later retry a no-op.

src/kestrel/RtlKestrelDevice.cpp[603-623]
src/kestrel/HalKestrel.h[240-259]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failed CAM rewrite leaves the responder potentially active, but clearing its saved address prevents another disarm attempt.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[603-627]
## Recommended Fix
Keep the armed address until CAM read-back confirms removal. On failure, preserve retryable state and report the unsuccessful rollback.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. ACK docs duplicate the API contract ✓ Resolved

Description

docs/kestrel-ack-responder.md repeats the responder's ADDR_CAM commitment and arming sequence
already documented in RtlKestrelDevice.h. Readers must update both the standalone guide and the
header when this contract changes, allowing the two descriptions to drift.

Code

docs/kestrel-ack-responder.md[R9-12]

+The AX MAC has no Jaguar-style port identity register to retarget: the RMAC
+answers for an address only when the firmware has COMMITTED a valid
+ADDR_CAM entry carrying it as SMA. `add_self_sta`'s NO_LINK entry is accepted
+over H2C and never committed (the physical entry, read through

Evidence

The checklist requires documentation to point to authoritative header doc-comments instead of
copying their contracts. The new guide describes the same committed ADDR_CAM requirement and
association-based arming sequence that the new RtlKestrelDevice header comment documents.

CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims: CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims: CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims: CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims: CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims: CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims: CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims: CLAUDE.md: Avoid Duplicated Header Contracts and One-Sided Measurement Claims
docs/kestrel-ack-responder.md[9-21]
src/kestrel/RtlKestrelDevice.h[87-96]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Kestrel ACK responder guide duplicates the mechanism and commitment contract already documented in the public device header, creating two maintenance points that can diverge.
## Fix Focus Areas
- docs/kestrel-ack-responder.md[9-21]
- src/kestrel/RtlKestrelDevice.h[87-96]
## Recommended Fix
Remove the duplicated API-contract explanation from the guide and point readers to the authoritative header doc-comment. Keep only measured results, setup details, limitations, and other information that is not already part of the API contract.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. A failed retarget can hide the old arm ✓ Resolved

Description

SetAckResponder() replaces _ack_responder with the requested MAC before invoking rollback, so
ClearAckResponder() checks only whether that new MAC is absent. If retargeting fails while the
previous CAM entry remains committed and the rollback rewrite also fails, the call returns false and
reports a successful clear without checking the address the hardware can still answer.

Code

src/kestrel/RtlKestrelDevice.cpp[R592-593]

+    _ack_responder = mac;
+    ClearAckResponder();

Evidence

The failure branch stores the new address, and clear copies that stored address for its sole CAM
comparison. Firmware submission and physical commitment are separate, so a failed retarget does not
establish that the old entry was removed.

src/kestrel/RtlKestrelDevice.cpp[573-600]
src/kestrel/RtlKestrelDevice.cpp[603-626]
src/kestrel/HalKestrel.h[269-290]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failed retarget overwrites the previously armed address before rollback, so verification can miss an old CAM entry.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[586-594]
- src/kestrel/RtlKestrelDevice.cpp[603-627]
## Recommended Fix
Preserve the prior armed address through the retarget attempt. On failure, verify the physical CAM against both the old and requested addresses, and retain retryable state if rollback is not confirmed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Timed disarm can overwrite a new arm ✓ Resolved

Description

SetAckResponder() and ClearAckResponder() have no serialization around _ack_responder, the
saved response register, or the CAM rewrite. If a caller retargets the public responder while the
new timer thread clears it, their state changes and asynchronous firmware commands can interleave,
leaving the final arm different from the one either call verified.

Code

src/kestrel/RtlKestrelDevice.cpp[R255-257]

+      _logger->info("DEVOURER_ACK_DISARM_AFTER_MS: disarming Kestrel ACK "
+                    "responder {} ms after completed bring-up", delay_ms);
+      ClearAckResponder();

Evidence

The timer directly calls clear on another thread. Both methods update the same optional and register
and issue commands for the same CAM entry without a lock; clear resets its optional before its
asynchronous rewrite finishes.

src/kestrel/RtlKestrelDevice.cpp[247-258]
src/kestrel/RtlKestrelDevice.cpp[565-579]
src/kestrel/RtlKestrelDevice.cpp[603-619]
src/kestrel/RtlKestrelDevice.h[97-104]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The timer invokes clear on a separate thread while public arm calls can update the same state and CAM.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[545-627]
- src/kestrel/RtlKestrelDevice.h[297-302]
## Recommended Fix
Protect the entire arm and clear transactions, including CAM read-back and saved-register updates, with a device-level responder mutex. Ensure the timer follows the same locking path.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View review recommended (1)

5. Arming changes injected TX context 🐞 Bug ≡ Correctness

Description

SetAckResponder() programs the responder address into CAM index 0 and MACID 0, replacing the
self-station context installed by InitWrite() for injected frames. When a TX-capable session arms
the responder, send_packet() still builds its management and data descriptors with MACID 0, so
injection no longer uses the self-station CAM context that bring-up established for it.

Code

src/kestrel/RtlKestrelDevice.cpp[568]

+  const bool sent = _hal.register_infra_sta(m, bssid);

Evidence

Bring-up sets CAM index 0 and MACID 0 for injected frames; the new arm overwrites those same
identifiers, while the transmit path continues to select MACID 0. The clear path restores the
injection context, confirming the shared ownership while armed.

src/kestrel/RtlKestrelDevice.cpp[327-336]
src/kestrel/HalKestrel.h[281-290]
src/kestrel/RtlKestrelDevice.cpp[568-568]
src/kestrel/RtlKestrelDevice.cpp[1383-1392]
src/kestrel/RtlKestrelDevice.cpp[609-613]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The responder reuses the CAM index and MACID that injected frames continue to reference.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[331-336]
- src/kestrel/RtlKestrelDevice.cpp[568-568]
- src/kestrel/HalKestrel.h[281-290]
- src/kestrel/RtlKestrelDevice.cpp[1383-1392]
## Recommended Fix
Allocate a distinct firmware station/CAM context for the responder where supported, leaving injection's MACID 0 context intact. If the hardware requires sharing, explicitly coordinate TX descriptors and document or enforce the resulting limitation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Destroying the device can race a pending test disarm ✓ Resolved

Description

~RtlKestrelDevice() runs quiesce_tx, stop_wp_drain, and pcie_quiesce before
_ack_disarm_thread is cancelled and joined by member destruction. If the device is destroyed
without Stop() as a DEVOURER_ACK_DISARM_AFTER_MS delay expires, the callback can call
ClearAckResponder() and send H2Cs or read CAM registers while transport teardown is underway,
including while PCIe ring indices are cleared.

Code

src/kestrel/RtlKestrelDevice.h[302]

+  std::jthread _ack_disarm_thread;

Evidence

The destructor body tears down the transport without touching _ack_disarm_thread; as a
std::jthread member, it is joined only after that body completes unless Stop() resets it first.
The thread callback calls ClearAckResponder(), whose register I/O and H2Cs use _hal and
_device, so it can access the transport during teardown.

src/kestrel/RtlKestrelDevice.cpp[95-103]
src/kestrel/RtlKestrelDevice.cpp[244-259]
src/kestrel/RtlKestrelDevice.h[69-72]
src/kestrel/RtlKestrelDevice.cpp[95-105]
src/kestrel/RtlKestrelDevice.cpp[247-258]
src/kestrel/RtlKestrelDevice.cpp[603-619]
src/kestrel/RtlKestrelDevice.h[68-74]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ACK-disarm callback can use the device while its destructor is quiescing the transport because the thread is not joined until member destruction.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[95-105]
- src/kestrel/RtlKestrelDevice.cpp[247-258]
## Recommended Fix
Add `_ack_disarm_thread = {};` as the first statement of `~RtlKestrelDevice()` to request stop and join the thread before quiescing TX or PCIe. Keep the existing early join in `Stop()`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[josephnef]

Approving, with one hardware caveat stated up front: there is no RTL8852BU on this bench today (two TX50UH / RTL8832CU only), so I could not reproduce the positive ACK rows. What I could do:

8852C refusal path (the die the PR says must refuse) — rxdemo on a TX50UH, ch6, DEVOURER_ACK_RESPONDER=02:12:34:56:78:9a, PR build 808d384:

Kestrel: join_info (macid=0 op=0 net_type=2 self=0 wifi=1 port=0 band=0) -> sent
Kestrel: upd_addr_cam_sta (... sma=02:12:34:56:78:9a tma=02:00:00:00:00:01) -> sent
[E] Kestrel: ACK responder for 02:12:34:56:78:9a did not arm: the firmware accepted but did not commit the ADDR_CAM entry (H2C sent)
Kestrel: fw_role_maintain ... / upd_addr_cam (net_type=0 sma=57:42:75:05:d6:00) / upd_cctl ...
Kestrel: hardware ACK responder disarmed (ADDR_CAM entry 0 no longer holds 02:12:34:56:78:9a)
terminate called after throwing an instance of 'std::runtime_error'
  what():  Kestrel: configured ACK responder could not be armed

Exactly the sequence described: H2Cs sent, 100 ms CAM poll never sees valid, NO_LINK rollback verified by read-back, then the configured-arm throw. No wedge; the adapter came straight back up on the next run.

No regression on the untouched path — same adapter, ch6, 12 s, master 74c6b37 vs this branch, two reps each: 5 / 5 / 5 / 5 rx.pkt (quiet 2.4 GHz lab), and the two Kestrel coex: LTE interface not ready errors are present on master too.

Build/CI — Linux Release build + ctest 82/82; full CI matrix green on 808d384.

Code — the second commit covers the qodo threads I would have raised (retryable clear, retarget rollback checking both addresses, mutex, destructor join order). Two small observations, neither blocking:

• The throw from arm_configured_ack_responder is the Jaguar1/Jaguar3 convention, so consistent — but on the 8852C it is the guaranteed outcome of setting the env var, and in rxdemo it surfaces as terminate + core dump rather than a logged exit. If that is ever softened it should be softened for all three at once, not here.
• The failure error line could carry waited_us/polls the way the success info line does; the debug line that has them is compiled out in Release (DEVOURER_LOG_MAX_LEVEL NDEBUG-derived), so a Release user on the C die sees no poll duration at all.

The 8852B rows stand on the author's measurement; when a TX20U Nano is back on this bench I'll run tests/ack_txreport_matrix.sh with the 8812CU as the CCX-reporting TX against it.