[PATCH net-next 0/7] eth: fix calling napi_enable() in atomic context

[PATCH net-next 0/7] eth: fix calling napi_enable() in atomic context

[Jakub Kicinski]

Dan has reported I missed a lot of drivers which call napi_enable()
in atomic with the naive coccinelle search for spin locks:
https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain

Fix them. Most of the fixes involve taking the netdev_lock()
before the spin lock. mt76 is special because we can just
move napi_enable() from the BH section.

All patches compile tested only.

Jakub Kicinski (7):
  eth: tg3: fix calling napi_enable() in atomic context
  eth: forcedeth: remove local wrappers for napi enable/disable
  eth: forcedeth: fix calling napi_enable() in atomic context
  eth: 8139too: fix calling napi_enable() in atomic context
  eth: niu: fix calling napi_enable() in atomic context
  eth: via-rhine: fix calling napi_enable() in atomic context
  wifi: mt76: move napi_enable() from under BH

 drivers/net/ethernet/broadcom/tg3.c           | 33 ++++++++++++++++---
 drivers/net/ethernet/nvidia/forcedeth.c       | 32 ++++++------------
 drivers/net/ethernet/realtek/8139too.c        |  4 ++-
 drivers/net/ethernet/sun/niu.c                | 10 +++++-
 drivers/net/ethernet/via/via-rhine.c          |  9 +++++
 .../net/wireless/mediatek/mt76/mt7603/mac.c   |  9 +++--
 .../net/wireless/mediatek/mt76/mt7615/pci.c   |  8 +++--
 .../wireless/mediatek/mt76/mt7615/pci_mac.c   |  8 +++--
 .../net/wireless/mediatek/mt76/mt76x0/pci.c   |  8 +++--
 .../net/wireless/mediatek/mt76/mt76x02_mmio.c |  8 +++--
 .../net/wireless/mediatek/mt76/mt76x2/pci.c   |  7 ++--
 .../net/wireless/mediatek/mt76/mt7915/mac.c   | 17 +++++++---
 .../net/wireless/mediatek/mt76/mt7921/pci.c   |  7 ++--
 .../wireless/mediatek/mt76/mt7921/pci_mac.c   |  7 ++--
 .../net/wireless/mediatek/mt76/mt7925/pci.c   |  7 ++--
 .../wireless/mediatek/mt76/mt7925/pci_mac.c   |  7 ++--
 .../net/wireless/mediatek/mt76/mt7996/mac.c   | 12 +++----
 17 files changed, 129 insertions(+), 64 deletions(-)

-- 
2.48.1

[PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Jakub Kicinski]

