Hi all
I have two patch series for BPI-R4:
The patches work great, but they were developed with Anthropic’s Fable AI. They are also a bit bigger than what I feel comfortable verifying and validating myself. What would be the best way to share these for verification?
You can send them against my repo. Sfp should only need one line in sfp.c
If they are working they can be sent upstream via ML (mt76 and netdev for sfp)
@frank-w I have submitted these against your 7.1-main. As you’ll see they are quite big, which is why I feel they need a more thorough review.
The SFP+ has been particularly hard to get working, but probably just because of my lack of expertise 
. Good news is they seem to be very stable, I’m using the BCM84891 as my main WAN link and have had no crashes in 4 days.
For all who are interested this is the mt7996 part
Sfp support is indeed more than exppected
I went through the mt7996 patch in this series line by line — here’s a verification and a couple of notes. (I’ll leave the BCM84891 SFP series to someone with that module in hand.)
What it does
Two one-line guards in mt7996_tx_prepare_skb(), both adding a sta NULL-check before the sta->mlo dereference:
- ... && sta->mlo && ...
+ ... && sta && sta->mlo && ...
The function is reachable with sta == NULL — group-addressed / multicast data scheduled from a vif txq in AP mode, and frames on the global wcid, carry no station pointer. The MLO link_id selection added later dereferenced sta->mlo unconditionally, so such a frame faulted:
Unable to handle kernel read from unreadable memory at
virtual address 000000000000001b
pc : mt7996_tx_prepare_skb+0x2a0/0x570 [mt7996e]
(0x1b is NULL plus the offset of the mlo field — a textbook NULL deref.)
Verdict: correct and complete.
I checked every sta / msta dereference in the whole function, not just the two touched lines:
msta is already derived safely — msta = sta ? (struct mt7996_sta *)sta->drv_priv : NULL; — and is only used inside the sta && sta->mlo block, so the guard covers it too.if (msta) / else if (mvif) — fine.sta == NULL is an expected input: the tail of the same function already had txp->fw.rept_wds_wcid = cpu_to_le16(sta ? wcid->idx : 0xfff);. So the function was always meant to handle a NULL sta — the MLO link_id logic simply broke that invariant, and this patch restores it. Right fix, right place.Minor, non-blocking: the software MLD-translation block dereferences vif (vif->link_conf[...], vif->addr) without an explicit vif check. In practice a non-NULL sta implies a non-NULL vif, and this predates the patch, so it isn’t a regression — just noting it for completeness.
Which trees this actually applies to
Worth flagging, since the impact is easy to mis-estimate:
link_id selection code at all → not affected; the patch is moot there.link_id block guarded (sta && sta->mlo); there, effectively only the second commit (EAPOL / MLD-translation block) is still missing.Concretely on the MTK-SDK snapshot: the fault actually reported in the commit message is on the link_id path — group-addressed / multicast data in AP mode — and on that snapshot the block is already guarded (sta && sta->mlo, plus the matching id == -EBUSY && sta check). So the observed crash is already covered there. The only block still unguarded is the second commit’s EAPOL / MLD-translation path, and reaching it requires an EAPOL frame with a NULL sta — which doesn’t arise in normal operation, since EAPOL is always unicast to an associated station. So on that snapshot the patch closes a real-but-not-observed gap rather than the crash people actually hit.
One thing worth aligning — the description.
The commit messages are accurate (they describe the NULL-sta deref precisely, with the trace). But the summary posted for the series calls it “the crash caused by the PCI reset on the mt7996, and possibly attempts a restart.” That doesn’t match the code: there’s no PCI reset and no restart here — it’s purely a NULL-pointer guard in the TX path. Worth syncing the wording so nobody reads it as a reset/recovery fix. In particular it’s unrelated to the MT7996 stuck firmware-semaphore issue — that one is a hardware lock inside the MCU that needs a GPIO reset line, a different failure at a different layer entirely.
Net: the mt7996 patch is a clean, correctly-scoped fix and is safe to take. Thanks @meehien for chasing it down, and @frank-w for posting it for review.
Thanks,for mt7996 reset issue there is also a pullrequest in my repo. Maybe you can review this too
Went through #193 as well — this one is a lot more interesting than the tx-path guard, because it fixes a real, fatal failure mode rather than a theoretical one. Here’s a review plus a few things worth confirming.
What it does (three layers):
mt7996_dev_gone() — the load-bearing part. Gates the recovery workers so they don’t touch MMIO on an endpoint the AER core has already taken offline. Without it, a read from the dead register window is an async external abort → SError → unconditional kernel panic on arm64 / mtk-pcie-gen3..error_detected / .slot_reset handlers on both PCI functions (primary + hif2) — proper AER participation instead of "AER: can't recover (no error_detected callback)".reset_gpio — a hard power-cycle of the card in slot_reset when PERST# alone can’t revive a brownout-wedged MCU. No-op when the DT has no reset-gpios.Applicability — this is a genuine gap, not a nice-to-have. Current mt76 (checked against the 2026.03.05 snapshot) has none of this: no pci_error_handlers, no MT76_REMOVED / pci_channel_offline guard in the reset path. So on any BPI-R4 with the WiFi card, a PCIe surprise-down (AER fatal, or an SFP hot-pull browning out the WiFi rail) currently panics the whole kernel. The dev_gone gate alone turns that into “WiFi down, system alive” — a real robustness win, and it needs no GPIO or DT change at all.
When does it actually fire? Rare but not exotic — it needs the endpoint to drop off the bus at runtime: an SFP hot-pull that browns out a shared WiFi rail, an unstable or under-volt supply, a card working loose (vibration, thermal cycling), a genuinely overheating card going unstable, or a fatal AER event. Stable bench conditions never trip it, which is why most of us have never seen it — but the failure it produces is a full kernel panic, not a WiFi hiccup, so the whole router goes down. That asymmetry (rare trigger, catastrophic result) is exactly what makes the gate worth taking.
On the BPI-R4 reset GPIO specifically — and this independently confirms a conclusion from the WiFi error-11 thread. There we argued that a wedged MT7996 MCU cannot be revived without a hardware reset line to the card’s power/reset rail, and that stock BPI-R4 boards simply don’t have one — only the manual switch. This series walks into exactly the same wall from a completely different failure path, and says so in its own words: the author’s note that “there is a GPIO line that’s not connected”, the reset_gpio being “absent on stock DT”, and the comment that “PERST# … may not clear an MCU wedged by a supply brownout; cycling the rail does.”
The practical consequence on a stock board: the reset_gpio path is a no-op, slot_reset will most likely hit the MT_HW_REV == 0xffffffff dead-read and return DISCONNECT rather than recovering, and full auto-recovery is simply not reachable without the board actually routing a reset line to the card. The patch handles that gracefully, which is the right call — worth stating plainly so nobody expects self-healing on a stock board. Two different bugs (a firmware-semaphore stuck at probe, and a brownout-wedged MCU after a PCIe error) hitting the same missing piece of hardware is about as clear as independent confirmation gets.
Concerns worth raising. I went through this by hand and also ran the series past an agentic reviewer; between the two passes, here are the things worth a look — all posed as questions, none a hard blocker:
mt7996_mac_work path. The panic backtrace in the first commit is mt7996_mac_work → mt7996_update_channel → mt7996_rr — i.e. the periodic survey read, not a reset worker. But the gate is added to mt7996_mac_reset_work and mt7996_reset, not to that survey path. Is mac_work guaranteed to be quiesced (or to bail on MT76_REMOVED) before it can do that MMIO read, or can it still fault in the window between the AER freeze and the reset worker running? The commit argues pci_channel_offline() trips “well before the reset worker,” but the trace that motivates the patch is the survey path, which doesn’t call dev_gone().mt7996_pci_mark_removed() sets the bit on &mdev->phy.state, while mt7996_dev_gone() tests &dev->mphy.state. Worth a quick confirm that those are the same state word — if not, the MT76_REMOVED half of the gate never trips after error_detected.slot_reset clears MT76_REMOVED, queues reset_work, and returns RECOVERED immediately; the actual reinit finishes later via ieee80211_restart_hw(). Since .resume can run before that work completes, is there anything that assumes the device is fully reinitialised by the time io_resume is called?hif->mt7996 after remove (use-after-free). mt7996_pci_remove() frees dev (via mt7996_unregister_device) but never clears the hif->mt7996 back-pointer set at probe. If the primary function is removed before a PCIe error reaches hif2, mt7996_hif_error_detected() would follow a dangling pointer into freed memory. (I checked the teardown — nothing nulls it.) A hif->mt7996 = NULL on remove, or a liveness check in the hif handler, would close it.pci_set_drvdata() is set early in probe; if probe fails after that point, the error path frees dev. Can slot_reset / error_detected run against that stale pointer in the window before probe completes? The existing reset paths gate on an init-done flag — the same guard here would be worth it.Overall: solid, well-reasoned, and it closes a real panic that current mt76 is exposed to — nice work. The three points above are worth a look, but none of them change that the dev_gone gate is worth taking on its own.