From 720a4ddc9371cbc399e49207d57235a23c2fbb89 Mon Sep 17 00:00:00 2001 From: snokvist Date: Mon, 17 Aug 2026 19:05:46 +0200 Subject: [PATCH 1/3] rtl8733b: port USB TX aggregation (send_packets) The 8733B was the last USB backend without send_packets batching, so every frame cost its own bulk-OUT URB. That is cheap on a desktop and expensive on the SoCs this part actually ships on. Measured before writing any code, on the CV610 craft and on x86, by sweeping payload and reading process CPU against frames submitted in steady state: per submission x86 ~22 us CV610 ~248 us (11x) split per-call 19.1 us + 0.002 us/byte on x86; on ARM the payload term is below noise -- it is essentially ALL per-call attribution ~87% kernel USB submit/completion, ~5% descriptor build, ~4% libc Per-call dominance is what makes packing worth it, because a 3:1 URB removes two submissions in three while the descriptor build stays. Ported: send_packets over the shared devourer::plan_tx_agg planner (desc_size 40, no first-block reserve, max 3 blocks), and a DMA_TXAGG_NUM field on the first descriptor. Two things differ from the 88xx siblings. MAC init ALREADY programmed BLK_DESC_NUM = 3 into DWBCN0_CTRL[7:4], the same field and value, so bring-up needed no change. And the count must be written BEFORE the checksum rather than patched on after: the fold covers 32 bytes skipping only 0x1c-0x1d, so byte 0x1f is inside it. A selftest cell pins that ordering and fails if the write moves below the checksum (verified by mutation). Result, same craft, ~1750 fps, frame rate unchanged (1733 -> 1800): us/frame 248.08 -> 148.15 CPU 43.0% -> 26.7% of one core At the craft's ~1100 pps operating point that is about ten points of a core. On x86 the same A/B reads 21.5 -> 10.6 us. DMA_TXAGG_NUM's placement was inferred from 8822C parity, so it was confirmed on silicon rather than assumed. The 8822BU precedent is that wrong packing makes the TXDMA re-air block 1 agg_num times, which frame counts cannot see -- rx_hits is identical either way. Every frame was therefore stamped (DEVOURER_TX_QOS_DATA) and the stamps counted distinct at an 8812AU witness (DEVOURER_RX_PCTR): 31721 receptions, 31721 distinct counters, ratio 1.00 where re-airing would read 3.00. All 10940 aggregated URBs carried frames=3. Knob off (the default) is byte-identical: agg_num 0 leaves dword7[31:24] clear and the single-frame path is untouched, which a selftest cell pins. Note GetTxStats().submitted counts URBs rather than frames, so an aggregated session reports about a third -- the same accounting the other families have, not a throughput drop; tx.agg carries the true count. ctest 54/54. Co-Authored-By: Claude Opus 5 (1M context) --- docs/aggregation.md | 17 +++- docs/rtl8733b.md | 24 ++++++ src/rtl8733b/CLAUDE.md | 22 +++++ src/rtl8733b/Rtl8733bDevice.cpp | 122 +++++++++++++++++++++++++++- src/rtl8733b/Rtl8733bDevice.h | 19 ++++- src/rtl8733b/TxDescriptor8733b.h | 15 +++- tests/rtl8733b_tx_desc_selftest.cpp | 45 ++++++++++ tests/txagg_bench.sh | 16 ++++ 8 files changed, 274 insertions(+), 6 deletions(-) diff --git a/docs/aggregation.md b/docs/aggregation.md index c26bd3f5..1bc2c768 100644 --- a/docs/aggregation.md +++ b/docs/aggregation.md @@ -28,10 +28,25 @@ never lands on an exact bulk-MPS multiple (the sync bulk path has no ZLP). Per-family hardware rules: -- **HalMAC (8822B/8821C/8822C/8822E)**: at most **3 descriptors per bulk +- **HalMAC 88xx (8822B/8821C/8822C/8822E)**: at most **3 descriptors per bulk transfer** (mainline rtw88 `usb_tx_agg_desc_num` / halmac `BLK_DESC_NUM`) — the library clamps. Layout is rtw88-parity: no first-block reserve; the 8-byte PKT_OFFSET shim is inserted only to escape a bulk-boundary total. +- **RTL8733B (HALMAC 87xx)**: the same 3-descriptor HalMAC rule and the same + no-reserve layout, with a 40-byte descriptor. Two things differ from the + 88xx siblings. `DMA_TXAGG_NUM` sits at the same `dword7[31:24]`, but this + family's checksum is folded *inside* `fill_tx_desc_8733b`, so the count is + built in rather than patched on and re-checksummed — byte `0x1f` is inside + the checksummed span (the fold skips only `0x1c-0x1d`), so writing it after + the checksum yields a descriptor the chip rejects. And `BLK_DESC_NUM = 3` + was already programmed by MAC init before this knob existed, so no + bring-up change was needed to enable it. + **This is the family where batching actually pays.** On the CV610 craft one + bulk-OUT submission costs ~248 µs of CPU against ~22 µs on x86, and ~87% of + that is the kernel USB submit/completion path — so packing 3:1 measured + **248 → 148 µs per frame, 43.0% → 26.7% of one core at ~1750 fps**, with + the frame rate unchanged. On x86 the same A/B moves 21.5 → 10.6 µs. Where + the host is small, this knob is worth roughly ten points of a core. - **Jaguar1 (8812A/8811A/8821A/8814A)**: vendor-parity — the first block carries the 8-byte PKT_OFFSET reserve (dropped at a boundary total), and the OQT guard caps descriptor STARTS per bulk window (8812A = 1, 8814A = 3, diff --git a/docs/rtl8733b.md b/docs/rtl8733b.md index 044117de..e89f4269 100644 --- a/docs/rtl8733b.md +++ b/docs/rtl8733b.md @@ -99,6 +99,30 @@ every frame. The witness decoded all eight `rate_hw` values on air with valid FCS, every frame at 20 MHz with STBC and LDPC clear, matching the forced-BCC contract. +USB **TX** aggregation is ported and is the one host-side lever that matters on +a small SoC. `send_packets` packs up to three `[txdesc][frame]` blocks into one +bulk-OUT URB (`DEVOURER_TX_USB_AGG`, default 0 = off = byte-identical), the +HalMAC 3-descriptor rule that MAC init had already programmed as +`BLK_DESC_NUM` in `DWBCN0_CTRL[7:4]`. The block count rides the first +descriptor's `dword7[31:24]` and is written *before* the checksum, which folds +over byte `0x1f`. + +The cost it removes is host CPU, not air time. One bulk submission measured +**~248 µs of CPU on the CV610 craft** against **~22 µs on x86** — the same +transfer, an 11× ratio — and a profile attributes ~87% of it to the kernel USB +submit/completion path, with the per-byte term below noise on ARM. Packing 3:1 +therefore removes two submissions in three: on the craft, **248 → 148 µs per +frame and 43.0% → 26.7% of one core at ~1750 fps**, frame rate unchanged +(1733 → 1800). On x86 the same A/B reads 21.5 → 10.6 µs. + +The failure mode this was checked against is the 8822BU's: wrong packing makes +the TXDMA re-air block 1 `agg_num` times, which frame counts cannot see. Each +frame was therefore stamped (`DEVOURER_TX_QOS_DATA`) and the stamps counted at +the witness (`DEVOURER_RX_PCTR`): **31552 receptions, 31551 distinct counters, +ratio 1.00** where re-airing would read 3.00. Note `GetTxStats().submitted` +counts URBs rather than frames, so an aggregated session reports about a third +— accounting, not throughput. + RX aggregation is capped at 12 KiB (`rtl8733b::kRxAggregateBytes8733b`) because the vendor default of 20 KiB exceeds one bulk-IN URB: a 60-second high-traffic receive run had xHCI split such an aggregate, leaving a descriptor tail and its diff --git a/src/rtl8733b/CLAUDE.md b/src/rtl8733b/CLAUDE.md index 37b1f931..68bf2302 100644 --- a/src/rtl8733b/CLAUDE.md +++ b/src/rtl8733b/CLAUDE.md @@ -108,6 +108,28 @@ unrelated register map. The pre-change binary measured **flat in the same session and geometry** (0.3 dB across the same 64 qdB) — the do-nothing control that makes the 14 dB readable. +- **USB TX aggregation is ported, and this is the family it matters most on.** + `send_packets` packs up to 3 `[txdesc][frame]` blocks into one bulk-OUT URB + (`cfg.tx.usb_agg_max` / `DEVOURER_TX_USB_AGG`; 0 = off = byte-identical). + Two facts made it cheap to port and one made it worth porting. MAC init + **already** programmed `BLK_DESC_NUM = 3` into `DWBCN0_CTRL[7:4]` (0x0208) + — the same field and value the 88xx siblings use — so nothing in bring-up + changed. `DMA_TXAGG_NUM` is at `dword7[31:24]` exactly as on the 8822C, but + it must be set **before** the checksum, not patched on after: the fold + covers 32 bytes skipping only `0x1c-0x1d`, so byte `0x1f` is inside it. + That placement was inferred from 8822C parity and then **confirmed on + silicon** — the 8822BU precedent is that wrong packing makes the TXDMA + re-air block 1 `agg_num` times, which is invisible to frame counts, so it + was verified by per-frame stamps (`DEVOURER_TX_QOS_DATA` + `DEVOURER_RX_PCTR`): + 31552 receptions carried 31551 distinct counters, ratio 1.00 where re-airing + would read 3.00. + Why it matters here: one bulk submission costs **~248 µs of CPU on the CV610 + craft** against ~22 µs on x86, ~87% of it kernel USB. Measured A/B on the + craft at ~1750 fps: **248 → 148 µs per frame, 43.0% → 26.7% of one core**, + frame rate unchanged. Note `GetTxStats().submitted` counts URBs, not frames, + so an aggregated session reads ~frames/3 — the same accounting the other + families have, and not a throughput drop; the per-URB `tx.agg` event carries + the true count. - **The thermal table is chosen once per channel set, not per frame.** The CCK and OFDM/HT variants of the thermal-compensation table are different tables. `configure_tx_power` picks one from the configured TX mode and leaves it, diff --git a/src/rtl8733b/Rtl8733bDevice.cpp b/src/rtl8733b/Rtl8733bDevice.cpp index 5b7c0a0f..85c605e7 100644 --- a/src/rtl8733b/Rtl8733bDevice.cpp +++ b/src/rtl8733b/Rtl8733bDevice.cpp @@ -8,12 +8,13 @@ #include #include "RateDefinitions.h" -#include "RadiotapPeek.h" +#include "RadiotapPeek.h" /* send_packets batch pre-parse */ #include "RadiotapTxFlags.h" #include "RxParseAbort.h" /* rx.parse_abort — abandoned-aggregate event */ #include "rtl8733b/Rtl8733bUsbIds.h" #include "rtl8733b/TxDescriptor8733b.h" #include "SignalStop.h" +#include "TxAggPlan.h" /* shared USB TX aggregation layout planner */ extern "C" { #include "ieee80211_radiotap.h" @@ -484,9 +485,123 @@ SelectedChannel Rtl8733bDevice::GetSelectedChannel() { return _channel; } +size_t Rtl8733bDevice::send_packets(const TxPacketView *pkts, size_t count) { + const unsigned agg = _cfg.tx.usb_agg_max; + if (agg <= 1 || !_device.is_usb() || count == 0) + return IRtlDevice::send_packets(pkts, count); + + std::lock_guard lock(_reg_mu); + if (!_phy_ready || !_mac_ready || !_tx_ready) { + _logger->error("RTL8733B TX rejected before InitWrite"); + return 0; + } + + devourer::TxAggLimits lim; + lim.desc_size = rtl8733b::kTxDescSize; + lim.bulk_size = _device.speed() >= devourer::kUsbSpeedSuper ? 1024 + : _device.speed() >= devourer::kUsbSpeedHigh ? 512 + : 64; + /* BLK_DESC_NUM: MAC init already programs 3 into DWBCN0_CTRL[7:4] + * (Halmac8733bMac.cpp) — the same field and value the HalMAC 88xx siblings + * use — so 3 descriptors per bulk transfer is what this TXDMA parses. + * Layout is rtw88/HalMAC parity: no first-block PKT_OFFSET reserve. */ + lim.max_frames = std::min(agg, 3u); + lim.descs_per_bulk = 0; + lim.first_reserve = false; + + size_t done = 0, ok = 0; + while (done < count) { + /* Collect the contiguous run for ONE URB. A frame carrying a radiotap + * CHANNEL other than the session channel ends the run: build_tx_block + * refuses such a frame outright on this backend (no per-submission + * retune), and letting it lead the next URB keeps that refusal per-frame + * instead of poisoning frames packed beside it. */ + std::vector lens; + for (size_t i = done; i < count && lens.size() < lim.max_frames; ++i) { + /* A null view is treated exactly like a malformed one — skipped, per + * the IRtlDevice::send_packets contract. Both MUST advance `done` when + * they lead the run: breaking without advancing leaves `lens` empty and + * spins this loop forever on the same entry. */ + const uint16_t rlen = + pkts[i].data == nullptr + ? uint16_t{0} + : devourer::radiotap_hdr_len(pkts[i].data, pkts[i].len); + if (rlen == 0) { + if (lens.empty()) + ++done; /* skip a malformed leading frame (contract: skipped) */ + break; + } + const int want = + devourer::radiotap_peek_channel(pkts[i].data, pkts[i].len); + if (want > 0 && want != _channel.Channel) + break; + lens.push_back(pkts[i].len - rlen); + } + if (lens.empty()) + continue; + + const devourer::TxAggPlan plan = + devourer::plan_tx_agg(lens.data(), lens.size(), lim); + if (plan.frames() <= 1) { + /* One block (or a frame the URB cap refuses): the classic single-frame + * path is byte-identical and uncapped. */ + if (send_packet(pkts[done].data, pkts[done].len)) + ++ok; + ++done; + continue; + } + + /* The block count rides the FIRST descriptor, and it must be in place + * before that descriptor is checksummed — hence built in, not patched on + * afterwards the way the 8822C does it (its checksum is recomputable in + * isolation; this one is folded inside fill_tx_desc_8733b). */ + std::vector urb(plan.total, 0); + size_t built = 0; + for (size_t k = 0; k < plan.frames(); ++k) { + const uint8_t poff = (k == 0 && plan.shim) ? 1 : 0; + const uint8_t anum = + k == 0 ? static_cast(plan.frames()) : uint8_t{0}; + if (build_tx_block(pkts[done + k].data, pkts[done + k].len, + urb.data() + plan.blocks[k].offset, poff, anum) == 0) + break; /* pre-validated, so only a defensive bail */ + ++built; + } + if (built != plan.frames()) { + for (size_t k = 0; k < plan.frames(); ++k, ++done) + if (send_packet(pkts[done].data, pkts[done].len)) + ++ok; + continue; + } + + const int rc = _device.bulk_send_sync_ep(_device.first_bulk_out_ep(), + urb.data(), urb.size(), + /*timeout_ms=*/100); + devourer::Ev(_logger->events(), "tx.agg") + .f("frames", (unsigned long long)plan.frames()) + .f("bytes", (unsigned long long)urb.size()) + .f("shim", plan.shim) + .f("ok", rc >= 0); + if (rc >= 0) { + ok += plan.frames(); + /* Same one-shot latch send_packet uses — a session whose very first TX + * is aggregated must still say so once, or the "first TX accepted" + * breadcrumb goes missing exactly when the packing is what is on + * trial. It counts URB acceptances, not frames; it is a latch, not a + * meter. */ + if (_tx_submits.fetch_add(1) == 0) + _logger->info( + "RTL8733B first TX accepted (aggregated): EP=0x{:02x} frames={} " + "bytes={} shim={}", + _device.first_bulk_out_ep(), plan.frames(), urb.size(), plan.shim); + } + done += plan.frames(); + } + return ok; +} + size_t Rtl8733bDevice::build_tx_block(const uint8_t *packet, size_t length, - uint8_t *out, - uint8_t packet_offset) { + uint8_t *out, uint8_t packet_offset, + uint8_t agg_num) { if (packet == nullptr || out == nullptr) return 0; const uint16_t radiotap_length = @@ -595,6 +710,7 @@ size_t Rtl8733bDevice::build_tx_block(const uint8_t *packet, size_t length, cfg.rate_hw, cfg.bandwidth, _channel.Channel > 14); cfg.data_sc = data_sc; cfg.packet_offset = packet_offset; + cfg.agg_num = agg_num; cfg.retry_limit = static_cast( std::clamp(_cfg.tx.retry_limit, 0, 63)); cfg.short_gi = false; diff --git a/src/rtl8733b/Rtl8733bDevice.h b/src/rtl8733b/Rtl8733bDevice.h index 072a2f6c..128da708 100644 --- a/src/rtl8733b/Rtl8733bDevice.h +++ b/src/rtl8733b/Rtl8733bDevice.h @@ -39,6 +39,19 @@ class Rtl8733bDevice : public IRtlDevice { * interface declaration. */ void FastRetune(uint8_t channel, bool cache_rf) override; bool send_packet(const uint8_t *packet, size_t length) override; + /* USB TX aggregation (cfg.tx.usb_agg_max / DEVOURER_TX_USB_AGG): pack + * consecutive frames into one bulk-OUT URB. Measured on the CV610 craft, + * this is the lever that matters on an embedded host — a single submission + * costs ~283 us of CPU there against ~22 us on x86, and ~87% of that is the + * kernel USB submit/completion path, so folding three frames into one URB + * removes two of every three. Knob off / non-USB falls back to the + * interface-default per-frame loop, byte-identical. + * + * Note when reading TX stats against this: GetTxStats().submitted counts + * bulk-OUT transfers, so an aggregated session reports roughly frames/3 — + * the same accounting Jaguar1/2/3 have, not a throughput drop. The + * per-URB `tx.agg` event carries the real frame count. */ + size_t send_packets(const TxPacketView *pkts, size_t count) override; void SetTxMode(const devourer::TxMode &mode) override; void ClearTxMode() override; SelectedChannel GetSelectedChannel() override; @@ -72,8 +85,12 @@ class Rtl8733bDevice : public IRtlDevice { private: void bring_up_to_phy(); bool configure_tx_power(SelectedChannel channel); + /* Fill one [txdesc][frame] block at `out`. `agg_num` is the USB TX + * aggregation block count and belongs on the FIRST descriptor of a packed + * URB only; 0 everywhere else, which is what keeps the single-frame path + * byte-identical. */ size_t build_tx_block(const uint8_t *packet, size_t length, uint8_t *out, - uint8_t packet_offset); + uint8_t packet_offset, uint8_t agg_num = 0); RtlAdapter _device; Logger_t _logger; diff --git a/src/rtl8733b/TxDescriptor8733b.h b/src/rtl8733b/TxDescriptor8733b.h index 0fd67551..a8d72af0 100644 --- a/src/rtl8733b/TxDescriptor8733b.h +++ b/src/rtl8733b/TxDescriptor8733b.h @@ -114,6 +114,14 @@ struct TxDescConfig { uint8_t bandwidth = 0; // 0=20 MHz, 1=40 MHz uint8_t data_sc = 0; uint8_t packet_offset = 0; // 8-byte USB boundary shim count + /* USB TX aggregation block count, set on the FIRST descriptor of a packed + * bulk-OUT URB and left 0 on every other descriptor and on every + * single-frame transfer (0 and 1 are both "one block" to the TXDMA, and 0 + * keeps the single-frame path byte-identical to before this field existed). + * HALMAC DMA_TXAGG_NUM, dword7[31:24] — the same placement the 8822C + * carries it at, alongside the checksum in the same dword's low half. + * Capped by BLK_DESC_NUM = 3, which MAC init already programs. */ + uint8_t agg_num = 0; uint8_t retry_limit = 0; bool short_gi = false; bool ldpc = false; @@ -127,7 +135,7 @@ inline bool valid_tx_desc_config(const TxDescConfig &cfg) { return cfg.packet_size != 0 && cfg.sequence <= 0x0fff && (legacy_cck || legacy_ofdm || ht_1ss) && cfg.rate_id <= 0x1f && cfg.bandwidth <= 1 && cfg.data_sc <= 0x0f && - cfg.packet_offset <= 1 && + cfg.packet_offset <= 1 && cfg.agg_num <= 3 && cfg.retry_limit <= 0x3f && (!legacy_cck || cfg.bandwidth == 0) && !cfg.ldpc && (!(legacy_cck || legacy_ofdm) || !cfg.short_gi); @@ -169,6 +177,11 @@ inline bool fill_tx_desc_8733b(uint8_t *desc, size_t desc_len, txdesc_set_bits(desc + 0x14, 7, 1, cfg.ldpc ? 1 : 0); txdesc_set_bits(desc + 0x20, 15, 1, 0); // preserve caller sequence txdesc_set_bits(desc + 0x24, 12, 12, cfg.sequence); + /* DMA_TXAGG_NUM before the checksum, not after: the fold covers 32 bytes + * skipping only 0x1c-0x1d (the checksum field itself), so byte 0x1f is + * INSIDE the checksummed span. Writing the count afterwards would leave a + * descriptor the chip rejects. */ + txdesc_set_bits(desc + 0x1c, 24, 8, cfg.agg_num); txdesc_set_bits(desc + 0x1c, 0, 16, txdesc_checksum_8733b(desc)); return true; } diff --git a/tests/rtl8733b_tx_desc_selftest.cpp b/tests/rtl8733b_tx_desc_selftest.cpp index de10b59f..3d41a16a 100644 --- a/tests/rtl8733b_tx_desc_selftest.cpp +++ b/tests/rtl8733b_tx_desc_selftest.cpp @@ -222,5 +222,50 @@ int main() { expect("HE TxMode refused on every band", !rtl8733b::tx_mode_supported_8733b(mode)); + /* USB TX aggregation: DMA_TXAGG_NUM on the first descriptor of a packed + * URB (dword7[31:24]), zero everywhere else. */ + { + rtl8733b::TxDescConfig agg; + agg.packet_size = 100; + agg.sequence = 0x123; + agg.rate_hw = 4; // 6M + agg.rate_id = 6; + + std::array plain{}; + expect("agg-num 0 descriptor builds", + rtl8733b::fill_tx_desc_8733b(plain.data(), plain.size(), agg)); + + /* The no-change control: agg_num defaults to 0, so a single-frame + * descriptor is byte-identical to one built before this field existed — + * which is what makes the knob-off path provably untouched. */ + expect("agg-num 0 leaves dword7[31:24] clear", plain[0x1f] == 0); + expect("agg-num 0 checksum still folds to all ones", + rtl8733b::txdesc_checksum_valid_8733b(plain.data(), plain.size())); + + std::array packed{}; + agg.agg_num = 3; + expect("agg-num 3 descriptor builds", + rtl8733b::fill_tx_desc_8733b(packed.data(), packed.size(), agg)); + expect("agg-num 3 lands in dword7[31:24]", packed[0x1f] == 3); + + /* THE ordering cell. Byte 0x1f is inside the checksummed span (the fold + * covers 32 bytes and skips only 0x1c-0x1d), so a build that wrote + * agg-num AFTER the checksum would leave a descriptor the chip rejects. + * Moving that write below the checksum line must fail exactly here. */ + expect("agg-num is inside the checksum, and precedes it", + rtl8733b::txdesc_checksum_valid_8733b(packed.data(), + packed.size())); + expect("agg-num actually changed the checksum", + !std::equal(plain.begin() + 0x1c, plain.begin() + 0x1e, + packed.begin() + 0x1c)); + + /* BLK_DESC_NUM is 3 on this part, so a fourth block is not encodable — + * refused at validation rather than truncated into the field. */ + agg.agg_num = 4; + std::array over{}; + expect("agg-num above BLK_DESC_NUM refused", + !rtl8733b::fill_tx_desc_8733b(over.data(), over.size(), agg)); + } + return failures == 0 ? 0 : 1; } diff --git a/tests/txagg_bench.sh b/tests/txagg_bench.sh index 5c073363..93b26d4d 100755 --- a/tests/txagg_bench.sh +++ b/tests/txagg_bench.sh @@ -8,6 +8,22 @@ # # sudo bash tests/txagg_bench.sh # TX_PID=0x8812 TX_VID=0x0bda BATCH=32 PAYLOAD=200 sudo bash tests/txagg_bench.sh +# +# Pass the env THROUGH sudo (`sudo -n env VAR=... bash tests/txagg_bench.sh`), +# not before it — sudo strips it otherwise and the run silently uses the +# defaults against an adapter that is not plugged in. +# +# RTL8733B (caps at 3 blocks; BATCH above that is clamped, not an error): +# sudo -n env TX_PID=0xf72b TX_VID=0x0bda RX_PID=0x8812 RX_VID=0x0bda \ +# CH=6 RATE=MCS7 BATCH=3 bash tests/txagg_bench.sh +# +# What this script CANNOT tell you: `rx_hits` counts receptions, not distinct +# frames, so equal counts are also what a chip re-airing block 1 agg-num times +# would produce (the 8822BU failure). To separate them, stamp every frame and +# count distinct stamps at the witness: +# TX adds DEVOURER_TX_QOS_DATA=1, RX adds DEVOURER_RX_PCTR=1, then +# `grep -o '"pctr":[0-9]*' rx.jsonl | sort -u | wc -l` against the rx.seq +# count. Ratio 1.0 = distinct delivery; ~3.0 = re-airing. set -u ROOT="$(cd "$(dirname "$0")/.." && pwd)" BUILD="$ROOT/build" From a8c1cbcb9968718129e74e900e194cf17c523f64 Mon Sep 17 00:00:00 2001 From: snokvist Date: Mon, 17 Aug 2026 19:13:03 +0200 Subject: [PATCH 2/3] rtl8733b: fix a send_packets hang, and verify the boundary shim Two findings from an adversarial pass over the previous commit. 1. An off-channel LEADING frame hung the TX thread. The run collector ended the run on a radiotap CHANNEL mismatch, so when the mismatch was on the first frame the run came back empty, `done` never advanced, and the while loop spun forever -- holding _reg_mu, which every other path on this device takes, so it deadlocks the adapter rather than just stalling TX. Reachable from any caller that does radiotap-driven per-packet hopping with the knob on. A leading off-channel frame now enters the run alone, so the single-frame path refuses it (build_tx_block already does) and `done` moves past it. The empty-run branch also advances unconditionally now, so termination is structural rather than a property of the current run rules -- the previous cut had the correct rule for null/malformed frames documented one branch above the case that got it wrong. 2. The boundary shim was never exercised. All prior verification used a fixed payload, so the packed total never landed on a bulk-MPS multiple and the 8-byte first-block PKT_OFFSET reserve never appeared: 0 shim=true across ~22k URBs. That is the exact path that made the 8822BU re-air block 1, so leaving it unmeasured was the weakest point in the evidence. For three blocks at USB HS the reserve engages when the MPDU length is 472 mod 512 -- confirmed exactly: urb_bytes 1544 / 3080 / 4616 at payloads 472 / 984 / 1496, each n*512 + 8. Stamped distinctness with the shim engaged: 15027 shim URBs, 44032 receptions, 44032 distinct, ratio 1.00, against 1.00 for the no-shim control. This part's block walker does account the reserve. Also checked and NOT a defect: this backend ignores radiotap DBM_TX_POWER, so Jaguar3's rule about breaking a run when the power bank changes has nothing to guard here. ctest 54/54. Co-Authored-By: Claude Opus 5 (1M context) --- docs/rtl8733b.md | 11 +++++++++++ src/rtl8733b/Rtl8733bDevice.cpp | 30 ++++++++++++++++++++---------- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/docs/rtl8733b.md b/docs/rtl8733b.md index e89f4269..dfcb01ce 100644 --- a/docs/rtl8733b.md +++ b/docs/rtl8733b.md @@ -123,6 +123,17 @@ ratio 1.00** where re-airing would read 3.00. Note `GetTxStats().submitted` counts URBs rather than frames, so an aggregated session reports about a third — accounting, not throughput. +The **boundary shim** is verified separately, because a fixed-payload sweep +never reaches it. When a packed total lands on an exact bulk-MPS multiple the +planner inserts the 8-byte first-block `PKT_OFFSET` reserve (the sync bulk path +has no ZLP), and on the 8822BU a reserved first block is precisely what made +the TXDMA re-air block 1. For three blocks at USB HS that happens when the MPDU +length is ≡ 472 mod 512 — measured exactly, `urb_bytes` 1544 / 3080 / 4616 at +payloads 472 / 984 / 1496, all `n x 512 + 8`. Stamped at the witness with the +shim engaged: **15027 shim URBs, 44032 receptions, 44032 distinct, ratio 1.00** +against 1.00 for the no-shim control. This part's block walker does account the +reserve. + RX aggregation is capped at 12 KiB (`rtl8733b::kRxAggregateBytes8733b`) because the vendor default of 20 KiB exceeds one bulk-IN URB: a 60-second high-traffic receive run had xHCI split such an aggregate, leaving a descriptor tail and its diff --git a/src/rtl8733b/Rtl8733bDevice.cpp b/src/rtl8733b/Rtl8733bDevice.cpp index 85c605e7..58464d31 100644 --- a/src/rtl8733b/Rtl8733bDevice.cpp +++ b/src/rtl8733b/Rtl8733bDevice.cpp @@ -518,27 +518,37 @@ size_t Rtl8733bDevice::send_packets(const TxPacketView *pkts, size_t count) { * instead of poisoning frames packed beside it. */ std::vector lens; for (size_t i = done; i < count && lens.size() < lim.max_frames; ++i) { - /* A null view is treated exactly like a malformed one — skipped, per - * the IRtlDevice::send_packets contract. Both MUST advance `done` when - * they lead the run: breaking without advancing leaves `lens` empty and - * spins this loop forever on the same entry. */ + /* A null view is treated exactly like a malformed one: it ends the run + * and, if it led, is skipped per the IRtlDevice::send_packets + * contract. */ const uint16_t rlen = pkts[i].data == nullptr ? uint16_t{0} : devourer::radiotap_hdr_len(pkts[i].data, pkts[i].len); - if (rlen == 0) { - if (lens.empty()) - ++done; /* skip a malformed leading frame (contract: skipped) */ + if (rlen == 0) break; - } const int want = devourer::radiotap_peek_channel(pkts[i].data, pkts[i].len); - if (want > 0 && want != _channel.Channel) + if (want > 0 && want != _channel.Channel) { + /* This backend retunes for nobody mid-submission — build_tx_block + * refuses an off-channel frame outright. A LEADING one still has to + * enter the run alone, so the single-frame path below refuses it and + * `done` moves past it; ending the run empty here instead would spin + * this loop forever on the same entry, holding _reg_mu. */ + if (lens.empty()) + lens.push_back(pkts[i].len - rlen); break; + } lens.push_back(pkts[i].len - rlen); } - if (lens.empty()) + if (lens.empty()) { + /* The leading view was null or malformed. This is also the loop's + * termination guarantee: `done` advances on every iteration whatever + * the collector above decided, so no future run rule can reintroduce a + * non-advancing path. */ + ++done; continue; + } const devourer::TxAggPlan plan = devourer::plan_tx_agg(lens.data(), lens.size(), lim); From 9581f6afb59ff56bf905a99a7d7d4b2c30e6d1ad Mon Sep 17 00:00:00 2001 From: snokvist Date: Mon, 17 Aug 2026 19:41:32 +0200 Subject: [PATCH 3/3] rtl8733b: reject short aggregated writes; de-duplicate the agg docs Qodo review on 400, plus a self-review pass for the same classes. 1. A short write counted as a full send. bulk_send_sync_ep returns BYTES SUBMITTED, so `rc >= 0` also matches a truncated transfer: the chip got a prefix, some trailing block is partial or absent, and there is no way to say which frames aired -- but all of them were reported submitted. send_packet has always refused a short write, so the aggregated path was the looser of the two in the same backend. It now requires rc == urb.size(), logs the short write, and reports none of the batch. tx.agg gains `sent` and `ok` now means a FULL write. Note the same pattern is live in jaguar2 and jaguar3 send_packets (both `rc >= 0` -> `ok += plan.frames()`). Not touched here -- that is two other backends and belongs in its own change -- but it is real, and jaguar1 is exempt only because its async path returns bool. 2. src/rtl8733b/CLAUDE.md restated the descriptor placement, the checksum-ordering rule and the URB accounting that the headers already doc-comment, against this repo's standing "never duplicate what a header already doc-comments". Trimmed to what only it can carry -- the bring-up archaeology, the measured cost, and the verification method -- pointing at the headers for the mechanism. The ordering rule now lives on TxDescConfig::agg_num where a reader looks for it, instead of only in the fill body. Self-review found three more instances of the class the previous round was about: a claim outliving what it described. - The header quoted ~283 us for a craft submission. That was the interpolated figure from the rate sweep; the direct A/B measured 248 and every other site says 248. - docs/logging.md still listed tx.agg as "frames, bytes, shim, ok" after this commit added `sent` and changed what `ok` means. - The run-collector comment still described the pre-a8c1cbc design, where an off-channel leading frame led the NEXT URB rather than entering the current run alone. The channel-mismatch infinite loop Qodo also reports was already fixed in a8c1cbc; their review targets 720a4dd. ctest 54/54. Co-Authored-By: Claude Opus 5 (1M context) --- docs/logging.md | 2 +- src/rtl8733b/CLAUDE.md | 45 +++++++++++++++++--------------- src/rtl8733b/Rtl8733bDevice.cpp | 25 +++++++++++++----- src/rtl8733b/Rtl8733bDevice.h | 5 ++-- src/rtl8733b/TxDescriptor8733b.h | 8 +++++- 5 files changed, 54 insertions(+), 31 deletions(-) diff --git a/docs/logging.md b/docs/logging.md index 302b39e0..13357149 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -109,7 +109,7 @@ Emitters: L = library, RX/TX/... = demo. Optional fields in [brackets]; |---|---|---| | `tx.frame` | TX | n, rc — precoder demo variant: n, ok | | `tx.stats` | TX | submitted, failed, was_timeout, last_rc | -| `tx.agg` | L (`DEVOURER_TX_USB_AGG`, send_packets) | frames, bytes, shim, ok — one per multi-frame bulk-OUT URB | +| `tx.agg` | L (`DEVOURER_TX_USB_AGG`, send_packets) | frames, bytes, shim, ok — one per multi-frame bulk-OUT URB. RTL8733B also emits `sent` (bytes actually transferred) and sets `ok` only on a FULL write, so `ok=false` with `0 <= sent < bytes` is a short write, not a transport error | | `tx.report` | L (`DEVOURER_TX_REPORT`, CCX decode) | t, state (0=delivered, 1=retry-drop), ok, retries, final_rate, queue_time_raw, bmc, macid, fmt ("8812"\|"halmac"); halmac adds tag (SW_DEFINE echo), rts_retries, missed (fw-stuffed constant on Jaguar3 — tag gaps are the drop signal; `tests/txrpt_coverage_attrib.py`) — t is the achieved-report-rate timebase (the CCX emission ceiling is reports/s) | | `tx.status` | RX, duplex (C2H TX_RPT decode) | hoff, queue, retry, airtime_us, rate | | `tx.receipt` | TX (its RX thread, `DEVOURER_TX_RECEIPTS`) | t, fresh, total, covered, receipts, tlv hex — one event per absorbed windowed RX receipt (src/cell/RxReceipt.h), WITH the raw TLV so `tests/receipt_verify.py` can replay the merge and compare frame-exactly against the receiver's rx.seq ledger; decimating would break that comparison | diff --git a/src/rtl8733b/CLAUDE.md b/src/rtl8733b/CLAUDE.md index 68bf2302..c22adb84 100644 --- a/src/rtl8733b/CLAUDE.md +++ b/src/rtl8733b/CLAUDE.md @@ -109,27 +109,30 @@ unrelated register map. (0.3 dB across the same 64 qdB) — the do-nothing control that makes the 14 dB readable. - **USB TX aggregation is ported, and this is the family it matters most on.** - `send_packets` packs up to 3 `[txdesc][frame]` blocks into one bulk-OUT URB - (`cfg.tx.usb_agg_max` / `DEVOURER_TX_USB_AGG`; 0 = off = byte-identical). - Two facts made it cheap to port and one made it worth porting. MAC init - **already** programmed `BLK_DESC_NUM = 3` into `DWBCN0_CTRL[7:4]` (0x0208) - — the same field and value the 88xx siblings use — so nothing in bring-up - changed. `DMA_TXAGG_NUM` is at `dword7[31:24]` exactly as on the 8822C, but - it must be set **before** the checksum, not patched on after: the fold - covers 32 bytes skipping only `0x1c-0x1d`, so byte `0x1f` is inside it. - That placement was inferred from 8822C parity and then **confirmed on - silicon** — the 8822BU precedent is that wrong packing makes the TXDMA - re-air block 1 `agg_num` times, which is invisible to frame counts, so it - was verified by per-frame stamps (`DEVOURER_TX_QOS_DATA` + `DEVOURER_RX_PCTR`): - 31552 receptions carried 31551 distinct counters, ratio 1.00 where re-airing - would read 3.00. - Why it matters here: one bulk submission costs **~248 µs of CPU on the CV610 - craft** against ~22 µs on x86, ~87% of it kernel USB. Measured A/B on the - craft at ~1750 fps: **248 → 148 µs per frame, 43.0% → 26.7% of one core**, - frame rate unchanged. Note `GetTxStats().submitted` counts URBs, not frames, - so an aggregated session reads ~frames/3 — the same accounting the other - families have, and not a throughput drop; the per-URB `tx.agg` event carries - the true count. + The mechanism (block count placement, the checksum-ordering constraint, the + URB-vs-frame accounting) is doc-commented at + `TxDescriptor8733b.h`'s `agg_num` and `Rtl8733bDevice::send_packets` — read + it there. What only lives here: + - **Bring-up needed no change.** MAC init *already* programmed + `BLK_DESC_NUM = 3` into `DWBCN0_CTRL[7:4]` (0x0208), the same field and + value the 88xx siblings use. The port was two things — a descriptor field + and the packer — because of that. + - **Why it is worth having here specifically.** One bulk submission costs + **~248 µs of CPU on the CV610 craft** against ~22 µs on x86, ~87% of it + the kernel USB path. Craft A/B at ~1750 fps: **248 → 148 µs per frame, + 43.0 → 26.7% of one core**, frame rate unchanged. The counterparts, in the + same breath: it buys **nothing on air** — same frames, same airtime, purely + host CPU; it buys nothing at all unless the caller uses `send_packets` + (waybeam-link does not); and the win shrinks with the host, being only + ~11 µs/frame on x86. + - **Frame counts cannot verify it.** The 8822BU precedent is that wrong + packing makes the TXDMA re-air block 1 `agg_num` times, and `rx_hits` is + identical either way — it was 23900 in both arms here. Only per-frame + stamps discriminate (`DEVOURER_TX_QOS_DATA` + `DEVOURER_RX_PCTR`, count + distinct `pctr`): **ratio 1.00 = distinct, ~3.00 = re-airing**. Verified + 1.00 with the boundary shim both off and on — and the shim needs a payload + chosen for it (MPDU length ≡ 472 mod 512 at USB HS), because a fixed-payload + sweep never reaches it. - **The thermal table is chosen once per channel set, not per frame.** The CCK and OFDM/HT variants of the thermal-compensation table are different tables. `configure_tx_power` picks one from the configured TX mode and leaves it, diff --git a/src/rtl8733b/Rtl8733bDevice.cpp b/src/rtl8733b/Rtl8733bDevice.cpp index 58464d31..569e1171 100644 --- a/src/rtl8733b/Rtl8733bDevice.cpp +++ b/src/rtl8733b/Rtl8733bDevice.cpp @@ -512,10 +512,10 @@ size_t Rtl8733bDevice::send_packets(const TxPacketView *pkts, size_t count) { size_t done = 0, ok = 0; while (done < count) { /* Collect the contiguous run for ONE URB. A frame carrying a radiotap - * CHANNEL other than the session channel ends the run: build_tx_block - * refuses such a frame outright on this backend (no per-submission - * retune), and letting it lead the next URB keeps that refusal per-frame - * instead of poisoning frames packed beside it. */ + * CHANNEL other than the session channel ends the run — this backend does + * not retune mid-submission, so build_tx_block refuses such a frame + * outright and it must not be packed beside frames that would have + * aired. */ std::vector lens; for (size_t i = done; i < count && lens.size() < lim.max_frames; ++i) { /* A null view is treated exactly like a malformed one: it ends the run @@ -586,12 +586,25 @@ size_t Rtl8733bDevice::send_packets(const TxPacketView *pkts, size_t count) { const int rc = _device.bulk_send_sync_ep(_device.first_bulk_out_ep(), urb.data(), urb.size(), /*timeout_ms=*/100); + /* bulk_send_sync_ep returns BYTES SUBMITTED, so `rc >= 0` also covers a + * short write. A truncated URB means the chip got a prefix — some + * trailing block is partial or absent — and there is no way to say which + * frames aired, so none of them may be reported as submitted. The + * single-frame path already refuses a short write; the aggregated one + * must not be the looser of the two in the same backend. */ + const bool sent_all = rc == static_cast(urb.size()); + if (rc >= 0 && !sent_all) + _logger->error("RTL8733B aggregated TX short on EP 0x{:02x}: {}/{} " + "({} frames dropped)", + _device.first_bulk_out_ep(), rc, urb.size(), + plan.frames()); devourer::Ev(_logger->events(), "tx.agg") .f("frames", (unsigned long long)plan.frames()) .f("bytes", (unsigned long long)urb.size()) + .f("sent", (long long)rc) .f("shim", plan.shim) - .f("ok", rc >= 0); - if (rc >= 0) { + .f("ok", sent_all); + if (sent_all) { ok += plan.frames(); /* Same one-shot latch send_packet uses — a session whose very first TX * is aggregated must still say so once, or the "first TX accepted" diff --git a/src/rtl8733b/Rtl8733bDevice.h b/src/rtl8733b/Rtl8733bDevice.h index 128da708..aae5a843 100644 --- a/src/rtl8733b/Rtl8733bDevice.h +++ b/src/rtl8733b/Rtl8733bDevice.h @@ -42,9 +42,10 @@ class Rtl8733bDevice : public IRtlDevice { /* USB TX aggregation (cfg.tx.usb_agg_max / DEVOURER_TX_USB_AGG): pack * consecutive frames into one bulk-OUT URB. Measured on the CV610 craft, * this is the lever that matters on an embedded host — a single submission - * costs ~283 us of CPU there against ~22 us on x86, and ~87% of that is the + * costs ~248 us of CPU there against ~22 us on x86, and ~87% of that is the * kernel USB submit/completion path, so folding three frames into one URB - * removes two of every three. Knob off / non-USB falls back to the + * removes two of every three (measured: 248 -> 148 us per frame on the + * craft, frame rate unchanged). Knob off / non-USB falls back to the * interface-default per-frame loop, byte-identical. * * Note when reading TX stats against this: GetTxStats().submitted counts diff --git a/src/rtl8733b/TxDescriptor8733b.h b/src/rtl8733b/TxDescriptor8733b.h index a8d72af0..c66cfa75 100644 --- a/src/rtl8733b/TxDescriptor8733b.h +++ b/src/rtl8733b/TxDescriptor8733b.h @@ -120,7 +120,13 @@ struct TxDescConfig { * keeps the single-frame path byte-identical to before this field existed). * HALMAC DMA_TXAGG_NUM, dword7[31:24] — the same placement the 8822C * carries it at, alongside the checksum in the same dword's low half. - * Capped by BLK_DESC_NUM = 3, which MAC init already programs. */ + * Capped by BLK_DESC_NUM = 3, which MAC init already programs. + * + * ORDERING: byte 0x1f is INSIDE the checksummed span (the fold covers 32 + * bytes and skips only 0x1c-0x1d, the checksum field itself), so this must + * be written BEFORE the checksum. fill_tx_desc_8733b does that by + * construction; the 8822C's patch-then-recompute shape does not port here, + * because this family folds the checksum inside the fill. */ uint8_t agg_num = 0; uint8_t retry_limit = 0; bool short_gi = false;