wifi: mt76: mt7925: fix rmmod hang and token idr race in unregister

mt7925e_unregister_device() had two independent bugs in its teardown
ordering, both in the RX NAPI/token-idr handling around
mt7925_tx_token_put(), plus a separate tasklet lifetime gap in
mt7925_pci_remove().

1. It called napi_disable() directly on every RX queue, then a few
   lines later mt792x_dma_cleanup() -> mt76_dma_cleanup() called
   napi_disable() on the same NAPI instances again, with no
   napi_enable() in between. napi_disable() leaves NAPI_STATE_SCHED
   set until a matching napi_enable(); a second, unpaired call on an
   already-disabled NAPI has nothing left to wait for that can ever
   clear that bit, so it hangs unconditionally.

   This is the same bug independently found and reported upstream by
   Mikhail Gavrilov ("wifi: mt76: mt792x: drop redundant napi_disable()
   in unregister path", 13b7e6a96a00) and by Devin Wittmayer
   (#70 review). Root cause: 4ab8f2122dcb added this same
   napi_disable() to mt76_dma_cleanup(), was reverted by f5f14a017454
   ("Needs to be fixed to avoid regression on mt762x"), then re-added
   by 332bbe9b2784 without removing the driver-side napi_disable()
   loops in mt7921/pci.c and mt7925/pci.c that predate it.

2. Once the redundant loop above was deleted without care, NAPI was no
   longer quiesced before mt7925_tx_token_put()'s idr_destroy(). A
   still in-flight RX NAPI poll can reach PKT_TYPE_TXRX_NOTIFY ->
   mt7925_mac_tx_free() -> mt76_token_release() -> idr_remove() on the
   same idr concurrently with that idr_destroy() -- a real
   use-after-free, not a redundant check. Confirmed independently
   twice: Sashiko (via Eric Biggers) against Mikhail's patch upstream,
   and Sashiko/Lucid-Duck against an earlier version of this fix
   (#70). tasklet_disable() alone is not sufficient: it
   only blocks *future* napi_schedule() calls, it does not wait out a
   poll already running. napi_disable() does wait for it, closing the
   race.

Fixed here by restoring a single napi_disable() loop in
mt7925e_unregister_device(), positioned before mt7925_tx_token_put()
as in bug 2, kept disabled afterward rather than re-enabled: upstream
PR #72 (following Mikhail's own proposed v2, reverting
332bbe9b2784) removed mt76_dma_cleanup()'s own napi_disable() entirely,
so this driver-side call is now the only one and there is nothing left
to double-disable. Re-enabling here would leave RX NAPI on going into
mt792x_dma_cleanup()'s netif_napi_del()/page_pool_destroy(), which is
exactly what those warn about if NAPI is still active. MT76_REMOVED
early-exits were added to mt792x_irq_tasklet(), mt792x_poll_tx() and
mt792x_poll_rx() (mt792x_dma.c) matching the pattern already used by
the non-PCI bus types in this driver family, so nothing can turn NAPI
back on in this window regardless.

Also: mt792x_dma_cleanup()/mt792x_wfsys_reset() do raw WFDMA/WFSYS
register I/O and need driver ownership of the chip.
mt76_unregister_device() above runs mac80211 vif teardown, which can
queue pm->ps_work and hand ownership back to firmware before
cancel_delayed_work_sync(&pm->ps_work) catches it. A new
__mt792x_mcu_drv_pmctrl(dev) call was added right before
mt76_unregister_device(), since that teardown itself needs ownership
too; the existing call right before mt792x_dma_cleanup() is unchanged
from before this fix and guarantees ownership is fresh again at the
point it's actually needed, independent of what ps_work did in
between.

Separately: mt7925_pci_remove() never called tasklet_kill() on
irq_tasklet before mt76_free_device(); the only existing
tasklet_kill() in this file is in the suspend path. tasklet_disable()
alone does not remove an already-scheduled instance from the pending
list, so a tasklet still pending when the containing struct is freed
is a use-after-free waiting to happen. Independently flagged upstream
by Mikhail Gavrilov in the same thread as bug 1. Added
tasklet_kill(&dev->mt76.irq_tasklet) in mt7925_pci_remove(), after
devm_free_irq() and before mt76_free_device().

That fix's first version also added a tasklet_disable() in
mt7925e_unregister_device(), reasoning it would help close bug 2 the
same way napi_disable() does. It doesn't, and it actively conflicts
with tasklet_kill(): tasklet_kill() waits for TASKLET_STATE_SCHED to
clear, but a *disabled* tasklet that gets scheduled never clears that
bit (the softirq handler sees it's disabled and just re-queues it), so
tasklet_kill() on a disabled-and-pending tasklet spins instead of
returning (#71 review, Lucid-Duck). mt792x_irq_handler()
already checks MT76_REMOVED before calling tasklet_schedule() at all,
so no new scheduling can happen once it's set (before this function
runs); mt792x_irq_tasklet()'s own MT76_REMOVED guard makes the tasklet
body itself inert if it does run once more from an IRQ that raced the
flag. Between those two, tasklet_disable() here was not preventing
anything tasklet_kill() doesn't already handle correctly on its own,
so it was dropped.

Fixes: 332bbe9b2784 ("wifi: mt76: fix connac2/3 DMA queue cleanup")
Link: #70
Link: #71
Link: #72
Link: https://lore.kernel.org/all/CABXGCsO07SExb+Z0PeN6MZ1fKC24Tvn3ehSyeQc-3qFC7jM7dQ@mail.gmail.com/

Verified: all tests/mt7927/*.sh source contracts covering this
function pass, clean build against 7.2.0-rc1-mt7927-monitor-v3, 4
consecutive real modprobe/modprobe -r cycles on physical MT7927
hardware idle, and 5 more with active RX traffic in flight during each
unload, zero hangs and zero dmesg warnings across all of them.

Not in this change: mt7921e_unregister_device() (mt7921/pci.c) has the
same missing tasklet_kill() gap in mt7921_pci_remove(), and
mt7921_pci_remove() sets MT76_REMOVED *after* calling unregister
(mt7925_pci_remove() sets it before), so the MT76_REMOVED guards added
here don't protect mt7921e either. Left out of scope for this PR;
flagging so it isn't mistaken for an oversight.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: Ashcal9669

mt7925/pci.c

@@ -40,29 +40,64 @@ static int mt7925e_init_reset(struct mt792x_dev *dev)
 
 static void mt7925e_unregister_device(struct mt792x_dev *dev)
 {
-	int i;
 	struct mt76_connac_pm *pm = &dev->pm;
 	struct ieee80211_hw *hw = mt76_hw(dev);
+	int i;
 
 	if (dev->phy.chip_cap & MT792x_CHIP_CAP_WF_RF_PIN_CTRL_EVT_EN)
 		wiphy_rfkill_stop_polling(hw->wiphy);
 
 	cancel_work_sync(&dev->reset_work);
 	cancel_work_sync(&dev->init_work);
+	__mt792x_mcu_drv_pmctrl(dev);
 	mt76_unregister_device(&dev->mt76);
-	mt76_for_each_q_rx(&dev->mt76, i)
-		napi_disable(&dev->mt76.napi[i]);
 	cancel_delayed_work_sync(&pm->ps_work);
 	cancel_delayed_work_sync(&dev->mlo_pm_work);
 	cancel_work_sync(&pm->wake_work);
 
+	/* Quiesce RX NAPI before tx_token_put()'s idr_destroy(): a still
+	 * in-flight poll can reach PKT_TYPE_TXRX_NOTIFY -> mt76_token_release()
+	 * -> idr_remove() on the same idr, racing idr_destroy() below
+	 * (morrownr/mt76 PR #70 review, Sashiko/Lucid-Duck).
+	 *
+	 * No tasklet_disable() here: mt792x_irq_handler() already checks
+	 * MT76_REMOVED (already set by the caller) before ever calling
+	 * tasklet_schedule(), so no *new* scheduling can happen regardless.
+	 * Disabling here too would leave irq_tasklet disabled with nothing to
+	 * re-enable it before mt7925_pci_remove()'s tasklet_kill() -- a
+	 * disabled-but-still-scheduled tasklet never clears
+	 * TASKLET_STATE_SCHED, so that kill would spin forever instead of
+	 * returning (morrownr/mt76 PR #71 review, Lucid-Duck). Left enabled,
+	 * mt792x_irq_tasklet()'s own MT76_REMOVED guard makes it a safe no-op
+	 * if it does still run once from an IRQ that raced the flag, and
+	 * tasklet_kill() can then wait it out normally.
+	 *
+	 * Left disabled from here on, deliberately not re-enabled: PR #72
+	 * reverted mt76_dma_cleanup()'s own napi_disable(), so this is now
+	 * the only disable in the whole teardown and mt792x_dma_cleanup()'s
+	 * netif_napi_del()/page_pool_destroy() below expect NAPI to already
+	 * be off. Re-enabling here would leave it on for that, which is what
+	 * those warn on.
+	 */
+	mt76_for_each_q_rx(&dev->mt76, i)
+		napi_disable(&dev->mt76.napi[i]);
+
 	mt7925_tx_token_put(dev);
+
+	/* Re-assert driver ownership: mt76_unregister_device() above can run
+	 * vif teardown that queues pm->ps_work, which can hand ownership back
+	 * to firmware before cancel_delayed_work_sync(&pm->ps_work) catches
+	 * it. The __mt792x_mcu_drv_pmctrl() call before mt76_unregister_device()
+	 * is new, added because that teardown itself needs driver ownership
+	 * too; this one, right before mt792x_dma_cleanup()/mt792x_wfsys_reset()
+	 * below (which do raw WFDMA/WFSYS register I/O), is unchanged from
+	 * before this fix and guarantees ownership is fresh at the point it's
+	 * actually needed, independent of what ps_work did during teardown.
+	 */
 	__mt792x_mcu_drv_pmctrl(dev);
 	mt792x_dma_cleanup(dev);
 	mt792x_wfsys_reset(dev);
 	skb_queue_purge(&dev->mt76.mcu.res_q);
-
-	tasklet_disable(&dev->mt76.irq_tasklet);
 }
 
 static void mt7925_reg_remap_restore(struct mt792x_dev *dev)
@@ -725,6 +760,17 @@ static void mt7925_pci_remove(struct pci_dev *pdev)
 	set_bit(MT76_REMOVED, &mdev->phy.state);
 	mt7925e_unregister_device(dev);
 	devm_free_irq(&pdev->dev, pdev->irq, dev);
+	/* irq_tasklet is never explicitly disabled (see the comment in
+	 * mt7925e_unregister_device()): a disabled-but-still-scheduled
+	 * tasklet never clears TASKLET_STATE_SCHED, which would make this
+	 * call spin instead of returning. It relies solely on
+	 * mt792x_irq_handler()'s and mt792x_irq_tasklet()'s own MT76_REMOVED
+	 * checks to stay inert, and tasklet_kill() here to make sure nothing
+	 * is left pending against dev before mt76_free_device() frees the
+	 * memory it points into (reported upstream:
+	 * lore.kernel.org/all/CABXGCsO07SExb+Z0PeN6MZ1fKC24Tvn3ehSyeQc-3qFC7jM7dQ@mail.gmail.com/).
+	 */
+	tasklet_kill(&dev->mt76.irq_tasklet);
 	mt76_free_device(&dev->mt76);
 	pci_free_irq_vectors(pdev);
 }

mt792x_dma.c

@@ -31,6 +31,17 @@ void mt792x_irq_tasklet(unsigned long data)
 	const struct mt792x_irq_map *irq_map = dev->irq_map;
 	u32 intr, mask = 0;
 
+	/* This can already be queued by mt792x_irq_handler() before
+	 * MT76_REMOVED is observed there. Bail out here too instead of
+	 * touching hardware registers and calling napi_schedule() during
+	 * teardown -- every other bus type in this driver family already
+	 * makes this check (usb.c, mt76x02_usb_mcu.c); PCI was missing it,
+	 * and napi_disable() during remove has no bound on how long it
+	 * waits for a NAPI poll that keeps getting rescheduled.
+	 */
+	if (test_bit(MT76_REMOVED, &dev->mphy.state))
+		return;
+
 	mt76_wr(dev, irq_map->host_irq_enable, 0);
 
 	intr = mt76_rr(dev, MT_WFDMA0_HOST_INT_STA);
@@ -515,6 +526,11 @@ int mt792x_poll_tx(struct napi_struct *napi, int budget)
 
 	dev = container_of(napi, struct mt792x_dev, mt76.tx_napi);
 
+	if (test_bit(MT76_REMOVED, &dev->mphy.state)) {
+		napi_complete(napi);
+		return 0;
+	}
+
 	if (!mt76_connac_pm_ref(&dev->mphy, &dev->pm)) {
 		napi_complete(napi);
 		queue_work(dev->mt76.wq, &dev->pm.wake_work);
@@ -538,6 +554,11 @@ int mt792x_poll_rx(struct napi_struct *napi, int budget)
 
 	dev = mt76_priv(napi->dev);
 
+	if (test_bit(MT76_REMOVED, &dev->mphy.state)) {
+		napi_complete(napi);
+		return 0;
+	}
+
 	if (!mt76_connac_pm_ref(&dev->mphy, &dev->pm)) {
 		napi_complete(napi);
 		queue_work(dev->mt76.wq, &dev->pm.wake_work);
@@ -620,4 +641,3 @@ int mt792x_wfsys_reset(struct mt792x_dev *dev)
 	return mt792x_wfsys_reset_default(dev);
 }
 EXPORT_SYMBOL_GPL(mt792x_wfsys_reset);
-

tests/mt7927/remove-tasklet-before-dma-contract.sh

@@ -0,0 +1,53 @@
+#!/bin/sh
+set -eu
+
+repo="${1:-.}"
+file="$repo/mt7925/pci.c"
+
+awk '
+	/static void mt7925e_unregister_device\(struct mt792x_dev \*dev\)/ { in_unreg = 1 }
+	in_unreg && /^}/ { in_unreg = 0 }
+	in_unreg && /tasklet_disable\(&dev->mt76\.irq_tasklet\);/ {
+		print "FAIL: mt7925e_unregister_device disables irq_tasklet without a matching enable; a disabled-but-scheduled tasklet never clears TASKLET_STATE_SCHED and hangs the tasklet_kill() in mt7925_pci_remove()"
+		bad = 1
+		exit 1
+	}
+
+	/static void mt7925_pci_remove\(struct pci_dev \*pdev\)/ { in_remove = 1 }
+	in_remove && /mt7925e_unregister_device\(dev\);/ { unregister_line = NR }
+	in_remove && /tasklet_kill\(&dev->mt76\.irq_tasklet\);/ { kill_line = NR }
+	in_remove && /mt76_free_device\(&dev->mt76\);/ { free_line = NR }
+	in_remove && /^}/ {
+		if (!unregister_line) {
+			print "FAIL: pci_remove does not call mt7925e_unregister_device"
+			exit 1
+		}
+		if (!kill_line) {
+			print "FAIL: pci_remove does not tasklet_kill irq_tasklet"
+			exit 1
+		}
+		if (!free_line) {
+			print "FAIL: pci_remove does not call mt76_free_device"
+			exit 1
+		}
+		if (kill_line < unregister_line) {
+			print "FAIL: irq_tasklet is killed before unregister_device runs"
+			exit 1
+		}
+		if (kill_line > free_line) {
+			print "FAIL: irq_tasklet is killed after mt76_free_device frees dev"
+			exit 1
+		}
+		found = 1
+		exit 0
+	}
+	END {
+		if (bad) exit 1
+		if (!found) {
+			print "FAIL: mt7925_pci_remove contract was not checked"
+			exit 1
+		}
+	}
+' "$file"
+
+echo "PASS irq_tasklet is not left disabled in unregister, and is killed between unregister and free in remove"