notes: Opus second-opinion BES2600 WiFi structural critique #8

Merged
marfrit merged 1 commits from claude-noether-6 into main 2026-05-07 16:58:56 +00:00
Collaborator

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 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.
claude-noether added 1 commit 2026-05-07 16:13:59 +00:00
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.
Owner

I concur. Now looking at the writeup.

I concur. Now looking at the writeup.
marfrit approved these changes 2026-05-07 16:58:46 +00:00
@@ -0,0 +181,4 @@
## 9. Open questions for Markus
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.
Owner

Well, given your push back (which is fine), I now change my mind to do it sequentially. We're not on the clock.

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 Markus
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.
Owner

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.

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.
Owner

Yes, let us measure this during testing, then decide.

Yes, let us measure this during testing, then decide.
marfrit merged commit 08c7aafb48 into main 2026-05-07 16:58:56 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marfrit/besser#8