mt7612u: access-point support — hardware beacon behind IRadio, open and WPA2-PSK verified on air

[snokvist]

Follows #419's integration (#420-#423). That gave devourer an MT7612U it can
open, receive and transmit on. This gives it one it can be an access point
on — open and WPA2-PSK — with no MediaTek-specific code in the AP layer.

It works, and devourer's own harnesses are what prove it

tests/ap_responder.cpp and tests/ap_wpa2.cpp against an MT7612U, with no
change to their AP logic. Both already took an IRadio* — #415 made them
genuinely backend-agnostic — so nothing in them knows this is MediaTek. (They
each gained a dozen lines that silence the beacon before _exit; see below.
Nothing in that is MediaTek-specific either.)

AP 5-1   station 2-1 (wlx40a5ef5027a1)   ch36 (5180 MHz)
== open network (ap_responder) ==
  PASS  open: beacon armed
  PASS  open: beacon on air
  PASS  open: station associated
  PASS  open: data plane (rtt min/avg/max/mdev = 0.567/0.812/1.013/0.150)
  PASS  open: hardware auto-ACK (auth at retry=0)
  PASS  open: nothing left airing after exit
== WPA2-PSK (ap_wpa2) ==
  PASS  wpa2: beacon armed
  PASS  wpa2: 4-way complete (MIC verified, station keyed)
  PASS  wpa2: encrypted data plane (rtt min/avg/max/mdev = 0.795/1.177/1.525/0.240)
  PASS  wpa2: nothing left airing after exit
== beacon lifecycle (StartBeacon / StopBeacon / re-arm) ==
  PASS  stop: armed - beacon on air
  PASS  stop: stopped - beacon gone
  PASS  stop: re-armed - beacon back
  PASS  stop: local contract checks

=== 14 passed, 0 failed ===

retry=0 on the auth is the load-bearing one: an un-ACKed frame is
retransmitted with FC Retry set, so retry=0 is the hardware ACK, and it is
the only evidence that the port identity and the APC slot are both right.

That output is tests/mt7612u_ap_onair.sh, added here, so the numbers are a
command rather than a story. 14/14 on ch36 and 14/14 on ch6 — both bands
carry association, the data plane, WPA2 and the auto-ACK, not just a beacon.

What was actually missing

Nothing in the AP layer, and no new driver primitives. StartBeacon,
UpdateBeaconPayload and StopBeacon fell through to IRadio's base-class
return false, so a MediaTek adapter beaconed nothing. Everything underneath
was already device-verified (mt_beacon_init / _write / _set_enable,
mt_ap_set_bssid), and SetAckResponder, ReadTsf and WriteTsf were
already implemented.

Three public entry points, because the backend reaches the subtree through the
public header and api_link holds that line (31 now):

mt7612u_beacon_start   radiotap-framed or bare MPDU; moves the MAC identity
                       (MT_MAC_ADDR *and* the MT_MAC_BSSID base) to addr2,
                       publishes addr3 in the APC slot, arms TBTT
mt7612u_beacon_update  reload in place, engine untouched
mt7612u_beacon_stop    disarm, and retract the identity it took

The identity is the whole trick. IRadio says addr2/addr3 set the port
MAC/BSSID, RtlJaguarDevice implements exactly that, and ap_responder's own
comment says "MACID = BSSID, set by StartBeacon". My first draft instead
refused a BSSID that was not the adapter's own MAC — which would have made
both shipped harnesses unusable on this part, since both hardcode
02:42:75:05:d6:00. It now follows mt76x02_mac_setaddr — move MT_MAC_ADDR
and the MT_MAC_BSSID base together, then derive the APC slot the way
mt76x02_util.c:310 does. Getting that wrong is the review finding below,
and it is the reason the two registers move together rather than one of them.

Also fixed, not MediaTek-specific

ap_responder, ap_wpa2 and ul_trigger_ap all arm a hardware beacon and
then _exit(0), skipping every destructor — so StopBeacon never ran and the
beacon kept airing after the process was gone, until the adapter was
power-cycled. Measured: a scan after exit found the SSID live at last seen: 308 ms ago. The beacon is hardware-autonomous on the Realtek parts too, and
IRadio.h has warned about this since it was written; beacon_update_probe.cpp
already called StopBeacon before its own _exit, these three did not.

Twelve lines each, not one: StopBeacon became able to fail during this
review (it could not before — see below), and #411 amended IRadio.h the same
day to say such a failure "must be retried (or followed by hardware shutdown)
before its shared port is reused". So each harness retries up to three times
and warns if the beacon is still armed, rather than calling it once and
discarding the result.

What review changed

Five rounds, two of which returned DO-NOT-MERGE and were right to. The
headline finding is still the one worth reading:

The APC slot index rested on a premise that is false in this codebase. I
derived it as (ta[0] & 2) ? 1 : 0, arguing mt76's
1 + (((macaddr[0] ^ addr[0]) >> 2) & 7) collapses because the identity had
just been retargeted. It does not. dev->macaddr is written once, from the
EEPROM, and MT_MAC_BSSID — the register carrying MBSS_MODE, and the base
the hardware derives the index from — is written once at init from the factory
MAC; the ACK-responder retarget moves only MT_MAC_ADDR. So the hardware was
indexing off a different address than the host assumed, and the constant was
correct only where 1 + (((factory[0] ^ ta[0]) >> 2) & 7) happens to be 1.

This bench is exactly such a case (40:a5:ef:…, so the XOR term is 0), which
means every on-air run agreed with the wrong reasoning. On an adapter whose
first byte is 0xe8 the AP would beacon perfectly and acknowledge nobody. The
bring-up gate had refused that case; the ABI flattened it to a constant. Fixed
by making the retarget a real setaddr, so the premise is true by construction.

Second: mt7612u_beacon_stop could not fail — mt_clear reports only its
read half and the mt_ap_set_bssid returns were dropped — which made
StopBeacon's failure branch unreachable and the new harness's assertion for
it vacuous, while the real hazard (an EP0 stall leaving the MAC beaconing)
reported success. Also fixed: the reserved-page copy could fail mid-frame and
report success (a torn beacon airs, worse than none); UpdateBeaconPayload
did not raise the suppression guard mt76 explicitly brackets for that reason;
Stop() discarded the result while promising a retry that did not exist; and
the public header still described the pre-fix behaviour.

The earlier round found eight more, one reproduced with a probe against the
real object file: beacon_split left four of nine mt7612u_tx_rate fields
indeterminate, and sgi/ldpc/stbc go straight into the rate word the MAC
transmits verbatim while power_adj short-circuits the derived TX power.

The last two rounds were about fixes that moved the bug rather than removing
it, which is worth calling out because it happened three times:

• The identity retract was added, and then the two failure paths inside the
  block that moves the identity were the ones not routed through it.
• Ownership of MT_MAC_ADDR was claimed before the call that transfers
  ownership away, so the claim was immediately clobbered and the success
  path stopped restoring.
• The retry that the retract relies on could not reach MT_MAC_ADDR at all:
  mt7612u_clear_ack_responder cleared its own "restore owed" flag
  unconditionally and early-returns on it, so attempts two and three were
  no-ops against a still-leaked port MAC. Both halves of the retry had to agree
  before either worked.
• And fixing that widened beacon_start's io_err bracket over a
  mt_set(MT_AUTO_RSP_EN) the code itself documents as a no-op (the gate is
  already on from mac_reset). mt_set is mt_rmw, which bumps io_err and
  skips its write when its read half stalls, and its return is ignored
  because the readback below is the real check — so one transient EP0 read
  stall tore down a verified-good arm and returned -2 for a beacon that was
  on the air.

