mt7612u: make a retune race-free against the live RX ring

[josephnef]

What

The two data races docs/mt7612u.md listed for a channel change while the RX ring is up, reproduced under ThreadSanitizer on the first MT7612U in the lab (Comfast CF-922AC) and fixed:

• RSSI correction tear. The per-channel chain offsets and LNA gain are one relaxed atomic word now (mt7612u_cal::rx_corr, packing in Mt7612uRxCorr.h), stored once per tune and loaded once per frame. A headless round-trip joins the mapping selftest.
• libusb sync API on a live context. libusb's sync layer reads its completion flag without synchronization on one early-return path, so nothing ordered the event thread's last lock of the transfer mutex before the caller's libusb_free_transfer() destroyed it (symbolized against libusb 1.0.30 with distro debug symbols). Register and MCU transfers now submit through the async API and wait on the library's own acquire/release flag.

Measured

• Before: 5 TSan reports in 150 s of cross-band sweeping (eeprom.cpp, and 3 inside libusb). After: 0 library/libusb reports over ~1000 retunes and 7221 frames; the 2 that remain are rxdemo globals shared by every generation.
• A/B neutral: retune p50 791 ms / mean 756 ms on both builds (65 dwells each); a 2000-frame injection witnessed by the RTL8822BU ground station delivered the same count from both builds at −20 dB and −10 dB back-off (1000 / 900 — the witness sits 20 cm away in saturation, so that is its ceiling).
• ctest 69/69.

Write-up: docs/mt7612u.md "Retune while the ring is up". The open-list item is gone.

🤖 Generated with Claude Code

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

PR Summary by Qodo

Make MT7612U live-ring retunes race-free

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Atomically publishes RSSI corrections so live RX frames cannot observe torn retune state.
• Rebuilds synchronous register and MCU transfers atop ordered libusb async completions.
• Adds mapping tests and documents TSan, latency, and delivery validation.

Diagram

sequenceDiagram
    actor Caller
    participant Tune as Retune Path
    participant USB as USB Helpers
    participant Libusb as libusb Events
    participant Corr as Atomic RX Corr
    participant RX as RX Parser
    par Retune I/O
        Caller->>Tune: Change channel
        Tune->>USB: Register and MCU I/O
        USB->>Libusb: Submit async transfer
        Libusb-->>USB: Release completion
        USB-->>Tune: Acquire completion
        Tune->>Corr: Store correction word
    and Frame delivery
        Libusb-->>RX: Deliver RX frame
        RX->>Corr: Load correction word
        Corr-->>RX: Return consistent triple
    end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Pause the RX ring during retunes

• ➕ Eliminates overlap between frame parsing and channel calibration.
• ➕ Avoids mixing live event handling with synchronous control requests.
• ➖ Interrupts reception and can drop frames during every channel change.
• ➖ Adds ring teardown and restart latency and risks endpoint recovery issues.

2. Serialize all USB work on one event thread

• ➕ Establishes single-threaded ownership of libusb operations and transfer lifetimes.
• ➕ Could provide a general command-queue model for future concurrent operations.
• ➖ Requires a larger architectural rewrite and request-response queue.
• ➖ Introduces additional deadlock, shutdown, and callback reentrancy concerns.

Recommendation: Keep the PR's targeted approach. A packed relaxed atomic exactly matches the consistency requirement for the independent RSSI snapshot, while async submission with acquire/release completion fixes transfer lifetime ordering without stopping the live ring. A single-owner USB command queue may be valuable if concurrency expands, but is disproportionate for the current retune path.

Files changed (8) +279 / -48

Bug fix (5) +206 / -15

Mt7612uRxCorr.hAdd packed RSSI correction helpers +32/-0

Add packed RSSI correction helpers

• Introduces device-independent helpers that pack two signed chain offsets and signed LNA gain into one 32-bit value and unpack them without losing sign information.

src/mt7612u/Mt7612uRxCorr.h

eeprom.cppPublish channel RSSI correction atomically +9/-4

