kestrel: ACK responder on its own macid / ADDR_CAM entry

docs/kestrel-ack-responder.md

@@ -26,7 +26,27 @@ was measured to get there.
   On ch149, shared with a neighbour AP, one of three runs without the clear
   dropped to 82.4% single-shot; three with it stayed at >= 99.6%. On a quiet
   ch161 it made no difference.
-- **Clear** is exercised live by rxdemo's `DEVOURER_ACK_DISARM_AFTER_MS`.
+- **Own macid.** The station sits at macid 127 / ADDR_CAM entry 127 (the last
+  entry of the 0x7f search range), apart from the injection context at macid 0
+  / entry 0 and from the low slots peer registrations take (`RegisterPeerSta`
+  callers use 1, 2, ...; it and `RegisterBeamformee` refuse 127). The
+  firmware commits entry 127 in the same ~2.2 ms as entry 1 or 0. With the
+  station at entry 1 during development, entry 0 read back byte-identical
+  before the arm, after it and after the clear. Sharing entry 0 (the previous
+  layout) had no measurable cost: a witness counted the same >= 2900 of 3000
+  injected frames armed and unarmed, on both layouts. The separation is for
+  the injection context's integrity, not a delivery fix.
+- **Unexplained cell.** In about 55 armed cells across development (station at
+  entry 1), one read the entry back committed (after 10 ms rather than ~2.2
+  ms) and answered no ACKs; other slow commits were fine and it has not
+  reproduced.
+- **Clear needs the host.** Measured at entry 1: after `join_info(disconnect)`
+  and a NO_LINK addr_cam, the entry stays VALID with the responder's address;
+  a role REMOVE, or role CREATE plus NO_LINK, leaves it valid too. Clearing
+  VALID from the host takes, and re-read for 2 s it stays clear. Because those
+  H2Cs land asynchronously, the clear repeats until the entry has read back
+  empty for 50 ms, and only that counts as disarmed. rxdemo's
+  `DEVOURER_ACK_DISARM_AFTER_MS` exercises it live.
 - **8852C:** its firmware accepts both commands and leaves the entry invalid,
   so the arm refuses. The vendor's 8852C association sends far more before the
   addr_cam (BB/RF offload, port configuration); which of it the C needs is open.

src/AdapterCaps.h

@@ -194,11 +194,14 @@ struct AdapterCaps {
    * tests/ack_txreport_matrix.sh run with the 8733B as the responder),
    * 8852B (Kestrel has no CCX report, so the soliciting TX is an 8832CU
    * counting the ACK frames it receives, one attempt per frame, MCS1 on
-   * ch161, 3 runs per row: 14230/14430 armed at Init, 14391/14431 armed at
-   * InitWrite, 14347/14425 retargeted, 0/6440 at the old address after the
-   * retarget, 0/6194 never armed, 0/6603 after a live disarm). The 8852C
-   * reads false: its firmware does not commit the station entry, and the
-   * arm refuses on the read-back.
+   * ch161, 3 runs per row, responder on its own macid 127 / ADDR_CAM entry
+   * 127: 14414/14422 armed at Init, 14376/14417 armed at InitWrite,
+   * 14419/14432 retargeted, 0/6597 at the old address after the retarget,
+   * 0/6575 never armed, 0/6561 after a live disarm; one cell in about 55
+   * across the development runs, at entry 1, read the entry back committed
+   * and answered no ACKs, not reproduced since). The 8852C reads false: its
+   * firmware does not commit the station entry, and the arm refuses on the
+   * read-back.
    *
    * The `on`, `retarget`, and legacy `off` rows establish arming, retargeting,
    * and a never-armed control. A backend-owned `disarmed` row supports only

src/kestrel/HalKestrel.h

