Compare commits
4 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 0b63ca3c24 | |||
| 4666e03254 | |||
| f232476240 | |||
| 08c7aafb48 |
@@ -0,0 +1,184 @@
|
|||||||
|
# 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.*
|
||||||
@@ -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.*
|
||||||
Reference in New Issue
Block a user