Publish channel RSSI correction atomically

• Computes RSSI offsets and LNA gain in local variables, then publishes the complete correction triple with one relaxed atomic store after EEPROM processing.

src/mt7612u/eeprom.cpp

internal.hReplace split calibration fields with atomic correction state +14/-2

Replace split calibration fields with atomic correction state

• Changes MT7612U calibration state from three independently accessed bytes to one atomic packed word. Includes the packing helpers and documents the caller/event-thread concurrency contract.

src/mt7612u/internal.h

rx.cppLoad one RSSI correction snapshot per frame +12/-4

Load one RSSI correction snapshot per frame

• Loads and unpacks the atomic calibration word once before correcting all RSSI chains, preventing a frame from combining values from different channel tunes.

src/mt7612u/rx.cpp

usb.cppOrder synchronous USB operations through async completions +139/-5

Order synchronous USB operations through async completions

• Reimplements blocking control and bulk helpers with libusb asynchronous transfers and an acquire/release completion flag. Adds status translation, timeout cancellation, safe completion waiting, and intentional leaking rather than freeing a transfer that remains in flight.

src/mt7612u/usb.cpp

Refactor (1) +4 / -1

bringup.cppRead packed calibration in bring-up diagnostics +4/-1

Read packed calibration in bring-up diagnostics

• Updates EEPROM gain diagnostics to atomically load and unpack the new RSSI correction representation before printing it.

src/mt7612u/tools/bringup.cpp

Tests (1) +24 / -0

mt7612u_mapping_selftest.cppTest packed RSSI correction round trips +24/-0

Test packed RSSI correction round trips

• Covers signed EEPROM values, int8 boundaries, zero initialization, and the final RSSI correction formula for the packed calibration representation.

tests/mt7612u_mapping_selftest.cpp

Documentation (1) +45 / -32

mt7612u.mdDocument race-free live-ring retuning +45/-32

Document race-free live-ring retuning

• Adds the retune concurrency design, TSan results, latency measurements, and frame-delivery comparisons. Removes the resolved race item and renumbers the remaining open work.

docs/mt7612u.md

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

Code Review by Qodo

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

1. Correction docs have two authorities ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

docs/mt7612u.md repeats the packed correction fields, publication semantics, and per-frame loading
behavior already documented with mt7612u_cal::rx_corr and Mt7612uRxCorr.h. A later change to the
packing or synchronization contract must update both descriptions, allowing the subsystem guide and
authoritative declarations to diverge.

Code

docs/mt7612u.md[R615-618]

+- **The per-channel RSSI correction is one word.** `mt_read_rx_gain()`
+  publishes the two chain offsets and the LNA gain as a single relaxed atomic
+  (`mt7612u_cal::rx_corr`), and `mt_rx_parse()` loads it once per frame. Three
+  separate bytes gave a frame parsed mid-retune the new offset with the old

Evidence

Compliance rule 2 requires repository documentation to refer to authoritative header declarations
instead of reproducing their field-level documentation. The added guide text repeats the same three
correction values and atomic publication behavior documented on the new declaration and packing
header.

CLAUDE.md: Do Not Duplicate Header Documentation: CLAUDE.md: Do Not Duplicate Header Documentation: CLAUDE.md: Do Not Duplicate Header Documentation: CLAUDE.md: Do Not Duplicate Header Documentation
docs/mt7612u.md[615-620]
src/mt7612u/internal.h[51-62]
src/mt7612u/Mt7612uRxCorr.h[3-12]

Agent prompt

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

## Issue description
The subsystem documentation duplicates implementation details already documented by the correction declaration and packing header, creating two authorities that can become inconsistent.
## Fix Focus Areas
- docs/mt7612u.md[615-620]
## Recommended Fix
Replace the duplicated field layout and synchronization explanation with a concise reference to `mt7612u_cal::rx_corr` and `Mt7612uRxCorr.h`, retaining only results or operational context that does not belong on the declarations.

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

---

2. A stalled transfer outlives its owner ✓ Resolved 📘 Rule violation ☼ Reliability