@@ -279,17 +279,47 @@ class HalKestrel {
    * with addr_cam_holds(). The 8852C firmware accepts both commands and does
    * not commit. */
   bool register_infra_sta(const uint8_t own[6], const uint8_t bssid[6],
-                          uint8_t macid = 0, uint8_t band = 0, uint8_t port = 0) {
+                          uint8_t macid, uint8_t addr_cam_idx,
+                          uint8_t bssid_cam_idx, uint8_t band = 0,
+                          uint8_t port = 0) {
     bool ok = _fw.fw_join_info(macid, reg::MAC_AX_NET_TYPE_INFRA,
                                reg::MAC_AX_SELF_ROLE_CLIENT,
                                reg::MAC_AX_WIFI_ROLE_STATION, port, band,
                                /*connect=*/true);
     ok = _fw.fw_upd_addr_cam_sta(macid, own, bssid, reg::MAC_AX_NET_TYPE_INFRA,
-                                 /*addr_cam_idx=*/0, /*bssid_cam_idx=*/1) &&
+                                 addr_cam_idx, bssid_cam_idx) &&
          ok;
     return ok;
   }
 
+  /* Undo register_infra_sta in the firmware: join_info(disconnect), then a
+   * NO_LINK addr_cam for `macid`. On the 8852B this does NOT invalidate the
+   * physical entry (it stays VALID with the station's SMA), so callers also
+   * clear_addr_cam_entry() and verify with addr_cam_holds(). */
+  bool unregister_infra_sta(const uint8_t own[6], uint8_t macid,
+                            uint8_t addr_cam_idx, uint8_t bssid_cam_idx,
+                            uint8_t band = 0, uint8_t port = 0) {
+    bool ok = _fw.fw_join_info(macid, reg::MAC_AX_NET_TYPE_INFRA,
+                               reg::MAC_AX_SELF_ROLE_CLIENT,
+                               reg::MAC_AX_WIFI_ROLE_STATION, port, band,
+                               /*connect=*/false);
+    ok = _fw.fw_upd_addr_cam(macid, own, reg::MAC_AX_NET_TYPE_NO_LINK,
+                             addr_cam_idx, bssid_cam_idx) &&
+         ok;
+    return ok;
+  }
+
+  /* Write one dword of the MAC's internal table SRAM (the write half of
+   * read_mac_mem32). */
+  void write_mac_mem32(uint32_t internal_addr, uint32_t value) {
+    _device.rtw_write32(reg::R_AX_FILTER_MODEL_ADDR, internal_addr);
+    _device.rtw_write32_wide(reg::R_AX_INDIR_ACCESS_ENTRY, value);
+  }
+  /* Clear VALID on address-CAM entry `idx`. */
+  void clear_addr_cam_entry(uint8_t idx) {
+    write_mac_mem32(reg::ADDR_CAM_MEM_BASE + idx * reg::ADDR_CAM_ENT_LONG_SIZE, 0u);
+  }
+
   /* One dword of the MAC's internal table SRAM (rtw89 mac_mem_read): the
    * internal address goes to R_AX_FILTER_MODEL_ADDR, the data comes back
    * through the R_AX_INDIR_ACCESS_ENTRY window (0x40000, a 32-bit-address

src/kestrel/RtlKestrelDevice.cpp

@@ -536,6 +536,17 @@ namespace {
  * on it (A1 alone selects the response), it only has to be a unicast address
  * distinct from the responder's own. */
 constexpr uint8_t kAckResponderBssid[6] = {0x02, 0x00, 0x00, 0x00, 0x00, 0x01};