The last of those was found by an adversarial pass over the diff, not by the
harness: every one of these failures leaves the beacon airing perfectly, so no
amount of watching the air finds them.

Relatedly, -1 no longer escapes from inside the identity block. It is
contracted as "nothing was touched, whatever was airing still is", and the two
exits returning it were reachable only after MT_MAC_ADDR had moved — and
the unwind restores the factory address, the only one saved anywhere. A
failed re-arm over a live beacon therefore retracted an identity that beacon
never had while its page and timers kept airing it, under a return value
promising nothing changed. There is no atomic re-arm to offer (one
MT_MAC_ADDR, one save slot, previous occupant not in it), so those exits take
the AP down deliberately and return -2. -1 now belongs to the input
refusals above the first hardware write.

Rebase notes

The AP work predates the C++ migration (#421) and the backend (#422). Rebasing
surfaced that beacon.c was compiled by nothing — the subtree Makefile
globs *.cpp and the CMake list is explicit — plus C11 _Atomic, implicit
void* conversions, and a memset over a no-longer-trivially-copyable type.
The original four AP commits and that repair are squashed into one, because
the first three do not build on today's master and a bisect through them
returns build failures rather than the commit you are looking for.

What this does not show

In docs/mt7612u-ap-mode.md at the same length as the numbers:

• Hardware CCMP is untested. The 4-way above is devourer's software
  CCMP, the same code the Realtek backends use. MT_WCID_KEY is not even
  defined in this tree and the key path is unreached, so "crypto becomes
  hardware on MediaTek" remains a claim. It is more than a flag: MT_WCID_KEY
  and MT_WCID_IV are both absent, mt_tx_build() sets MT_TXD_INFO_WIV
  unconditionally (meaning "not encrypted") where mt76 gates it on the WCID
  having a key, and RX would have to strip the IV/PN itself. Opened as its own
  issue rather than guessed at here, and deliberately not proposing an
  IRadio key surface — per your guidance, that contract should be designed
  against two backends rather than one.
• The encryption was not independently captured. No third radio sniffed the
  Protected bit; "encrypted" rests on the verified MIC plus traffic reaching a
  CCMP-only station.
• The station is the same silicon (MT7612U on mt76x2u), so it is not an
  independent-generation witness. The RTL8812AU witness in the earlier Stage
  A/B section is.
• One AP, one station, ~20 cm apart; longest run 70 s. No soak, no second
  station, no rekey, no roaming, no channel change while beaconing.

Verification

• 63/63 ctest with DEVOURER_MT7612U=ON, and with it OFF; make -C src/mt7612u check green (4 binaries, api_link at 31 entry points); no
  warnings in either build.
• Every commit builds, both build systems — checked per commit, because the
  original series did not (the first three predate the C++ migration and their
  repair came fourth; they are squashed into one).
• On-air, after the review fixes: tests/mt7612u_ap_onair.sh 14/14 on ch36 and
  14/14 on ch6.
• tests/regress.py 4/4 on the injector path — devourer↔devourer,
  devourer↔kernel both directions — so the AP changes did not disturb the
  transmit/receive behaviour MT7612U integration: wire src/mt7612u in behind IRadio #419 shipped.
• The addr2/addr3 update guard has a positive control as well as two negative
  ones, run against a live beacon: an unchanged payload must be accepted, so
  a guard that refused everything would not pass. Only the addr3 arm
  discriminates against the pre-fix code — addr2 was already guarded — and the
  test says which is which rather than counting both as new coverage.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01Tba83kymS5W2v1vn2yRxrj

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

PR Summary by Qodo

Add MT7612U hardware-beacon access point support

✨ Enhancement 🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Adds MT7612U hardware beacon lifecycle support for open and WPA2 access points.
• Retargets MAC/BSSID identity and validates reserved-page writes to preserve hardware ACK behavior.
• Adds on-air lifecycle tests, teardown safeguards, and documented limitations.

Diagram

graph TD
  T["On-air suite"] -->|runs| H["AP harnesses"] -->|calls| I["IRadio API"] -->|dispatches| W["MT7612U wrapper"] -->|calls| C["Beacon C API"] -->|loads| R["Reserved page"] -->|drives| M["MT7612U MAC"] -->|beacons and ACKs| S["Linux station"]

Loading

High-Level Assessment

The reserved-page hardware beacon behind IRadio is the appropriate design. A host-timed beacon loop would introduce USB scheduling jitter and require pre-TBTT machinery, while MediaTek-specific AP logic would break the existing backend abstraction. Keeping software CCMP in the shared harness preserves portability; hardware key installation should remain separate because IRadio currently has no key-management API.

Files changed (17) +1849 / -5

Enhancement (6) +626 / -1

Mt7612uRadio.cppExpose safe beacon lifecycle operations through IRadio +110/-1

Expose safe beacon lifecycle operations through IRadio

• Implements start, update, and stop operations over the MT7612U C API with serialized state tracking. Stop now retries beacon shutdown during device teardown, unsupported timing controls log errors, and capabilities report hardware beacon TSF insertion.

src/mt7612u/Mt7612uRadio.cpp

Mt7612uRadio.hDeclare MT7612U beacon and timing overrides +13/-0

Declare MT7612U beacon and timing overrides

• Adds IRadio beacon lifecycle and timing-control overrides. Tracks whether a beacon is active so updates, repeated stops, and object teardown follow the interface contract.

src/mt7612u/Mt7612uRadio.h

beacon.cppImplement reserved-page hardware beacon control +418/-0

Implement reserved-page hardware beacon control

• Adds beacon parsing, TXWI construction, reserved-page loading, TBTT enablement, safe in-place updates, and checked shutdown. It also retargets MAC/BSSID identity, programs the correct APC slot, protects updates from torn transmissions, and restores owned identity on stop.

src/mt7612u/beacon.cpp

mt7612u.hPublish the MT7612U beacon C API +58/-0

Publish the MT7612U beacon C API

• Declares public start, update, and stop functions. Documents framing, identity ownership, hardware behavior, validation rules, update suppression, and failure semantics.

src/mt7612u/include/mt7612u/mt7612u.h

internal.hAdd internal beacon state and primitives +19/-0

Add internal beacon state and primitives

• Adds beacon identity ownership state, the TX beacon option, and declarations for reserved-page and APC BSSID helpers.

src/mt7612u/internal.h

tx.cppEncode hardware beacon TXWI controls +8/-0

Encode hardware beacon TXWI controls

• Extends TXWI construction to request MAC-provided TSF timestamps and sequence numbers for beacon frames.

src/mt7612u/tx.cpp

Bug fix (4) +52 / -4

regs.hCorrect MBSS masks and define beacon registers +28/-4

Correct MBSS masks and define beacon registers

• Fixes incorrectly shifted MBSS mode, beacon-count, and local-address masks. Adds reserved-page, bypass, synchronization, RX filter, address, and TX timestamp definitions required by AP mode.

src/mt7612u/regs.h

ap_responder.cppStop open-network beacons before forced exit +8/-0

Stop open-network beacons before forced exit

• Explicitly stops the autonomous hardware beacon before _exit bypasses destructors, preventing the SSID from remaining on air after the harness ends.

tests/ap_responder.cpp

ap_wpa2.cppStop WPA2 beacons before forced exit +8/-0

Stop WPA2 beacons before forced exit

• Explicitly disarms the hardware beacon before the WPA2 harness uses _exit, avoiding persistent on-air state after completion.

tests/ap_wpa2.cpp

ul_trigger_ap.cppStop trigger-AP beacons before forced exit +8/-0

Stop trigger-AP beacons before forced exit

• Disarms the autonomous beacon before _exit skips normal radio destruction, preventing stale AP transmissions.

tests/ul_trigger_ap.cpp

Tests (2) +54 / -0

api_link.cVerify the public beacon symbols link from C +3/-0

Verify the public beacon symbols link from C

• Adds all three beacon lifecycle functions to the public C ABI link test.

src/mt7612u/tests/api_link.c

frame_shape.cppTest hardware beacon TXWI flags +51/-0

Test hardware beacon TXWI flags

• Verifies beacon frames request hardware TSF insertion and sequence assignment without requesting ACKs. It also confirms ordinary data frames retain their existing TXWI behavior.

src/mt7612u/tests/frame_shape.cpp

Documentation (1) +304 / -0

mt7612u-ap-mode.mdDocument MT7612U access-point support and limitations +304/-0

Document MT7612U access-point support and limitations

• Documents verified open and WPA2-PSK operation, hardware evidence, implementation gaps, and operational constraints. It explicitly distinguishes software CCMP validation from unimplemented hardware key installation.

docs/mt7612u-ap-mode.md

Other (4) +813 / -0

CMakeLists.txtCompile the MT7612U beacon implementation +1/-0

Compile the MT7612U beacon implementation

• Adds beacon.cpp to the MT7612U target so the new public beacon API is linked into devourer builds.

CMakeLists.txt

bringup.cppAdd hardware beacon and AP bring-up gates +378/-0

Add hardware beacon and AP bring-up gates

• Adds static-beacon and AP receiver modes for validating on-air beacons, TSF progression, BSSID matching, and hardware auto-ACK behavior. Teardown explicitly disables the beacon and removes residual BSSID state.

src/mt7612u/tools/bringup.cpp

mt7612u_ap_onair.shAutomate end-to-end MT7612U AP verification +273/-0

Automate end-to-end MT7612U AP verification

• Adds hardware orchestration for open association, WPA2 four-way handshake, data traffic, auto-ACK evidence, and beacon shutdown/re-arm. It flushes scan caches and power-cycles during cleanup to avoid false lifecycle results.

tests/mt7612u_ap_onair.sh

mt7612u_beacon_stop_check.cppAdd an external-witness beacon lifecycle harness +161/-0

Add an external-witness beacon lifecycle harness

• Drives armed, stopped, and re-armed phases for observation by a second radio. It also checks repeated-stop and update-without-active-beacon contracts.

tests/mt7612u_beacon_stop_check.cpp

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

Code Review by Qodo

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

1. Access points report ready without airing 📘 Rule violation ≡ Correctness

Description

mt_beacon_set_enable() discards the interval RMW and timer-enable register results and returns
zero unconditionally, while mt7612u_beacon_start() does not check the accumulated I/O-error count
across initialization and enablement. If an EP0 USB transfer fails, StartBeacon() can still set
_beacon_active = true and report that beaconing is active to AP harnesses and callers even though
the interval is stale or the beacon engine was never armed.

Code

src/mt7612u/beacon.cpp[R162-164]

+		mt_rmw(d, MT_BEACON_TIME_CFG, MT_BEACON_TIME_CFG_INTVAL,
+		       FIELD_PREP(MT_BEACON_TIME_CFG_INTVAL, interval_tu << 4));
+		mt_set(d, MT_BEACON_TIME_CFG, bits);

Evidence

The enable helper returns zero after unchecked register operations, and that value flows through the
public start path into _beacon_active = true. Although mt_rmw() reports read-half failures
directly, write failures are recorded separately in the register-I/O error accumulator, which
callers are documented to check at sequence boundaries; the current path checks only the
reserved-page copy and bypass write rather than the complete initialization, interval, and
timer-enable sequence, so both direct RMW failures and accumulated write failures can be missed,
contrary to compliance rule 19.

CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults: CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults: CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults: CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults: CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults: CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults: CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults: CLAUDE.md: Unsupported Backend Features Must Fail Explicitly or Use Documented Defaults
src/mt7612u/beacon.cpp[150-168]
src/mt7612u/usb.cpp[240-249]
src/mt7612u/beacon.cpp[348-351]
src/mt7612u/beacon.cpp[93-118]
src/mt7612u/usb.cpp[206-238]
src/mt7612u/beacon.cpp[150-169]
src/mt7612u/Mt7612uRadio.cpp[884-890]
src/mt7612u/usb.cpp[182-191]
src/mt7612u/usb.cpp[230-249]

Agent prompt

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

## Issue description
Beacon enablement reports success even when USB register operations used to initialize beaconing, configure the interval, or arm the timer fail, allowing the public start path to mark an inactive or incorrectly configured beacon as active.
## Fix Focus Areas
- src/mt7612u/beacon.cpp[150-169]
- src/mt7612u/beacon.cpp[348-351]
- src/mt7612u/usb.cpp[206-249]
## Recommended Fix
Capture the I/O-error count before the complete beacon initialization and enable sequence, propagate each `mt_rmw()` failure immediately, and return failure if any initialization, interval, or timer-enable write increments the error counter. If enablement fails after partially configuring the hardware, best-effort disarm or suppress the beacon slot before returning so callers cannot receive success for an inactive or incorrectly configured beacon.

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

---

2. A replacement can orphan a live beacon 🐞 Bug ☼ Reliability

Description

StartBeacon clears _beacon_active before the replacement has either validated its input or
disarmed the existing engine. When a repeated start fails before mt_beacon_init, the previous
autonomous beacon remains on air while update, stop, and destructor cleanup all treat it as
inactive.

Code

src/mt7612u/Mt7612uRadio.cpp[R884-887]

+  _beacon_active = false;
+  if (mt7612u_beacon_start(_dev, beacon, len,
+                           static_cast<unsigned>(interval_tu)) != 0)
+    return false;

Evidence

The wrapper clears its state before entering the C routine, but that routine validates the frame and
performs identity/APC writes before reaching the operation that disables the previous beacon. Both
StopBeacon and UpdateBeaconPayload refuse when the cleared state is observed.

src/mt7612u/Mt7612uRadio.cpp[873-900]
src/mt7612u/Mt7612uRadio.cpp[931-946]
src/mt7612u/beacon.cpp[282-350]

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 repeated StartBeacon can leave the previous hardware beacon running while `_beacon_active` is false, preventing subsequent cleanup.
## Fix Focus Areas
- src/mt7612u/Mt7612uRadio.cpp[873-890]
- src/mt7612u/beacon.cpp[282-351]
## Recommended Fix
Make replacement transactional: retain the prior active state until the old engine is definitely disarmed, and distinguish failures occurring before versus after disarm so `_beacon_active` always reflects the actual hardware state.

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

---

3. Test exits can leave beacons airing 🐞 Bug ☼ Reliability

Description

The AP harnesses call StopBeacon once, ignore its failure result, and immediately use _exit,
while the lifecycle harness similarly discards its final stop result. A transient or persistent USB
failure therefore bypasses destructor retries and can leave the autonomous beacon active even though
these additions are intended to guarantee silence.

Code

tests/ap_responder.cpp[R300-301]

+  if (g_dev) g_dev->StopBeacon();
_exit(0);

Evidence

The new calls discard the boolean that specifically reports stop failure and then invoke _exit,
which skips the radio destructor's three retries. The standalone lifecycle check also reports
success without incorporating its final stop result.

tests/ap_responder.cpp[293-301]
tests/ap_wpa2.cpp[426-434]
tests/ul_trigger_ap.cpp[449-457]
tests/mt7612u_beacon_stop_check.cpp[155-160]
src/mt7612u/Mt7612uRadio.cpp[642-655]

Agent prompt

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

## Issue description
Hardware tests ignore StopBeacon failures and can exit while the autonomous MAC is still transmitting.
## Fix Focus Areas
- tests/ap_responder.cpp[293-301]
- tests/ap_wpa2.cpp[426-434]
- tests/ul_trigger_ap.cpp[449-457]
- tests/mt7612u_beacon_stop_check.cpp[155-160]
## Recommended Fix
Retry `StopBeacon` with a bounded policy, return a failing process status if it cannot be stopped, and avoid `_exit` until cleanup succeeds or the harness has explicitly power-cycled the target device.

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

---

View action required (2)

4. Cleanup can power-cycle another device 🐞 Bug ☼ Reliability

Description

The root harness writes directly to the authorized attribute for a default or caller-supplied USB
sysfs path without validating its vendor, product, or previous authorization state. A stale or
mistyped AP_SYSFS therefore disconnects an unrelated USB device on every exit and then forcibly
enables it three seconds later.

Code

tests/mt7612u_ap_onair.sh[R64-67]

+  echo 0 > "/sys/bus/usb/devices/$AP_SYSFS/authorized" 2>/dev/null
+  sleep 2
+  echo 1 > "/sys/bus/usb/devices/$AP_SYSFS/authorized" 2>/dev/null
+  sleep 3

Evidence

AP_SYSFS defaults to a topology-dependent path, and cleanup writes zero and one without first
reading identity or prior state. The cleanup trap runs for normal exits, errors, and signals.

tests/mt7612u_ap_onair.sh[38-40]
tests/mt7612u_ap_onair.sh[56-69]

Agent prompt

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

## Issue description
Root cleanup power-cycles an unverified sysfs path and can disrupt an unrelated USB device.
## Fix Focus Areas
- tests/mt7612u_ap_onair.sh[38-40]
- tests/mt7612u_ap_onair.sh[56-68]
## Recommended Fix
Resolve and verify the selected device's VID/PID and expected interface before registering cleanup, record its original authorization state, and restore that exact state rather than always writing one.

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

---

5. Failed beacon setup changes later traffic 🐞 Bug ≡ Correctness

Description

mt7612u_beacon_start() retargets MT_MAC_ADDR and the BSS base before programming the APC slot,
reserved page, and beacon timer, but its later failure returns do not restore the previous identity
or APC state. When any post-retarget operation fails, StartBeacon() leaves _beacon_active false
so Stop() skips the only restoration path, and subsequent injector or monitor activity continues
matching and auto-acknowledging the attempted AP identity.

Code

src/mt7612u/beacon.cpp[R348-351]

+	if (mt_ap_set_bssid(dev, idx, bssid)) return -1;
+	mt_beacon_init(dev);
+	if (mt_beacon_write(dev, mpdu, mpdu_len, &rate)) return -1;
+	return mt_beacon_set_enable(dev, 1, interval_tu);

Evidence

The cited beacon-start code changes the responder identity and BSS base before multiple fallible
operations, returns immediately on failures, and records beacon ownership only after both base
writes succeed; the ACK setter also marks ack_saved before issuing and verifying its register
operations. The wrapper clears its active flag before starting and only invokes beacon cleanup when
that flag remains set, so an initial failed start cannot reach the existing restoration path during
teardown.

src/mt7612u/beacon.cpp[321-351]
src/mt7612u/caps.cpp[80-104]
src/mt7612u/Mt7612uRadio.cpp[878-887]
src/mt7612u/Mt7612uRadio.cpp[647-653]
src/mt7612u/beacon.cpp[320-351]
src/mt7612u/caps.cpp[71-104]
src/mt7612u/Mt7612uRadio.cpp[884-900]

Agent prompt

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

## Issue description
`mt7612u_beacon_start()` changes the MAC/BSS identity before several fallible operations, but its early returns do not restore the prior identity or APC state. The C++ wrapper then treats the beacon as inactive, preventing teardown from invoking the existing restoration path.
## Fix Focus Areas
- src/mt7612u/beacon.cpp[320-351]
- src/mt7612u/Mt7612uRadio.cpp[878-887]
- src/mt7612u/caps.cpp[71-104]
## Recommended Fix
Capture the pre-start identity ownership and relevant register state, then funnel every failure after identity retargeting through a single rollback path. On failure, disable or suppress the beacon engine as needed, clear programmed APC state, restore the previous MAC/BSSID and ACK identity only when this call acquired it, and clear `beacon_took_identity` without disturbing a responder already owned by the caller; ensure returning `false` cannot leave altered device state behind.

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

---

6. Stopping a beacon can erase a responder 🐞 Bug ≡ Correctness

Description

Beacon start records identity ownership only from the responder state that existed at that moment,
while a later SetAckResponder does not transfer that ownership. If a caller arms or changes the
responder after starting the beacon, mt7612u_beacon_stop restores the factory identity and
silently disarms that newer responder.

Code

src/mt7612u/beacon.cpp[R326-330]

+		/* Only claim ownership if nobody else already held the identity: a
+		 * caller who armed a responder first owns the saved factory address,
+		 * and restoring it on beacon stop would silently disarm them. */
+		if (!was_taken)
+			dev->beacon_took_identity = 1;

Evidence

beacon_took_identity is set only during beacon start and is never adjusted by the public responder
methods. Stop unconditionally clears the saved responder and restores the factory address whenever
that stale bit remains set.

src/mt7612u/beacon.cpp[320-331]
src/mt7612u/beacon.cpp[409-415]
src/mt7612u/Mt7612uRadio.cpp[850-860]
src/mt7612u/caps.cpp[107-130]

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 responder configured after beacon start is overwritten when beacon stop restores the factory identity under stale ownership metadata.
## Fix Focus Areas
- src/mt7612u/beacon.cpp[320-331]
- src/mt7612u/beacon.cpp[409-415]
- src/mt7612u/Mt7612uRadio.cpp[850-860]
## Recommended Fix
Model beacon and responder ownership explicitly, update ownership whenever `SetAckResponder` or `ClearAckResponder` runs, and restore only the identity that immediately preceded the beacon-owned value.

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

---

7. A header-only frame becomes a beacon 🐞 Bug ≡ Correctness

Description

beacon_split treats rlen == len as a bare MPDU even though the parser has identified a valid
radiotap header consuming the entire buffer. A header-only input of sufficient length is
consequently interpreted as an 802.11 frame, and its bytes can be used as the port addresses and
reserved-page contents.

Code

src/mt7612u/beacon.cpp[R210-215]

+	if (rlen > 0 && (size_t)rlen < len) {
+		*mpdu = p + rlen;
+		*mpdu_len = len - (size_t)rlen;
+	} else {
+		/* rlen == 0 (no radiotap) or rlen == len (a header with no frame
+		 * after it): treat the buffer as a bare MPDU. OFDM 6 Mbps is the

Evidence

The parser returns the declared radiotap length for a valid header, and the normal send path rejects
a length consuming the whole buffer. The beacon path instead sends that case through bare-frame
validation and later reads addr2 and addr3 from it.

src/mt7612u/beacon.cpp[201-245]
src/mt7612u/beacon.cpp[282-286]
src/mt7612u/radiotap.cpp[108-126]
src/mt7612u/radiotap.cpp[233-243]

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 valid radiotap header that consumes the entire input is incorrectly reclassified as a bare 802.11 frame.
## Fix Focus Areas
- src/mt7612u/beacon.cpp[201-223]
- src/mt7612u/radiotap.cpp[233-243]
## Recommended Fix
Accept the bare-MPDU branch only when parsing returns zero; reject `rlen >= len` when a radiotap header was recognized, matching the existing packet-send validation.

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

---

8. Cleanup kills unrelated wireless clients 🐞 Bug ☼ Reliability

Description

The on-air harness uses system-wide process-name kills for wpa_supplicant and its generated test
executables instead of terminating processes it started. Running or exiting the root harness can
therefore disconnect unrelated host interfaces or terminate another concurrent hardware test.

Code

tests/mt7612u_ap_onair.sh[R56-59]

+cleanup() {
+  pkill -x apr_onair apw_onair bstop_onair 2>/dev/null
+  pkill -x wpa_supplicant 2>/dev/null
+  [ -n "${STA_IF:-}" ] && { ip addr flush dev "$STA_IF" 2>/dev/null

Evidence

Cleanup unconditionally calls pkill -x for all generated binary names and every wpa_supplicant
process. The WPA2 cell repeats the global supplicant kill on both failure and normal completion
without scoping it to STA_IF.

tests/mt7612u_ap_onair.sh[56-60]
tests/mt7612u_ap_onair.sh[176-200]

Agent prompt

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

## Issue description
Harness cleanup terminates every process with shared executable names, including unrelated wireless sessions and concurrent tests.
## Fix Focus Areas
- tests/mt7612u_ap_onair.sh[56-60]
- tests/mt7612u_ap_onair.sh[176-200]
## Recommended Fix
Capture each spawned process PID, run the supplicant in the foreground or with a dedicated PID file, and terminate only those owned PIDs during cell teardown and the exit trap.

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

---

View review recommended (4)

9. Nearby access points can pass the test 🐞 Bug ≡ Correctness

Description

The seen helper counts scan output only by SSID and never verifies the expected BSSID of the
adapter under test. Another access point using devourerAP or mtStopCheck can make arm and re-arm
checks pass, make stop checks fail, or cause the exact-count comparison to reject an otherwise valid
run.

Code

tests/mt7612u_ap_onair.sh[R92-98]

+seen() {
+  local i n best=0
+  for i in 1 2 3; do
+    n=$(iw dev "$STA_IF" scan flush freq "$FREQ" 2>/dev/null | grep -c "SSID: $1")
+    n=${n:-0}
+    [ "$n" -gt "$best" ] && best=$n
+    [ "$best" -gt 0 ] && break

Evidence

seen greps only SSID: lines and its callers compare the resulting count to exactly zero or one.
Both tested SSIDs are fixed constants, so scan entries from other BSSIDs are indistinguishable from
the device under test.

tests/mt7612u_ap_onair.sh[82-102]
tests/mt7612u_ap_onair.sh[134-162]
tests/mt7612u_ap_onair.sh[241-255]
tests/mt7612u_beacon_stop_check.cpp[54-59]

Agent prompt

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

## Issue description
On-air verdicts identify beacons by shared SSID strings rather than the unique BSSID under test.
## Fix Focus Areas
- tests/mt7612u_ap_onair.sh[82-102]
- tests/mt7612u_ap_onair.sh[134-162]
- tests/mt7612u_ap_onair.sh[241-255]
## Recommended Fix
Determine the expected BSSID for each cell and parse each scan BSS block so only an entry with both that BSSID and SSID counts; use presence rather than an exact global SSID count.

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

---

10. Beacon updates can air corrupted frames 🐞 Bug ☼ Reliability

Description

mt7612u_beacon_update() performs the active-slot suppression with unchecked mt_wr() and starts
the multipart reserved-page copy, while mt_beacon_write() snapshots io_err only after that guard
write. If the suppression write fails, a TBTT can read mixed old and new page content during the
copy, and the later copy and re-enable operations can still allow the update to report success.

Code

src/mt7612u/beacon.cpp[R363-373]

+	/* Suppress the slot for the duration of the copy. mt_wr_copy() spans many
+	 * 64-byte EP0 transactions, so a TBTT landing mid-copy would air a TORN
+	 * beacon - leading bytes new, trailing bytes old. mt76 brackets the same
+	 * write for the same reason ("Prevent corrupt transmissions during
+	 * update", mt76x02_usb_core.c). mt_beacon_write() lowers the guard again
+	 * on its way out, which is why this is the only half needed here.
+	 *
+	 * Still no mt_beacon_init() and no set_enable(): the engine is armed, and
+	 * re-initialising it would clear the timer bits mid-flight. */
+	mt_wr(dev, MT_BCN_BYPASS_MASK, 0xffff);
+	return mt_beacon_write(dev, mpdu, mpdu_len, &rate);

Evidence

The update comment establishes that slot suppression is required to prevent TBTT from racing the
multipart copy and transmitting mixed content. However, mt_wr() only records failures in io_err,
and mt_beacon_write() captures its error baseline after the unchecked suppression write, excluding
that earlier failure from the helper's result.

src/mt7612u/beacon.cpp[363-373]
src/mt7612u/beacon.cpp[93-118]
src/mt7612u/usb.cpp[182-191]
src/mt7612u/beacon.cpp[88-101]
src/mt7612u/beacon.cpp[354-374]

Agent prompt

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

## Issue description
Beacon updates rely on `MT_BCN_BYPASS_MASK` to prevent TBTT from reading the reserved page during a multipart USB copy, but the suppression write is not checked. Because the copy helper snapshots the error count only after that write, the update can overwrite the page and report success even when suppression failed.
## Fix Focus Areas
- src/mt7612u/beacon.cpp[354-374]
- src/mt7612u/beacon.cpp[88-118]
- src/mt7612u/usb.cpp[182-238]
## Recommended Fix
Use `mt_wr_chk()` for the bypass-mask suppression write, or snapshot `mt_io_errors()` before it and abort before copying if the error count changes. Include suppression, copying, and unsuppression in the same error boundary; keep the slot suppressed after a failed copy and only unsuppress it after the reserved-page write completes successfully.

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

---

11. ACK responders retain the beacon address 🐞 Bug ≡ Correctness

Description

mt7612u_beacon_start() overwrites an already armed responder's MAC address but deliberately does
not claim restoration ownership when ack_saved was set. mt7612u_beacon_stop() consequently
leaves the responder programmed for the beacon's addr2 rather than the address supplied by the
earlier SetAckResponder() call.

Code

src/mt7612u/beacon.cpp[R321-330]

+		const int was_taken = dev->ack_saved;
+		if (mt7612u_set_ack_responder(dev, ta))
+			return -1;
+		if (mt_mac_set_bss_base(dev, ta))
+			return -1;
+		/* Only claim ownership if nobody else already held the identity: a
+		 * caller who armed a responder first owns the saved factory address,
+		 * and restoring it on beacon stop would silently disarm them. */
+		if (!was_taken)
+			dev->beacon_took_identity = 1;

Evidence

A previously armed responder sets ack_saved, but beacon start still writes its own addr2 into the
responder register and avoids setting beacon_took_identity. Stop only restores the register when
that ownership flag is set, so the original responder target is neither retained nor restored.

src/mt7612u/beacon.cpp[321-331]
src/mt7612u/beacon.cpp[409-415]
src/mt7612u/caps.cpp[80-104]
src/mt7612u/caps.cpp[107-130]

Agent prompt

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

## Issue description
Starting a beacon after `SetAckResponder()` overwrites the single hardware responder identity, but stopping the beacon does not restore the previously armed responder address. The current saved state only retains the factory address, so the prior responder target is lost.
## Fix Focus Areas
- src/mt7612u/beacon.cpp[321-331]
- src/mt7612u/beacon.cpp[409-415]
- src/mt7612u/caps.cpp[71-130]
## Recommended Fix
Either reject beacon start while an independent ACK responder owns the identity, or add explicit nested ownership state that saves and restores the prior responder target after beacon stop. Keep the factory-address restore behavior for the case where beaconing is the first identity owner.

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

---

12. Malformed beacon input can be transmitted 🐞 Bug ≡ Correctness

Description

beacon_split() treats a zero return from mt_radiotap_parse() as a bare MPDU, although that
parser also returns zero for malformed radiotap version, length, and present-map headers. A caller
supplying such a radiotap-framed beacon can therefore have its header bytes interpreted as 802.11
data and loaded into the autonomous beacon page rather than receiving a failed start.

Code

src/mt7612u/beacon.cpp[R201-223]

+	rlen = mt_radiotap_parse(p, len, r);
+	/* Three return classes, not two: <0 means "this IS a radiotap header and
+	 * it is malformed". Treating that as a bare MPDU would parse the radiotap
+	 * bytes as an 802.11 header and read the BSSID out of the middle of it.
+	 * mt7612u_send_packet() refuses on <= 0; so does this. */
+	if (rlen < 0) {
+		ERR("beacon: malformed radiotap header");
+		return -1;
+	}
+	if (rlen > 0 && (size_t)rlen < len) {
+		*mpdu = p + rlen;
+		*mpdu_len = len - (size_t)rlen;
+	} else {
+		/* rlen == 0 (no radiotap) or rlen == len (a header with no frame
+		 * after it): treat the buffer as a bare MPDU. OFDM 6 Mbps is the
+		 * basic rate every station must decode, which is what a beacon wants. */
+		r->phy = MT7612U_PHY_OFDM;
+		r->mcs = 0;
+		r->nss = 1;
+		r->bw = MT7612U_BW_20;
+		*mpdu = p;
+		*mpdu_len = len;
+	}

Evidence

The beacon branch only rejects negative parser results and explicitly classifies zero as no
radiotap. The parser returns zero not only when the first byte is not a radiotap version but also
for invalid header lengths and invalid extended presence maps, proving that malformed radiotap input
reaches the bare-MPDU path.

src/mt7612u/beacon.cpp[201-223]
src/mt7612u/radiotap.cpp[104-124]

Agent prompt

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

## Issue description
`beacon_split()` assumes `mt_radiotap_parse() == 0` always means no radiotap is present, but the parser uses the same result for several malformed-header cases. This violates the beacon API's radiotap-or-bare-MPDU input contract and can produce an invalid stored beacon.
## Fix Focus Areas
- src/mt7612u/beacon.cpp[201-223]
- src/mt7612u/radiotap.cpp[104-124]
## Recommended Fix
Distinguish a clearly bare MPDU from a buffer that begins as a radiotap header but fails radiotap validation. Reject the latter, ideally by making the parser return a negative error for malformed fixed-header and present-map cases while preserving zero exclusively for absent radiotap.

ⓘ 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]

Reviewed at 424170b. Built it (-DDEVOURER_MT7612U=ON), 63/63 ctest, make -C src/mt7612u check green including the new beacon TXWI cell, no new warnings — the verification claims hold. I also checked the register work line by line against reference/mt76 @ be5ce79.

Verified against the vendor source

• The MT_MAC_BSSID_DW1 correction is right, bit for bit: mt76x02_regs.h:281-286 gives ADDR 15:0, MBSS_MODE 17:16, MBEACON_N 20:18, MBSS_LOCAL_BIT 21, _B2 22, _B3 23. MT_BCN_OFFSET_BASE 0x041c, MT_BCN_BYPASS_MASK 0x108c, SYNC_MODE 18:17 all match too, and init.cpp:212-216 now reproduces mt76x02_mac_setaddr (mt76x02_mac.c:755-760: MBSS_MODE=3, LOCAL_BIT, then MBEACON_N=7) exactly.
• mt_beacon_init() matches mt76x02_init_beacon_config (mt76x02_beacon.c:205-213) step for step, and the 0xff00 | ~(0xff00 >> 1) unsuppress matches mt76x02_usb_core.c:223 with beacon_data_count == 1. The slot→bit mapping running down from bit 7 is easy to read backwards from the regs.h comment alone; the beacon.cpp comment does say it.
• The APC-slot reasoning now holds by construction. mt76x02_add_interface (mt76x02_util.c:309-310) derives 1 + (((mphy.macaddr[0] ^ addr[0]) >> 2) & 7) after mt76x02_mac_setaddr, and mt_mac_set_bss_base() is what makes the same invariant true here. The premise correction in the last commit is the right fix.

Blocking

1. src/mt7612u/beacon.cpp:347-350 — the identity retarget still leaks on two failure paths.

const int was_taken = dev->ack_saved;
if (mt7612u_set_ack_responder(dev, ta))
        return -1;
if (mt_mac_set_bss_base(dev, ta))
        return -1;

Both of those bare return -1s skip unwind_identity(). mt7612u_set_ack_responder (caps.cpp:96-110) writes MT_MAC_ADDR_DW0/DW1 and then readback-verifies, so its failure path returns with the port identity already moved — and mt_mac_set_bss_base can only fail after MT_MAC_ADDR has moved successfully. Either way the adapter is left answering for a BSS that was never armed, which is the exact class the f3f08a9/424170b rounds say they closed ("a failed start left it retargeted with no beacon… every failure path unwinds now").

Worse in combination with the new return code: -1 is contracted as "refused before the hardware was touched — whatever was airing still is", so Mt7612uRadio::StartBeacon deliberately leaves _beacon_active alone. A failed re-arm here therefore reports the previous beacon as live while MT_MAC_ADDR now points at the new ta. Both exits need goto fail_pre with took set from !was_taken.

Worth fixing before merge

2. beacon.cpp:433-441 — a failed mt7612u_beacon_update leaves every slot suppressed.

update raises BCN_BYPASS_MASK = 0xffff and nothing lowers it except a successful mt_beacon_write. But mt_beacon_write can refuse on caller-controlled length at :78-83 — which is checked after the guard is up — and can also fail on mt_tx_build or a torn reserved-page copy. Any of those returns with the AP off the air, _beacon_active still true, and no path that re-lowers the guard short of a later successful update. A rejected payload should not silently take the AP down; check the length before raising the guard and unwind the guard on every failure exit.

3. beacon.cpp:426-441 — update accepts any addr2/addr3.

IRadio.h:394-396 says changing addr2/addr3 mid-flight is unsupported and that the port registers keep the StartBeacon identity. beacon_split happily parses a beacon carrying a different BSSID and loads it into the page, so the airing BSSID stops matching the programmed APC slot and MT_MAC_ADDR — beacons perfectly, ACKs nobody, which is precisely the silent failure the start path now goes to real lengths to prevent. Refusing costs a memcmp against what start took.

4. tests/mt7612u_ap_onair.sh cleanup() — the "power-cycle" is an authorized toggle.

The comment says "only a port power-cycle is certain to silence it" and then writes 0/1 to …/authorized. This tree's own CLAUDE.md (Hardware testing) records that as a trap: an authorized toggle is not a cold cycle, it leaves chip state, and VBUS never drops — so an autonomously beaconing MAC keeps beaconing straight through it, which is the one thing this cleanup exists to prevent between cells. Use REGRESS_VBUS_MAP/uhubctl like the rest of the tree, or drop the claim and say what it actually guarantees.

Nits

• tests/mt7612u_ap_onair.sh: wait_gone() is defined and never called.
• regs.h: MT_RX_FILTR_CFG_MCAST / _BCAST are added and unused (the doc's own item-4 conclusion — clear DUP for an AP so retry evidence stays visible — isn't implemented either).
• Mt7612uRadio.cpp AdjustBeaconTiming / …Fine / PinBeaconTbtt: the log is an improvement, but IRadio.h:452,507 already document 0 as the legitimate "no active beacon" answer, so a programmatic caller is exactly as blind as before. Fine to keep; the comment overclaims what the override changes.
• docs/mt7612u-ap-mode.md:142-227: "The gap — the driver primitives to add" is now a future-tense description, with LOC estimates and admittedly-unresolvable pre-migration .c:line citations, of work this PR ships. Items 1, 2, 4, 5 and 6 are done; only item 3 (key install) is still open, and the header already says so. Standing rule here is current-state-only docs — git is the changelog. Fold what's still load-bearing (the MBSS-mask trap, the "address match + beacon is the AP" finding, the measured RX-filter default) into the current-state sections and delete the rest.

One verification gap

The MBSS mask correction changes MT_MAC_BSSID_DW1 at init, on the plain injector/monitor path merged in #422/#423 — from MBSS_MODE=4 (invalid) / MBEACON_N=15 to 3 / 7. Going from invalid to correct is almost certainly an improvement, but it is not AP-scoped, and the verification here is ctest plus the three AP cells. One tests/regress.py pass on the MT7612U before merge would close that.

qodo-gate is failing on both runs, so the merge is blocked on that regardless.

Verdict: changes requested on #1; I'd want #2-#4 in the same round.

[josephnef]

Second pass, same head (424170b), findings anchored inline this time.

Recap of where the review stands: I built the branch (-DDEVOURER_MT7612U=ON) — 63/63 ctest, make -C src/mt7612u check green including the new beacon TXWI cell, no new warnings, so the PR's verification claims hold. I also checked the register work against reference/mt76 @ be5ce79 and it is a faithful port: the MT_MAC_BSSID_DW1 mask correction matches mt76x02_regs.h:281-286 bit for bit, mt_beacon_init matches mt76x02_init_beacon_config (mt76x02_beacon.c:205-213) step for step, the 0xff00 | ~(0xff00 >> 1) unsuppress matches mt76x02_usb_core.c:223, and init.cpp:212-216 now reproduces mt76x02_mac_setaddr exactly. The APC-slot premise fix is right — mt_mac_set_bss_base() is genuinely what makes the index derivation true rather than lucky on this bench.

One blocking finding (beacon.cpp:349), three worth fixing in the same round, four nits — all inline.

And one verification gap that has no line to attach to: the MBSS mask correction changes MT_MAC_BSSID_DW1 at init, on the plain injector/monitor path merged in #422/#423, from MBSS_MODE=4 (invalid) / MBEACON_N=15 to 3 / 7. Going from invalid to correct is almost certainly an improvement, but it is not AP-scoped and the verification here is ctest plus the three AP cells. One tests/regress.py pass on the MT7612U before merge would close it.

qodo-gate is failing on both runs, so the merge is blocked on that regardless.

[snokvist]

Thanks — that's a thorough one, and the blocker is a fair hit. Pushed c5c0c7e.

The blocker. You're right, and it stings a bit: I added unwind_identity() specifically to close "a failed start leaves the identity retargeted", and then routed every failure path through it except the two inside the block that does the moving. The ordering was the real bug — ownership was claimed after both writes, so neither failure could be unwound even in principle. Claimed before them now.

The tri-state interaction you spotted is the worse half. -1 contractually means "the hardware was never touched", and StartBeacon acts on that by leaving _beacon_active alone — so a failed re-arm would report the previous beacon as live while MT_MAC_ADDR had already moved. A wrong answer, not just a leak.

#2 and #3 fixed as described: the length rule is applied before the guard goes up and every failure exit lowers it; update remembers the addr2 start programmed and refuses a beacon that would change it.

#4, the authorized toggle. The comment was wrong and is fixed. But I measured the specific concern rather than taking it on faith, and it does not hold for this part:

armed                -> SSID seen
host process killed  -> SSID STILL seen     (the beacon is autonomous)
authorized toggle    -> SSID gone

Re-enumeration stops the timer. CLAUDE.md is still right that it is not a cold cycle — VBUS never drops, chip state survives — so the comment now claims only what that sequence shows. I did not make uhubctl the default because both adapters here are on xhci root ports, which this rig has previously wedged to the point of needing a machine power-off; AP_VBUS=<hubloc>:<port> takes the uhubctl path for anyone on a switchable hub port. Say the word if you want it to refuse without AP_VBUS.

Nits: wait_gone() removed. The RX-filter defines are now used — beacon_start clears DUP, because a station's retransmission is the evidence that tells you whether your ACKs are landing, and auth ... retry=0 is the AP harness's pass condition. _MCAST/_BCAST stay unused with the reason recorded (mt76 leaves both undropped in every mode); happy to delete them instead. The steer-trio comment overclaimed and now says plainly that a programmatic caller is no better off — the log only helps an operator reading a run. "The gap — the driver primitives to add" is rewritten as current state; five of its six items ship here and git is the changelog.

Your verification gap, closed. You were right that this wasn't AP-scoped — the MBSS mask correction changes MT_MAC_BSSID_DW1 at init on the plain injector path from #422/#423, and my evidence was ctest plus three AP cells. tests/regress.py on the two MT7612U, ch36:

	TX = devourer	TX = kernel
RX = devourer	9100 hits / 9000 TX ✓	400 / 487 ✓
RX = kernel	9231 hits / 9000 TX ✓	478 / 479 ✓

AP harness after everything: 14 passed, 0 failed.

One open question back to you, unchanged from the PR body: hardware CCMP is the single remaining gap and it is reachable in ~40 lines over primitives already here — but IRadio has no key surface at all, so it means adding an interface member most backends cannot implement. That is your call rather than mine; happy to open it as its own issue.

[josephnef]

Re-reviewed at c5c0c7e. Rebuilt: 63/63 ctest, make -C src/mt7612u check green, no new warnings.

Findings 2, 3 and the nits are all properly closed — len_fits_slot() factored ahead of the guard with an unwind on the write failure, beacon_ident refusing an addr2 change, wait_gone() gone, the steer-trio comment trimmed to what it delivers, and "The gap" rewritten as current state. Thank you for the regress.py run on the injector path; 4/4 green closes the verification gap I flagged, and that was the answer I wanted rather than an argument.

On #4: your measurement beats my objection, and I'll take it. authorized → SSID gone is the thing that matters here, and the comment now claims only that. AP_VBUS for a switchable hub port is the right shape — no, don't make it mandatory; your bench's root ports are exactly the case that would make the script unrunnable for the person most likely to run it.

On #1: the fix moved the bug rather than removing it. mt7612u_set_ack_responder() clears beacon_took_identity itself, so claiming ownership before calling it means the claim is wiped on the success path and StopBeacon no longer restores the identity. Detail inline at :365. Failure paths are correct now — it is the success path that regressed.

One new one at :418 on the DUP clear, which I think is the right change made in the wrong place: it is never restored, including on a fail_pre exit that is contracted as "hardware never touched".

On hardware CCMP — open it as its own issue, and don't add a key surface to IRadio for it. A method most backends answer with a not-ported default is the pattern this tree already uses, but a crypto key interface is a much bigger contract than FastRetune (key lifetime, GTK vs PTK, rekey, who owns the replay counter), and getting it wrong is expensive to undo once callers exist. It also isn't blocking anything: the software CCMP path is shipped, measured here, and shared with the Realtek backends. Land the beacon plane, then design the key surface against two backends rather than one.

Everything else stands; happy to approve once :365 and :418 are closed.

[snokvist]

@josephnef pushed 6465ab1. Both your open threads are answered, and two of the five fixes in it are defects this branch introduced — one of them in the first draft of this very round.

:365 — the ordering. You were right. beacon_took_identity is set after set_ack_responder now, so the ownership-transfer write at caps.cpp:90 no longer clobbers the claim, while took is still decided before the writes so the failure paths inside the retarget block know we own it. The !ack_saved guard is gone too, for a second reason: ack_saved is also set by this function's own call, so a re-arm over a live beacon and any session with rx.ack_responder both took the "somebody else got here first" branch and disabled the restore for the session — and the branch protected nothing when it fired, because the retarget had already overwritten that responder's address.

:418 — removed, not restored. Your residue argument is right and I started writing the save/restore. It turns out the clear was a no-op: mt7612u_set_monitor_rx() already leaves DUP clear on every path that beacons, deliberately, for the same evidence reason. So restoring it at stop would have switched duplicate filtering on in a session that had it off and destroyed the retry=0 evidence — worse than the residue. The comment that replaced it was wrong in the same way ("the default is already what an AP wants"; 0x00015f97 has DUP set), and is fixed.

Three you have not seen, because they came out of an adversarial pass over my own diff rather than from the bench:

1. Fixing the retry widened beacon_start's io_err bracket over a mt_set(MT_AUTO_RSP_EN) that the code itself documents as a no-op. mt_set is mt_rmw, which bumps io_err and skips its write when its read half stalls, and its return is ignored because the readback is the real check — so one transient EP0 read stall tore down a verified-good arm and returned -2 for a beacon that was on the air.
2. -1 was escaping from inside the identity block, where "nothing was touched" is already false. The unwind restores the factory address — the only one saved anywhere — so a failed re-arm over a live beacon retracted an identity that beacon never had while its page and timers kept airing it. Those exits return -2 now and fail_pre is deleted; details in your tri-state thread above.
3. beacon_update was guarding addr2 only. You wrote addr2/addr3 and I implemented one of them, then described it as done.

Every one of those leaves the beacon airing perfectly, which is why the harness could not find them.

regs.h: _MCAST/_BCAST are deleted — they were the two this branch added and nothing uses them. My earlier reply claiming they carried "the reason stated" was false; regs.h had no such comment.

Docs: the future-tense section is gone. Three findings survive it as current-state prose — the address-match-is-the-AP property, the MBSS/APC index trap, and the measured RX-filter default. Also corrected: the harness takes CH=, not DEVOURER_CHANNEL= (which it exports to the child, so the documented invocation would have run ch36 with a 5180 MHz scan).

Hardware CCMP is #425, deliberately without an IRadio key surface — your call that it should be designed against two backends. It also names the thing nobody has measured: software CCMP's per-frame cost on this stack, without which "hardware crypto is a gain here" stays an assumption.

Verification on 6465ab1: 63/63 ctest both with and without DEVOURER_MT7612U, subtree check green, and tests/mt7612u_ap_onair.sh 14/14 on ch36 and 14/14 on ch6 — both bands carrying association, data plane, WPA2 and auth … retry=0, not just a beacon. The update guard now has a positive control alongside its two negative arms; only the addr3 arm discriminates against the pre-fix code, and the test says so rather than counting both as new coverage.

I resolved the twelve qodo threads that are fixed and left yours for you.

[josephnef]

Verified at 6465ab1 — rebuilt, 63/63 ctest, subtree check green, no new warnings. Both my threads are closed, and closed properly rather than papered over. Resolving them; approving.

:365. Right shape now: took decided before the writes so the in-flight failures know we own the identity, dev->beacon_took_identity set after set_ack_responder so its ownership-transfer write at caps.cpp:90 can't clobber the claim. I checked the success path is the one that actually changed — that was the regression.

Dropping the !ack_saved guard is the better call and your reasoning for it is the part I'd have missed: ack_saved is set by this function's own call, so a re-arm over a live beacon took the "somebody else got here first" branch and disabled the restore for the session — while the retarget had already overwritten the address the branch was nominally protecting. A guard that fires on its own side effect and protects nothing when it does is worse than no guard.

:418. Removing it beats the save/restore I asked for, and for a reason I had backwards: mt7612u_set_monitor_rx() rewrites MT_RX_FILTR_CFG to PHY_ERR (+CRC_ERR) wholesale, so DUP is already clear on every path that beacons — Mt7612uRadio::StartRxLoop():407, and an AP has to receive. Restoring it at stop would have switched duplicate filtering on in a session that had it off and destroyed the retry=0 evidence. My finding was right about the residue and wrong about the fix; good catch not taking it at face value.

The three you found yourselves are the interesting ones, and all three are the same shape — the beacon airs perfectly and the harness cannot see the defect:

• The io_err bracket over mt_set(MT_AUTO_RSP_EN) is the sharpest. mt_rmw bumping the counter on a failed read of a bit the code documents as already-set, tearing down a verified-good arm and returning -2 for a beacon that is on the air — that is a false negative manufactured by a fix for a false positive. mt_io_restore() around it is right, and it is the existing pattern (fw.cpp:299), not a new mechanism.
• -1 escaping from inside the identity block: agreed, and deleting fail_pre is the right resolution rather than trying to make the promise true. I checked the flow — every -1 in beacon_start is now above the mt_io_errors() snapshot at :388, and everything past it goes to fail_post. The tri-state means what it says now.
• addr3: fair. I wrote "addr2/addr3" and only read back the addr2 half of what shipped.

The test. Saying which arm discriminates, and keeping a positive control against a guard that could pass both negatives by refusing everything, is the right instinct — a test whose arms aren't told apart reads as twice the coverage it has. Same for the CH= vs DEVOURER_CHANNEL= correction; a documented invocation that runs ch36 with a 5180 MHz scan is the kind of thing that costs someone an afternoon.

Verification. ch36 and ch6, 14/14 each, carrying association, data plane, WPA2 and auth … retry=0 — that is a stronger claim than the single-band table the PR body started with, and it is the right one to have made before merge. regress.py closed the injector-path question. All checks green including qodo-gate.

Ship it. #425 is the right home for hardware CCMP, and naming software CCMP's unmeasured per-frame cost in it is what keeps "hardware crypto is a gain here" honest until someone measures it.