tg3 has a spin lock protecting most of the config,
switch to taking netdev_lock() explicitly on enable/start
paths. Disable/stop paths seem to not be under the spin
lock (since napi_disable() already couldn't sleep),
so leave that side as is.

tg3_restart_hw() releases and re-takes the spin lock,
we need to do the same because dev_close() needs to
take netdev_lock().

Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: pavan.chebbi@broadcom.com
CC: mchan@broadcom.com
CC: kuniyu@amazon.com
CC: romieu@fr.zoreil.com
---
 drivers/net/ethernet/broadcom/tg3.c | 33 +++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 9cc8db10a8d6..4aebba80bcd2 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7424,7 +7424,7 @@ static void tg3_napi_enable(struct tg3 *tp)
 
 	for (i = 0; i < tp->irq_cnt; i++) {
 		tnapi = &tp->napi[i];
-		napi_enable(&tnapi->napi);
+		napi_enable_locked(&tnapi->napi);
 		if (tnapi->tx_buffers) {
 			netif_queue_set_napi(tp->dev, txq_idx,
 					     NETDEV_QUEUE_TYPE_TX,
@@ -7445,9 +7445,10 @@ static void tg3_napi_init(struct tg3 *tp)
 	int i;
 
 	for (i = 0; i < tp->irq_cnt; i++) {
-		netif_napi_add(tp->dev, &tp->napi[i].napi,
-			       i ? tg3_poll_msix : tg3_poll);
-		netif_napi_set_irq(&tp->napi[i].napi, tp->napi[i].irq_vec);
+		netif_napi_add_locked(tp->dev, &tp->napi[i].napi,
+				      i ? tg3_poll_msix : tg3_poll);
+		netif_napi_set_irq_locked(&tp->napi[i].napi,
+					  tp->napi[i].irq_vec);
 	}
 }
 
@@ -11271,7 +11272,9 @@ static int tg3_restart_hw(struct tg3 *tp, bool reset_phy)
 		tg3_timer_stop(tp);
 		tp->irq_sync = 0;
 		tg3_napi_enable(tp);
+		netdev_unlock(tp->dev);
 		dev_close(tp->dev);
+		netdev_lock(tp->dev);
 		tg3_full_lock(tp, 0);
 	}
 	return err;
@@ -11299,6 +11302,7 @@ static void tg3_reset_task(struct work_struct *work)
 
 	tg3_netif_stop(tp);
 
+	netdev_lock(tp->dev);
 	tg3_full_lock(tp, 1);
 
 	if (tg3_flag(tp, TX_RECOVERY_PENDING)) {
@@ -11318,12 +11322,14 @@ static void tg3_reset_task(struct work_struct *work)
 		 * call cancel_work_sync() and wait forever.
 		 */
 		tg3_flag_clear(tp, RESET_TASK_PENDING);
+		netdev_unlock(tp->dev);
 		dev_close(tp->dev);
 		goto out;
 	}
 
 	tg3_netif_start(tp);
 	tg3_full_unlock(tp);
+	netdev_unlock(tp->dev);
 	tg3_phy_start(tp);
 	tg3_flag_clear(tp, RESET_TASK_PENDING);
 out:
@@ -11683,9 +11689,11 @@ static int tg3_start(struct tg3 *tp, bool reset_phy, bool test_irq,
 	if (err)
 		goto out_ints_fini;
 
+	netdev_lock(dev);
 	tg3_napi_init(tp);
 
 	tg3_napi_enable(tp);
+	netdev_unlock(dev);
 
 	for (i = 0; i < tp->irq_cnt; i++) {
 		err = tg3_request_irq(tp, i);
@@ -12569,6 +12577,7 @@ static int tg3_set_ringparam(struct net_device *dev,
 		irq_sync = 1;
 	}
 
+	netdev_lock(dev);
 	tg3_full_lock(tp, irq_sync);
 
 	tp->rx_pending = ering->rx_pending;
@@ -12597,6 +12606,7 @@ static int tg3_set_ringparam(struct net_device *dev,
 	}
 
 	tg3_full_unlock(tp);
+	netdev_unlock(dev);
 
 	if (irq_sync && !err)
 		tg3_phy_start(tp);
@@ -12678,6 +12688,7 @@ static int tg3_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam
 			irq_sync = 1;
 		}
 
+		netdev_lock(dev);
 		tg3_full_lock(tp, irq_sync);
 
 		if (epause->autoneg)
@@ -12707,6 +12718,7 @@ static int tg3_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam
 		}
 
 		tg3_full_unlock(tp);
+		netdev_unlock(dev);
 	}
 
 	tp->phy_flags |= TG3_PHYFLG_USER_CONFIGURED;
@@ -13911,6 +13923,7 @@ static void tg3_self_test(struct net_device *dev, struct ethtool_test *etest,
 			data[TG3_INTERRUPT_TEST] = 1;
 		}
 
+		netdev_lock(dev);
 		tg3_full_lock(tp, 0);
 
 		tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
@@ -13922,6 +13935,7 @@ static void tg3_self_test(struct net_device *dev, struct ethtool_test *etest,
 		}
 
 		tg3_full_unlock(tp);
+		netdev_unlock(dev);
 
 		if (irq_sync && !err2)
 			tg3_phy_start(tp);
@@ -14365,6 +14379,7 @@ static int tg3_change_mtu(struct net_device *dev, int new_mtu)
 
 	tg3_set_mtu(dev, tp, new_mtu);
 
+	netdev_lock(dev);
 	tg3_full_lock(tp, 1);
 
 	tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
@@ -14384,6 +14399,7 @@ static int tg3_change_mtu(struct net_device *dev, int new_mtu)
 		tg3_netif_start(tp);
 
 	tg3_full_unlock(tp);
+	netdev_unlock(dev);
 
 	if (!err)
 		tg3_phy_start(tp);
@@ -18164,6 +18180,7 @@ static int tg3_resume(struct device *device)
 
 	netif_device_attach(dev);
 
+	netdev_lock(dev);
 	tg3_full_lock(tp, 0);
 
 	tg3_ape_driver_state_change(tp, RESET_KIND_INIT);
@@ -18180,6 +18197,7 @@ static int tg3_resume(struct device *device)
 
 out:
 	tg3_full_unlock(tp);
+	netdev_unlock(dev);
 
 	if (!err)
 		tg3_phy_start(tp);
@@ -18260,7 +18278,9 @@ static pci_ers_result_t tg3_io_error_detected(struct pci_dev *pdev,
 done:
 	if (state == pci_channel_io_perm_failure) {
 		if (netdev) {
+			netdev_lock(netdev);
 			tg3_napi_enable(tp);
+			netdev_unlock(netdev);
 			dev_close(netdev);
 		}
 		err = PCI_ERS_RESULT_DISCONNECT;
@@ -18314,7 +18334,9 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev)
 
 done:
 	if (rc != PCI_ERS_RESULT_RECOVERED && netdev && netif_running(netdev)) {
+		netdev_lock(netdev);
 		tg3_napi_enable(tp);
+		netdev_unlock(netdev);
 		dev_close(netdev);
 	}
 	rtnl_unlock();
@@ -18340,12 +18362,14 @@ static void tg3_io_resume(struct pci_dev *pdev)
 	if (!netdev || !netif_running(netdev))
 		goto done;
 
+	netdev_lock(netdev);
 	tg3_full_lock(tp, 0);
 	tg3_ape_driver_state_change(tp, RESET_KIND_INIT);
 	tg3_flag_set(tp, INIT_COMPLETE);
 	err = tg3_restart_hw(tp, true);
 	if (err) {
 		tg3_full_unlock(tp);
+		netdev_unlock(netdev);
 		netdev_err(netdev, "Cannot restart hardware after reset.\n");
 		goto done;
 	}
@@ -18357,6 +18381,7 @@ static void tg3_io_resume(struct pci_dev *pdev)
 	tg3_netif_start(tp);
 
 	tg3_full_unlock(tp);
+	netdev_unlock(netdev);
 
 	tg3_phy_start(tp);
 
-- 
2.48.1

Re: [PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Tue, Jan 21, 2025 at 2:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> tg3 has a spin lock protecting most of the config,
> switch to taking netdev_lock() explicitly on enable/start
> paths. Disable/stop paths seem to not be under the spin
> lock (since napi_disable() already couldn't sleep),

You meant napi_disable() could sleep, right?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Jakub Kicinski]

On Tue, 21 Jan 2025 15:06:36 -0800 Michael Chan wrote:
> On Tue, Jan 21, 2025 at 2:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > tg3 has a spin lock protecting most of the config,
> > switch to taking netdev_lock() explicitly on enable/start
> > paths. Disable/stop paths seem to not be under the spin
> > lock (since napi_disable() already couldn't sleep),  
> 
> You meant napi_disable() could sleep, right?

Yes :)

Re: [PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

On Tue, Jan 21, 2025 at 2:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> tg3 has a spin lock protecting most of the config,
> switch to taking netdev_lock() explicitly on enable/start
> paths. Disable/stop paths seem to not be under the spin
> lock (since napi_disable() already couldn't sleep),
> so leave that side as is.
>
> tg3_restart_hw() releases and re-takes the spin lock,
> we need to do the same because dev_close() needs to
> take netdev_lock().

The patch looks good to me.  One minor suggestion is to add the Sparse macros:

__releases(tp->dev->lock)
__acquires(tp->dev->lock)

to tg3_restart_hw() since we already do the same thing there for tp->lock.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Jakub Kicinski]

On Tue, 21 Jan 2025 23:41:53 -0800 Michael Chan wrote:
> One minor suggestion is to add the Sparse macros:
> 
> __releases(tp->dev->lock)
> __acquires(tp->dev->lock)
> 
> to tg3_restart_hw() since we already do the same thing there for tp->lock.

Does anyone actually use the sparse lock checking?
IIUC it's disabled by default, it's too noisy.
netdev_lock / netdev_unlock don't have the annotations.

Re: [PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Jakub Kicinski]

On Wed, 22 Jan 2025 06:27:13 -0800 Jakub Kicinski wrote:
> netdev_lock / netdev_unlock don't have the annotations.

Looks like sparse -Wcontext is happy either way, so I'll add it.

Re: [PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Simon Horman]

On Wed, Jan 22, 2025 at 03:48:33PM -0800, Jakub Kicinski wrote:
> On Wed, 22 Jan 2025 06:27:13 -0800 Jakub Kicinski wrote:
> > netdev_lock / netdev_unlock don't have the annotations.
> 
> Looks like sparse -Wcontext is happy either way, so I'll add it.

FWIIW, I do notice when these annotations get flagged by Sparse.
Well, at lest some of the time. Though I do confess that I'm not
always able to figure out a satisfactory resolution.

Re: [PATCH net-next 1/7] eth: tg3: fix calling napi_enable() in atomic context

[Eric Dumazet]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> tg3 has a spin lock protecting most of the config,
> switch to taking netdev_lock() explicitly on enable/start
> paths. Disable/stop paths seem to not be under the spin
> lock (since napi_disable() already couldn't sleep),
> so leave that side as is.
>
> tg3_restart_hw() releases and re-takes the spin lock,
> we need to do the same because dev_close() needs to
> take netdev_lock().
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH net-next 2/7] eth: forcedeth: remove local wrappers for napi enable/disable

[Jakub Kicinski]

The local helpers for calling napi_enable() and napi_disable()
don't serve much purpose and they will complicate the fix in
the subsequent patch. Remove them, call the core functions
directly.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: rain.1986.08.12@gmail.com
CC: zyjzyj2000@gmail.com
---
 drivers/net/ethernet/nvidia/forcedeth.c | 30 +++++++------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 720f577929db..b00df57f2ca3 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -1120,20 +1120,6 @@ static void nv_disable_hw_interrupts(struct net_device *dev, u32 mask)
 	}
 }
 
-static void nv_napi_enable(struct net_device *dev)
-{
-	struct fe_priv *np = get_nvpriv(dev);
-
-	napi_enable(&np->napi);
-}
-
-static void nv_napi_disable(struct net_device *dev)
-{
-	struct fe_priv *np = get_nvpriv(dev);
-
-	napi_disable(&np->napi);
-}
-
 #define MII_READ	(-1)
 /* mii_rw: read/write a register on the PHY.
  *
@@ -3114,7 +3100,7 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
 		 * Changing the MTU is a rare event, it shouldn't matter.
 		 */
 		nv_disable_irq(dev);
-		nv_napi_disable(dev);
+		napi_disable(&np->napi);
 		netif_tx_lock_bh(dev);
 		netif_addr_lock(dev);
 		spin_lock(&np->lock);
@@ -3143,7 +3129,7 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
 		spin_unlock(&np->lock);
 		netif_addr_unlock(dev);
 		netif_tx_unlock_bh(dev);
-		nv_napi_enable(dev);
+		napi_enable(&np->napi);
 		nv_enable_irq(dev);
 	}
 	return 0;
@@ -4731,7 +4717,7 @@ static int nv_set_ringparam(struct net_device *dev,
 
 	if (netif_running(dev)) {
 		nv_disable_irq(dev);
-		nv_napi_disable(dev);
+		napi_disable(&np->napi);
 		netif_tx_lock_bh(dev);
 		netif_addr_lock(dev);
 		spin_lock(&np->lock);
@@ -4784,7 +4770,7 @@ static int nv_set_ringparam(struct net_device *dev,
 		spin_unlock(&np->lock);
 		netif_addr_unlock(dev);
 		netif_tx_unlock_bh(dev);
-		nv_napi_enable(dev);
+		napi_enable(&np->napi);
 		nv_enable_irq(dev);
 	}
 	return 0;
@@ -5277,7 +5263,7 @@ static void nv_self_test(struct net_device *dev, struct ethtool_test *test, u64
 	if (test->flags & ETH_TEST_FL_OFFLINE) {
 		if (netif_running(dev)) {
 			netif_stop_queue(dev);
-			nv_napi_disable(dev);
+			napi_disable(&np->napi);
 			netif_tx_lock_bh(dev);
 			netif_addr_lock(dev);
 			spin_lock_irq(&np->lock);
@@ -5334,7 +5320,7 @@ static void nv_self_test(struct net_device *dev, struct ethtool_test *test, u64
 			/* restart rx engine */
 			nv_start_rxtx(dev);
 			netif_start_queue(dev);
-			nv_napi_enable(dev);
+			napi_enable(&np->napi);
 			nv_enable_hw_interrupts(dev, np->irqmask);
 		}
 	}
@@ -5594,7 +5580,7 @@ static int nv_open(struct net_device *dev)
 	ret = nv_update_linkspeed(dev);
 	nv_start_rxtx(dev);
 	netif_start_queue(dev);
-	nv_napi_enable(dev);
+	napi_enable(&np->napi);
 
 	if (ret) {
 		netif_carrier_on(dev);
@@ -5632,7 +5618,7 @@ static int nv_close(struct net_device *dev)
 	spin_lock_irq(&np->lock);
 	np->in_shutdown = 1;
 	spin_unlock_irq(&np->lock);
-	nv_napi_disable(dev);
+	napi_disable(&np->napi);
 	synchronize_irq(np->pci_dev->irq);
 
 	del_timer_sync(&np->oom_kick);
-- 
2.48.1

Re: [PATCH net-next 2/7] eth: forcedeth: remove local wrappers for napi enable/disable

[Zhu Logan]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> The local helpers for calling napi_enable() and napi_disable()
> don't serve much purpose and they will complicate the fix in
> the subsequent patch. Remove them, call the core functions
> directly.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

I am fine with this.
Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com>

Zhu Yanjun

> ---
> CC: rain.1986.08.12@gmail.com
> CC: zyjzyj2000@gmail.com
> ---
>  drivers/net/ethernet/nvidia/forcedeth.c | 30 +++++++------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index 720f577929db..b00df57f2ca3 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -1120,20 +1120,6 @@ static void nv_disable_hw_interrupts(struct net_device *dev, u32 mask)
>         }
>  }
>
> -static void nv_napi_enable(struct net_device *dev)
> -{
> -       struct fe_priv *np = get_nvpriv(dev);
> -
> -       napi_enable(&np->napi);
> -}
> -
> -static void nv_napi_disable(struct net_device *dev)
> -{
> -       struct fe_priv *np = get_nvpriv(dev);
> -
> -       napi_disable(&np->napi);
> -}
> -
>  #define MII_READ       (-1)
>  /* mii_rw: read/write a register on the PHY.
>   *
> @@ -3114,7 +3100,7 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
>                  * Changing the MTU is a rare event, it shouldn't matter.
>                  */
>                 nv_disable_irq(dev);
> -               nv_napi_disable(dev);
> +               napi_disable(&np->napi);
>                 netif_tx_lock_bh(dev);
>                 netif_addr_lock(dev);
>                 spin_lock(&np->lock);
> @@ -3143,7 +3129,7 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
>                 spin_unlock(&np->lock);
>                 netif_addr_unlock(dev);
>                 netif_tx_unlock_bh(dev);
> -               nv_napi_enable(dev);
> +               napi_enable(&np->napi);
>                 nv_enable_irq(dev);
>         }
>         return 0;
> @@ -4731,7 +4717,7 @@ static int nv_set_ringparam(struct net_device *dev,
>
>         if (netif_running(dev)) {
>                 nv_disable_irq(dev);
> -               nv_napi_disable(dev);
> +               napi_disable(&np->napi);
>                 netif_tx_lock_bh(dev);
>                 netif_addr_lock(dev);
>                 spin_lock(&np->lock);
> @@ -4784,7 +4770,7 @@ static int nv_set_ringparam(struct net_device *dev,
>                 spin_unlock(&np->lock);
>                 netif_addr_unlock(dev);
>                 netif_tx_unlock_bh(dev);
> -               nv_napi_enable(dev);
> +               napi_enable(&np->napi);
>                 nv_enable_irq(dev);
>         }
>         return 0;
> @@ -5277,7 +5263,7 @@ static void nv_self_test(struct net_device *dev, struct ethtool_test *test, u64
>         if (test->flags & ETH_TEST_FL_OFFLINE) {
>                 if (netif_running(dev)) {
>                         netif_stop_queue(dev);
> -                       nv_napi_disable(dev);
> +                       napi_disable(&np->napi);
>                         netif_tx_lock_bh(dev);
>                         netif_addr_lock(dev);
>                         spin_lock_irq(&np->lock);
> @@ -5334,7 +5320,7 @@ static void nv_self_test(struct net_device *dev, struct ethtool_test *test, u64
>                         /* restart rx engine */
>                         nv_start_rxtx(dev);
>                         netif_start_queue(dev);
> -                       nv_napi_enable(dev);
> +                       napi_enable(&np->napi);
>                         nv_enable_hw_interrupts(dev, np->irqmask);
>                 }
>         }
> @@ -5594,7 +5580,7 @@ static int nv_open(struct net_device *dev)
>         ret = nv_update_linkspeed(dev);
>         nv_start_rxtx(dev);
>         netif_start_queue(dev);
> -       nv_napi_enable(dev);
> +       napi_enable(&np->napi);
>
>         if (ret) {
>                 netif_carrier_on(dev);
> @@ -5632,7 +5618,7 @@ static int nv_close(struct net_device *dev)
>         spin_lock_irq(&np->lock);
>         np->in_shutdown = 1;
>         spin_unlock_irq(&np->lock);
> -       nv_napi_disable(dev);
> +       napi_disable(&np->napi);
>         synchronize_irq(np->pci_dev->irq);
>
>         del_timer_sync(&np->oom_kick);
> --
> 2.48.1
>

Re: [PATCH net-next 2/7] eth: forcedeth: remove local wrappers for napi enable/disable

[Eric Dumazet]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> The local helpers for calling napi_enable() and napi_disable()
> don't serve much purpose and they will complicate the fix in
> the subsequent patch. Remove them, call the core functions
> directly.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH net-next 3/7] eth: forcedeth: fix calling napi_enable() in atomic context

[Jakub Kicinski]

napi_enable() may sleep now, take netdev_lock() before np->lock.

Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: rain.1986.08.12@gmail.com
CC: zyjzyj2000@gmail.com
CC: kuniyu@amazon.com
CC: romieu@fr.zoreil.com
---
 drivers/net/ethernet/nvidia/forcedeth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index b00df57f2ca3..499e5e39d513 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -5562,6 +5562,7 @@ static int nv_open(struct net_device *dev)
 	/* ask for interrupts */
 	nv_enable_hw_interrupts(dev, np->irqmask);
 
+	netdev_lock(dev);
 	spin_lock_irq(&np->lock);
 	writel(NVREG_MCASTADDRA_FORCE, base + NvRegMulticastAddrA);
 	writel(0, base + NvRegMulticastAddrB);
@@ -5580,7 +5581,7 @@ static int nv_open(struct net_device *dev)
 	ret = nv_update_linkspeed(dev);
 	nv_start_rxtx(dev);
 	netif_start_queue(dev);
-	napi_enable(&np->napi);
+	napi_enable_locked(&np->napi);
 
 	if (ret) {
 		netif_carrier_on(dev);
@@ -5597,6 +5598,7 @@ static int nv_open(struct net_device *dev)
 			round_jiffies(jiffies + STATS_INTERVAL));
 
 	spin_unlock_irq(&np->lock);
+	netdev_unlock(dev);
 
 	/* If the loopback feature was set while the device was down, make sure
 	 * that it's set correctly now.
-- 
2.48.1

Re: [PATCH net-next 3/7] eth: forcedeth: fix calling napi_enable() in atomic context

[Zhu Logan]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> napi_enable() may sleep now, take netdev_lock() before np->lock.
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Thanks,
Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com>

Zhu Yanjun

> ---
> CC: rain.1986.08.12@gmail.com
> CC: zyjzyj2000@gmail.com
> CC: kuniyu@amazon.com
> CC: romieu@fr.zoreil.com
> ---
>  drivers/net/ethernet/nvidia/forcedeth.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index b00df57f2ca3..499e5e39d513 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -5562,6 +5562,7 @@ static int nv_open(struct net_device *dev)
>         /* ask for interrupts */
>         nv_enable_hw_interrupts(dev, np->irqmask);
>
> +       netdev_lock(dev);
>         spin_lock_irq(&np->lock);
>         writel(NVREG_MCASTADDRA_FORCE, base + NvRegMulticastAddrA);
>         writel(0, base + NvRegMulticastAddrB);
> @@ -5580,7 +5581,7 @@ static int nv_open(struct net_device *dev)
>         ret = nv_update_linkspeed(dev);
>         nv_start_rxtx(dev);
>         netif_start_queue(dev);
> -       napi_enable(&np->napi);
> +       napi_enable_locked(&np->napi);
>
>         if (ret) {
>                 netif_carrier_on(dev);
> @@ -5597,6 +5598,7 @@ static int nv_open(struct net_device *dev)
>                         round_jiffies(jiffies + STATS_INTERVAL));
>
>         spin_unlock_irq(&np->lock);
> +       netdev_unlock(dev);
>
>         /* If the loopback feature was set while the device was down, make sure
>          * that it's set correctly now.
> --
> 2.48.1
>

Re: [PATCH net-next 3/7] eth: forcedeth: fix calling napi_enable() in atomic context

[Eric Dumazet]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> napi_enable() may sleep now, take netdev_lock() before np->lock.
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH net-next 4/7] eth: 8139too: fix calling napi_enable() in atomic context

[Jakub Kicinski]

napi_enable() may sleep now, take netdev_lock() before tp->lock and
tp->rx_lock.

Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: romieu@fr.zoreil.com
CC: kuniyu@amazon.com
---
 drivers/net/ethernet/realtek/8139too.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c
index 9ce0e8a64ba8..a73dcaffa8c5 100644
--- a/drivers/net/ethernet/realtek/8139too.c
+++ b/drivers/net/ethernet/realtek/8139too.c
@@ -1684,6 +1684,7 @@ static void rtl8139_tx_timeout_task (struct work_struct *work)
 	if (tmp8 & CmdTxEnb)
 		RTL_W8 (ChipCmd, CmdRxEnb);
 
+	netdev_lock(dev);
 	spin_lock_bh(&tp->rx_lock);
 	/* Disable interrupts by clearing the interrupt mask. */
 	RTL_W16 (IntrMask, 0x0000);
@@ -1694,11 +1695,12 @@ static void rtl8139_tx_timeout_task (struct work_struct *work)
 	spin_unlock_irq(&tp->lock);
 
 	/* ...and finally, reset everything */
-	napi_enable(&tp->napi);
+	napi_enable_locked(&tp->napi);
 	rtl8139_hw_start(dev);
 	netif_wake_queue(dev);
 
 	spin_unlock_bh(&tp->rx_lock);
+	netdev_unlock(dev);
 }
 
 static void rtl8139_tx_timeout(struct net_device *dev, unsigned int txqueue)
-- 
2.48.1

Re: [PATCH net-next 4/7] eth: 8139too: fix calling napi_enable() in atomic context

[Eric Dumazet]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> napi_enable() may sleep now, take netdev_lock() before tp->lock and
> tp->rx_lock.
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH net-next 5/7] eth: niu: fix calling napi_enable() in atomic context

[Jakub Kicinski]

napi_enable() may sleep now, take netdev_lock() before np->lock.

Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: willy@infradead.org
CC: romieu@fr.zoreil.com
CC: kuniyu@amazon.com
---
 drivers/net/ethernet/sun/niu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index d7459866d24c..72177fea1cfb 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -6086,7 +6086,7 @@ static void niu_enable_napi(struct niu *np)
 	int i;
 
 	for (i = 0; i < np->num_ldg; i++)
-		napi_enable(&np->ldg[i].napi);
+		napi_enable_locked(&np->ldg[i].napi);
 }
 
 static void niu_disable_napi(struct niu *np)
@@ -6116,7 +6116,9 @@ static int niu_open(struct net_device *dev)
 	if (err)
 		goto out_free_channels;
 
+	netdev_lock(dev);
 	niu_enable_napi(np);
+	netdev_unlock(dev);
 
 	spin_lock_irq(&np->lock);
 
@@ -6521,6 +6523,7 @@ static void niu_reset_task(struct work_struct *work)
 
 	niu_reset_buffers(np);
 
+	netdev_lock(np->dev);
 	spin_lock_irqsave(&np->lock, flags);
 
 	err = niu_init_hw(np);
@@ -6531,6 +6534,7 @@ static void niu_reset_task(struct work_struct *work)
 	}
 
 	spin_unlock_irqrestore(&np->lock, flags);
+	netdev_unlock(np->dev);
 }
 
 static void niu_tx_timeout(struct net_device *dev, unsigned int txqueue)
@@ -6761,7 +6765,9 @@ static int niu_change_mtu(struct net_device *dev, int new_mtu)
 
 	niu_free_channels(np);
 
+	netdev_lock(dev);
 	niu_enable_napi(np);
+	netdev_unlock(dev);
 
 	err = niu_alloc_channels(np);
 	if (err)
@@ -9937,6 +9943,7 @@ static int __maybe_unused niu_resume(struct device *dev_d)
 
 	spin_lock_irqsave(&np->lock, flags);
 
+	netdev_lock(dev);
 	err = niu_init_hw(np);
 	if (!err) {
 		np->timer.expires = jiffies + HZ;
@@ -9945,6 +9952,7 @@ static int __maybe_unused niu_resume(struct device *dev_d)
 	}
 
 	spin_unlock_irqrestore(&np->lock, flags);
+	netdev_unlock(dev);
 
 	return err;
 }
-- 
2.48.1

Re: [PATCH net-next 5/7] eth: niu: fix calling napi_enable() in atomic context

[Eric Dumazet]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> napi_enable() may sleep now, take netdev_lock() before np->lock.
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH net-next 6/7] eth: via-rhine: fix calling napi_enable() in atomic context