Description

submit_and_wait() returns after cancellation fails while the submitted transfer's sync_done()
callback still points to the stack-local done atomic supplied by sync_control() or
sync_bulk(). A later libusb completion dereferences that expired callback state, and bulk
transfers from MCU-response and RX-flush callers can also retain stack buffers while normal radio
teardown releases the untracked transfer's handle and context.

Code

src/mt7612u/usb.cpp[R186-189]

+			ERR("transfer on ep %02x never completed after cancel: "
+			    "leaking it rather than freeing an in-flight transfer",
+			    t->endpoint);
+			return LIBUSB_ERROR_OTHER;

Evidence

The give-up branch explicitly returns without completing or freeing the submitted transfer, while
sync_done() unconditionally dereferences t->user_data; both synchronous wrappers set that
pointer to a local atomic, and the bulk wrapper passes the caller's buffer directly to libusb even
though existing callers use stack arrays. Because teardown protection tracks only asynchronous ring
transfers, this abandoned synchronous transfer preserves neither its callback and buffer lifetimes
nor the handle and context lifetime required for a later completion.

CLAUDE.md: Preserve libusb Ownership and Teardown Order: CLAUDE.md: Preserve libusb Ownership and Teardown Order: CLAUDE.md: Preserve libusb Ownership and Teardown Order: CLAUDE.md: Preserve libusb Ownership and Teardown Order
src/mt7612u/usb.cpp[141-145]
src/mt7612u/usb.cpp[176-192]
src/mt7612u/usb.cpp[201-225]
src/mt7612u/usb.cpp[234-245]
src/mt7612u/async.cpp[214-262]
src/mt7612u/init.cpp[518-529]
src/mt7612u/usb.cpp[865-903]
src/mt7612u/usb.cpp[163-192]
src/mt7612u/usb.cpp[197-219]
src/mt7612u/usb.cpp[231-245]
src/mt7612u/mcu.cpp[25-40]
src/mt7612u/init.cpp[256-266]

Agent prompt

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

## Issue description
The cancellation give-up path returns while a libusb transfer remains active, allowing its callback state and any caller-owned bulk buffer to outlive their stack storage. The transfer is also absent from teardown tracking, so the radio can release its handle or context while the transfer remains outstanding.
## Fix Focus Areas
- src/mt7612u/usb.cpp[141-145]
- src/mt7612u/usb.cpp[163-192]
- src/mt7612u/usb.cpp[197-246]
## Recommended Fix
Do not return from the synchronous wrapper while its callback can still run. Continue servicing events until cancellation completes and the acquire load observes the callback, then inspect and free the transfer. If an irrecoverable path must leave the transfer pending, move the callback state and any bulk buffer into transfer-owned storage, coordinate caller and callback ownership so resources are freed exactly once, and integrate the outstanding transfer with teardown so the radio cannot release its handle or context before eventual completion.

ⓘ 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 keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[josephnef]

Review addressed, re-validated on the unit:

• Stalled transfer outliving its owner — everything the callback can touch (transfer, buffer, completion word) is now one heap-owned sync_xfer; an atomic exchange decides which of the two parties frees it, the caller's bulk buffer is copied both ways, and a give-up marks the device stranded so mt_close() leaks the handle/context rather than closing under a transfer libusb still owns (the ring's existing policy). 1fbf7ec.
• Doc duplication — the two design bullets in docs/mt7612u.md are references to the declarations now; only the measured context stays. Same commit.
• One more, found on re-validation: the per-call libusb_alloc_transfer() left a benign, intermittent TSan class (event thread's first lock of a fresh transfer's mutex ordered only through the kernel URB handoff). Transfers are pooled at mt_dev_state_init(), before any event thread exists. dfe6916.

Re-run of the same 150 s cross-band sweep under TSan: 0 library/libusb reports (only the two rxdemo globals); witnessed 2000-frame injection: 800 hits on the saturated ground station, inside the earlier 800–1000 band. ctest 69/69.