wifi: mt76: mt7925: re-arm monitor sniffer on chip_reset recovery - #69
wifi: mt76: mt7925: re-arm monitor sniffer on chip_reset recovery#69Ashcal9669 wants to merge 6 commits into
Conversation
245677e ("feat(firmware): add MT7925 firmware and link to MT7927") introduced a regression instead of the intended fix. It deleted the real firmware/mt7927/BT_RAM_CODE_MT6639_2_1_hdr.bin (688341 bytes) and replaced firmware/mt7925/BT_RAM_CODE_MT7925_1_1_hdr.bin (446831 bytes, identical to linux-firmware's copy) with a 119328-byte file missing the ALPS platform tag at offset 16 -- not the genuine MT7925 blob. It also added firmware/mt7927/BT_RAM_CODE_MT7925_1_1_hdr.bin as a symlink to the (now-corrupted) mt7925 file, which is not a path any known btusb request for this chip resolves to. Both binaries were already correct in the repo before 245677e; this drops that commit's firmware changes entirely rather than replacing them, restoring the pre-regression files. Verified byte-for-byte: both now carry the "ALPS" header tag at offset 16 that the broken file lacked, and match their pre-regression sizes exactly. Confirmed independently on hardware: an MT7927 user reported ~170/min firmware-load failures every boot on 245677e, resolved by installing this repo's real BT_RAM_CODE_MT6639_2_1_hdr.bin under its correct path; no MT7925 Bluetooth hardware was available to verify the mt7925 side on-device, so that half is confirmed by header inspection only. Separately, firmware/README.md documented the wrong install path for the MT7927 BT file (missing the mediatek/mt7927/ prefix the driver and Makefile's install_fw target actually use); corrected here. The Makefile compression-flag fix from 245677e (xz --check=crc32) is unrelated to the firmware regression and has been split out to its own PR so it isn't held up by this. Signed-off-by: Ashcal9669 <ashcalili@gmail.com>
mt7925_mac_reset_work()'s recovery path reconnects STATION and AP vifs via mt7925_vif_connect_iter(), but that function has no handling at all for NL80211_IFTYPE_MONITOR: it never calls mt7925_mcu_set_sniffer()/mt7925_mcu_config_sniffer() for a monitor vif. mac80211's own logical state (interface up, channel) survives the reset, but the firmware's sniffer state does not, and nothing in this recovery path puts it back -- a monitor vif goes silently deaf after a chip reset regardless of which band it was on. This is a distinct bug from the one PR morrownr#66 fixes: mac80211's ieee80211_reconfig() is not involved here at all. mt7925_mac_reset_work() never calls ieee80211_restart_hw(), so reconfig() is never reached; this is the driver's own firmware-recovery path silently dropping monitor state on its way back up, independent of anything mac80211 does. The first version of this patch reused mt7925_sniffer_interface_iter() directly, applying it to every active interface via a hw-level monitor flag rather than per-vif type. That is the same pattern mt7925_config()/mt7925_configure_filter() already use, but reusing it unscoped in the reset-recovery path means it would call mt7925_mcu_set_sniffer(dev, vif, true) against STATION/AP vifs too whenever a monitor vif happens to coexist, and none of the underlying MCU calls had their return values checked. Fixed properly instead: add a struct mt7925_sniffer_iter_data (dev, error, monitor_only) so the iterator can restrict itself to actual monitor vifs, check and propagate every MCU call's return value, and expose a dedicated mt7925_sniffer_rearm() entry point (monitor_only set) for the reset path to call under dev->mt76.mutex, with the failure logged and queues woken only after the re-arm attempt completes. mt7925_config()/mt7925_configure_filter() now use the same struct (monitor_only left unset, preserving their existing behavior) and also check/log the iterator's error instead of ignoring it. Signed-off-by: Ashcal9669 <ashcalili@gmail.com>
e80d921 to
1b4531f
Compare
|
Force-pushed a correction to this patch -- the original version had two real problems I should have caught before opening this:
Fixed: added a Verified: clean build, and added a structural contract ( Still not hardware-tested against a live chip_reset, same caveat as before. |
|
@Ashcal9669 Tested it. It does not re-arm on either chip here, for two different reasons. MT7927 on 7.1.5: mt7925_sniffer_rearm never runs. mt792x_core.c sets NO_VIRTUAL_MONITOR when is_mt7927(), and mac80211 only sets IEEE80211_CONF_MONITOR in the branch that flag skips, so the guard in mac.c is never true on this chip. ftrace shows mac_reset_work and nothing after it. MT7925 on 7.2-rc5: the re-arm does fire, but beacons still go to 1. The iterator keeps the is_mt7927 gate on config_sniffer, so the sniffer is enabled without ever being configured for the channel. change_chanctx calls config_sniffer unconditionally for a monitor vif, which is why that path works.
Keying off vif->type rather than the hw flag, and dropping the is_mt7927 gate on config_sniffer, makes it work on both here. |
Lucid-Duck tested the previous version on real hardware and found two distinct reasons it didn't actually work on either chip: 1. On MT7927, mt7925_sniffer_rearm() never ran at all. mt792x_core.c sets NO_VIRTUAL_MONITOR when is_mt7927(), and mac80211 only sets IEEE80211_CONF_MONITOR in the code path that flag causes mac80211 to skip. The mac.c reset-recovery guard checked exactly that flag, so it was never true on this chip -- ftrace showed mac_reset_work running and nothing after it. 2. On MT7925, the re-arm did fire, but mt7925_mcu_config_sniffer() was still gated behind is_mt7927(), so the sniffer got enabled without ever being told which channel to actually deliver. change_chanctx already calls config_sniffer unconditionally for a monitor vif, which is why that path has always worked; the reset path's reuse of mt7925_sniffer_interface_iter() inherited a gate that made sense for its original mt7925_configure_filter() caller (MT7927-only) but not for a general recovery path covering both chips. Fix both by deriving monitor state from the vif's own type (vif->type == NL80211_IFTYPE_MONITOR) instead of the hw-level IEEE80211_CONF_MONITOR flag, and dropping the is_mt7927() gate on config_sniffer so it runs for any monitor vif on either chip. Verified against Lucid's exact failure signatures with a new structural contract (tests/mt7927/pr69-reset-rearm-contract.sh, not part of this PR) plus the existing reset-sniffer-recovery-contract.sh. Not yet re-tested on live hardware against a real chip_reset -- still flagging that plainly. Signed-off-by: Ashcal9669 <ashcalili@gmail.com>
|
@Lucid-Duck Thanks for the real hardware A/B, that pinpointed two distinct bugs I missed:
Force-pushed: monitor state now derives from Not able to re-verify against a live chip_reset myself right now -- would appreciate the same stock-vs-patched check you ran before if you have hardware time for it. |
Testing on real MT7927/MT7925 hardware (not just the structural contract) showed the previous commit still didn't work on either chip: chip_reset followed by beacon counting still dropped to ~1 on both, unchanged from before that commit. The reason: the caller in mt7925_mac_reset_work() still gated the call to mt7925_sniffer_rearm() behind "hw->conf.flags & IEEE80211_CONF_MONITOR" -- the exact hw-level flag the previous commit's message already established is never set for MT7927 (mac80211 only sets it on the virtual-monitor path, which MT7927 skips via NO_VIRTUAL_MONITOR). Fixing the iterator's internal logic didn't help because the function was never being called in the first place on that chip. Call mt7925_sniffer_rearm() unconditionally instead. It already scopes itself to actual monitor vifs internally (via monitor_only), so calling it when none are active is a cheap iterate-and-return. Signed-off-by: Ashcal9669 <ashcalili@gmail.com>
|
Force-pushed another fix on top -- real hardware testing (chip_reset + beacon counting on both chips, not just the structural contract) showed the previous commit still didn't work. The reason: the reset-work caller in |
|
@Ashcal9669 Worth holding this one. The re-arm call in mac.c takes dev->mutex across ieee80211_iterate_active_interfaces(), which takes local->iflist_mtx. mac80211 already goes the other way, holding iflist_mtx and calling driver ops that take dev->mutex, via ieee80211_del_virtual_monitor -> drv_unassign_vif_chanctx -> mt792x_unassign_vif_chanctx. That is an ABBA inversion. I built a 7.2-rc5 kernel with PROVE_LOCKING and confirmed it on MT7922: Taking the lock inside the iterator callback instead, so the order is only ever iflist_mtx then dev->mutex, is the right direction. Fair warning it does not end there. The driver has roughly a dozen sites taking dev->mutex across ieee80211_iterate_*, including mt7921_config and mt7925_config, and I get the same splat on completely stock code with no patch applied. So this is pre-existing and your patch inherits it rather than causes it. I am writing that up for linux-wireless separately. |
2 participants
Follow-up to #66, per Devin Wittmayer's finding there:
chip_resetdrops monitor beacon reception to ~1/6s on both stock and patched builds becausemt7925_mac_reset_work()never reachesieee80211_restart_hw()/ieee80211_reconfig()at all — this is a separate bug in the driver's own reset-recovery path, not the mac80211-level gap #66 fixes.mt7925_mac_reset_work()'s recovery reconnects STATION/AP vifs viamt7925_vif_connect_iter(), which has no monitor-vif handling whatsoever — it never callsmt7925_mcu_set_sniffer()/mt7925_mcu_config_sniffer()for one. mac80211's logical state (interface up, correct channel) survives the reset; firmware's sniffer state does not, and nothing puts it back.Fix reuses the existing
mt7925_sniffer_interface_iter()helper — the same mechanismmt7925_config()(onIEEE80211_CONF_CHANGE_MONITOR) andmt7925_configure_filter()already use — rather than adding new logic. Exposed the previously-static helper viamt7925.hformac.cto call.Not yet hardware-tested against a live
chip_reset— flagging that plainly rather than claiming validation I don't have. Would appreciate an A/B check similar to what was done for #66 if anyone has hardware handy.