You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 47151d0
Browse filesBrowse the repository at this point in the historyBrowse files
authored
mt7612u: no TX transfer timeout - a full ring refuses, it never cancels (#470)
@josephnef - the TX half of the #461 / #467 follow-ups, ready for
review.
Addresses the "frames lost to the TX transfer timeout" finding of #461,
and
closes three of the #467 follow-ups that touch the `txs` gate. Based on
master
after #466 (`1134df9`).
#461 is the motivation, and it now has a second data point: on the
maintainer's second unit, unicast-to-peer gate arms (b-e) landed only
0-11 of
60 own status entries per arm, with ~10 per arm carrying the previous
arm's
pktid. The maintainer's #467 review reports master reading the same on
that
unit. The same pattern reproduces on master on the author's bench, and
it is
gone on this branch (see "What is measured"). This PR removes the loss
that
the gate and the ring caused themselves.
### What changed
Library (`3d9aecb`, review fixes in `65c999a`, `989beef`, `11329ab` and
`3863e76`):
- **TX bulk transfers carry no timeout**, as mt76's TX URBs carry none.
The
async ring gave each one 1000 ms. A chip that NAKs bulk OUT for longer
than
that (its queue full behind slow frames, e.g. unacknowledged unicast at
a
deep retry limit) had libusb cancel frames that the chip would have
taken a
moment later. Those frames had already been reported as sent.
- **The slot wait is bounded instead.** `mt_async_tx_submit` waits at
most
1000 ms for a free slot. A ring that is still full at that point refuses
the
submit (-1), which every caller already counts as failed.
`mt7612u_send_packets` stops at the first refusal, so it does not wait
out
the bound once for every remaining batch.
- **No transfer is submitted after a stop's cancel pass.**
`mt_async_stop`
sets `stopping` (and clears `rx_active`) under the ring lock, wakes slot
waiters, and then cancels. A TX submit happens under the lock and only
while `stopping` is clear; the RX resubmit in `rx_done` is likewise
decided
and submitted in one step under the lock. Before this, a sender parked
in
the slot wait could take a slot freed by the cancel pass and submit
after
it. With no timeout, nothing would ever cancel that transfer: on a
NAKing
chip it outlasts the stop's 2 s wait and strands the device. This does
NOT make a send safe against a concurrent stop that frees the ring - see
"What it can't do".
- **`mt7612u_tx_wire_failed()`** (public C API, `api_link.c` updated)
counts
frames whose transfer was submitted and then failed, was cancelled, or
was
stranded by a stop. The count is per device (it survives a ring
restart),
monotonic, lock-free to read, and in frames (an aggregated transfer adds
all
of its frames). When a stop strands a slot, it counts that slot's frames
under the ring lock and then clears the slot's device pointer. A late
completion, reaped by another thread pumping a shared libusb context,
then
cannot touch the device that `mt7612u_close()` frees. That covers RX
too:
a stranded stop clears the RX slots' device pointer as well, and
`rx_done`
parses a completed transfer under the lock it is cleared under, so a
late
bulk-IN completion neither reads a freed device nor resubmits. The
parser
therefore takes no lock and counts nothing: it reports an invalid PHY by
return code and `rx_done` counts it under the lock it holds (an earlier
revision counted from inside the parser and self-deadlocked on one
corrupt
RXWI). Only submitted slots go into the count: a slot reserved but not
yet
submitted when the stop's wait expires is refused by its submitter, so
(as
the header says) it is not in it. The decision to strand still counts
that
slot, on purpose - it is what keeps the stop from freeing the ring under
a
submitter between its reserve and its submit - so such a slot can strand
the device although libusb never owned it. The count is an upper bound:
a
cancelled or short aggregate counts all of its frames, and a stranded
transfer is never corrected if it completes later.
- Header docs for `mt7612u_tx` / `mt7612u_send_packets` now state the
bounded
wait, the refusal, and the cost: as in mt76, nothing reclaims a slot
whose
transfer the host controller never completes, so a fully wedged ring
keeps
refusing until it is stopped. `docs/mt7612u.md`'s `TxStats::failed`
caveat now
points at the new counter. The RX callback doc says a callback that
transmits stalls the event thread for up to the ~1 s slot wait when the
TX
ring is full.
- **New headless cell `mt7612u_rx_ring`**
(`tests/mt7612u_rx_ring_selftest.cpp`)
drives the ring's RX completion through a test hook with a hand-filled
transfer, no context and no device: an invalid-PHY frame must return and
move `rx_invalid`, a valid frame is delivered once, and a stranded slot
parses and counts nothing. With the parser that counted under the lock
it
fails itself after 2 s (checked by mutation); ctest `TIMEOUT 10` backs
it.
Bring-up tool, `mt7612uprobe txs` (`c9e3243`, `d6d98b6`, `e296998`,
`878ee7c`):
- `DEVOURER_TXS_TRACE=1` prints a `TXS` line for every submit, every
expired
wait, every popped status entry (raw words, pktid, WCID,
classification),
every EXT change on an empty FIFO, and every slow status read, plus a
per-arm summary. The arm table and its parsers are unchanged.
- **Paced to the chip, then to the status.** Each frame waits up to 1.5
s for
its transfer. A transfer that completed in error is resent and not
counted
as sent. A transfer still in flight after 1.5 s counts as sent, because
a
resend would air the frame twice. The per-frame status wait is floored
at
2 s. The 1 Hz PHY tick keeps running through both waits.
- An arm cut short (attempt cap, wall clock) prints UNSETTLED, so a
short arm
whose few frames all filed status cannot pass as a clean row in the
uplink
harness. A refused submit re-baselines the arm's USB counters.
- **A stale-EXT claim the arm did not need is handed back** (#467). One
review arm read `61/60, 1 stale-EXT entries claimed`: all 60 own entries
arrived anyway, so the claimed one was not the arm's own. At arm end, if
the own-pktid entries reach (or, with a MAC duplicate, exceed) the
frames
sent and exactly one entry was claimed, it moves back to the late (or
foreign) column and its SUCCESS bit is un-counted. A claim taken while
the
arm's first transfer was in flight is handed back the same way if that
transfer then fails on the wire. An arm whose surplus claim was handed
back reads n/n and counts as settled for the next arm's claim: it owes
no
entries, which is the condition; a duplicate still disarms it.
- `tests/mt7612u_sta_uplink.sh`: the DUT `timeout` bound follows the new
waits (`d6d98b6`). The header now says the bound is per harness arm
(5424 s at FRAMES=60, 17400 s at 200) while the runtime figure covers
the
whole run, and states the margin: ~2.7x over every gate arm holding its
status at 2 s a frame, ~5x over the measured run (`0931bd6`, #467).
Docs (`957145b`, `e296998`, `878ee7c`, `974c89a`, `3863e76`):
`docs/mt7612u-tx-retry.md` gets a new section,
"Status posted on the next submit", and records the ~1.1 s
unacknowledged
retire on ch6 as open, with the protection and timeout registers
decoded. It
also says the claim's settled condition cascades: one UNSETTLED arm
disables
the claim until an arm settles on its own, so on a unit whose arms do
not
settle the claim buys almost nothing (#467).
### Why
On the uplink harness at ch6 (#461), an unacknowledged frame takes
1.1-1.3 s
from status to status. The gate paced at 110 ms, so it filled the chip's
queue. Bulk OUT stopped accepting, and the 1000 ms transfer timeout then
cancelled 9-50 of every 60 frames per unicast arm. None of that showed
in any
TX counter a consumer reads, and the surviving backlog filed status in
later
arms' rows. Pacing the gate fixes the gate. Removing the timeout fixes
the
same loss for any library caller that transmits faster than the air
drains.
libusb implements a transfer timeout by cancelling the transfer, so the
timeout was never a stronger backstop for a wedged chip than the stop's
own
cancel pass. The RX ring has always run with no timeout, and an unplug
still
completes every transfer through the event thread.
### What is measured
On the final head:
Bench: Linux 7.0 host, ch6, near field. DUT MT7612U (0e8d:7612); ACK
responder RTL8812CU; AP/STA peer RTL8812BU. Head benched: `878ee7c`. The
maintainer benched `11329ab` (PASS, FRAMES=20). The final head `3863e76`
differs from `11329ab` only in `tx_err` accounting on a stop-refused
submit,
plus comments and docs.
- `tests/mt7612u_sta_uplink.sh` (FRAMES=60, RETRY_LIMIT=15,
`DEVOURER_TXS_TRACE=1`): **PASS**, rc 0.
- Arm A (peer answers): 60/60 acked, 100% ok, retries mean 0.1, max 1.
- Arm B (control, peer answers another address): 0/60 acked, retries
16.0.
- Gate arms a-h, both receiver passes: every arm 60/60 own entries, with
0 submit failures, 0 per-frame status timeouts, 0 late entries from the
previous arm, 0 foreign, 0 WIRE-FAIL and 0 UNSETTLED.
- The stale-EXT claim fired once per arm on several arms (a, c, d, g, h
on the first pass; b, e, f, g, h on the second). No arm read
61/60, none showed a late entry, and no claim was handed back.
- **Same unit, same rig, master `1134df9`** (FRAMES=20): **FAIL**, rc 1.
- Arm A: 0/20 acked (entries below sent). Arm B: 0/20.
- Unicast gate arms b-e landed 4/20, 0/20, 0/20 and 0/20 own entries on
both passes, and nearly every arm read UNSETTLED.
- This is the #461 pattern from the maintainer's second unit, reproduced
here on master and gone on this branch.
- `tests/mt7612u_sta_onair.sh`: 16 passed, 0 failed, 0 inconclusive.
- `tests/mt7612u_ap_onair.sh`: 14 passed, 0 failed.
- No log from these runs contains "leaking the ring" or "still in
flight".
Not measured:
- An unplug during TX.
- An RX soak on a busy channel beyond the harness runs.
- Starting and stopping RX while another thread transmits (the race
listed
under "What it can't do").
Earlier numbers come from the pre-review head (`f5278d0`, without any of
the review
fixes): uplink A 20/20,
B 0/20, 0 WIRE-FAIL; sta_onair 16/16; ap_onair 14/14.
### What it can't do
- `Mt7612uRadio::GetTxStats` does not read `mt7612u_tx_wire_failed()`
yet. Its
`failed` still counts refusals only (documented in `docs/mt7612u.md`).
- Nothing here explains the ~1.1 s per unacknowledged frame on ch6
against
~46 ms on ch36. That is recorded as open.
- The refusal is a fixed 1000 ms bound, not a queue-depth policy like
mt76's.
A caller that holds a lock across a send (as `Mt7612uRadio` holds `_mu`)
still holds it for up to 1 s on a stalled chip.
- **A send concurrent with a ring stop that frees the ring is still
unsafe**,
as it was on master. `mt_async_tx_submit` reads `d->a` without
synchronisation, and a sender parked in the slot wait can wake on the
ring's mutex after the stop deleted it. `Mt7612uRadio::StopRxLoop` runs
`mt7612u_rx_stop` without `_mu` while the send paths hold it, so the
backend can hit this. It is not fixed here: `d->a` is read
unsynchronised
in `tx.cpp`, `radiotap.cpp` and `init.cpp` as well, so a real fix is a
lifetime change to the ring pointer (a user count the stop drains, or a
device-level lock), and `StopRxLoop` cannot simply take `_mu` - the stop
joins the event thread, whose RX callback can take `_mu`.
- The stop race, the RX resubmit and the stop/strand ordering have no
headless test, because nothing in the tree fakes a libusb-owned
transfer;
they are argued from the locking. `mt7612u_rx_ring` covers the RX
completion's parse and counting only (rx_active stays 0 there, so it
never
resubmits). The `txs` claim hand-back has no test: the accounting
lives in the hardware-only probe tool, and nothing reaches it without a
device.
- If a slow transfer, already counted as sent by the gate, later fails,
the
gate attributes that failure to the frame being waited on and resends
it.
With no timeout, only an unplug or a stall produces such a late failure.
The same can cross an arm boundary: the previous arm's last transfer
outlives `TXS_USB_WAIT_MS`, is counted sent, and fails after the next
arm
re-baselines its USB counters. The new arm's first frame then sees
`tx_err` move, is taken as wire-failed and resent (aired twice), and
`txs_unclaim_one` hands back a claim that was legitimately this arm's.
Rare for the same reason; documented, not fixed.
### Follow-ups
- Wire `mt7612u_tx_wire_failed()` into `GetTxStats::failed`.
- Make a send safe against a concurrent ring stop (the `d->a` lifetime
above); then the strand decision can count only submitted slots.
- Find the cause of the ch6 unacknowledged retire.
### #467 items
Closed here: the over-attributed stale-EXT claim, the `stale_settled`
cascade note, and the uplink harness's per-arm bound. The #461
second-unit
data point is recorded above. The station-harness items of #467 (the
`sta_identity` verdict branch, the firmware preflight, the INT/TERM exit
code, the `ap_onair` reap, the `tail_ms` clamp) are left to a separate
PR.
### Review record
- Opus adversarial review of the whole delta. Fixed: the stop vs.
slot-wait
submit race, the use-after-free of a stranded slot's device pointer, the
per-batch repeat of the 1 s wait in `send_packets`, the dead
`tx_timed_out` counter (it could no longer move once the timeout was
gone),
the PHY tick gap during the transfer wait, and a garbled library commit
message. Commits are reordered: library first, then the gate, with the
gate
fix-up squashed in.
- Flash reviews A1 + A2 (on the pre-rebase diff). Fixed: the short arm
that
read as settled; the stale `tx_err` baseline after a refused submit; the
stranded-slot use-after-free (the same one Opus found); the undocumented
loss of the timeout's self-heal; the dead `tx_timed_out`; the stale
docs (the `api_link` count, the uplink RUNTIME comment, the
`docs/mt7612u.md` caveat, and the "never counted as sent" overclaim).
Declined: wiring the counter into `GetTxStats` (lifetime and locking
change, listed as a follow-up); a headless test (nothing in the tree
fakes
libusb transfers); gate rc 1/3 scoring in the harness (predates this
PR);
resetting `last_ext` per arm (the EXT register persists across arms,
which
is what the trace compares against).
- Flash review of the fix delta. Fixed: the stranded stop left the RX
slots'
device pointer set, so a late completed bulk-IN transfer could parse
through a freed device; a reserved-but-unsubmitted slot was counted into
`tx_wire_failed` and then refused; the submit-under-lock comment wrongly
said libusb holds no lock across a completion callback (it holds the
context's event lock, taken by `libusb_try_lock_events`; there is no
cycle
because `libusb_submit_transfer` never takes it, and neither does this
driver).
- Flash + Opus review of that fix round. Fixed: parsing under the ring
lock
self-deadlocked on an invalid PHY, because the parser counted it under
the
same lock (Opus reproduced the hang; now the `mt7612u_rx_ring` cell);
the
RX resubmit outside the lock; the `tx_wire_failed` "never reached the
chip" overclaim; the `submitted` comment; the RX callback's stall under
a
full TX ring, now documented; the claim hand-back's `==` (now `>=`), its
effect on the settled condition (documented), and a claim taken during a
first transfer that then failed (now handed back). Declined, recorded
above: the sender vs. concurrent stop lifetime, and with it counting
only
submitted slots in the strand decision.
- Qodo review on #470 (`11329ab`). Fixed: the slot wait refused on
`cv.wait_until` timing out without re-scanning, so a slot freed at the
deadline was ignored (it now re-checks the ring and refuses only when no
slot is free at the deadline); internal.h's `tx_wire_failed` comment
still
said "never reached the chip". Kept documented, not fixed: the gate
resending the wrong frame on a late wire failure (see "What it can't
do"). No headless test for the slot-wait fix: the window is between the
condition variable's timeout and re-taking the lock, and reaching it
ends in `libusb_submit_transfer`, which needs a real device handle.
- Maintainer review on #470 (`3863e76`). The docs and comments this PR
touches now state current behaviour and measurements, with no
before/after narration and no issue numbers in prose or headings (the
flagged lines and a sweep of the rest). `docs/mt7612u-tx-retry.md`
states
the stale-EXT claim's blind spot: the claimed entry satisfies the
per-frame
wait and the settle loop one status early, so the hand-back fires only
when the arm's last own entry pops inside the arm, and a wrong claim can
read n/n (no gate change). A submit refused because a stop began no
longer counts in `tx_err`; a libusb submit failure still does.
`MT_TX_SLOT_WAIT_MS` sits below the includes.
### Verification
- `cmake -DDEVOURER_MT7612U=ON -DDEVOURER_REQUIRE_STA_CRYPTO_TESTS=ON`:
84/84.
- The same build with `-DDEVOURER_SANITIZE=address+undefined`: 84/84.
- CI subsets: mt7612u-only 73/73, jaguar2-only 73/73, jaguar3-only
72/72.
- `mt7612u_rx_ring` mutation check: with the old in-parser count
restored
the cell fails ("rx_done did not return within 2 s"); with the fix it
passes.
- The subtree's `frame_shape` test (built by its own Makefile, not
ctest)
passes against the updated invalid-PHY return.
- No new compiler warnings in `src/mt7612u`.
- `bash -n` and `shellcheck -x -S warning tests/mt7612u_sta_uplink.sh`:
clean. (Plain `shellcheck -x` reports SC2329 at info level for the
trap-invoked `cleanup`, and reports it on master too.)
- `git diff --check`: clean.
- On air: see "What is measured" (benched on `878ee7c`).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3
---------
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-authored-by: Joseph <162703152+josephnef@users.noreply.github.com>
Copy file name to clipboardExpand all lines: docs/mt7612u.md
+9-7Lines changed: 9 additions & 7 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -380,11 +380,11 @@ and nothing was ever queued on the healthy path.
380
380
381
381
## Offline tests
382
382
383
-
`ctest` runs eight MediaTek cells. The first four are C++ and need neither
383
+
`ctest` runs nine MediaTek cells. The first four are C++ and need neither
384
384
hardware nor the `DEVOURER_MT7612U` option — the code they cover is
385
-
header-only. The fifth is C++ and needs the option, because it calls the
386
-
subtree's own symbols. The sixth is header-only C++ gated on the option. The
387
-
last two are Python and need the `reference/mt76` submodule:
385
+
header-only. The fifth and sixth are C++ and need the option, because they
386
+
call the subtree's own symbols. The seventh is header-only C++ gated on the
387
+
option. The last two are Python and need the `reference/mt76` submodule:
388
388
389
389
| cell | what it holds |
390
390
|---|---|
@@ -393,6 +393,7 @@ last two are Python and need the `reference/mt76` submodule:
393
393
|`mt7612u_rx_queue`| the RX hand-off queue's two load-bearing properties: a full ring drops the **newest** frame and counts it, and a popped slot outlives the queue lock. Broken, the first reorders frames or wedges the part and the second is a use-after-free — and both look like a healthy link until a packet processor falls behind |
394
394
|`mt7612u_tsf_read`| that the TSF read stays coherent when the low word wraps between any two of its register accesses: a scripted counter swept across the wrap one microsecond at a time, 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 cannot see the bug. Mutation-tested by hand three ways — no retry, a retry that keeps the first high word, a retry that skips the low-word re-read — each fails |
395
395
|`mt7612u_tsf_api`| that a failed TSF read stays distinguishable from a value at the C entry points: `mt7612u_read_tsf_chk` refuses a NULL device or output with -1 and does not write through the pointer, and `mt7612u_read_tsf` answers 0. `0xffffffff` is a legitimate register word here, so only the return code can carry a failure. Not free: deleting the guard in `mt7612u_read_tsf_chk` segfaults the cell (hand-run). Reaches the NULL refusals only; a failed transfer on a live device, the `Mt7612uRadio::ReadTsf` throw and the `tsf_write` caps bit need the part (`bringup caps` prints it, and `Mt7612uRadio::GetAdapterCaps` takes `tsf_write_ok` from it rather than restating it) |
396
+
|`mt7612u_rx_ring`| the async ring's RX completion (`rx_done`, through the test hook `mt_async_rx_done_for_test`), driven with hand-filled transfers and no device: an invalid-PHY frame returns and counts `rx_invalid` and `rx_dropped`, a valid frame is delivered once, and a stranded slot (device pointer cleared by a stop) parses and counts nothing. The parse runs under the ring lock, so a parser that takes that lock again self-deadlocks on one corrupt RXWI; the cell fails itself after 2 s if the call does not return. It does not cover the resubmit or the stop/strand ordering, which need a transfer libusb owns |
396
397
|`mt7612u_station_identity`| the station-identity decision and ownership hand-off (`src/mt7612u/StationIdentity.h`): both register reads fail closed, a failed port read is reported as a failed read rather than a mismatch, argument checks run before any I/O, the port comparison is tri-state (a failed read is UNKNOWN, never DIFFERENT), a verified move drops the arm, an unreadable one keeps it, and a failed start that unwinds restores it. Header-only, gated on `DEVOURER_MT7612U`, which the MediaTek CI jobs set. The hardware counterpart is `mt7612uprobe staid` (`docs/mt7612u-station-identity.md`) |
397
398
|`mt7612u_usb_ids_vs_mt76`| that the id table above really is the complete `mt76x2u_device_table` from the pinned reference, byte-checked. An id we *forgot* is invisible to `mt7612u_usb_ids` — the adapter just falls through to the Realtek path — and the first draft of that header had 11 of the 16, taken from the host's kernel tree |
398
399
|`mt7612u_initvals_generated`| that the checked-in `initvals.h` still matches what `tools/extract_mt7612u_tables.py` produces from the pinned reference |
@@ -421,7 +422,7 @@ nothing. Each fails the cell, and each names the property it broke.
421
422
422
423
| test | what it holds |
423
424
|---|---|
424
-
|`api_link`| takes the address of all 39 public entry points while including only the public header, so a declaration that loses its definition is a link error. Still compiled as C, which is what keeps the `extern "C"` guard honest now the library itself is C++ |
425
+
|`api_link`| takes the address of all 40 public entry points while including only the public header, so a declaration that loses its definition is a link error. Still compiled as C, which is what keeps the `extern "C"` guard honest while the library itself is C++ |
425
426
|`frame_shape`|`mt_hdrlen_from_fc()` over management, all eight control subtypes and the five data shapes; the RX L2-pad fold on a synthetic QoS frame, with a negative control that redoes the old fixed-24 fold and asserts the QoS Control really is destroyed; the radiotap VHT bandwidth mapping over all eleven codes the part can express; the txwi pktid for a normal send (0), `MT_TXOPT_TXS` alone and `MT_TXOPT_PKTID(id)`|
426
427
|`field_macros`|`MT_CTZ` against `__builtin_ctz` over all 32 single-bit and all 528 contiguous masks, plus a `FIELD_PREP`/`FIELD_GET` round-trip, plus a static initialiser that fails to compile if the macro stops being constant-foldable, plus the `MT_TX_RETRY_CFG` read-modify-write (both limits set, bits 16+ kept) |
427
428
|`log_sink`| that `mt7612u_set_log_sink()`**diverts** rather than copies — stderr must stay silent while a sink is installed — that the sink gets the bare message with no prefix to double up, that every level letter arrives, and that NULL restores the built-in sink instead of silencing the library |
@@ -556,8 +557,9 @@ Stated because the numbers above are uniformly favourable.
556
557
accepted and the URB then failed to deliver does not move it. The library's
557
558
own `tx_err` cannot simply be added: it double-counts submit failures, counts
558
559
URBs where this counts frames, and restarts at zero when a ring is torn down,
559
-
so `failed` would go backwards across an RX restart. Needs a monotonic
560
-
wire-failure counter in the library.
560
+
so `failed` would go backwards across an RX restart. `failed` counts
561
+
refusals only. `mt7612u_tx_wire_failed()` is the library's monotonic
562
+
per-frame wire-failure counter; `GetTxStats` does not read it yet.
561
563
-**Effectively one witness implementation.** Every on-air number outside the
562
564
regression matrix is an RTL8812AU running this project's `rxdemo`: `paggr`,
563
565
`bw` and `rate` are that implementation's reading, not an independent
0 commit comments