diff --git a/docs/kestrel-ack-responder.md b/docs/kestrel-ack-responder.md index c383d893..7a1e55c2 100644 --- a/docs/kestrel-ack-responder.md +++ b/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. diff --git a/src/AdapterCaps.h b/src/AdapterCaps.h index 42e6dc5f..e69bd25c 100644 --- a/src/AdapterCaps.h +++ b/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 diff --git a/src/kestrel/HalKestrel.h b/src/kestrel/HalKestrel.h index 4fea87e5..d02cd0f5 100644 --- a/src/kestrel/HalKestrel.h +++ b/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 diff --git a/src/kestrel/RtlKestrelDevice.cpp b/src/kestrel/RtlKestrelDevice.cpp index 62e6cf49..cab9eb20 100644 --- a/src/kestrel/RtlKestrelDevice.cpp +++ b/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,7 +580,8 @@ 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 @@ -577,7 +589,7 @@ bool RtlKestrelDevice::SetAckResponder(const devourer::MacAddr &mac) { 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 RtlKestrelDevice::disarm_ack_locked(std::span 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); diff --git a/src/kestrel/RtlKestrelDevice.h b/src/kestrel/RtlKestrelDevice.h index 76982311..95ae505d 100644 --- a/src/kestrel/RtlKestrelDevice.h +++ b/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 disarm_ack_locked(std::span 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 _ack_disarm_after_ms; std::jthread _ack_disarm_thread; /* DEVOURER_ACK_RESPONDER: arm at the end of bring-up, throw if refused. */