From fe44d85440751c5c53f40e9513406764c10e1a27 Mon Sep 17 00:00:00 2001 From: Joseph <162703152+josephnef@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:03:59 +0300 Subject: [PATCH 1/3] mt7612u: make a retune race-free against the live RX ring Two data races when a channel change runs while the libusb event thread is delivering frames, both reproduced with ThreadSanitizer on a Comfast CF-922AC driving rxdemo through a cross-band DEVOURER_RX_SWEEP: - mt_read_rx_gain() rewrote the two chain RSSI offsets and the LNA gain as three separate bytes while mt_rx_parse() read them per frame on the event thread, so a frame parsed mid-retune got the new offset with the old LNA gain. They are now one relaxed atomic word (mt7612u_cal::rx_corr), stored once per tune and loaded once per frame; Mt7612uRxCorr.h carries the packing so the headless mapping selftest can pin the sign handling. - Every register access and MCU exchange went through libusb's synchronous API on a context whose events another thread was handling. libusb's sync layer reads its completion flag with no synchronization on the path where handle_events returns early for an expired timeout, so nothing ordered the event thread's last lock of the transfer mutex (io.c:1702) before the caller's libusb_free_transfer() destroyed it; x86 program order hid it. mt_vendor_req() and mt_bulk() now submit through the async API and wait on their own acquire/release flag, pumping events themselves when nobody else is. A transfer that never completes is cancelled, then leaked with a diagnostic rather than freed in flight. Measured after: 150 s, ~1000 retunes, 7221 frames, zero library or libusb reports; the remaining two are rxdemo globals shared by every generation. A/B-neutral where it could cost: retune p50 791 ms on both builds, and a witnessed 2000-frame injection delivered the same count from both at two power back-offs. Co-Authored-By: Claude Fable 5.1 --- docs/mt7612u.md | 77 ++++++++------- src/mt7612u/Mt7612uRxCorr.h | 32 +++++++ src/mt7612u/eeprom.cpp | 13 ++- src/mt7612u/internal.h | 16 +++- src/mt7612u/rx.cpp | 16 +++- src/mt7612u/tools/bringup.cpp | 5 +- src/mt7612u/usb.cpp | 144 ++++++++++++++++++++++++++++- tests/mt7612u_mapping_selftest.cpp | 24 +++++ 8 files changed, 279 insertions(+), 48 deletions(-) create mode 100644 src/mt7612u/Mt7612uRxCorr.h diff --git a/docs/mt7612u.md b/docs/mt7612u.md index 102b634f..5ddf495c 100644 --- a/docs/mt7612u.md +++ b/docs/mt7612u.md @@ -598,46 +598,59 @@ Stated because the numbers above are uniformly favourable. metadata and every consumer that trims four bytes are in place - but the adapter still has to USE it: see Open. +## Retune while the ring is up + +A channel change from the caller's thread while the libusb event thread is +delivering frames is the one concurrent operation this backend has, and it is +measured race-free: `rxdemo` under ThreadSanitizer (`DEVOURER_SANITIZE=thread`) +with `DEVOURER_RX_SWEEP=1,6,11,36,40,44,48` at 150 ms dwells — a full, +cross-band tune every dwell — ran 150 s on a Comfast CF-922AC (SuperSpeed, +`40:a5:ef:5f:65:51`), about a thousand retunes and 7221 frames, with **zero +reports inside the library or libusb**. The two reports that remain are in +`rxdemo` itself (its frame counter and the demos' signal-handler stop flag) +and are shared with every generation. + +Two facts about the design hold that result up: + +- **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 + LNA gain — a wrong RSSI, not a crash, which is the class this port cares + most about. The hot path pays one load. +- **Synchronous transfers do not use libusb's synchronous API.** Every + register access and MCU exchange (`usb.cpp`: `mt_vendor_req`, `mt_bulk`) is + an async submission waited on the library's own acquire/release flag. + libusb's own sync layer reads its completion flag with no synchronization + on the path where `handle_events` returns early for an expired timeout, so + nothing orders the event thread's last lock of the transfer's mutex before + the caller's `libusb_free_transfer()` destroys it. Program order hides that + on x86; the ARM hosts this library is meant for get no such promise. The + replacement is A/B-neutral where it could cost: retune latency on the + 7-bin sweep above read p50 791 ms / mean 756 ms on both the sync-API build + and this one (65 dwells each), and a 2000-frame injection witnessed by an + RTL8822BU delivered the same count from both builds at two power back-offs + (1000 at −20 dB, 900 at −10 dB; the ground station sits 20 cm away in + saturation, so those are its ceiling, not the transmitter's). + +For scale: the same stress on the shipping RTL8812AU path produces **8** TSan +reports, all in devourer's own Jaguar1 state (`RtlJaguarDevice.cpp`, +`RtlAdapter.h`). Retune-during-RX is race-free here and not there. + ## Open list Ordered, and honest about which are unknowns rather than typing: -1. **Two data races the library has when a channel change runs while the RX - ring is up.** Found with ThreadSanitizer against real hardware, driving - `rxdemo` with `DEVOURER_RX_SWEEP` so the main thread retunes while the - libusb event thread delivers frames. Neither is in `Mt7612uRadio` — that - class's `_mu` discipline held, and TSan reported nothing inside it — and - neither is reachable from the bring-up harness, which sets the channel - before starting a ring and measured 0 warnings over 10667 frames. - - - `mt_read_rx_gain()` (`eeprom.cpp`) rewrites the per-channel `lna_gain` and - `rssi_offset[]` from the EEPROM on every tune, while `mt_rx_parse()` - (`rx.cpp`) reads them on the event thread to correct each frame's RSSI. A - frame parsed mid-retune therefore gets a mixed correction, i.e. a wrong - RSSI for that frame. Wrong number, not a crash — which is the class this - port cares most about. - - A synchronous control transfer issued during the tune (`mt_rr_chk` -> - `libusb_control_transfer`) reaches `libusb_free_transfer()`, destroying a - transfer's mutex, while the async ring's event thread locks it. That is - inside libusb, and it is the documented hazard of mixing the synchronous - API with a dedicated event thread on one context. - - For scale: the same stress on the shipping RTL8812AU path produces **8** - TSan reports, all in devourer's own Jaguar1 state (`RtlJaguarDevice.cpp`, - `RtlAdapter.h`). Retune-during-RX is not a race-free operation anywhere in - this project today, so this is a shared gap rather than a MediaTek - regression — but the two above are specific and fixable, and the second one - is a use-after-destroy rather than a torn read. -2. `mt76x2_phy_tssi_compensate()` — periodic temperature correction. Without +1. `mt76x2_phy_tssi_compensate()` — periodic temperature correction. Without it output power drifts with die temperature. -3. Cold-boot verification on a host with switchable USB power. -4. A witness on different silicon. The regression matrix added a second witness +2. Cold-boot verification on a host with switchable USB power. +3. A witness on different silicon. The regression matrix added a second witness *implementation* (`tcpdump` over `mt76x2u`), and a second *unit* of the part — but not a second board revision, and not a decoder outside the MediaTek family. -5. 80 MHz; VHT and NSS=2 on air. -6. Retune tuning — batch registers via `CMD_RANDOM_WRITE`, drop the inter-command +4. 80 MHz; VHT and NSS=2 on air. +5. Retune tuning — batch registers via `CMD_RANDOM_WRITE`, drop the inter-command sleep, skip the `RXDCOC` on a fast path. Worth doing only if 10–20 ms is useful to someone. -7. Whether the single MCS6 frame in the rate-LUT control arm (1 of 84) is a +6. Whether the single MCS6 frame in the rate-LUT control arm (1 of 84) is a witness decode artefact or a real fallback. Unexplained. diff --git a/src/mt7612u/Mt7612uRxCorr.h b/src/mt7612u/Mt7612uRxCorr.h new file mode 100644 index 00000000..3928faad --- /dev/null +++ b/src/mt7612u/Mt7612uRxCorr.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: BSD-3-Clause-Clear */ +/* + * The per-channel RSSI correction as one word. + * + * mt7612u_cal::rx_corr packs the two chain RSSI offsets and the LNA gain + * (mt76x02_mac_get_rssi's three inputs) into a single uint32_t so that a + * retune publishes all three at once and the RX parser reads them in one + * load. Pure integer packing, no device dependency - kept in its own header + * so the headless mapping selftest can pin the sign handling. + * + * Layout: byte 0 = chain-0 offset, byte 1 = chain-1 offset, byte 2 = LNA + * gain, each a signed dB; byte 3 unused. + */ +#ifndef MT7612U_RX_CORR_H +#define MT7612U_RX_CORR_H + +#include + +static inline uint32_t mt_rx_corr_pack(int8_t off0, int8_t off1, int8_t lna) +{ + return (uint32_t)(uint8_t)off0 | ((uint32_t)(uint8_t)off1 << 8) | + ((uint32_t)(uint8_t)lna << 16); +} + +static inline void mt_rx_corr_unpack(uint32_t v, int8_t off[2], int8_t *lna) +{ + off[0] = (int8_t)(v & 0xff); + off[1] = (int8_t)((v >> 8) & 0xff); + *lna = (int8_t)((v >> 16) & 0xff); +} + +#endif diff --git a/src/mt7612u/eeprom.cpp b/src/mt7612u/eeprom.cpp index b286275d..0661f0bb 100644 --- a/src/mt7612u/eeprom.cpp +++ b/src/mt7612u/eeprom.cpp @@ -248,7 +248,7 @@ static uint8_t get_5g_rx_gain(struct mt7612u_dev *d, uint8_t chan) void mt_read_rx_gain(struct mt7612u_dev *d, uint8_t chan, int band) { - int8_t lna_2g, lna_5g[3]; + int8_t lna_2g, lna_5g[3], rssi_offset[2], lna_gain; uint16_t rssi_off, v; uint8_t gain, lna = 0; @@ -273,9 +273,9 @@ void mt_read_rx_gain(struct mt7612u_dev *d, uint8_t chan, int band) rssi_off = (band == 0) ? mt_ee(d, MT_EE_RSSI_OFFSET_2G_0) : mt_ee(d, MT_EE_RSSI_OFFSET_5G_0); - d->cal.rssi_offset[0] = field_valid(rssi_off & 0xff) + rssi_offset[0] = field_valid(rssi_off & 0xff) ? (int8_t)sign_extend_optional(rssi_off & 0xff, 7) : 0; - d->cal.rssi_offset[1] = field_valid(rssi_off >> 8) + rssi_offset[1] = field_valid(rssi_off >> 8) ? (int8_t)sign_extend_optional(rssi_off >> 8, 7) : 0; /* mt76x02_get_lna_gain(): which LNA entry applies to this channel. */ @@ -290,8 +290,13 @@ void mt_read_rx_gain(struct mt7612u_dev *d, uint8_t chan, int band) uint16_t c1 = mt_ee(d, MT_EE_NIC_CONF_1); int ext = (band == 0) ? (c1 & MT_EE_NIC_CONF_1_LNA_EXT_2G) : (c1 & MT_EE_NIC_CONF_1_LNA_EXT_5G); - d->cal.lna_gain = ext ? 0 : (int8_t)sign_extend(lna, 8); + lna_gain = ext ? 0 : (int8_t)sign_extend(lna, 8); } + /* One store: the RX event thread reads this word per frame, and the + * three values only make sense together (see mt7612u_cal::rx_corr). */ + d->cal.rx_corr.store(mt_rx_corr_pack(rssi_offset[0], rssi_offset[1], + lna_gain), + std::memory_order_relaxed); d->cal.mcu_gain = (uint32_t)(lna_2g & 0xff); d->cal.mcu_gain |= (uint32_t)(lna_5g[0] & 0xff) << 8; diff --git a/src/mt7612u/internal.h b/src/mt7612u/internal.h index a55a579d..269d6a7d 100644 --- a/src/mt7612u/internal.h +++ b/src/mt7612u/internal.h @@ -20,6 +20,7 @@ * because MSVC has no and devourer builds Windows first-class. * Nothing outside this subtree includes this header; the public C ABI in * include/mt7612u/mt7612u.h is unaffected and stays C-includable. */ +#include #include #include #include @@ -28,6 +29,7 @@ #include #include #include "regs.h" +#include "Mt7612uRxCorr.h" #include "include/mt7612u/mt7612u.h" /* Per-rate TX power, 0.5 dB units, exactly mt76x02_rate_power's layout. */ @@ -46,8 +48,18 @@ struct mt_tx_power_info { /* EEPROM-derived values the host computes with (firmware does the rest). */ struct mt7612u_cal { - int8_t rssi_offset[2]; - int8_t lna_gain; + /* The per-channel RSSI correction (mt76x02_mac_get_rssi's two chain + * offsets and the LNA gain), packed into one word so that a retune + * publishes all three at once. mt_read_rx_gain() rewrites them on every + * tune from the caller's thread while mt_rx_parse() reads them on the + * libusb event thread for every frame; as three separate bytes a frame + * parsed mid-retune got the new offset with the old LNA gain - a wrong + * RSSI, which ThreadSanitizer reported against real hardware. One + * relaxed atomic word is the whole fix: the reader sees either the old + * triple or the new, never a mix, and the hot path pays one load. + * Encode/decode with mt_rx_corr_pack()/mt_rx_corr_unpack() + * (Mt7612uRxCorr.h). */ + std::atomic rx_corr; int8_t high_gain[2]; uint8_t init_cal_done; uint8_t channel_cal_done; diff --git a/src/mt7612u/rx.cpp b/src/mt7612u/rx.cpp index 27ba3a77..dc706d17 100644 --- a/src/mt7612u/rx.cpp +++ b/src/mt7612u/rx.cpp @@ -91,10 +91,18 @@ int mt_rx_parse(struct mt7612u_dev *d, uint8_t *buf, int n, * the EEPROM (mt76x02_mac_get_rssi); with them at zero these are the * raw chip values, which is still enough to compare two chains. */ info->n_chains = (uint8_t)((d->chainmask & 0xf) > 1 ? 2 : 1); - for (int c = 0; c < 4; c++) - info->rssi[c] = (int8_t)((int8_t)rxwi[12 + c] + - (c < 2 ? d->cal.rssi_offset[c] : 0) - - d->cal.lna_gain); + { + /* One load of the packed triple: a retune on another thread + * republishes all three at once, so this frame is corrected + * consistently with either the old channel or the new one. */ + int8_t rssi_offset[2], lna_gain; + mt_rx_corr_unpack(d->cal.rx_corr.load(std::memory_order_relaxed), + rssi_offset, &lna_gain); + for (int c = 0; c < 4; c++) + info->rssi[c] = (int8_t)((int8_t)rxwi[12 + c] + + (c < 2 ? rssi_offset[c] : 0) - + lna_gain); + } for (int i = 0; i < 4; i++) info->bbp[i] = get_le32(rxwi + 16 + 4 * i); diff --git a/src/mt7612u/tools/bringup.cpp b/src/mt7612u/tools/bringup.cpp index 933c1ddd..ae8c6bd7 100644 --- a/src/mt7612u/tools/bringup.cpp +++ b/src/mt7612u/tools/bringup.cpp @@ -1812,9 +1812,12 @@ static int gate_caps(uint8_t chan) c.max_mpdu_tx, c.max_mpdu_rx, mt_rr(&dev, MT_MAX_LEN_CFG) & 0xfff); + int8_t rssi_offset[2], lna_gain; + mt_rx_corr_unpack(dev.cal.rx_corr.load(std::memory_order_relaxed), + rssi_offset, &lna_gain); printf("\nRX gain from EEPROM: rssi_offset=[%d,%d] lna_gain=%d " "high_gain=[%d,%d] mcu_gain=0x%08x\n", - dev.cal.rssi_offset[0], dev.cal.rssi_offset[1], dev.cal.lna_gain, + rssi_offset[0], rssi_offset[1], lna_gain, dev.cal.high_gain[0], dev.cal.high_gain[1], dev.cal.mcu_gain); printf(" raw EEPROM: LNA_GAIN=0x%04x RSSI_OFF_5G_0=0x%04x " "RSSI_OFF_5G_1=0x%04x GRP4_5_RX_HIGH_GAIN=0x%04x\n", diff --git a/src/mt7612u/usb.cpp b/src/mt7612u/usb.cpp index 728f8295..e21b5ace 100644 --- a/src/mt7612u/usb.cpp +++ b/src/mt7612u/usb.cpp @@ -16,6 +16,7 @@ #include #endif +#include #include "internal.h" /* See the LOG/WARN/ERR contract in internal.h. The whole line is formatted @@ -110,6 +111,141 @@ static uint64_t now_us(void) std::chrono::steady_clock::now().time_since_epoch()).count(); } +/* + * Synchronous transfers, built on libusb's async API. + * + * libusb's own libusb_control_transfer()/libusb_bulk_transfer() are not used + * here because this library runs a second event thread (async.cpp) on the same + * context. libusb's sync layer decides the transfer is over by reading its + * completion flag with no synchronization against the thread that set it - + * on the path where handle_events returns early for an expired timeout there + * is no lock between the event thread's last touch of the transfer and the + * caller's libusb_free_transfer(), which destroys the transfer's mutex. On + * x86 program order hides it; on the ARM hosts this library is meant for + * nothing does. ThreadSanitizer reports it against real hardware whenever a + * register access runs while the RX ring is up (a retune, the 1 Hz tick). + * + * So: submit through the async API, and wait on our own flag with + * acquire/release semantics. The callback's release store happens after + * everything libusb did with the transfer on the event thread, and the + * acquire load below orders the free after it. While waiting, this thread + * pumps events itself when no one else is (a caller with no RX ring), or + * parks as an event waiter when the ring's thread holds the events lock - + * the same two behaviours the sync API has, minus the unordered read. + * + * A transfer that has not completed is never freed: past the transfer's own + * timeout plus a margin it is cancelled, and if even the cancellation does + * not come back the transfer is leaked with a diagnostic rather than freed + * under the event thread. + */ +static void LIBUSB_CALL sync_done(struct libusb_transfer *t) +{ + static_cast *>(t->user_data) + ->store(1, std::memory_order_release); +} + +static int status_to_rc(enum libusb_transfer_status st) +{ + switch (st) { + case LIBUSB_TRANSFER_COMPLETED: return 0; + case LIBUSB_TRANSFER_TIMED_OUT: return LIBUSB_ERROR_TIMEOUT; + case LIBUSB_TRANSFER_STALL: return LIBUSB_ERROR_PIPE; + case LIBUSB_TRANSFER_NO_DEVICE: return LIBUSB_ERROR_NO_DEVICE; + case LIBUSB_TRANSFER_OVERFLOW: return LIBUSB_ERROR_OVERFLOW; + default: return LIBUSB_ERROR_IO; + } +} + +/* Submit `t` (already filled, callback sync_done, user_data `done`) and wait + * for it. Returns a LIBUSB_ERROR_* code, 0 on completion. On return the + * transfer is complete and may be read and freed - except when this returns + * LIBUSB_ERROR_OTHER, which means it was leaked (see above). */ +static int submit_and_wait(struct mt7612u_dev *d, struct libusb_transfer *t, + std::atomic *done, unsigned timeout_ms) +{ + int rc = libusb_submit_transfer(t); + if (rc) + return rc; + + const auto cancel_at = std::chrono::steady_clock::now() + + std::chrono::milliseconds(timeout_ms) + + std::chrono::seconds(2); + const auto give_up_at = cancel_at + std::chrono::seconds(2); + bool cancelled = false; + + while (!done->load(std::memory_order_acquire)) { + struct timeval tv = { 0, 20000 }; + int r = libusb_handle_events_timeout_completed(d->ctx, &tv, NULL); + if (r < 0 && r != LIBUSB_ERROR_INTERRUPTED) + mt_usleep(1000); /* a dead context: don't spin flat out */ + const auto now = std::chrono::steady_clock::now(); + if (!cancelled && now > cancel_at) { + libusb_cancel_transfer(t); + cancelled = true; + } else if (cancelled && now > give_up_at) { + ERR("transfer on ep %02x never completed after cancel: " + "leaking it rather than freeing an in-flight transfer", + t->endpoint); + return LIBUSB_ERROR_OTHER; + } + } + return status_to_rc(t->status); +} + +/* libusb_control_transfer(), over submit_and_wait(). Same contract: the + * number of data bytes transferred on success, a LIBUSB_ERROR_* otherwise. */ +static int sync_control(struct mt7612u_dev *d, uint8_t type, uint8_t req, + uint16_t val, uint16_t idx, unsigned char *data, + uint16_t len, unsigned timeout_ms) +{ + std::atomic done{0}; + struct libusb_transfer *t = libusb_alloc_transfer(0); + if (!t) + return LIBUSB_ERROR_NO_MEM; + unsigned char *buf = + (unsigned char *)malloc(LIBUSB_CONTROL_SETUP_SIZE + len); + if (!buf) { + libusb_free_transfer(t); + return LIBUSB_ERROR_NO_MEM; + } + libusb_fill_control_setup(buf, type, req, val, idx, len); + if ((type & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_OUT && len) + memcpy(buf + LIBUSB_CONTROL_SETUP_SIZE, data, len); + libusb_fill_control_transfer(t, d->h, buf, sync_done, &done, timeout_ms); + t->flags = LIBUSB_TRANSFER_FREE_BUFFER; + + int rc = submit_and_wait(d, t, &done, timeout_ms); + if (rc == LIBUSB_ERROR_OTHER) + return rc; /* leaked on purpose; buffer goes with it */ + if (rc == 0) { + rc = t->actual_length; + if ((type & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_IN && rc > 0) + memcpy(data, buf + LIBUSB_CONTROL_SETUP_SIZE, (size_t)rc); + } + libusb_free_transfer(t); + return rc; +} + +/* libusb_bulk_transfer(), over submit_and_wait(). *xfered is filled on a + * timeout too, as libusb's is. */ +static int sync_bulk(struct mt7612u_dev *d, uint8_t ep, unsigned char *buf, + int len, int *xfered, unsigned timeout_ms) +{ + std::atomic done{0}; + struct libusb_transfer *t = libusb_alloc_transfer(0); + if (!t) + return LIBUSB_ERROR_NO_MEM; + libusb_fill_bulk_transfer(t, d->h, ep, buf, len, sync_done, &done, + timeout_ms); + int rc = submit_and_wait(d, t, &done, timeout_ms); + if (rc == LIBUSB_ERROR_OTHER) + return rc; + if (xfered) + *xfered = t->actual_length; + libusb_free_transfer(t); + return rc; +} + int mt_vendor_req(struct mt7612u_dev *d, uint8_t req, uint8_t type, uint16_t val, uint16_t idx, void *buf, size_t len) { @@ -121,9 +257,8 @@ int mt_vendor_req(struct mt7612u_dev *d, uint8_t req, uint8_t type, d->io_lock.lock(); for (int i = 0; i < VEND_RETRIES; i++) { - rc = libusb_control_transfer(d->h, type, req, val, idx, - (unsigned char *)buf, (uint16_t)len, - CTRL_TIMEOUT_MS); + rc = sync_control(d, type, req, val, idx, (unsigned char *)buf, + (uint16_t)len, CTRL_TIMEOUT_MS); if (rc >= 0 || rc == LIBUSB_ERROR_NO_DEVICE) goto out; mt_usleep(5000); @@ -288,8 +423,7 @@ int mt_bulk(struct mt7612u_dev *d, uint8_t ep, void *buf, int len, int *xfered, unsigned timeout_ms) { int n = 0; - int rc = libusb_bulk_transfer(d->h, ep, (unsigned char *)buf, len, - &n, timeout_ms); + int rc = sync_bulk(d, ep, (unsigned char *)buf, len, &n, timeout_ms); if (xfered) *xfered = n; return rc; } diff --git a/tests/mt7612u_mapping_selftest.cpp b/tests/mt7612u_mapping_selftest.cpp index f048e413..3a0e9d76 100644 --- a/tests/mt7612u_mapping_selftest.cpp +++ b/tests/mt7612u_mapping_selftest.cpp @@ -9,6 +9,7 @@ * translations are pure functions in a header with this test under them * rather than inline in the device class. */ #include "mt7612u/Mt7612uMapping.h" +#include "mt7612u/Mt7612uRxCorr.h" #include #include @@ -50,6 +51,29 @@ int main() { expect("rssi round-trips to dBm", static_cast(rssi_to_raw(-63)) - 110 == -63); + /* --- the packed per-channel RSSI correction: signs survive the word --- */ + { + int8_t off[2] = {0, 0}, lna = 0; + /* Real EEPROM values: negative chain offsets and a positive LNA gain. */ + mt_rx_corr_unpack(mt_rx_corr_pack(-5, -3, 8), off, &lna); + expect("rx_corr keeps chain-0 offset sign", off[0] == -5); + expect("rx_corr keeps chain-1 offset sign", off[1] == -3); + expect("rx_corr keeps lna gain", lna == 8); + mt_rx_corr_unpack(mt_rx_corr_pack(127, -128, -1), off, &lna); + expect("rx_corr int8 extremes round-trip", + off[0] == 127 && off[1] == -128 && lna == -1); + /* The zero word - what a device reads before its first tune - corrects + * nothing, which is the raw-chip-value behaviour rx.cpp documents. */ + mt_rx_corr_unpack(0, off, &lna); + expect("rx_corr zero word is no correction", + off[0] == 0 && off[1] == 0 && lna == 0); + /* And the correction rx.cpp applies from it matches the byte-wise form. */ + const int8_t raw = -60; + mt_rx_corr_unpack(mt_rx_corr_pack(-5, -3, 8), off, &lna); + expect("rx_corr corrected RSSI = raw + off - lna", + (int8_t)(raw + off[0] - lna) == -73); + } + /* --- per-chain signal: 2T2R, and rssi[2] is the NOISE FLOOR --- */ { struct mt7612u_rx_info i {}; From 1fbf7ec7e8f6bf875489827df5f1f309bfebf4cd Mon Sep 17 00:00:00 2001 From: Joseph <162703152+josephnef@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:18:34 +0300 Subject: [PATCH 2/3] mt7612u: a stalled sync transfer is handed to its callback, not left pointing at a dead frame Review of the async-backed sync helpers: the give-up path returned while the transfer's callback still pointed at a stack-local completion word, and a bulk transfer at the caller's stack buffer. Everything the callback can touch now lives in one heap-owned sync_xfer; the last of the two parties to arrive frees it (an atomic exchange decides which), the caller's bulk buffer is copied both ways, and a give-up marks the device stranded so mt_close() leaks the USB handle and context instead of closing underneath a transfer libusb still owns - the ring's existing policy. Also trims the doc's two design bullets to references: the layout and the lifetime rules are documented once, on the declarations. Co-Authored-By: Claude Fable 5.1 --- docs/mt7612u.md | 21 +++--- src/mt7612u/usb.cpp | 169 +++++++++++++++++++++++++++++--------------- 2 files changed, 121 insertions(+), 69 deletions(-) diff --git a/docs/mt7612u.md b/docs/mt7612u.md index 5ddf495c..2791ee41 100644 --- a/docs/mt7612u.md +++ b/docs/mt7612u.md @@ -612,20 +612,15 @@ and are shared with every generation. Two facts about the design hold that result up: -- **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 - LNA gain — a wrong RSSI, not a crash, which is the class this port cares - most about. The hot path pays one load. +- **The per-channel RSSI correction is one word**, published once per tune + and loaded once per frame — the layout and why are on the declaration + (`mt7612u_cal::rx_corr`, `Mt7612uRxCorr.h`). What that bought: three + separate bytes gave a frame parsed mid-retune a wrong RSSI, which is the + class of bug this port cares most about. - **Synchronous transfers do not use libusb's synchronous API.** Every - register access and MCU exchange (`usb.cpp`: `mt_vendor_req`, `mt_bulk`) is - an async submission waited on the library's own acquire/release flag. - libusb's own sync layer reads its completion flag with no synchronization - on the path where `handle_events` returns early for an expired timeout, so - nothing orders the event thread's last lock of the transfer's mutex before - the caller's `libusb_free_transfer()` destroys it. Program order hides that - on x86; the ARM hosts this library is meant for get no such promise. The + register access and MCU exchange is an async submission waited on the + library's own acquire/release flag; the hazard in libusb's own sync layer + and the lifetime rules are documented once, at the helpers in `usb.cpp`. The replacement is A/B-neutral where it could cost: retune latency on the 7-bin sweep above read p50 791 ms / mean 756 ms on both the sync-API build and this one (65 dwells each), and a 2000-frame injection witnessed by an diff --git a/src/mt7612u/usb.cpp b/src/mt7612u/usb.cpp index e21b5ace..526be0c3 100644 --- a/src/mt7612u/usb.cpp +++ b/src/mt7612u/usb.cpp @@ -17,6 +17,7 @@ #endif #include +#include #include "internal.h" /* See the LOG/WARN/ERR contract in internal.h. The whole line is formatted @@ -133,18 +134,43 @@ static uint64_t now_us(void) * parks as an event waiter when the ring's thread holds the events lock - * the same two behaviours the sync API has, minus the unordered read. * - * A transfer that has not completed is never freed: past the transfer's own - * timeout plus a margin it is cancelled, and if even the cancellation does - * not come back the transfer is leaked with a diagnostic rather than freed - * under the event thread. + * A transfer that has not completed is never freed, and its callback never + * outlives its state: everything the callback can touch - the transfer, its + * buffer and the completion word - lives on the heap in one sync_xfer that + * the LAST of the two parties to arrive frees. Past the transfer's own timeout + * plus a margin the waiter cancels it; if even the cancellation does not come + * back it hands ownership to the callback (state ABANDONED) and returns, and + * the device is marked stranded so mt_close() leaks the USB handle and context + * instead of closing underneath a transfer libusb still owns - the same policy + * the async ring already has. */ -static void LIBUSB_CALL sync_done(struct libusb_transfer *t) +namespace { +enum { XFER_PENDING = 0, XFER_DONE = 1, XFER_ABANDONED = 2 }; + +struct sync_xfer { + std::atomic state{XFER_PENDING}; + struct libusb_transfer *t = nullptr; + unsigned char *buf = nullptr; /* heap copy the transfer reads/writes */ +}; + +void sync_xfer_free(struct sync_xfer *x) +{ + libusb_free_transfer(x->t); /* also frees x->buf: FREE_BUFFER */ + delete x; +} + +void LIBUSB_CALL sync_done(struct libusb_transfer *t) { - static_cast *>(t->user_data) - ->store(1, std::memory_order_release); + auto *x = static_cast(t->user_data); + /* Release: everything libusb did with the transfer on this thread is + * ordered before the waiter's acquire load. If the waiter has already + * given up, this callback is the last party and frees. */ + if (x->state.exchange(XFER_DONE, std::memory_order_acq_rel) == + XFER_ABANDONED) + sync_xfer_free(x); } -static int status_to_rc(enum libusb_transfer_status st) +int status_to_rc(enum libusb_transfer_status st) { switch (st) { case LIBUSB_TRANSFER_COMPLETED: return 0; @@ -156,14 +182,33 @@ static int status_to_rc(enum libusb_transfer_status st) } } -/* Submit `t` (already filled, callback sync_done, user_data `done`) and wait - * for it. Returns a LIBUSB_ERROR_* code, 0 on completion. On return the - * transfer is complete and may be read and freed - except when this returns - * LIBUSB_ERROR_OTHER, which means it was leaked (see above). */ -static int submit_and_wait(struct mt7612u_dev *d, struct libusb_transfer *t, - std::atomic *done, unsigned timeout_ms) +/* Allocate a sync_xfer with a `len`-byte heap buffer. NULL on allocation + * failure. */ +struct sync_xfer *sync_xfer_new(size_t len) +{ + auto *x = new (std::nothrow) sync_xfer; + if (!x) + return nullptr; + x->t = libusb_alloc_transfer(0); + x->buf = (unsigned char *)malloc(len ? len : 1); + if (!x->t || !x->buf) { + free(x->buf); + libusb_free_transfer(x->t); + delete x; + return nullptr; + } + return x; +} + +/* Submit x->t (already filled with callback sync_done and user_data x) and + * wait for it. Returns a LIBUSB_ERROR_* code, 0 on completion; on any return + * but LIBUSB_ERROR_OTHER the transfer is complete, still owned by the caller, + * and may be read then freed with sync_xfer_free(). LIBUSB_ERROR_OTHER means + * the transfer was abandoned to its callback: the caller owns nothing. */ +int submit_and_wait(struct mt7612u_dev *d, struct sync_xfer *x, + unsigned timeout_ms) { - int rc = libusb_submit_transfer(t); + int rc = libusb_submit_transfer(x->t); if (rc) return rc; @@ -173,78 +218,90 @@ static int submit_and_wait(struct mt7612u_dev *d, struct libusb_transfer *t, const auto give_up_at = cancel_at + std::chrono::seconds(2); bool cancelled = false; - while (!done->load(std::memory_order_acquire)) { + while (x->state.load(std::memory_order_acquire) != XFER_DONE) { struct timeval tv = { 0, 20000 }; int r = libusb_handle_events_timeout_completed(d->ctx, &tv, NULL); if (r < 0 && r != LIBUSB_ERROR_INTERRUPTED) mt_usleep(1000); /* a dead context: don't spin flat out */ const auto now = std::chrono::steady_clock::now(); if (!cancelled && now > cancel_at) { - libusb_cancel_transfer(t); + libusb_cancel_transfer(x->t); cancelled = true; } else if (cancelled && now > give_up_at) { ERR("transfer on ep %02x never completed after cancel: " - "leaking it rather than freeing an in-flight transfer", - t->endpoint); + "abandoning it to its callback and stranding the device", + x->t->endpoint); + d->transfers_stranded = 1; + /* The callback may have landed between the loop test and + * here; then WE are the last party and free. */ + if (x->state.exchange(XFER_ABANDONED, + std::memory_order_acq_rel) == XFER_DONE) + sync_xfer_free(x); return LIBUSB_ERROR_OTHER; } } - return status_to_rc(t->status); + return status_to_rc(x->t->status); } /* libusb_control_transfer(), over submit_and_wait(). Same contract: the * number of data bytes transferred on success, a LIBUSB_ERROR_* otherwise. */ -static int sync_control(struct mt7612u_dev *d, uint8_t type, uint8_t req, - uint16_t val, uint16_t idx, unsigned char *data, - uint16_t len, unsigned timeout_ms) +int sync_control(struct mt7612u_dev *d, uint8_t type, uint8_t req, + uint16_t val, uint16_t idx, unsigned char *data, + uint16_t len, unsigned timeout_ms) { - std::atomic done{0}; - struct libusb_transfer *t = libusb_alloc_transfer(0); - if (!t) + const bool out = (type & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_OUT; + struct sync_xfer *x = sync_xfer_new(LIBUSB_CONTROL_SETUP_SIZE + len); + if (!x) return LIBUSB_ERROR_NO_MEM; - unsigned char *buf = - (unsigned char *)malloc(LIBUSB_CONTROL_SETUP_SIZE + len); - if (!buf) { - libusb_free_transfer(t); - return LIBUSB_ERROR_NO_MEM; - } - libusb_fill_control_setup(buf, type, req, val, idx, len); - if ((type & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_OUT && len) - memcpy(buf + LIBUSB_CONTROL_SETUP_SIZE, data, len); - libusb_fill_control_transfer(t, d->h, buf, sync_done, &done, timeout_ms); - t->flags = LIBUSB_TRANSFER_FREE_BUFFER; - - int rc = submit_and_wait(d, t, &done, timeout_ms); + libusb_fill_control_setup(x->buf, type, req, val, idx, len); + if (out && len) + memcpy(x->buf + LIBUSB_CONTROL_SETUP_SIZE, data, len); + libusb_fill_control_transfer(x->t, d->h, x->buf, sync_done, x, + timeout_ms); + x->t->flags = LIBUSB_TRANSFER_FREE_BUFFER; + + int rc = submit_and_wait(d, x, timeout_ms); if (rc == LIBUSB_ERROR_OTHER) - return rc; /* leaked on purpose; buffer goes with it */ + return rc; if (rc == 0) { - rc = t->actual_length; - if ((type & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_IN && rc > 0) - memcpy(data, buf + LIBUSB_CONTROL_SETUP_SIZE, (size_t)rc); + rc = x->t->actual_length; + if (!out && rc > 0) + memcpy(data, x->buf + LIBUSB_CONTROL_SETUP_SIZE, (size_t)rc); } - libusb_free_transfer(t); + sync_xfer_free(x); return rc; } /* libusb_bulk_transfer(), over submit_and_wait(). *xfered is filled on a - * timeout too, as libusb's is. */ -static int sync_bulk(struct mt7612u_dev *d, uint8_t ep, unsigned char *buf, - int len, int *xfered, unsigned timeout_ms) -{ - std::atomic done{0}; - struct libusb_transfer *t = libusb_alloc_transfer(0); - if (!t) + * timeout too, as libusb's is. The caller's buffer is copied both ways so + * that an abandoned transfer holds no pointer into the caller's frame. */ +int sync_bulk(struct mt7612u_dev *d, uint8_t ep, unsigned char *buf, int len, + int *xfered, unsigned timeout_ms) +{ + const bool in = (ep & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_IN; + struct sync_xfer *x = sync_xfer_new((size_t)len); + if (!x) return LIBUSB_ERROR_NO_MEM; - libusb_fill_bulk_transfer(t, d->h, ep, buf, len, sync_done, &done, + if (!in && len) + memcpy(x->buf, buf, (size_t)len); + libusb_fill_bulk_transfer(x->t, d->h, ep, x->buf, len, sync_done, x, timeout_ms); - int rc = submit_and_wait(d, t, &done, timeout_ms); - if (rc == LIBUSB_ERROR_OTHER) + x->t->flags = LIBUSB_TRANSFER_FREE_BUFFER; + + int rc = submit_and_wait(d, x, timeout_ms); + if (rc == LIBUSB_ERROR_OTHER) { + if (xfered) *xfered = 0; return rc; + } + const int n = x->t->actual_length; + if (in && n > 0) + memcpy(buf, x->buf, (size_t)n); if (xfered) - *xfered = t->actual_length; - libusb_free_transfer(t); + *xfered = n; + sync_xfer_free(x); return rc; } +} /* namespace */ int mt_vendor_req(struct mt7612u_dev *d, uint8_t req, uint8_t type, uint16_t val, uint16_t idx, void *buf, size_t len) From dfe69160c0094fe4caf65998d1ac1d736a804c8a Mon Sep 17 00:00:00 2001 From: Joseph <162703152+josephnef@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:25:57 +0300 Subject: [PATCH 3/3] mt7612u: pool the sync helpers' transfers so their mutexes are born before the event thread With per-call libusb_alloc_transfer() the only ordering between a transfer's mutex initialisation on the caller's thread and the event thread's first lock of it runs through the kernel's URB handoff, which ThreadSanitizer cannot see - the one report class left after the sync rework, benign and intermittent. Transfers for the sync helpers now come from a small pool allocated in mt_dev_state_init(), before any event thread exists, so thread creation orders them; an empty pool falls back to allocating, correct but noisier. Buffers stay per-call and are freed by the helper, not by libusb. Co-Authored-By: Claude Fable 5.1 --- src/mt7612u/internal.h | 11 +++++++++ src/mt7612u/usb.cpp | 56 +++++++++++++++++++++++++++++++----------- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/mt7612u/internal.h b/src/mt7612u/internal.h index 269d6a7d..2a727aaa 100644 --- a/src/mt7612u/internal.h +++ b/src/mt7612u/internal.h @@ -81,6 +81,7 @@ struct mt7612u_cal { * RX hot path does no cross-thread write at all. */ }; +#define MT_SYNC_POOL 4 /* pooled transfers for the sync helpers */ #define MT_RX_RING 16 /* 16 slots, not 32: the slots now carry a full aggregate, so this is the * difference between 256 KB and 512 KB of ring. Depth is not what buys @@ -220,6 +221,16 @@ struct mt7612u_dev { unsigned io_err; /* EP0 transfers that exhausted their retries */ int transfers_stranded; /* libusb still owns a cancelled ring */ + /* libusb_transfer objects for the synchronous helpers (usb.cpp), taken + * from here rather than allocated per call. Allocated in + * mt_dev_state_init(), i.e. before any event thread exists, so the + * thread's first lock of a transfer's mutex is ordered after its + * initialisation by thread creation - a per-call allocation is ordered + * only through the kernel's URB handoff, which ThreadSanitizer cannot + * see and reports. Empty pool = allocate fresh (correct, just noisier). */ + std::mutex sync_pool_mu; + struct libusb_transfer *sync_pool[MT_SYNC_POOL]; + int sync_pool_n; uint16_t max_mpdu_rx; /* from MT_MAX_LEN_CFG at init, less the FCS */ uint64_t stats_last_us; /* previous mt7612u_link_stats() mark */ int ch_time_armed; /* channel timers configured and zeroed */ diff --git a/src/mt7612u/usb.cpp b/src/mt7612u/usb.cpp index 526be0c3..de245a22 100644 --- a/src/mt7612u/usb.cpp +++ b/src/mt7612u/usb.cpp @@ -149,13 +149,26 @@ enum { XFER_PENDING = 0, XFER_DONE = 1, XFER_ABANDONED = 2 }; struct sync_xfer { std::atomic state{XFER_PENDING}; + struct mt7612u_dev *d = nullptr; struct libusb_transfer *t = nullptr; unsigned char *buf = nullptr; /* heap copy the transfer reads/writes */ }; +/* Return the transfer to the device pool (see mt7612u_dev::sync_pool) if + * there is room, else free it. Never called on an in-flight transfer. */ void sync_xfer_free(struct sync_xfer *x) { - libusb_free_transfer(x->t); /* also frees x->buf: FREE_BUFFER */ + free(x->buf); + bool pooled = false; + { + std::lock_guard lk(x->d->sync_pool_mu); + if (x->d->sync_pool_n < MT_SYNC_POOL) { + x->d->sync_pool[x->d->sync_pool_n++] = x->t; + pooled = true; + } + } + if (!pooled) + libusb_free_transfer(x->t); delete x; } @@ -184,12 +197,19 @@ int status_to_rc(enum libusb_transfer_status st) /* Allocate a sync_xfer with a `len`-byte heap buffer. NULL on allocation * failure. */ -struct sync_xfer *sync_xfer_new(size_t len) +struct sync_xfer *sync_xfer_new(struct mt7612u_dev *d, size_t len) { auto *x = new (std::nothrow) sync_xfer; if (!x) return nullptr; - x->t = libusb_alloc_transfer(0); + x->d = d; + { + std::lock_guard lk(d->sync_pool_mu); + if (d->sync_pool_n > 0) + x->t = d->sync_pool[--d->sync_pool_n]; + } + if (!x->t) + x->t = libusb_alloc_transfer(0); x->buf = (unsigned char *)malloc(len ? len : 1); if (!x->t || !x->buf) { free(x->buf); @@ -250,7 +270,7 @@ int sync_control(struct mt7612u_dev *d, uint8_t type, uint8_t req, uint16_t len, unsigned timeout_ms) { const bool out = (type & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_OUT; - struct sync_xfer *x = sync_xfer_new(LIBUSB_CONTROL_SETUP_SIZE + len); + struct sync_xfer *x = sync_xfer_new(d, LIBUSB_CONTROL_SETUP_SIZE + len); if (!x) return LIBUSB_ERROR_NO_MEM; libusb_fill_control_setup(x->buf, type, req, val, idx, len); @@ -258,7 +278,7 @@ int sync_control(struct mt7612u_dev *d, uint8_t type, uint8_t req, memcpy(x->buf + LIBUSB_CONTROL_SETUP_SIZE, data, len); libusb_fill_control_transfer(x->t, d->h, x->buf, sync_done, x, timeout_ms); - x->t->flags = LIBUSB_TRANSFER_FREE_BUFFER; + x->t->flags = 0; int rc = submit_and_wait(d, x, timeout_ms); if (rc == LIBUSB_ERROR_OTHER) @@ -279,14 +299,14 @@ int sync_bulk(struct mt7612u_dev *d, uint8_t ep, unsigned char *buf, int len, int *xfered, unsigned timeout_ms) { const bool in = (ep & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_IN; - struct sync_xfer *x = sync_xfer_new((size_t)len); + struct sync_xfer *x = sync_xfer_new(d, (size_t)len); if (!x) return LIBUSB_ERROR_NO_MEM; if (!in && len) memcpy(x->buf, buf, (size_t)len); libusb_fill_bulk_transfer(x->t, d->h, ep, x->buf, len, sync_done, x, timeout_ms); - x->t->flags = LIBUSB_TRANSFER_FREE_BUFFER; + x->t->flags = 0; int rc = submit_and_wait(d, x, timeout_ms); if (rc == LIBUSB_ERROR_OTHER) { @@ -525,17 +545,25 @@ static int mt_identify(struct mt7612u_dev *d, const char **err) void mt_dev_state_init(struct mt7612u_dev *d) { /* io_lock is a std::recursive_mutex member, constructed with the device, - * so there is nothing to initialise here and no path that can reach the - * tick with an unusable lock. Kept as a named seam because both open - * paths call it and the cal sentinels (low_gain=-1 etc.) belong to the - * same "device state is ready" step - those are reset per-tune in - * mt_set_channel_ex(), which always runs before the first PHY tick. */ - (void)d; + * so it needs nothing here. What does: the sync helpers' transfer pool, + * which has to exist before any event thread does (see the field). An + * allocation failure leaves the pool short and the helpers allocate per + * call instead - the cal sentinels (low_gain=-1 etc.) are reset per-tune + * in mt_set_channel_ex(), which always runs before the first PHY tick. */ + std::lock_guard lk(d->sync_pool_mu); + while (d->sync_pool_n < MT_SYNC_POOL) { + struct libusb_transfer *t = libusb_alloc_transfer(0); + if (!t) + break; + d->sync_pool[d->sync_pool_n++] = t; + } } void mt_dev_state_destroy(struct mt7612u_dev *d) { - (void)d; + std::lock_guard lk(d->sync_pool_mu); + while (d->sync_pool_n > 0) + libusb_free_transfer(d->sync_pool[--d->sync_pool_n]); } /*