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

@@ -0,0 +1,136 @@
+# Patch C v2 — Phase 4 Plan: atomic_t prep + direct-deliver
+
+**Author:** Claude (noether)
+**Status:** Phase 4 v2 — Phase 7 of Patch C (notes/patch-c-phase4-plan-2026-05-07.md, PR #9 merged) failed with a thread-safety race; this is the redesign.
+**Decision:** Option B from PR #3 close-out comment — `atomic_t` prep refactor first, direct-deliver on top.
+
+---
+
+## §0 What just happened (Phase 7 of Patch C)
+
+Reproduced verbatim from boot -1 of ohm 2026-05-07 20:18:10 CEST, ~13 s into a 4 MB/s nc stress:
+
+```
+WARNING: at wsm_release_tx_buffer+0x84/0xa0 [bes2600], CPU#0: kworker/0:3H/3912
+Workqueue: bes_sdio sdio_rx_work [bes2600]
+pc : wsm_release_tx_buffer+0x84/0xa0 [bes2600]
+lr : bes2600_bh_handle_rx_skb+0x134/0x370 [bes2600]
+sdio_rx_work+0x2a8/0x540 [bes2600]
+bes2600_wlan: wsm_release_tx_buffer failed: -1
+```
+
+Storm continued; chip wedged; ohm fell off the WiFi (wlan0).  Patch C module preserved at `/var/tmp/bes2600.patchC-broken.ko` for forensics.  Patch B rolled back, currently on disk on ohm.  Lesson saved as `feedback_phase6_contract_threadsafety` memory.
+
+## §1 Why it failed
+
+`wsm_release_tx_buffer()` (bh.c:222–243) does **unlocked** read–modify–write on `hw_priv->hw_bufs_used`.  Pre-Patch-C invariant was single-writer = BH thread; the lock that mattered was structural, not annotated.  Patch C's direct-deliver moved one writer (RX-confirm decrement) into `sdio_rx_work` workqueue context.  BH thread + sdio_rx_work race on the int counter; underflow below zero, WARN, return -1, bookkeeping corrupt, TX wedges.
+
+Phase 6 contract block correctly cited `wsm_handle_rx`'s sleepability and held-lock invariants — but stopped at the called function's signature.  It did not enumerate `hw_bufs_used` as shared state mutated by the callee.  That's the gap.
+
+## §2 Shared-state delta table (the thing missing from Patch C)
+
+Every field that `bes2600_bh_handle_rx_skb` mutates either directly or transitively, with current protection and required action:
+
+| field | declared at | written by (today) | written by (after Patch C v2) | current protection | action needed |
+|---|---|---|---|---|---|
+| `hw_priv->hw_bufs_used` | bes2600.h | `wsm_alloc_tx_buffer` (bh thread, TX submit), `wsm_release_tx_buffer` (bh thread, RX confirm), `main.c:543` (init) | + `wsm_release_tx_buffer` from sdio_rx_work | single-writer = BH thread (structural) | **convert to `atomic_t`** |
+| `hw_priv->hw_bufs_used_vif[i]` | bes2600.h | `wsm_release_vif_tx_buffer` (bh thread), `bh.c:1271` (vif TX submit), init | + `wsm_release_vif_tx_buffer` from sdio_rx_work | single-writer = BH thread | **convert to `atomic_t [N]`** |
+| `hw_priv->wsm_rx_seq[i]` | bes2600.h | bh thread RX | sdio_rx_work only | single-writer = BH/sdio_rx context (was BH, now is sdio_rx_work, but still **one writer**) | OK — single writer |
+| `hw_priv->wsm_tx_pending[i]` | bes2600.h | `bes2600_bh_inc_pending_count` (TX submit, BH thread), `bes2600_bh_dec_pending_count` (RX confirm) | dec moves to sdio_rx_work; inc stays BH | single-writer = BH | **also needs `atomic_t`** |
+| `hw_priv->lmac_mon_timer` / `mcu_mon_timer` | bes2600.h | mod_timer / del_timer_sync from BH | ditto from sdio_rx_work | timer API is internally locked | OK — `mod_timer` is concurrency-safe |
+| `hw_priv->wsm_cmd.lock` (taken inside wsm_handle_rx) | wsm_buf | bh thread (today) | sdio_rx_work | spinlock | OK — already protected |
+| `hw_priv->vif_lock` (taken inside wsm_handle_rx for some paths) | per vif | bh thread today | sdio_rx_work | spinlock | OK |
+| `priv->bh_evt_wq` wake-up | bes2600.h | wsm_release_tx_buffer when count hits 0 | ditto from sdio_rx_work | wake_up is concurrency-safe | OK |
+| `bes2600_pwr_clear_busy_event` (called inside release) | bes_pwr | bh thread | sdio_rx_work | internal locking via `bes_power.lock` | OK |
+| `hw_priv->buf_released` | bes2600.h | only `wsm_release_buffer_to_fw` (MCAST_FWDING ifdef, AP-only) | unchanged — BH only | single-writer = BH | OK — not on Patch C v2 hot path |
+
+**Three fields require atomic_t conversion:** `hw_bufs_used`, `hw_bufs_used_vif[]`, `wsm_tx_pending[]`.  Everything else is already concurrency-safe or moves cleanly to single-writer-in-sdio_rx_work.
+
+## §3 Read-site survey (the rest of the work — atomic_read swaps)
+
+`grep -hE "hw_bufs_used\b|hw_bufs_used_vif\b" *.c *.h | wc -l` = **57 references** across the source tree:
+
+- 5 writers (above)
+- 52 readers — converted mechanically to `atomic_read()`.  Distribution:
+  - `bh.c`: 22 read sites (most in the bh main loop, BUG_ON gates, idle / suspend predicates)
+  - `sta.c`: 3 sites (PM idle check at sta.c:1231–1253)
+  - `bes2600_sdio.c`: 1 site (PM idle check at line 958)
+  - `main.c`: 2 sites (init zero, teardown wait)
+  - `debug.c`: 1 site (debugfs stats)
+  - `itp.c`: 1 site (test mode)
+
+`wsm_tx_pending[i]` site count is smaller — ~6 references, all in bh.c and the timer monitors.  Same mechanical conversion.
+
+## §4 Plan v2 — two-step
+
+**Patch C-prep** (NFC, lands first):
+
+- Convert `hw_bufs_used` from `int` → `atomic_t`.
+- Convert `hw_bufs_used_vif[CW12XX_MAX_VIFS]` from `int[]` → `atomic_t[]`.
+- Convert `wsm_tx_pending[2]` from `int[]` → `atomic_t[]`.
+- Update writers:
+  - `wsm_alloc_tx_buffer`: `atomic_inc(&hw_priv->hw_bufs_used)`.
+  - `wsm_release_tx_buffer`: rewrite with `atomic_fetch_sub_release(count, &hw_priv->hw_bufs_used)` — returns prior value.  Re-derive the "tx restart" predicate (`prior >= numInpChBufs - 1`) and the "wake bh_evt_wq + clear busy" predicate (`prior - count == 0`) from that.  WARN if `prior - count < 0`.
+  - `wsm_release_vif_tx_buffer`: same pattern on the array element.
+  - `bes2600_bh_inc/dec_pending_count`: use `atomic_inc` and `atomic_dec_return` (need post-decrement value to decide whether to del_timer).
+- Update all 52+6 read sites: mechanical `atomic_read()` swap.
+- `main.c:543` init: `atomic_set(&hw_priv->hw_bufs_used_vif[i], 0)`.
+
+**Patch C-prep does NOT change behaviour.**  Same atomic ordering (`_release` / `_acquire` chosen to match the implicit memory ordering the BH-only path had).  Phase 7 of C-prep alone should show **identical** numbers to pre-patch baseline (`run-20260507-patchC-preflight`): 1.36 MB/s, 86.4 sdio_rx_work/sec, 90.3 dispatches per 1000 RX pkts, 0 bh_work redispatches.  If Phase 7 of C-prep shows a delta, the atomic ordering is wrong and we loop back here, not to C v2.
+
+**Patch C v2** (the actual structural change, lands on top of C-prep):
+
+- Identical to Patch C as merged in PR #3 (since closed): direct-deliver from `bes2600_sdio_extract_packets` into `bes2600_bh_handle_rx_skb`, no `rx_queue` indirection, no bh wake-up for RX.
+- The contract block in `bh.c::bes2600_bh_handle_rx_skb` is **expanded** to include the shared-state delta table from §2 of this plan, with explicit citations.
+- Same minimum-diff scope as Patch C: keep `rx_queue`, `pipe_read`, `bh_rx_helper` for clean bisection; remove in a follow-up hygiene patch.
+
+## §5 What will NOT be touched (deferred or out of scope)
+
+- mac80211-side `ieee80211_rx_irqsafe` → `ieee80211_rx_list` migration: that's Patch C2, gated on Task #19 kerneldoc verification.
+- The `#if 0` graveyard in bh.c, the `asm volatile("nop")` placeholder, the BUG_ON in steady-state hot path: still symptom-shaped per `feedback_dont_patch_downstream_artifacts`.  Re-evaluate at Task #24 after C v2 / D / E land.
+- `ba_lock` (Patch D) and `ps_state_lock` (Patch E): independent.
+
+## §6 Risk list (per Phase 6 contract-thread-safety memory)
+
+1. **C-prep memory ordering**: I've chosen `atomic_fetch_sub_release` for `wsm_release_tx_buffer` to mirror the implicit BH-thread ordering (release before subsequent atomic ops on `bh_evt_wq` / `bes_power`).  If the BH thread or other readers expect `_acquire` semantics on the value, we get reordering bugs that are hard to reproduce.  **Mitigation:** pair with `_acquire` reads where the read-then-decision pattern is critical (e.g., the bh main loop's `if (!hw_priv->hw_bufs_used)` idle predicate).  Cite the kerneldoc reference for `atomic_fetch_sub_release` in the commit message.
+
+2. **`wsm_tx_pending[]` decrement-side timer interaction**: `bes2600_bh_dec_pending_count` does `if (--hw_priv->wsm_tx_pending[idx] == 0) del_timer_sync(timer); else mod_timer(timer, ...)`.  After atomic_t conversion: `if (atomic_dec_return(&hw_priv->wsm_tx_pending[idx]) == 0) ...`.  But *another* thread could `atomic_inc` between our dec and the timer call, racing the del_timer.  `del_timer_sync` is internally safe (it can be called concurrently with `mod_timer`), but the **decision** "whether to delete vs mod" is racy.  **Mitigation:** even after atomic conversion, this function still needs to be called from a single context.  Verify `inc/dec_pending_count` callers — if both sides only fire from BH and sdio_rx_work and never overlap on the same idx, we're fine; if not, this needs a lock.
+
+3. **`hw_bufs_used_vif[]` array vs `wsm_alloc_tx_buffer`**: vif counter increment lives at bh.c:1271, called from bh thread TX-submit path.  Decrement (`wsm_release_vif_tx_buffer`) called from RX-confirm.  After Patch C v2 the decrement is in sdio_rx_work — same race shape as the global counter.  Already covered by the atomic_t array conversion.
+
+4. **PM idle predicate at sta.c:1239**: reads `hw_priv->hw_bufs_used_vif[priv->if_id]` to decide can-sleep.  Currently racy (was already reading BH-mutated state from a non-BH PM context).  Atomic conversion makes the read coherent.  PM context's read-then-decide is still fundamentally a snapshot — no change in semantics, just no torn-read.
+
+5. **Reboot / module-unload teardown** (`main.c:840`): `wait_event_timeout(... !hw_priv->hw_bufs_used ...)`.  Becomes `... !atomic_read(...)`.  No semantic change — the wait_event macro re-evaluates the predicate on each wake.
+
+6. **Phase 7 rig: Patch C v2 still wedges chip if I missed anything**: now mitigated by ohm's new wired interface (enu1, 192.168.88.80) — survives bes2600 wedges, lets us collect dmesg / ftrace / journalctl from a wedged ohm without reboot.  See `reference_ohm_wired_iface` memory.
+
+## §7 Phase 5 review handover
+
+PR on git.reauktion.de/marfrit/besser, this file as the artifact (per `feedback_phase5_surface_is_pr`).  Specifically request reviewer focus on §2 shared-state delta table — that's the part that should have caught Patch C's bug.  Don't curate.
+
+## §8 Phase 6 implementation order
+
+1. Branch off `cleanups` on bes2600-dkms-mobian: `bes2600/atomic-tx-buf-counters` (= Patch C-prep).
+2. Mechanical refactor: `int hw_bufs_used` → `atomic_t hw_bufs_used`, all reads → `atomic_read`, all writes → atomic ops.  Same for vif array and tx_pending array.  No other changes.
+3. Build, install, smoke-test.  Phase 7 of C-prep.  Should be a no-op delta.
+4. PR + Phase 5 review + merge.
+5. Branch off C-prep: `bes2600/sdio-rx-direct-deliver-v2` (= Patch C v2).
+6. Re-apply the Patch C delta (3 files: bh.h, bh.c, bes2600_sdio.c — same edits as PR #3).
+7. Build, install, Phase 7 N=3 stress ramp.
+8. PR + Phase 5 review + merge.
+
+## §9 Phase 7 v2 protocol (per `feedback_phase7_stress_ramp` + wired-rig)
+
+1. Pre-C-prep baseline rep N=3 (re-anchor, since current N=1 baseline is from `run-20260507-patchC-preflight`).
+2. Apply C-prep, N=3.  Compare to pre.  Expect: zero meaningful delta.  If non-zero → memory-ordering bug, loop back to §4 atomic-ordering choice.
+3. Apply C v2, N=3.  Compare to C-prep baseline.  Expect: §4.5 of original Patch C plan's predicted delta (rx_queue lock acquires → 0, observed RX KB/s lifts toward ≥1 MB/s sustained @ 4MB/s).
+4. **All Phase 7 stress runs use the wired path (`ssh mfritsche@192.168.88.80`) for telemetry collection.**  When the chip wedges (it shouldn't this time, but planning for it), wlan0 stops responding but enu1 stays alive.  Collect dmesg / ftrace / journalctl over enu1 BEFORE rebooting.  This is the data we lost in Patch C boot -1 because wlan0 was the only path.
+5. N=3 reps per phase per `feedback_phase7_stress_ramp`.  Don't accept N=1 as verification.
+
+## §10 Closeout
+
+If C-prep + C v2 both pass Phase 7: proceed to D (ba_lock atomicization), E (ps_state_lock skip).  Markus's "we're not on the clock" applies — sequencing per bisection clarity, not delivery deadline.
+
+---
+
+*Plan written 2026-05-07 by Claude (noether), in response to Patch C Phase 7 failure.  Phase 5 review = PR comments on this artifact at git.reauktion.de/marfrit/besser.  Don't curate the shared-state delta table for the reviewer — that's the part the previous round's reviewer should have caught me on.*