[Jakub Kicinski]

napi_enable() may sleep now, take netdev_lock() before rp->lock.
napi_enable() is hidden inside init_registers().

Note that this patch orders netdev_lock after rp->task_lock,
to avoid having to take the netdev_lock() around disable path.

Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: kevinbrace@bracecomputerlab.com
CC: romieu@fr.zoreil.com
CC: kuniyu@amazon.com
---
 drivers/net/ethernet/via/via-rhine.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 894911f3d560..f27157561082 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1696,7 +1696,10 @@ static int rhine_open(struct net_device *dev)
 	rhine_power_init(dev);
 	rhine_chip_reset(dev);
 	rhine_task_enable(rp);
+
+	netdev_lock(dev);
 	init_registers(dev);
+	netdev_unlock(dev);
 
 	netif_dbg(rp, ifup, dev, "%s() Done - status %04x MII status: %04x\n",
 		  __func__, ioread16(ioaddr + ChipCmd),
@@ -1727,6 +1730,8 @@ static void rhine_reset_task(struct work_struct *work)
 
 	napi_disable(&rp->napi);
 	netif_tx_disable(dev);
+
+	netdev_lock(dev);
 	spin_lock_bh(&rp->lock);
 
 	/* clear all descriptors */
@@ -1740,6 +1745,7 @@ static void rhine_reset_task(struct work_struct *work)
 	init_registers(dev);
 
 	spin_unlock_bh(&rp->lock);
+	netdev_unlock(dev);
 
 	netif_trans_update(dev); /* prevent tx timeout */
 	dev->stats.tx_errors++;
@@ -2541,9 +2547,12 @@ static int rhine_resume(struct device *device)
 	alloc_tbufs(dev);
 	rhine_reset_rbufs(rp);
 	rhine_task_enable(rp);
+
+	netdev_lock(dev);
 	spin_lock_bh(&rp->lock);
 	init_registers(dev);
 	spin_unlock_bh(&rp->lock);
+	netdev_unlock(dev);
 
 	netif_device_attach(dev);
 
-- 
2.48.1

Re: [PATCH net-next 6/7] eth: via-rhine: fix calling napi_enable() in atomic context

[Eric Dumazet]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> napi_enable() may sleep now, take netdev_lock() before rp->lock.
> napi_enable() is hidden inside init_registers().
>
> Note that this patch orders netdev_lock after rp->task_lock,
> to avoid having to take the netdev_lock() around disable path.
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: kevinbrace@bracecomputerlab.com
> CC: romieu@fr.zoreil.com
> CC: kuniyu@amazon.com
> ---
>  drivers/net/ethernet/via/via-rhine.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>

Hmm. I  think you forgot this chunk :

diff --git a/drivers/net/ethernet/via/via-rhine.c
b/drivers/net/ethernet/via/via-rhine.c
index 894911f3d5603c109cba7651b6b047b59ec85c9..4e59b5da063e1690fce0f210c33143a42745810
100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1568,7 +1568,7 @@ static void init_registers(struct net_device *dev)
        if (rp->quirks & rqMgmt)
                rhine_init_cam_filter(dev);

-       napi_enable(&rp->napi);
+       napi_enable_locked(&rp->napi);

        iowrite16(RHINE_EVENT & 0xffff, ioaddr + IntrEnable);





> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> index 894911f3d560..f27157561082 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
> @@ -1696,7 +1696,10 @@ static int rhine_open(struct net_device *dev)
>         rhine_power_init(dev);
>         rhine_chip_reset(dev);
>         rhine_task_enable(rp);
> +
> +       netdev_lock(dev);
>         init_registers(dev);
> +       netdev_unlock(dev);
>
>         netif_dbg(rp, ifup, dev, "%s() Done - status %04x MII status: %04x\n",
>                   __func__, ioread16(ioaddr + ChipCmd),
> @@ -1727,6 +1730,8 @@ static void rhine_reset_task(struct work_struct *work)
>
>         napi_disable(&rp->napi);
>         netif_tx_disable(dev);
> +
> +       netdev_lock(dev);
>         spin_lock_bh(&rp->lock);
>
>         /* clear all descriptors */
> @@ -1740,6 +1745,7 @@ static void rhine_reset_task(struct work_struct *work)
>         init_registers(dev);
>
>         spin_unlock_bh(&rp->lock);
> +       netdev_unlock(dev);
>
>         netif_trans_update(dev); /* prevent tx timeout */
>         dev->stats.tx_errors++;
> @@ -2541,9 +2547,12 @@ static int rhine_resume(struct device *device)
>         alloc_tbufs(dev);
>         rhine_reset_rbufs(rp);
>         rhine_task_enable(rp);
> +
> +       netdev_lock(dev);
>         spin_lock_bh(&rp->lock);
>         init_registers(dev);
>         spin_unlock_bh(&rp->lock);
> +       netdev_unlock(dev);
>
>         netif_device_attach(dev);
>
> --
> 2.48.1
>

[PATCH net-next 7/7] wifi: mt76: move napi_enable() from under BH

[Jakub Kicinski]

mt76 does a lot of:

  local_bh_disable();
  napi_enable(...napi);
  napi_schedule(...napi);
  local_bh_enable();

local_bh_disable() is not a real lock, its most likely taken
because napi_schedule() requires it. napi_enable() needs
to take a mutex, so move it from under the BH protection.

Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: nbd@nbd.name
CC: lorenzo@kernel.org
CC: ryder.lee@mediatek.com
CC: shayne.chen@mediatek.com
CC: sean.wang@mediatek.com
CC: kvalo@kernel.org
CC: matthias.bgg@gmail.com
CC: angelogioacchino.delregno@collabora.com
CC: quan.zhou@mediatek.com
CC: johannes.berg@intel.com
CC: emmanuel.grumbach@intel.com
CC: leitao@debian.org
CC: mingyen.hsieh@mediatek.com
CC: leon.yen@mediatek.com
CC: deren.wu@mediatek.com
CC: chui-hao.chiu@mediatek.com
CC: kuniyu@amazon.com
CC: romieu@fr.zoreil.com
CC: linux-wireless@vger.kernel.org
---
 drivers/net/wireless/mediatek/mt76/mt7603/mac.c |  9 ++++-----
 drivers/net/wireless/mediatek/mt76/mt7615/pci.c |  8 ++++++--
 .../net/wireless/mediatek/mt76/mt7615/pci_mac.c |  8 +++++---
 drivers/net/wireless/mediatek/mt76/mt76x0/pci.c |  8 +++++---
 .../net/wireless/mediatek/mt76/mt76x02_mmio.c   |  8 +++++---
 drivers/net/wireless/mediatek/mt76/mt76x2/pci.c |  7 +++++--
 drivers/net/wireless/mediatek/mt76/mt7915/mac.c | 17 +++++++++++++----
 drivers/net/wireless/mediatek/mt76/mt7921/pci.c |  7 +++++--
 .../net/wireless/mediatek/mt76/mt7921/pci_mac.c |  7 +++++--
 drivers/net/wireless/mediatek/mt76/mt7925/pci.c |  7 +++++--
 .../net/wireless/mediatek/mt76/mt7925/pci_mac.c |  7 +++++--
 drivers/net/wireless/mediatek/mt76/mt7996/mac.c | 12 ++++++------
 12 files changed, 69 insertions(+), 36 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
index a259f4dd9540..413973d05b43 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
@@ -1479,14 +1479,13 @@ static void mt7603_mac_watchdog_reset(struct mt7603_dev *dev)
 	tasklet_enable(&dev->mt76.pre_tbtt_tasklet);
 	mt7603_beacon_set_timer(dev, -1, beacon_int);
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
-	napi_schedule(&dev->mt76.tx_napi);
-
 	napi_enable(&dev->mt76.napi[0]);
-	napi_schedule(&dev->mt76.napi[0]);
-
 	napi_enable(&dev->mt76.napi[1]);
+
+	local_bh_disable();
+	napi_schedule(&dev->mt76.tx_napi);
+	napi_schedule(&dev->mt76.napi[0]);
 	napi_schedule(&dev->mt76.napi[1]);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/pci.c b/drivers/net/wireless/mediatek/mt76/mt7615/pci.c
index 9a278589df4e..68010e27f065 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/pci.c
@@ -164,12 +164,16 @@ static int mt7615_pci_resume(struct pci_dev *pdev)
 		dev_err(mdev->dev, "PDMA engine must be reinitialized\n");
 
 	mt76_worker_enable(&mdev->tx_worker);
-	local_bh_disable();
+
 	mt76_for_each_q_rx(mdev, i) {
 		napi_enable(&mdev->napi[i]);
-		napi_schedule(&mdev->napi[i]);
 	}
 	napi_enable(&mdev->tx_napi);
+
+	local_bh_disable();
+	mt76_for_each_q_rx(mdev, i) {
+		napi_schedule(&mdev->napi[i]);
+	}
 	napi_schedule(&mdev->tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/pci_mac.c b/drivers/net/wireless/mediatek/mt76/mt7615/pci_mac.c
index a0ca3bbdfcaf..c2e4e6aabd9f 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/pci_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/pci_mac.c
@@ -262,12 +262,14 @@ void mt7615_mac_reset_work(struct work_struct *work)
 
 	mt76_worker_enable(&dev->mt76.tx_worker);
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
-	napi_schedule(&dev->mt76.tx_napi);
-
 	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_enable(&dev->mt76.napi[i]);
+	}
+
+	local_bh_disable();
+	napi_schedule(&dev->mt76.tx_napi);
+	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_schedule(&dev->mt76.napi[i]);
 	}
 	local_bh_enable();
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
index 1eb955f3ca13..b456ccd00d58 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
@@ -282,14 +282,16 @@ static int mt76x0e_resume(struct pci_dev *pdev)
 
 	mt76_worker_enable(&mdev->tx_worker);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(mdev, i) {
 		mt76_queue_rx_reset(dev, i);
 		napi_enable(&mdev->napi[i]);
+	}
+	napi_enable(&mdev->tx_napi);
+
+	local_bh_disable();
+	mt76_for_each_q_rx(mdev, i) {
 		napi_schedule(&mdev->napi[i]);
 	}
-
-	napi_enable(&mdev->tx_napi);
 	napi_schedule(&mdev->tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
index 7d840ad4ae65..a82c75ba26e6 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
@@ -504,12 +504,14 @@ static void mt76x02_watchdog_reset(struct mt76x02_dev *dev)
 	mt76_worker_enable(&dev->mt76.tx_worker);
 	tasklet_enable(&dev->mt76.pre_tbtt_tasklet);
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
-	napi_schedule(&dev->mt76.tx_napi);
-
 	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_enable(&dev->mt76.napi[i]);
+	}
+
+	local_bh_disable();
+	napi_schedule(&dev->mt76.tx_napi);
+	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_schedule(&dev->mt76.napi[i]);
 	}
 	local_bh_enable();
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c b/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c
index 67c9d1caa0bd..727bfdd00b40 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c
@@ -151,12 +151,15 @@ mt76x2e_resume(struct pci_dev *pdev)
 
 	mt76_worker_enable(&mdev->tx_worker);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(mdev, i) {
 		napi_enable(&mdev->napi[i]);
-		napi_schedule(&mdev->napi[i]);
 	}
 	napi_enable(&mdev->tx_napi);
