besser/notes/patch-c-phase4-plan-2026-05-07.md

# Patch C — Phase 4 Plan: collapse sdio_rx_work into BH

**Author:** Claude (noether)
**Status:** Phase 4 — pending Phase 5 second-model review before any Phase 6 code.
**Scope:** **item 1 only** (per merged PR #8 inline review: "do it sequentially; we're not on the clock").
**Item 2** (batch deliver via `ieee80211_rx_list`) splits to **Patch C2**, gated on Task #19 kerneldoc verification.

---

## §0 Substrate — anchored

Bug #5 anchor (recorded 2026-05-07, see `notes/phase1-bug5-2026-05-07.md`):

- Sender: netcat-over-WiFi, 4 MB/s cap, 2.4 GHz, AVM AP, single-STA
- Receiver: ohm (PineTab2, RK3566 + BES2600WM-SDIO)
- N=3 baseline reps: 725 / 663 / 75 KB/s (rep 3 saw link-death at ~9 min)
- `perf record -g` during 4MB/s window: `_raw_spin_unlock_irqrestore` ≈ 20% CPU
- ftrace lock-instrumentation, system-wide: `workqueue_execute_start` ≈ 5,643/sec
- Driver-side count: `wsm_cmd_send` 13/sec — wsm command path is *not* the dispatch source; the contributor is the per-SDIO-transaction relay.

Root cause traced in PR #7 (Sonnet review) and concurred in PR #8 (Opus review): RX path adds two synchronization points per frame and one wait-queue wake-up per IRQ batch via `sdio_rx_work` → `rx_queue` → `bh_work` indirection.

## §1 Goal (locked)

Reduce per-RX-frame overhead enough that observed receive ≥ 1.0 MB/s sustained @ 4 MB/s sender, with `_raw_spin_unlock_irqrestore` < 15 % CPU during the 4 MB/s window. No 30-min cascade to link-death.

(This is a partial step toward Phase 1's full target of ≥ 2 MB/s sustained @ 4 MB/s with < 10 % lock CPU. The full target is jointly addressed by Patch C + Patch C2; Patch C alone should *cross half the gap*.)

## §2 Situation

- `bes2600.ko` srcversion `1B3B3ED0…` deployed on ohm (c-stack + Patch A + Patch B).
- `cleanups` branch on `marfrit/bes2600-dkms` is the current source-of-truth.
- Build sandbox `/var/tmp/c6-sandbox/` on ohm, native `make -j4`.
- `BES2600_RX_IN_BH` is **defined** in Makefile — `bes2600_bh_rx_helper` is the active RX consumer.
- ohm reachable. Markus pushes the reboot button; never me.
- Test rig under `/root/bes2600-samples/` — `rep-trace.sh` per-rep capture script.

## §3 Baseline measurements

Reused from Bug #5 Phase 0 (above). No re-anchor needed for Patch C — same regime.

**Specific Phase-3-units that this plan's predictions reference:**

| metric | tool | current value (4MB/s window) |
|---|---|---|
| observed receive throughput | netcat receiver byte-count | 75–725 KB/s, rep-variance high |
| `_raw_spin_unlock_irqrestore` CPU% | perf record / report | ~20% |
| `workqueue_execute_start`/sec | ftrace `workqueue:workqueue_execute_start` | ~5,643/sec system-wide |
| `bes_sdio` workqueue dispatches | `cat /sys/kernel/tracing/events/workqueue/.../filter` filtered by `bes_sdio` | not measured pre-patch — **TODO before Phase 6** |
| RX SKB rate at mac80211 boundary | trace `mac80211:drv_rx_irqsafe` count | not measured pre-patch — **TODO before Phase 6** |

Phase 6 must not start until the two TODOs above are filled in — otherwise Phase 7 has no reference point for the predicted-delta comparison.

## §4 Plan

### §4.1 What will be touched

- `bes2600_sdio.c::sdio_rx_work` — the relay loop. After this patch, it still drains the SDIO bus into SKBs but **delivers SKBs directly into `wsm_handle_rx`** instead of `skb_queue_tail`-ing them onto `self->rx_queue`.
- `bes2600_sdio.c::bes2600_sdio_extract_packets` — the inner per-SKB extractor. Changes the in-loop action from `skb_queue_tail(&self->rx_queue, skb)` to a direct call (or callback) into the wsm dispatcher.
- `bes2600_sdio.c::bes2600_sdio_pipe_read` — becomes unused, removed.
- `bh.c::bes2600_bh_rx_helper` — its `BES_SDIO_RX_MULTIPLE_ENABLE` branch is no longer reachable for RX (RX path no longer feeds bh). Either gate the helper, or remove the helper outright if `bh_rx` atomic is no longer raised on RX.

### §4.2 What will NOT be touched

- `ieee80211_rx_irqsafe()` call sites — that's Patch C2 (item 2).
- TX path — `sdio_tx_work`, `bes2600_bh_tx_helper`, etc. Untouched.
- `sdio_wq` workqueue alloc — stays. After patch it hosts only `tx_work` + `scan_work` + (briefly during patch) `rx_work`. Renaming is cosmetic and out of scope.
- The bh thread itself — still runs, still handles TX, still watches the timeouts.
- `bh.c` `#if 0` graveyard — separate hygiene patch, not bundled.
- `__bes2600_irq_enable(1)` commented-out / `asm volatile("nop")` placeholder — **deferred** per `feedback_dont_patch_downstream_artifacts`. These are symptom-shaped; Patch C may dissolve them. Re-evaluate at Task #24 (post-Patch-E observation).
- `bh_rx` / `bh_tx` atomic split — out of scope.

### §4.3 Approach choice — Option A (sdio_rx_work direct delivery)

Two structural options surveyed in PR #8 §2.1; recap:

| | Option A: direct delivery from sdio_rx_work | Option B: subsume sdio_rx_work into bh thread |
|---|---|---|
| diff size | small | medium |
| eliminates `rx_queue->lock` × 2 per frame | yes | yes |
| eliminates `sdio_wq.rx_work` workqueue dispatch per IRQ | no | yes |
| changes who calls `wsm_handle_rx` | sdio_wq context (already process context) | bh thread |
| TX/RX SDIO bus contention | unchanged (sdio_rx_work and sdio_tx_work already share `bes2600_sdio_lock`) | adds bh ↔ sdio_tx_work contention on the SDIO mutex |
| bisection isolation | clean: only the rx_queue handoff is removed | mixes "remove handoff" with "subsume thread" |

**Choosing Option A.** Reasons:
1. Smaller diff = clearer Phase-7 attribution. If RX KB/s rises, we know it was the rx_queue handoff, not the workqueue topology.
2. Per Markus's PR #8 review: split was for bisection clarity. Option A is narrower than Option B.
3. The remaining cost (per-IRQ `sdio_wq.rx_work` dispatch) is ≤ 1 dispatch per IRQ batch; multi-RX coalescing means several frames per dispatch. If Phase 7 of Patch C shows that dispatch IS the residual cost, that becomes a concrete data point and motivates a *measured* Option-B follow-up, not a speculative one.

### §4.4 Implementation sketch (preview — actual code in Phase 6)

**Today** (`bes2600_sdio.c:783–831`):
```c
static int bes2600_sdio_extract_packets(...) {
    for each packet:
        skb = dev_alloc_skb(...);
        memcpy(skb->data, &data[pos], packet_len);
        spin_lock(&self->rx_queue_lock);
        skb_queue_tail(&self->rx_queue, skb);    // ← handoff
        spin_unlock(&self->rx_queue_lock);
}
static void sdio_rx_work(...) {
    bes2600_sdio_extract_packets(...);
    self->irq_handler(self->irq_priv);           // ← wakes bh_wq
}
// bh thread later: pipe_read = skb_dequeue(rx_queue) → wsm_handle_rx(skb)
```

**After patch** (sketch):
```c
static int bes2600_sdio_extract_packets(struct sbus_priv *self, u32 ctrl_reg, u8 *data) {
    for each packet:
        skb = dev_alloc_skb(...);
        memcpy(skb->data, &data[pos], packet_len);
        ret = wsm_handle_rx(self->core, wsm_id_from(skb), wsm_hdr_of(skb), &skb);
        if (skb) dev_kfree_skb(skb);
        // no rx_queue, no spinlock, no wake-up
}
static void sdio_rx_work(...) {
    bes2600_sdio_extract_packets(...);
    // self->irq_handler(...) is no longer called for RX-only wakes
    // (it remains called for TX-confirm-completion paths, if any)
}
```

Caveats discovered during sketch:
- `wsm_handle_rx`'s signature wants `(hw_priv, id, wsm_hdr*, **skb)`. `extract_packets` doesn't currently parse the wsm header — we either parse it inline (cheap; the cost is one `__le16_to_cpu`) or defer parsing into a new `bes2600_sdio_deliver_rx(skb)` helper that wraps it.
- `hw_priv` is reachable as `self->core`.
- Need to verify `wsm_handle_rx` is callable from sdio_wq context. **Hypothesis:** yes, because today's bh thread is also process-context-via-workqueue and that's where wsm_handle_rx already runs. Phase 6 contract-cite from `wsm.h` / call-graph confirms.
- The `irq_handler(self->irq_priv)` wakeup at sdio_rx_work:902 — keep it, but confirm whether bh actually has remaining work after RX is gone. Possibilities: TX-confirm completions (`wsm_release_tx_buffer`) still need a bh wake. Verify in Phase 6.

### §4.5 Predicted delta (Phase 3 units)

Conservative because Patch C is item 1 only, not items 1+2.

| metric | predicted change | confidence |
|---|---|---|
| `rx_queue->lock` acquire/release rate | → 0 (lock is removed entirely; struct field deleted) | high |
| RX-path wait-queue wakes (`bh_wq` from sdio_rx_work for RX) | → 0 (TX-confirm wakes remain) | high |
| `_raw_spin_unlock_irqrestore` CPU% | 20 % → 12–15 % | **medium** — the rx_queue lock is one of several contributors; I don't have per-lock breakdown pre-patch |
| `workqueue_execute_start`/sec | marginal change (≤ 5 %) | high — sdio_wq dispatch still happens per IRQ |
| observed receive @ 4 MB/s | floor lifts from 75 KB/s → ≥ 1.0 MB/s; rep-variance shrinks | **medium** — rep 3's link death has multiple causes (decrypt-storm path is Patch A's territory; AP-side `aid 30` rejection is also possible) |
| Phase 7 N=3 outcome | all reps ≥ 1 MB/s sustained for 30 min @ 4 MB/s | **medium** |

**Honest acknowledgement:** the medium-confidence predictions are the ones where Phase 7 either confirms the model or surfaces a new bug. If `_raw_spin_unlock_irqrestore` only drops to 18 %, the next-largest contributor was something else — `pool->lock` (workqueue infrastructure) or `ba_lock` — and Patch D/E/C2 become the answer.

### §4.6 Risks

1. **`wsm_handle_rx` not callable from sdio_wq**: low probability (process context, same shape as today's bh), but a cite-failure here means revert to Option B. **Phase 6 must produce a `wsm.h` contract citation** before code lands.
2. **TX-confirm wake-ups stop firing**: if `wsm_handle_rx` was the only thing that ultimately bumped `bh_tx`, removing it from bh's input causes TX-confirm starvation. Mitigation: keep `irq_handler(irq_priv)` call in sdio_rx_work for now; let the bh's wait_event re-evaluate `bh_tx` on every wake. **Verify in Phase 6 that `wsm_release_tx_buffer` still wakes bh.**
3. **SKB allocation under memory pressure**: `dev_alloc_skb` in extract_packets currently `msleep(100)` retries up to 10×. Calling `wsm_handle_rx` directly from extract_packets keeps us in sdio_wq context during sleep; that's the same as today, so no new risk.
4. **rcu / locking invariants in `wsm_handle_rx`**: it traverses `priv->vif_list`, may grab `priv->vif_lock`. Currently called from bh thread. After patch: called from sdio_wq context. Both are process context, both can sleep. No new risk *unless* there's a held lock at sdio_wq level that wsm_handle_rx tries to re-acquire. **Phase 6 lock-graph audit required.**
5. **`bes2600_chrdev_is_bus_error()` early-return**: currently checked in `pipe_read`. After patch, must move into `extract_packets` or `sdio_rx_work` so RX during a bus-error window still gets dropped, not passed to mac80211.
6. **Multi-vif RX serialization**: the `rx_queue` is per-sbus_priv, not per-vif. After patch, multi-vif demux happens inside `wsm_handle_rx` (same as today). No new risk; same ceiling.

### §4.7 Phase 5 review handover

Goal/Situation/Measurements/Plan paste verbatim into DokuWiki when Markus initiates handover. **Do not curate** the plan for the reviewer — including the "medium-confidence" predictions and the §4.6 risk list verbatim. Reviewer should see the same uncertainty I have.

### §4.8 Phase 7 protocol (after Phase 6 lands)

Per `feedback_phase7_stress_ramp.md` — **stress ramp, not steady cap**:

1. Pre-patch baseline (re-anchor): 5 min @ 1 MB/s, 10 min @ 2 MB/s, 30 min @ 4 MB/s. Capture ftrace `workqueue/`, `lock/`, `mac80211/`, `mmc/`. perf record during the 4 MB/s window.
2. Apply Patch C, install, reboot (Markus pushes).
3. Post-patch: identical ramp, identical instrumentation.
4. Compute deltas in **the same units** as §3 baseline. Compare to §4.5 predictions. Any unexplained delta is a finding, not a footnote — log it and loop back to Phase 4 if the model is wrong.
5. **N=3 reps** post-patch. The user's stress-ramp memory and the receipts checklist both require this.
6. Capture `sdio_work_debug` output and `dmesg` if any storm fires (Patch A's counter should hold steady).
7. If Phase 7 numbers match prediction → Phase 8 memory update + proceed to Patch C2.
8. If they don't match → loop back to Phase 4. Don't paper-fix.

## §5 Out-of-scope items recorded for follow-on patches

- **Patch C2**: items 2 — `ieee80211_rx_list` batch delivery. Gated on Task #19 kerneldoc verification.
- **Patch D**: ba_lock atomicization at `txrx.c:998-1005, 1632`. Independent.
- **Patch E**: ps_state_lock skip when `pm_unsupported = true` at `txrx.c:1942-1948`. Independent, gated on c7 latch.
- **Task #24**: post-Patch-E observation of bh.c `asm volatile("nop")`, commented-out `__bes2600_irq_enable(1)`, BUG_ON in steady-state hot path. Symptom-shaped; observe before patching.
- **Task #25**: measure `sw_mci_check_r1_ready` on RK3566 during testing.

---

*Plan written 2026-05-07 by Claude (noether). Awaiting Phase 5 second-model review on DokuWiki, initiated by Markus.*