IRadio: WriteTsf reports success, and the MT7612U reports false - #430
Conversation
PR Summary by QodoReport TSF write support and reject MT7612U no-op writes
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Code Review by Qodo
1.
|
josephnef
left a comment
There was a problem hiding this comment.
The direction is right: an honest bool beats a silent no-op, and the MT7612U evidence (positive control + every load sequence, both bands) is solid. I built the head in a clean worktree and independently confirmed the Realtek true path on this bench: RTL8822B took=true err +50443 µs and RTL8822C took=true err +50274 µs after a +5 s write and a 50 ms sleep. radio_iface selftest passes; make -C src/mt7612u bringup and make check are clean. No MT7612U here, so the tsfwrite gate itself is not re-run.
Two blockers, both inline: the Jaguar1 false is measured wrong on an RTL8821AU (the bare REG_TSFTR write takes, both word orders, no beacon armed), and the new "true means both transfers landed" promise is false over PCIe. The gate findings and the contract note are non-blocking but worth folding in while you are there.
Nits not anchored inline:
tsf_varianttakes an untypedwhich0/1/2 plus ahigh_word_firstbool that is dead unlesswhich == 0, andgate_tsfwriterepeats the four-line read/arm/accumulate block six times. An enum for the four write orders plus a smallrun_arm()helper removes the unreachable combination and the copy-paste.- The liveness window (
> 10000 && < 200000over 50 ms) is written twice, intsf_clock_aliveand at the tail oftsf_variant, as bare literals. One helper plus a named window pair keeps the control and the per-arm check from ever disagreeing. - "Target + the control round trip" (IRadio.h, time-distribution.md, the J2 comment) drops the sleep term. The recorded J2 datum is +53 ms including the sleep, n=1, 8822B only, 8821C unmeasured. Quote the number or scope the claim ("moves to ~target; magnitude not characterised") — the same rule the PR body already applies to the MT7612U evidence.
Probe used for the measurements (a second RtlAdapter on the claimed handle after InitWrite, raw 0x0560/0x0564 writes, target = fresh read + 5 s, readback after 20 ms, liveness re-read 50 ms later) is a scratch tool; happy to share it if you want to repeat on an 8812AU.
|
Thanks for the independent Realtek numbers and the 8821AU cells; that measurement changed the design. I've pushed 45cf642 (one commit on top of 9280d29), with per-thread replies inline. Summary:
The unanchored nits:
Two things came out of a further adversarial pass, beyond your list:
Verification: 8-1, channel 149 (raw, only the library 5-1, channel 6: Not re-measured this round:
If an 8812AU comes back to the bench I'd take you up on the probe. |
josephnef
left a comment
There was a problem hiding this comment.
Both blockers are fixed and the round-2 head is measured here. I built 45cf642 in a clean worktree (63/63 ctest, make -C src/mt7612u bringup + make check clean) and ran the PR's own WriteTsf — the override, not the raw pair — on every Realtek die on this bench: 5 forward (+5 s) and 5 backward (−5 s) writes each, 20 ms settle, host clock bracketing every call, 50 ms liveness re-read after each readback.
| die | tsf_write_ok |
took / landed / bool agrees | readback error − host time (WriteTsf return → ReadTsf return) |
50 ms re-read |
|---|---|---|---|---|
| RTL8821AU (J1, Archer T2U Plus 2357:0120, ch36, no beacon) | true | 10 / 10 / 10 | −300 … −15 µs | +51.3 … +51.8 ms |
| RTL8822B (J2, 0bda:b812) | true | 10 / 10 / 10 | −197 … +44 µs | +50.3 … +50.8 ms |
| RTL8812BU (J2, 8822B die, 2357:012d) | true | 10 / 10 / 10 | −226 … −8 µs | +51.3 … +51.6 ms |
| RTL8822C (J3, 0bda:c812) | true | 10 / 10 / 10 | −334 … +211 µs | +50.7 … +51.1 ms |
So the Jaguar1 override does load the counter through the library path, both directions, and the "override itself has not run on Jaguar1 hardware" caveat can come out (it is currently stated in three places — see the AdapterCaps.h comment). Same scope as before: one 8821AU unit; the 8812AU/8814AU were not on the bench. The PCIe readback and the USB near-wrap readback branches are not hardware-exercised here either.
The inline notes are non-blocking: one real race worth a one-line fix (J1 ReadTsf unlocked against the new locked pair write), one place where the new TBTT statement contradicts the same file's 8814A note, the roster duplication, and the false wording. Two smaller ones not anchored to a diff line:
struct mt7612u_caps(src/mt7612u/include/mt7612u/mt7612u.h) already carries negative bits (narrowband,fast_retune) so a C consumer need not assume; withmt7612u_write_tsfdeleted, atsf_write : 1bit there would make the C ABI say whatAdapterCaps::tsf_write_oksays instead of leaving a link error as the only signal.- Follow-up, not this PR:
PinBeaconTbtt/AdjustBeaconTimingFineon all three Jaguar backends still hand-roll the same 0x0560/0x0564 pair (AdjustBeaconTimingFine without the wrap retry, and every steer write discards the bool). They all already hold the lock RtlTsf.h prescribes, soread_tsftr/write_tsftrare drop-in there too.
IRadio::WriteTsf was a void method whose default was a silent no-op, so a caller could not tell a chip that moved its TSF from one that ignored the write. The MT7612U was exactly that case: we wrote MT_TSF_TIMER_DW0/DW1 and returned, while the registers hold a free-running counter that does not load. Swept on hardware (two MT7612U, ch6 and 5 GHz, firmware running, MT_BEACON_TIME_CFG TIMER_EN set): twelve writes across six sequences - both words in each order with the MAC running, each word alone, and the MAC- and timer-stopped variants - every one ignored, with the clock still advancing at wall rate and a positive control proving it was alive. No sequence loads the TSF on this part, so there is no sequence to fix. Make WriteTsf report whether the write is real. The default returns false (the RTL8733B and Kestrel have no TSF write either); the RTL8822C (jaguar3, readback-verified) and RTL8822B return true; the RTL8812AU moves its TSF only as part of the beacon-steer sequence, not a standalone write, so it returns false explicitly rather than by inheritance; and the MT7612U returns false without touching a register. A part whose registers exist but do not load and a part with no path at all both mean "no standalone write here", which is what false now says. `bringup tsfwrite [chan]` is the silicon-characterization cell: it PASSes only while every load sequence is confirmed ignored and the clock is confirmed alive, so a future firmware that enables loading trips it. docs/mt7612u.md and docs/time-distribution.md record the absence and the per-backend answers. API note for reviewers: this changes the public IRadio signature from void to bool and removes the standalone C symbol mt7612u_write_tsf (declaration, definition and its api_link presence check). There are no in-tree callers; out-of-tree callers must adapt, which is the intent - a silent no-op is worse than a compile error.
The WriteTsf comment and docs/time-distribution.md hedged Jaguar2's true as inferred from Jaguar3's register pair. It is now measured on the bench: RTL8822B, write of target 6230820 us read back 6284239 (the target plus the 110 us control round trip and the sleep between reads), took true. Say so; no behaviour change.
Review follow-ups on the WriteTsf contract change: - Jaguar2/Jaguar3 now return the AND of the two rtw_write results. Both words are still written even if the first transfer fails (the pair is the unit; a half-updated counter is not something to leave behind), but a failed transfer is no longer dressed as success. - The mt7612u tsfwrite gate reads MT_BEACON_TIME_CFG with mt_rr_chk and skips the timer-off arm when the read fails, instead of writing the 0xffffffff failure sentinel back as configuration; every arm now also requires the clock to still be advancing at the wall rate, so a counter that stalls during the sweep cannot be read as "ignored" and PASS. - mt_mac_stop runs before the mt_mac_start failure return, and the gate's own config read has a failure verdict. - Jaguar1's false override says why: the standalone method deliberately does not perform the beacon-steer sequence's EN_BCN_FUNCTION toggle and TBTT re-download, which move the beacon grid rather than set the clock. - docs: "no tested sequence loads the TSF" rather than an absolute, with the mainline mt76 absence of a mt76x02 .set_tsf beside it; the IRadio comment states the contract and points at time-distribution.md for the per-backend state instead of enumerating them; api_link's entry count corrected to 30. Re-verified on hardware both bands: ch6 and ch149 each pass with the clock control ALIVE and all six arms no-op; ctest 63/63; make -C src/mt7612u check passes.
Review round from the maintainer: - Jaguar1 implements WriteTsf. The bare REG_TSFTR pair was measured to load on an RTL8821AU (raw pair, both word orders, no beacon armed), so the explicit false was a fake in the other direction. Under _port0_mu so it cannot interleave with a beacon-steer sequence. - The REG_TSFTR read/write moves into src/RtlTsf.h, shared by J1/J2/J3. A PCIe register write is a posted MMIO store that always reports success, so write_tsftr reads the counter back there; on USB it also reads back when the target's low word is close enough to wrap that the pair could tear between the two transfers. A throwing readback is false. - AdapterCaps::tsf_write_ok is the static half of the contract (emitted as adapter.caps tsf_write); the WriteTsf bool is only the per-call transport result. - The MT7612U drops its WriteTsf override (identical to the default) and records the measurement on tsf_write_ok = false instead. - The readback claim becomes a table of every recorded cell, with the sleep term, the source of each row, and the unmeasured dies named. The TBTT notes are scoped to J2/J3, with the J1 caveat. - bringup tsfwrite: every TSF read is checked (a failed read is a transport fault, not a dead clock); each arm waits out a low-word wrap before taking its base; an arm slower than its host-time bounds is inconclusive rather than "ignored"; the timer-off arm reads the counter while TIMER_EN is still clear; the arms are a table over a write-order enum sharing one named liveness window. PASS on two units, both bands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
45cf642 to
0ecd680
Compare
|
Rebased onto master (cb98f10) to clear the conflict with #432 in |
|
Follow-ups from the review are tracked in #434. |
…dly (#436) > Standalone follow-up to #430, and #434 item 5 in full: the checked, wrap-safe > library `ReadTsf`, plus the `tsf_write` bit in the C caps. #434 items 1–4 > (Jaguar1 `ReadTsf` lock, the 8814A TBTT note, the roster copies, the > `WriteTsf` false wording) are Realtek-side and are not touched here. `mt7612u_read_tsf` read `MT_TSF_TIMER_DW0` then `DW1` with the unchecked `mt_rr` and joined them. The two halves are not latched. A read whose halves straddle the 2^32 µs low-word wrap therefore returns a value 2^32 µs (71.6 min) off. The first wrap comes 71.6 min after bring-up, which restarts the counter. A failed transfer joined `0xffffffff` into a plausible wrong clock, and `Mt7612uRadio::ReadTsf` passed both through as a TSF. ## The read tears on hardware, and the fix does not `bringup tsfwrap [gap]`, new in this PR and reproducible from the tree, judges every read against a least-squares host-clock model fitted over the preceding minute, not against the read under test. The gate does three things: 1. **Forced read.** Around the predicted wrap it drives the library's read template through a reader that sleeps to a schedule, so the wrap lands in a chosen gap of the read. 2. **Positive control.** It interleaves a plain DW0,DW1 read across the same wrap. 3. **Continuous check.** It checks every continuous `mt7612u_read_tsf_chk` read near the wrap. Each value is judged over the host interval bracketing its control transfer. One transfer took ~10 ms on the slower unit's bus, which a single timestamp would have misjudged by that much. One run per unit, both at the real 32-bit wrap: | | `40:a5:ef:50:27:a1`, gap 1 | `40:a5:ef:5a:32:f8`, gap 2 | |---|---|---| | forced read | retried, −3 µs off the model | retried, −10 µs | | DW0,DW1 control across the same wrap | +2^32 − 6 µs | +2^32 + 4955 µs | | continuous reads checked near the wrap | 1.70 M of 31.7 M, worst 348 µs | 211 k of 3.9 M, worst 74 µs | | failed reads, backwards steps, reads off the model | 0, 0, 0 | 0, 0, 0 | Both runs `PASS`. The second unit is on a USB 2.0 bus where a control transfer can take ~10 ms, which is where its control's extra 4955 µs comes from: a read is judged over the interval that bracketed its transfer, so that is slack in the measurement rather than in the counter. The control tearing by 2^32 is also the latch answer: a DW0 read does not freeze DW1. **What that does not show.** - **The exported function's own retry never ran on hardware.** The forced read occupies the wrap instant, so `mt7612u_read_tsf_chk` never takes its retry across the wrap in that run. The retry path is covered by the forced template read, which is the same code through another reader, and by the headless cell below. - **n is one wrap per unit.** - **The smoke mode cannot tear a read.** `wrap_bits` < 32 reports `SMOKE`, never `PASS`. **Failure path, checked once by hand, not a gate.** Under a 100 Hz `ReadTsf` poll I de-authorised the adapter's whole bus (`usbN/authorized`, a logical disconnect, not a cable pull). That gave 795 plausible reads, then 200 of 200 calls threw, with no value returned after the disconnect and a clean teardown. De-authorising only the device (`8-1/authorized`) is *not* a disconnect on this part: EP0 kept answering and the TSF kept reading correctly for 40 s. The first attempt would have "passed" vacuously on that. ## Structure - **`src/mt7612u/Mt7612uTsfRead.h`** is a pure template. It reads high, low, high; if the high word moved, it re-reads the low word and pairs it with the second high word. - Any failed access fails the read and leaves `*out` untouched. - `0xffffffff` is a legitimate word, which is why a return code carries failure rather than a sentinel value. - The approach matches `read_tsftr` in `src/RtlTsf.h`, which re-reads both words instead. - **`mt7612u_read_tsf_chk(dev, &out)`** is a new C entry point returning 0 or -1. `mt7612u_read_tsf` uses the same read and returns 0 on failure. - **`Mt7612uRadio::ReadTsf`** throws `std::ios_base::failure` on a failed read, as the Realtek USB backends already do. The `IRadio::ReadTsf` note now says that for USB, keeps the PCIe hedge `RtlTsf.h` carries, and names the RTL8733B as returning 0. - **`struct mt7612u_caps`** gains `tsf_write : 1` (0 here), so the C ABI says what `AdapterCaps::tsf_write_ok` says. - **`bringup`**: - The `tsfwrite` gate calls the library reader, so there is one copy of the read discipline instead of two. - The beacon gate fails on any failed TSF read. Before, a failed read cleared only the chain, so a failing transport could still report a live timer. - The `caps` gate prints no word-order verdict from failed raw reads. - **`timesync`, `tdma`, `chanmig`**: the two timing demos skip that marker rather than stamp a wrong time; `chanmig` stamps its existing "no TSF" 0 and warns once. - **`api_link`** gains `mt7612u_read_tsf_chk` plus the two public functions it was already missing, `mt7612u_ch_time` and `mt7612u_phy_tick`. All 33 now resolve. The two omissions predate this branch and would normally be their own change; they ride here because this PR rewrites the doc line that claims the cell covers *all* public entry points, and that claim was false. Say the word and I will split them out. - **`tests/mt7612u_tsf_wrap.sh`** wraps the gate so the invocation, the ~72 min per wrap, one gap per run, one adapter being enough, and the re-run rule are not folk knowledge. `SMOKE=1` checks the schedule, the model and the plumbing in ~2.5 min against a 16.7 s carry, and reports `SMOKE`, never `PASS`. ## Caller-visible behaviour changes - **`Mt7612uRadio::ReadTsf` throws** on a failed read, where it returned a garbage value. Any MT7612U consumer that does not catch exceptions will now exit on a transport failure instead of carrying a wrong clock. That matches what every Realtek USB backend already does. - **`mt7612u_read_tsf` returns 0 on failure** instead of a value built from `0xffffffff` words. - **Each TSF read costs 3 control transfers (4 across a wrap)** instead of 2. `ReadTsf` is caller-cadence, never on the send path. - **The `timesync` and `tdma` masters skip a marker** when the TSF read fails, and each says so once rather than at marker rate. - **`struct mt7612u_caps` gains `tsf_write`.** It lands in spare padding of an existing bitfield, so `sizeof` is unchanged (32 B on x86-64; no 32-bit toolchain here to check that layout), and `mt7612u_get_caps` zeroes the struct first, so a caller built against the new header and linked to an older library reads 0 — which is the right answer for this part. - **`bringup tsfwrap` exit codes:** 0 PASS, 1 the part failed the measurement, 2 bad invocation (as everywhere else in the tool), 3 no verdict — interrupted, or the wrap landed in the other gap, which is a re-run rather than a defect. ## Out of scope, stated The `ReadTsf` callers that still do not catch (`kestrelprobe`, `beacon_steer_check`, `dl_departure_tx`, `pcie_ptp_beacon`, `pcie_txegress_tx`, `tsf_probe`'s first loop) are Kestrel-, Realtek- or PCIe-specific bench tools. Their behaviour is unchanged by this PR: those backends already throw. ## Headless coverage `mt7612u_tsf_api` (ctest, needs `DEVOURER_MT7612U`, which CI's MediaTek jobs set) pins the C entry points' failure contract: `_chk` refuses a NULL device or output with -1 and does not write through the pointer, `mt7612u_read_tsf` answers 0, and `struct mt7612u_caps` carries `tsf_write`. `0xffffffff` is a legitimate register word here, so only the return code can carry a failure. It reaches the NULL refusals only — a failed transfer on a live device, and the `Mt7612uRadio::ReadTsf` throw, need the part. `mt7612u_tsf_read` (ctest, runs with `DEVOURER_MT7612U=OFF` too) sweeps a scripted counter across the wrap one microsecond at a time, so the wrap falls in every gap of the read. It also covers every failure position, and an all-ones low word as a value. The pre-fix DW0,DW1 order runs against the same sweep and must tear, or the cell fails. I also hand-mutated the template three ways (no retry; a retry that keeps the first high word; a retry that skips the low-word re-read), and each fails the cell. That mutation run is not in the tree. The cell covers the read discipline, not the wiring (`docs/mt7612u.md` says so); the wiring is what the gate covers. ## Test plan ```sh cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DDEVOURER_MT7612U=ON cmake --build build -j ctest --test-dir build # 68/68, mt7612u_tsf_read + mt7612u_tsf_api make -C src/mt7612u check # api_link: 33 resolved # hardware (firmware from linux-firmware), ~72 min per run SMOKE=1 MT7612U_DEV=<bus-port> tests/mt7612u_tsf_wrap.sh # ~2.5 min, SMOKE MT7612U_DEV=<bus-port> tests/mt7612u_tsf_wrap.sh # both gaps, ~2.4 h DEVS="<port-a> <port-b>" tests/mt7612u_tsf_wrap.sh # one gap each, ~72 min MT7612U_DEV=<bus-port> src/mt7612u/bringup tsfwrite 6 # still PASS through the library reader MT7612U_DEV=<bus-port> src/mt7612u/bringup caps 149 # tsf_write=0, checked word-order read ``` 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JP51Yp3WSbfiMDuDHJByHW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
IRadio::WriteTsfwasvoidwith a silent no-op default, so a caller could nottell a chip that moves its TSF from one that discards the write. The MT7612U was
that case: we wrote
MT_TSF_TIMER_DW0/DW1and the registers hold afree-running counter that does not load.
This PR splits the question in two:
AdapterCaps::tsf_write_ok, resolved at construction. It is true onJaguar1/2/3 and false on the MT7612U (measured), Kestrel and the RTL8733B.
WriteTsfreturnsbool, whether the transport accepted thiswrite. On PCIe, and on USB near a low-word wrap, that answer comes from a
readback.
A
bringup tsfwritecell characterises the MT7612U's absence of a load path, soa future firmware that enables loading trips it.
It is ignored, on hardware, in both bands, on two units
Units 8-1 (
40:a5:ef:50:27:a1) and 5-1 (40:a5:ef:5a:32:f8), channels 6 and149, firmware running,
MT_BEACON_TIME_CFGfree-running. All four runs PASS.8-1, channel 149:
How the gate avoids a false PASS:
so a frozen counter cannot be mistaken for a successful load. Every arm also
re-checks liveness.
as a transport fault, not a dead clock.
single-word arms can't misjudge a load across a carry.
readback of a real load into a "no-op": a slow arm is inconclusive and FAILs.
(
base + (1ull<<32)).TIMER_ENrestarts the counter from ~0, whichwould wipe a load before the normal readback. So this arm also reads the
counter while the timer is still clear. It reads 0, not the target.
And the Realtek parts accept it. Every recorded readback of a +5 s write is
in
docs/time-distribution.md: the source, the sleep, and error − sleep per row.In summary:
WriteTsf, host-timed)On the RTL8812BU, 40 host-timed writes (0/20/50 ms sleeps, plus backward −5 s
writes) all returned true and landed, and readback error minus the host time
from
WriteTsfreturning toReadTsfreturning was −485…+38 µs: the counterloads the target and the residual is host time. The 8821AU cells are the maintainer's scratch probe on the register pair the J1
override writes; the override itself has not run on J1 hardware. The
8812AU/8814AU, 8821C (USB and CE) and 8822E share the pair and code path and are
not separately measured.
Structure
IRadio::WriteTsfbecomesbool, and the default returns false.AdapterCaps::tsf_write_ok(emitted astsf_writeinadapter.caps)answers "does this part have one".
tsf_write_okpart, false is a transport failure, and the counter maybe half-updated: read back, and retry only while the device is present.
src/RtlTsf.hholdsdevourer::read_tsftr/write_tsftr, shared byJaguar1/2/3. Both words are always attempted.
low word is within 100 ms of wrapping. There the pair can tear between the
transfers, so it is read back.
success, so every write is read back. A device that has left the bus
typically reads all-ones and fails.
_port0_mu, so it cannot interleavewith a beacon steer. On J1 the TBTT is recorded as locked to the TSF grid, so
expect an active beacon's TBTT to move with a write. That, and whether the
beacon keeps airing without the steer's re-download, is unmeasured through
WriteTsf._reg_muand route through the helper.tsf_write_ok = falseand does not overrideWriteTsf,so the
IRadiodefault refuses without touching a register.mt7612u_write_tsf(its declaration, definition andapi_linkentry) isdeleted; a helper that only ever no-ops is a trap, not an API.
bringup tsfwrite [chan]is the characterisation cell, next tocaps.What this does not show
WriteTsfmovesthe reported TSF and the beacon timestamp; the TBTT is a separate per-port
timer there.
PinBeaconTbtt/AdjustBeaconTimingFineare the TBTT levers.at all. That is a source fact, not a bench measurement.
are unchanged). The J2 helper did run, on an RTL8812BU;
What review changed, because it is most of the value here
Adversarial rounds 1–2 and Qodo (commits dda2e71 … 9280d29):
lo_ok && hi_ok;MT_BEACON_TIME_CFGread in the timer-off arm;mt_mac_stopon a failed start.Maintainer review (45cf642):
8821AU, so J1 now writes.
helper reads back.
to
AdapterCaps::tsf_write_ok.clock control used unchecked reads.
named liveness window; the readback claim now carries its sleep term and n.
A further adversarial pass on that response:
wrap guard;
All six are fixed above.
API/surface breaks (intentional)
mt7612u_write_tsfremoved from the public header, its definition, andthe
api_linkcheck. This is the hard break: it is a C symbol, so anyout-of-tree user fails to link.
api_linkis built only bysrc/mt7612u/Makefile; CI does not build it.IRadio::WriteTsf:void→bool. Return types are not part of theItanium mangled name, so this breaks source compatibility for out-of-tree
callers, not the ABI.
AdapterCaps::tsf_write_okadded, andadapter.capsgainstsf_write.An
AdapterCapslayout change breaks ABI for anyone compiled against the oldstruct, as every new caps flag does.
There are no in-tree callers of
WriteTsformt7612u_write_tsf.Test plan