+/* The responder's own station: a macid and ADDR_CAM entry apart from the
+ * injection context InitWrite programs at macid 0 / entry 0 (which the WD of
+ * every injected frame keeps pointing at), and from the low slots peer
+ * registrations take by convention (RegisterPeerSta callers use 1, 2, ...):
+ * the last entry of the 0x7f ADDR_CAM search range, reserved against
+ * RegisterPeerSta / RegisterBeamformee. BSSID CAM 1, apart from the injection
+ * context's index 0 (peers use 0 too, and only the addr entry selects the
+ * response). */
+constexpr uint8_t kAckMacid = 127;
+constexpr uint8_t kAckAddrCamIdx = 127;
+constexpr uint8_t kAckBssidCamIdx = 1;
 
 std::string mac_text(const uint8_t *m) {
   char b[18];
@@ -569,15 +580,16 @@ bool RtlKestrelDevice::SetAckResponder(const devourer::MacAddr &mac) {
   const uint32_t resp0 = _device.rtw_read32(r::R_AX_TRXPTCL_RESP_0);
   if (!_ack_responder)
     _ack_resp0_saved = resp0;
-  const bool sent = _hal.register_infra_sta(m, bssid);
+  const bool sent = _hal.register_infra_sta(m, bssid, kAckMacid, kAckAddrCamIdx,
+                                            kAckBssidCamIdx);
   _device.rtw_write32(r::R_AX_TRXPTCL_RESP_0, resp0 & ~r::B_AX_RSP_CHK_ALL);
   /* The H2Cs going out says nothing about the firmware committing them: read
    * the physical CAM entry the RMAC matches against. The firmware applies the
    * queue asynchronously, so poll for a bounded time rather than read once. */
   const auto t0 = std::chrono::steady_clock::now();
   bool committed = false;
   int polls = 0;
-  while (sent && !(committed = _hal.addr_cam_holds(/*idx=*/0, m)) &&
+  while (sent && !(committed = _hal.addr_cam_holds(kAckAddrCamIdx, m)) &&
          std::chrono::steady_clock::now() - t0 < std::chrono::milliseconds(100)) {
     ++polls;
     std::this_thread::sleep_for(std::chrono::milliseconds(1));
@@ -604,9 +616,9 @@ bool RtlKestrelDevice::SetAckResponder(const devourer::MacAddr &mac) {
     return false;
   }
   _ack_responder = mac;
-  _logger->info("Kestrel: hardware ACK responder armed for {} (ADDR_CAM "
-                "entry 0 committed, read back after {} us)", mac_text(m),
-                waited_us);
+  _logger->info("Kestrel: hardware ACK responder armed for {} (macid {}, "
+                "ADDR_CAM entry {} committed, read back after {} us)",
+                mac_text(m), kAckMacid, kAckAddrCamIdx, waited_us);
   return true;
 }
 
@@ -623,31 +635,64 @@ void RtlKestrelDevice::ClearAckResponder() {
 std::optional<devourer::MacAddr>
 RtlKestrelDevice::disarm_ack_locked(std::span<const devourer::MacAddr> held) {
   namespace r = kestrel::reg;
-  /* Back to what InitWrite registers for macid 0: the NO_LINK self entry,
-   * which the firmware does not commit -- so the RMAC stops answering. */
-  _hal.register_sta_role(0, 0, 0);
-  _hal.add_self_sta(kInjectSA, /*macid=*/0);
+  /* Disconnect the responder's station in the firmware; the injection
+   * context at macid 0 is never touched. */
+  _hal.unregister_infra_sta(held.front().bytes.data(), kAckMacid,
+                            kAckAddrCamIdx, kAckBssidCamIdx);
   _device.rtw_write32(r::R_AX_TRXPTCL_RESP_0, _ack_resp0_saved);
-  /* The rewrite is as asynchronous as the arm: give it the same bound. */
+  /* The 8852B firmware keeps a disconnected station's entry VALID (measured:
+   * neither join_info(disconnect) + a NO_LINK addr_cam, nor a role REMOVE,
+   * nor role CREATE + NO_LINK clears it), so the host clears VALID itself, as
+   * the vendor's no-firmware mac_upd_addr_cam path writes the CAM. The H2Cs
+   * above land asynchronously (2-20 ms measured for the arm) and rewrite the
+   * entry, so the clear repeats until the entry has read back empty for 50 ms
+   * straight, well past that tail. The 150 ms bound only ends the wait while
+   * the entry is still held; an entry that reads empty is always given its
+   * full 50 ms, so the worst case is about 200 ms. */
   const auto still_held = [&]() -> const devourer::MacAddr * {
     for (const auto &a : held)
-      if (_hal.addr_cam_holds(/*idx=*/0, a.bytes.data()))
+      if (_hal.addr_cam_holds(kAckAddrCamIdx, a.bytes.data()))
         return &a;
     return nullptr;
   };
-  const auto t0 = std::chrono::steady_clock::now();
-  const devourer::MacAddr *left;
-  while ((left = still_held()) &&
-         std::chrono::steady_clock::now() - t0 < std::chrono::milliseconds(100))
+  using clock = std::chrono::steady_clock;
+  const auto t0 = clock::now();
+  auto clean_since = t0;
+  const devourer::MacAddr *left = nullptr;
+  /* The address the entry last held; kept on record if the clear is not
+   * confirmed, so the next ClearAckResponder retries it. */
+  const devourer::MacAddr *last_held = &held.front();
+  bool first = true;
+  bool stable = false;
+  for (;;) {
+    if (first || left) {
+      _hal.clear_addr_cam_entry(kAckAddrCamIdx);
+      clean_since = clock::now();
+      first = false;
+    }
+    left = still_held();
+    if (left) {
+      last_held = left;
+      if (clock::now() - t0 >= std::chrono::milliseconds(150))
+        break;
+    } else if (clock::now() - clean_since >= std::chrono::milliseconds(50)) {
+      stable = true;
+      break;
+    }
     std::this_thread::sleep_for(std::chrono::milliseconds(1));
-  if (left) {
+  }
+  /* Success only once the entry has stayed empty for the whole interval: an
+   * empty read right after a late rewrite was cleared is not a confirmation. */
+  if (!stable) {
     _logger->warn("Kestrel: ACK responder clear did not take: ADDR_CAM entry "
-                  "0 still holds {}, the MAC may keep answering for it "
-                  "(ClearAckResponder retries)", mac_text(left->bytes.data()));
-    return *left;
+                  "{} still holds {}, the MAC may keep answering for it "
+                  "(ClearAckResponder retries)", kAckAddrCamIdx,
+                  mac_text(last_held->bytes.data()));
+    return *last_held;
   }
-  _logger->info("Kestrel: hardware ACK responder disarmed (ADDR_CAM entry 0 "
-                "no longer holds {})", mac_text(held.front().bytes.data()));
+  _logger->info("Kestrel: hardware ACK responder disarmed (ADDR_CAM entry {} "
+                "no longer holds {})", kAckAddrCamIdx,
+                mac_text(held.front().bytes.data()));
   return std::nullopt;
 }
 
@@ -1042,12 +1087,24 @@ bool RtlKestrelDevice::ConfigureUlOfdma(const devourer::UlOfdmaConfig &cfg) {
   return _hal.configure_ul_ofdma(cfg);
 }
 
+bool RtlKestrelDevice::peer_slot_reserved(const char *who, uint8_t macid,
+                                          uint8_t addr_cam_idx) const {
+  if (macid != kAckMacid && addr_cam_idx != kAckAddrCamIdx)
+    return false;
+  _logger->error("Kestrel: {} refused: macid {} / ADDR_CAM entry {} overlaps "
+                 "the ACK responder's reserved macid {} / entry {}",
+                 who, macid, addr_cam_idx, kAckMacid, kAckAddrCamIdx);
+  return true;
+}
+
 bool RtlKestrelDevice::RegisterPeerSta(const uint8_t peer_mac[6], uint8_t macid,
                                        uint8_t addr_cam_idx) {
   if (!_tx_up) {
     _logger->error("Kestrel: RegisterPeerSta before InitWrite");
     return false;
   }
+  if (peer_slot_reserved("RegisterPeerSta", macid, addr_cam_idx))
+    return false;
   return _hal.register_peer_sta(peer_mac, macid, addr_cam_idx);
 }
 
@@ -1058,6 +1115,8 @@ bool RtlKestrelDevice::RegisterBeamformee(const uint8_t peer_mac[6],
     _logger->error("Kestrel: RegisterBeamformee before InitWrite");
     return false;
   }
+  if (peer_slot_reserved("RegisterBeamformee", macid, addr_cam_idx))
+    return false;
   /* The peer must exist as a fw role/ADDR_CAM entry first (same as a scheduled-
    * UL peer); then layer the CSI/bf params + hw sounding maps on top. */
   bool ok = _hal.register_peer_sta(peer_mac, macid, addr_cam_idx);

src/kestrel/RtlKestrelDevice.h

@@ -91,19 +91,19 @@ class RtlKestrelDevice : public IRtlRadio {
    * once the firmware has COMMITTED a valid ADDR_CAM entry for it, and it does
    * that only for a station registered the way a real association registers
    * one (HalKestrel::register_infra_sta). The arm programs that station with
-   * `mac` as its own address, clears the response-abort CCA gates, and is
-   * verified by reading the physical CAM entry back -- false when the entry
-   * did not commit, which is the 8852C today. Post-bring-up only. Clear puts
-   * macid 0 back to the bring-up NO_LINK entry and verifies the CAM no
-   * longer holds `mac`; an address still held after a failed clear (or a
-   * failed retarget, which checks the old address too) stays recorded, so a
-   * later ClearAckResponder retries it. Arm and clear are serialized.
-   *
-   * The responder shares macid 0 / ADDR_CAM entry 0 with injection: while
-   * armed, injected frames (whose WD carries macid 0) go out under the
-   * station context the arm registered, and clear restores the bring-up
-   * entry. A second macid for the responder is untested: whether the
-   * firmware commits an entry other than 0 has not been measured. */
+   * `mac` as its own address, on its own macid 127 / ADDR_CAM entry 127 so
+   * the injection context at macid 0 / entry 0 (what every injected frame's
+   * WD points at) is left as bring-up programmed it, and RegisterPeerSta /
+   * RegisterBeamformee refuse that slot so a peer cannot take it; clears the
+   * response-abort CCA gates; and is verified by reading the physical CAM
+   * entry back -- false when the entry did not commit, which is the 8852C
+   * today. Post-bring-up only. Clear disconnects that station and clears
+   * the entry's VALID bit from the host (the 8852B firmware leaves a
+   * disconnected station's entry valid), and succeeds only once the entry
+   * has read back empty for 50 ms; an address still held, or not confirmed
+   * gone, after a failed clear (or a failed retarget, which checks the old
+   * address too) stays recorded, so a later ClearAckResponder retries it.
+   * Arm and clear are serialized. */
   bool SetAckResponder(const devourer::MacAddr &mac) override;
   void ClearAckResponder() override;
   /* rxdemo's live-disarm measurement hook (DEVOURER_ACK_DISARM_AFTER_MS): the
@@ -316,6 +316,10 @@ class RtlKestrelDevice : public IRtlRadio {
    * timeout, nullopt once gone. Caller holds _ack_mutex. */
   std::optional<devourer::MacAddr>
   disarm_ack_locked(std::span<const devourer::MacAddr> held);
+  /* True (and logged) when a peer registration would take the ACK
+   * responder's reserved macid or ADDR_CAM entry. */
+  bool peer_slot_reserved(const char *who, uint8_t macid,
+                          uint8_t addr_cam_idx) const;
   std::optional<uint32_t> _ack_disarm_after_ms;
   std::jthread _ack_disarm_thread;
   /* DEVOURER_ACK_RESPONDER: arm at the end of bring-up, throw if refused. */