notes: Patch C v3 Phase 4 plan — drop sdio_rx_work, match cw1200 #11

2026-05-07T19:36:16Z

claude-noether commented

2026-05-07 19:36:16 +00:00

Supersedes PR #10's v2 plan. After cw1200 mainline mining confirmed upstream has no sdio_rx_work workqueue, the right move is the architectural pivot, not the atomic_t workaround.

TL;DR

v2 plan was: atomic_t prep + direct-deliver from sdio_rx_work
v3 plan is: delete sdio_rx_work entirely, IRQ → bh-direct (cw1200 shape)
Single-writer-from-bh invariant restored by construction → no atomic_t needed
Bigger structural diff but lands the right architecture in one move

Specifically request reviewer focus on §6 risk #6

__bes2600_irq_enable(1) is commented out in the bh-loop done: label and has an asm volatile("nop") placeholder in its place (bh.c:1518-1520). This is the symptom-shaped artifact noted in the Opus critique (PR #8). v3 needs to know: is IRQ re-enable load-bearing for the SDIO RX path? cw1200 explicitly re-enables; bes2600 commented it out. Audit blocks Phase 6.

What's NOT in v3

atomic_t prep refactor — not needed under v3's structural fix
ieee80211_rx_list batch delivery — that's still Patch C2, gated on Task #19 kerneldoc verification
Patch D (ba_lock) and E (ps_state_lock) — independent
bh.c #if 0 graveyard removal — still task #24 post-cleanup hygiene

State

Patch F merged via PR #4 → cleanups baseline
Patch B running on ohm; Patch F module staged on disk awaiting reboot (already triggered, ohm coming back)
v3 will branch off cleanups-with-F
Phase 7 telemetry over wired enu1 path

Supersedes PR #10's v2 plan. After cw1200 mainline mining confirmed upstream has no sdio_rx_work workqueue, the right move is the architectural pivot, not the atomic_t workaround. ## TL;DR - v2 plan was: atomic_t prep + direct-deliver from sdio_rx_work - v3 plan is: delete sdio_rx_work entirely, IRQ → bh-direct (cw1200 shape) - Single-writer-from-bh invariant restored by construction → no atomic_t needed - Bigger structural diff but lands the right architecture in one move ## Specifically request reviewer focus on §6 risk #6 `__bes2600_irq_enable(1)` is commented out in the bh-loop `done:` label and has an `asm volatile("nop")` placeholder in its place (bh.c:1518-1520). This is the symptom-shaped artifact noted in the Opus critique (PR #8). v3 needs to know: is IRQ re-enable load-bearing for the SDIO RX path? cw1200 explicitly re-enables; bes2600 commented it out. Audit blocks Phase 6. ## What's NOT in v3 - atomic_t prep refactor — not needed under v3's structural fix - `ieee80211_rx_list` batch delivery — that's still Patch C2, gated on Task #19 kerneldoc verification - Patch D (ba_lock) and E (ps_state_lock) — independent - bh.c #if 0 graveyard removal — still task #24 post-cleanup hygiene ## State - Patch F merged via PR #4 → cleanups baseline - Patch B running on ohm; Patch F module staged on disk awaiting reboot (already triggered, ohm coming back) - v3 will branch off cleanups-with-F - Phase 7 telemetry over wired enu1 path

claude-noether added 1 commit 2026-05-07 19:36:17 +00:00

notes: Patch C v3 Phase 4 plan — drop sdio_rx_work, match cw1200 d01400140b

Supersedes v2 (PR #10).  cw1200 mining (~/src/linux-rockchip, 228
cw1200 commits) confirmed: upstream cw1200 has no sdio_rx_work
workqueue at all.  IRQ handler bumps bh_rx + wakes bh_wq; bh thread
does the SDIO read inline via cw1200_bh_rx_helper.  Single thread =
single writer for hw_bufs_used = no race by construction.  Same int
hw_bufs_used as bes2600, never atomic_t'd in 16 years upstream.

v3 brings bes2600 into that shape:

  - delete sdio_rx_work, self->rx_work, self->rx_queue,
    self->rx_queue_lock, bes2600_sdio_pipe_read
  - GPIO IRQ handler calls self->irq_handler directly (matches
    cw1200_sdio_irq_handler shape)
  - bes2600_bh_rx_helper's BES_SDIO_RX_MULTIPLE_ENABLE branch
    replaced with inline SDIO read + extract_packets + per-skb
    delivery via new bes2600_bh_handle_rx_skb()
  - GPIO wake-flag bracketing moves into bh thread

§5 shared-state delta table (the v2 lesson, applied):  zero fields
require new locking.  hw_bufs_used / hw_bufs_used_vif / wsm_tx_pending
all stay single-writer-from-bh.  v2's atomic_t prep is mooted.

§6 risk #6 is the open question for reviewer:  bes2600's
__bes2600_irq_enable(1) call is commented out in the BH-loop done:
label with an asm volatile("nop") in its place.  Either SDIO IRQ
is auto-managed (so commenting out is fine) or the current code
relies on sdio_rx_work being queued regardless of driver-side IRQ
flag.  Block Phase 6 on this audit.

Patch F (PR #4 merged) is the new baseline.  v3 will branch off
F-merged cleanups.  Phase 7 N=3 stress ramp uses wired enu1 path
(192.168.88.80) for wedge-resilient telemetry collection.

claude-noether commented

2026-05-07 19:41:34 +00:00

§6 risk #6 audit — RESOLVED, not a blocker

Went and looked while you review. __bes2600_irq_enable() at hwio.c:327 is a stub:

int __bes2600_irq_enable(int enable)
{
    return 0;
}

No register read, no write, no flag manipulation. Always returns 0. Whatever this function did on cw1200 silicon (writes ST90TDS_CONFIG_REG_ID to set/clear ST90TDS_CONF_IRQ_RDY_ENABLE, see cw1200's __cw1200_irq_enable in hwio.c:267) it does NOT do on bes2600.

So:

The commented-out call at bh.c:1520 (//__bes2600_irq_enable(1)) and its asm volatile("nop") replacement: harmless. The Bestechnic engineer who commented it out was right; the function is dead code.
The remaining call at bh.c:1527 (__bes2600_irq_enable(0)) on the bh-thread exit path: a no-op call.
The fwio.c parallels in cw1200 (lines 491, 520) — bes2600 has its own bes_fw.c that doesn't need them; bes2600's IRQ enable isn't driver-managed at this level.
The main.c teardown call in cw1200 (line 601) — bes2600 doesn't have an analogous call, again because the function is a stub.

Implication for v3 plan: no need to add an IRQ re-enable in the new bh-side read path. bes2600's SDIO IRQ enable is managed by sdio_claim_irq and chip-side firmware. v3's IRQ flow is identical to today's flow minus the relay; the disabled-during-bh-operation concern doesn't exist on this silicon.

Phase 6 of v3 unblocked on this risk. The other risks in §6 still stand.

Readiness summary:

v2 plan PR #10: superseded by this PR
v3 plan §6 risk #6: resolved (this comment)
Other v3 risks: nominal
Patch F merged + smoke-tested live on ohm (srcversion A9438692…, no WARN, link clean)
v3 will branch off cleanups-with-F

Ready for Phase 6 v3 implementation as soon as Phase 5 review of this plan passes.

## §6 risk #6 audit — RESOLVED, not a blocker Went and looked while you review. `__bes2600_irq_enable()` at `hwio.c:327` is a **stub**: ```c int __bes2600_irq_enable(int enable) { return 0; } ``` No register read, no write, no flag manipulation. Always returns 0. Whatever this function did on cw1200 silicon (writes `ST90TDS_CONFIG_REG_ID` to set/clear `ST90TDS_CONF_IRQ_RDY_ENABLE`, see cw1200's `__cw1200_irq_enable` in hwio.c:267) it does NOT do on bes2600. So: - The commented-out call at `bh.c:1520` (`//__bes2600_irq_enable(1)`) and its `asm volatile("nop")` replacement: harmless. The Bestechnic engineer who commented it out was right; the function is dead code. - The remaining call at `bh.c:1527` (`__bes2600_irq_enable(0)`) on the bh-thread exit path: a no-op call. - The `fwio.c` parallels in cw1200 (lines 491, 520) — bes2600 has its own `bes_fw.c` that doesn't need them; bes2600's IRQ enable isn't driver-managed at this level. - The `main.c` teardown call in cw1200 (line 601) — bes2600 doesn't have an analogous call, again because the function is a stub. **Implication for v3 plan:** no need to add an IRQ re-enable in the new bh-side read path. bes2600's SDIO IRQ enable is managed by `sdio_claim_irq` and chip-side firmware. v3's IRQ flow is identical to today's flow minus the relay; the disabled-during-bh-operation concern doesn't exist on this silicon. **Phase 6 of v3 unblocked** on this risk. The other risks in §6 still stand. Readiness summary: - v2 plan PR #10: superseded by this PR - v3 plan §6 risk #6: resolved (this comment) - Other v3 risks: nominal - Patch F merged + smoke-tested live on ohm (srcversion `A9438692…`, no WARN, link clean) - v3 will branch off cleanups-with-F Ready for Phase 6 v3 implementation as soon as Phase 5 review of this plan passes.

marfrit merged commit 1e408c9d33 into main

2026-05-07 19:41:44 +00:00

marfrit referenced this issue from a commit

2026-05-07 19:41:46 +00:00

Merge pull request 'notes: Patch C v3 Phase 4 plan — drop sdio_rx_work, match cw1200' (#11) from claude-noether-9 into main

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marfrit/besser#11