+
+	local_bh_disable();
+	mt76_for_each_q_rx(mdev, i) {
+		napi_schedule(&mdev->napi[i]);
+	}
 	napi_schedule(&mdev->tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/mac.c b/drivers/net/wireless/mediatek/mt76/mt7915/mac.c
index 13bdc0a7174c..2ba6eb3038ce 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/mac.c
@@ -1356,10 +1356,15 @@ mt7915_mac_restart(struct mt7915_dev *dev)
 
 	mt7915_dma_reset(dev, true);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(mdev, i) {
 		if (mdev->q_rx[i].ndesc) {
 			napi_enable(&dev->mt76.napi[i]);
+		}
+	}
+
+	local_bh_disable();
+	mt76_for_each_q_rx(mdev, i) {
+		if (mdev->q_rx[i].ndesc) {
 			napi_schedule(&dev->mt76.napi[i]);
 		}
 	}
@@ -1419,8 +1424,9 @@ mt7915_mac_restart(struct mt7915_dev *dev)
 	if (phy2)
 		clear_bit(MT76_RESET, &phy2->mt76->state);
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
+
+	local_bh_disable();
 	napi_schedule(&dev->mt76.tx_napi);
 	local_bh_enable();
 
@@ -1570,9 +1576,12 @@ void mt7915_mac_reset_work(struct work_struct *work)
 	if (phy2)
 		clear_bit(MT76_RESET, &phy2->mt76->state);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_enable(&dev->mt76.napi[i]);
+	}
+
+	local_bh_disable();
+	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_schedule(&dev->mt76.napi[i]);
 	}
 	local_bh_enable();
