notes: Opus second-opinion BES2600 WiFi structural critique #8
Reference in New Issue
Block a user
Delete Branch "claude-noether-6"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Independent code-review writeup (Opus 4.7) against Sonnet 4.6 review (notes/architect-review-bug5-2026-05-07.md, merged via #7).
Concurs on items 1+2 (RX relay collapse + batch delivery) and 4+5 (ba_lock atomics, ps_state_lock skip). Pushes back on the 9-events-per-frame quantification and on framing BES_SDIO_OPTIMIZED_LEN as togglable.
New findings: driver is genealogically a CW12xx driver with chip-name search-and-replace done halfway (CW12XX_MAX_VIFS, cw12xx_hwpriv_to_vifpriv, is_hardware_cw1250 still in active code paths); ~700-line #if 0 fossil in bh.c preserving the cw1200 ancestor BH; Allwinner-specific sw_mci_check_r1_ready in the SDIO bus path; asm volatile("nop") placeholder where IRQ re-enable used to live; BUG_ON in steady-state hot path; vendor-SDK Makefile shape (per-customer board variants from one tree); 8 EXPORT_SYMBOLs from a nominally-single-binary module.
Recommends ordering: Patch C (items 1+2 wrapped, user directive) high-risk-first, Patches D+E as small individually-verifiable cleanups, explicit don't-touch list (no CW12XX rename, no workqueue topology rewrite, no Allwinner abstraction).
Patch C is gated on Task #19 (kerneldoc contract verification for ieee80211_rx_list from process context). See §9 of the writeup for three open questions for Markus before Patch C drafting starts.
Independent code-review writeup (Opus 4.7) against Sonnet's review of the same tree. Concurs with Sonnet on items 1+2 (RX relay, batch delivery) and items 4+5 (ba_lock atomics, ps_state_lock skip-when-pm_unsupported); pushes back on the "9 workqueue events per frame" quantification and records BES_SDIO_OPTIMIZED_LEN as hard-baked rather than togglable. New findings: cw12xx-not-bes2600 genealogy still active in source, ~700 lines of #if 0 fossil in bh.c, Allwinner-specific sw_mci_check_r1_ready in the SDIO bus path, asm volatile("nop") placeholder where IRQ re-enable used to live, BUG_ON in steady-state hot path, vendor-SDK Makefile shape that pollutes every diff, 8 EXPORT_SYMBOLs from a nominally-single-binary module. Recommends ordering: Patch C (1+2 wrapped) high-risk-first, Patches D+E as small individually-verifiable cleanups, explicit don't-touch list. Notes ieee80211_rx_list contract verification (task #19) blocks Patch C.I concur. Now looking at the writeup.
@@ -0,0 +181,4 @@## 9. Open questions for Markus1. **Patch C split-or-merge:** user directive is "wrap together". I'd note that a Phase-7 regression in the merged patch is harder to bisect than two sequential Phase-7 runs. Keeping the directive but recording the bisect-cost as known.Well, given your push back (which is fine), I now change my mind to do it sequentially. We're not on the clock.
@@ -0,0 +182,4 @@## 9. Open questions for Markus1. **Patch C split-or-merge:** user directive is "wrap together". I'd note that a Phase-7 regression in the merged patch is harder to bisect than two sequential Phase-7 runs. Keeping the directive but recording the bisect-cost as known.2. **`__bes2600_irq_enable(1)` commented out:** is IRQ re-enable intentionally always-on now, or is the `nop` a deletion-in-progress bug? Reading the c-stack history doesn't tell me. Worth a "what was this for" pass before any RX-architecture patch lands.Let's make this an experiment. The big locks and the irq disable tell a story about an engineer trying to make a deadline I think. Done right, this might go away by itself.
@@ -0,0 +183,4 @@1. **Patch C split-or-merge:** user directive is "wrap together". I'd note that a Phase-7 regression in the merged patch is harder to bisect than two sequential Phase-7 runs. Keeping the directive but recording the bisect-cost as known.2. **`__bes2600_irq_enable(1)` commented out:** is IRQ re-enable intentionally always-on now, or is the `nop` a deletion-in-progress bug? Reading the c-stack history doesn't tell me. Worth a "what was this for" pass before any RX-architecture patch lands.3. **`sw_mci_check_r1_ready` on RK3566:** should we test or just trust the path is rare-error? My read is: trust + `WARN_ON` if it's ever called, then react.Yes, let us measure this during testing, then decide.