@@ -1581,8 +1590,8 @@ void mt7915_mac_reset_work(struct work_struct *work)
 
 	mt76_worker_enable(&dev->mt76.tx_worker);
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
+	local_bh_disable();
 	napi_schedule(&dev->mt76.tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
index ba870e1b05fb..a0c9df3c2cc7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
@@ -523,12 +523,15 @@ static int mt7921_pci_resume(struct device *device)
 
 	mt76_worker_enable(&mdev->tx_worker);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(mdev, i) {
 		napi_enable(&mdev->napi[i]);
-		napi_schedule(&mdev->napi[i]);
 	}
 	napi_enable(&mdev->tx_napi);
+
+	local_bh_disable();
+	mt76_for_each_q_rx(mdev, i) {
+		napi_schedule(&mdev->napi[i]);
+	}
 	napi_schedule(&mdev->tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
index 2452b1a2d118..881812ba03ff 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
@@ -81,9 +81,12 @@ int mt7921e_mac_reset(struct mt792x_dev *dev)
 
 	mt792x_wpdma_reset(dev, true);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_enable(&dev->mt76.napi[i]);
+	}
+
+	local_bh_disable();
+	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_schedule(&dev->mt76.napi[i]);
 	}
 	local_bh_enable();
@@ -115,8 +118,8 @@ int mt7921e_mac_reset(struct mt792x_dev *dev)
 	err = __mt7921_start(&dev->phy);
 out:
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
+	local_bh_disable();
 	napi_schedule(&dev->mt76.tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7925/pci.c b/drivers/net/wireless/mediatek/mt76/mt7925/pci.c
index f36893e20c61..c7b5dc1dbb34 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7925/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7925/pci.c
@@ -556,12 +556,15 @@ static int mt7925_pci_resume(struct device *device)
 
 	mt76_worker_enable(&mdev->tx_worker);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(mdev, i) {
 		napi_enable(&mdev->napi[i]);
-		napi_schedule(&mdev->napi[i]);
 	}
 	napi_enable(&mdev->tx_napi);
+
+	local_bh_disable();
+	mt76_for_each_q_rx(mdev, i) {
+		napi_schedule(&mdev->napi[i]);
+	}
 	napi_schedule(&mdev->tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7925/pci_mac.c b/drivers/net/wireless/mediatek/mt76/mt7925/pci_mac.c
index faedbf766d1a..4578d16bf456 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7925/pci_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7925/pci_mac.c
@@ -101,12 +101,15 @@ int mt7925e_mac_reset(struct mt792x_dev *dev)
 
 	mt792x_wpdma_reset(dev, true);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(&dev->mt76, i) {
 		napi_enable(&dev->mt76.napi[i]);
-		napi_schedule(&dev->mt76.napi[i]);
 	}
 	napi_enable(&dev->mt76.tx_napi);
+
+	local_bh_disable();
+	mt76_for_each_q_rx(&dev->mt76, i) {
+		napi_schedule(&dev->mt76.napi[i]);
+	}
 	napi_schedule(&dev->mt76.tx_napi);
 	local_bh_enable();
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
index bc8cba4dca47..019c925ae600 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
@@ -1695,7 +1695,6 @@ mt7996_mac_restart(struct mt7996_dev *dev)
 
 	mt7996_dma_reset(dev, true);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(mdev, i) {
 		if (mtk_wed_device_active(&dev->mt76.mmio.wed) &&
 		    mt76_queue_is_wed_rro(&mdev->q_rx[i]))
@@ -1703,10 +1702,11 @@ mt7996_mac_restart(struct mt7996_dev *dev)
 
 		if (mdev->q_rx[i].ndesc) {
 			napi_enable(&dev->mt76.napi[i]);
+			local_bh_disable();
 			napi_schedule(&dev->mt76.napi[i]);
+			local_bh_enable();
 		}
 	}
-	local_bh_enable();
 	clear_bit(MT76_MCU_RESET, &dev->mphy.state);
 	clear_bit(MT76_STATE_MCU_RUNNING, &dev->mphy.state);
 
@@ -1764,8 +1764,8 @@ mt7996_mac_restart(struct mt7996_dev *dev)
 	if (phy3)
 		clear_bit(MT76_RESET, &phy3->mt76->state);
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
+	local_bh_disable();
 	napi_schedule(&dev->mt76.tx_napi);
 	local_bh_enable();
 
@@ -1958,23 +1958,23 @@ void mt7996_mac_reset_work(struct work_struct *work)
 	if (phy3)
 		clear_bit(MT76_RESET, &phy3->mt76->state);
 
-	local_bh_disable();
 	mt76_for_each_q_rx(&dev->mt76, i) {
 		if (mtk_wed_device_active(&dev->mt76.mmio.wed) &&
 		    mt76_queue_is_wed_rro(&dev->mt76.q_rx[i]))
 			continue;
 
 		napi_enable(&dev->mt76.napi[i]);
+		local_bh_disable();
 		napi_schedule(&dev->mt76.napi[i]);
+		local_bh_enable();
 	}
-	local_bh_enable();
 
 	tasklet_schedule(&dev->mt76.irq_tasklet);
 
 	mt76_worker_enable(&dev->mt76.tx_worker);
 
-	local_bh_disable();
 	napi_enable(&dev->mt76.tx_napi);
+	local_bh_disable();
 	napi_schedule(&dev->mt76.tx_napi);
 	local_bh_enable();
 
-- 
2.48.1

Re: [PATCH net-next 7/7] wifi: mt76: move napi_enable() from under BH

[Eric Dumazet]

On Tue, Jan 21, 2025 at 11:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> mt76 does a lot of:
>
>   local_bh_disable();
>   napi_enable(...napi);
>   napi_schedule(...napi);
>   local_bh_enable();
>
> local_bh_disable() is not a real lock, its most likely taken
> because napi_schedule() requires it. napi_enable() needs
> to take a mutex, so move it from under the BH protection.
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

napi_schedule() can run from arbitrary contexts though...

BH protection seems strange to me, but this is orthogonal to your fix.

Re: [PATCH net-next 7/7] wifi: mt76: move napi_enable() from under BH

[Jakub Kicinski]

On Wed, 22 Jan 2025 15:25:46 +0100 Eric Dumazet wrote:
> napi_schedule() can run from arbitrary contexts though...
> 
> BH protection seems strange to me, but this is orthogonal to your fix.

Right, it doesn't need the BH "protection", it's just what we 
do to make sure there is a softirq hook point after we call it.
Since local_irq_restore() does not call softirqs. 

I didn't know how to express that in a way that would be understandable
for most :S

Thanks for catching the other bug!