Skip to contentSkip to conversation

Reapply "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"

PATCH Applied Jakub Kicinski 6 participants 6 messages Jun 3 – Jun 8

Subsystems (3)
Networking Network drivers Broadcom bnxt_en 50 gigabit Ethernet driver
lore
Share & tools
lore

Jakub Kicinski

author maintainer
#0 lore raw

PATCH 1/1 Reapply "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"

This reverts commit 850d9248d2eac662f869c766a598c877690c74e5. This reapplies commit 325eb217e41f ("bnxt_en: bring back rtnl_lock() in the bnxt_open() path").

Breno reports a lockdep warning in bnxt. During FW reset the driver may end up calling netif_set_real_num_tx_queues() (if queue count changes), so calls to bnxt_open() still require rtnl_lock.

net/sched/sch_generic.c:1416 suspicious rcu_dereference_protected() usage!

   dev_qdisc_change_real_num_tx+0x54/0xe0
   netif_set_real_num_tx_queues+0x4ed/0xa80
   __bnxt_open_nic+0x9cb/0x3490
   bnxt_open+0x1cb/0x370
   bnxt_fw_reset_task+0x80d/0x1e80
   process_scheduled_works+0x9c1/0x13b0

The reverted commit was just an optimization / experiment so let's go back to taking the lock.

Reported-by Breno Leitao <[email protected]>
Link https://lore.kernel.org/[email protected] open here
Fixes 850d9248d2ea ("Revert "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"")
Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 36 ++++++++++++++++++-----
 1 file changed, 29 insertions(+), 7 deletions(-)
drivers/net/ethernet/broadcom/bnxt/bnxt.c +29 -7
index 008c34cff7b4..35e1f8f663c7 100644
@@ -14388,13 +14388,28 @@ static void bnxt_unlock_sp(struct bnxt *bp)
 	netdev_unlock(bp->dev);
 }
 
+/* Same as bnxt_lock_sp() with additional rtnl_lock */
+static void bnxt_rtnl_lock_sp(struct bnxt *bp)
+{
+	clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
+	rtnl_lock();
+	netdev_lock(bp->dev);
+}
+
+static void bnxt_rtnl_unlock_sp(struct bnxt *bp)
+{
+	set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
+	netdev_unlock(bp->dev);
+	rtnl_unlock();
+}
+
 /* Only called from bnxt_sp_task() */
 static void bnxt_reset(struct bnxt *bp, bool silent)
 {
-	bnxt_lock_sp(bp);
+	bnxt_rtnl_lock_sp(bp);
 	if (test_bit(BNXT_STATE_OPEN, &bp->state))
 		bnxt_reset_task(bp, silent);
-	bnxt_unlock_sp(bp);
+	bnxt_rtnl_unlock_sp(bp);
 }
 
 /* Only called from bnxt_sp_task() */

@@ -14402,9 +14417,9 @@ static void bnxt_rx_ring_reset(struct bnxt *bp)
 {
 	int i;
 
-	bnxt_lock_sp(bp);
+	bnxt_rtnl_lock_sp(bp);
 	if (!test_bit(BNXT_STATE_OPEN, &bp->state)) {
-		bnxt_unlock_sp(bp);
+		bnxt_rtnl_unlock_sp(bp);
 		return;
 	}
 	/* Disable and flush TPA before resetting the RX ring */

@@ -14443,7 +14458,7 @@ static void bnxt_rx_ring_reset(struct bnxt *bp)
 	}
 	if (bp->flags & BNXT_FLAG_TPA)
 		bnxt_set_tpa(bp, true);
-	bnxt_unlock_sp(bp);
+	bnxt_rtnl_unlock_sp(bp);
 }
 
 static void bnxt_fw_fatal_close(struct bnxt *bp)

@@ -15358,15 +15373,17 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 		bp->fw_reset_state = BNXT_FW_RESET_STATE_OPENING;
 		fallthrough;
 	case BNXT_FW_RESET_STATE_OPENING:
-		while (!netdev_trylock(bp->dev)) {
+		while (!rtnl_trylock()) {
 			bnxt_queue_fw_reset_work(bp, HZ / 10);
 			return;
 		}
+		netdev_lock(bp->dev);
 		rc = bnxt_open(bp->dev);
 		if (rc) {
 			netdev_err(bp->dev, "bnxt_open() failed during FW reset\n");
 			bnxt_fw_reset_abort(bp, rc);
 			netdev_unlock(bp->dev);
+			rtnl_unlock();
 			goto ulp_start;
 		}
 

@@ -15386,6 +15403,7 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 			bnxt_dl_health_fw_status_update(bp, true);
 		}
 		netdev_unlock(bp->dev);
+		rtnl_unlock();
 		bnxt_ulp_start(bp);
 		bnxt_reenable_sriov(bp);
 		netdev_lock(bp->dev);

@@ -16379,7 +16397,7 @@ static int bnxt_queue_start(struct net_device *dev,
 		   rc);
 	napi_enable_locked(&bnapi->napi);
 	bnxt_db_nq_arm(bp, &cpr->cp_db, cpr->cp_raw_cons);
-	bnxt_reset_task(bp, true);
+	netif_close(dev);
 	return rc;
 }
 

@@ -17230,6 +17248,7 @@ static int bnxt_resume(struct device *device)
 	struct bnxt *bp = netdev_priv(dev);
 	int rc = 0;
 
+	rtnl_lock();
 	netdev_lock(dev);
 	rc = pci_enable_device(bp->pdev);
 	if (rc) {

@@ -17274,6 +17293,7 @@ static int bnxt_resume(struct device *device)
 
 resume_exit:
 	netdev_unlock(bp->dev);
+	rtnl_unlock();
 	if (!rc) {
 		bnxt_ulp_start(bp);
 		bnxt_reenable_sriov(bp);

@@ -17445,6 +17465,7 @@ static void bnxt_io_resume(struct pci_dev *pdev)
 	int err;
 
 	netdev_info(bp->dev, "PCI Slot Resume\n");
+	rtnl_lock();
 	netdev_lock(netdev);
 
 	err = bnxt_hwrm_func_qcaps(bp);

@@ -17462,6 +17483,7 @@ static void bnxt_io_resume(struct pci_dev *pdev)
 		netif_device_attach(netdev);
 
 	netdev_unlock(netdev);
+	rtnl_unlock();
 	if (!err) {
 		bnxt_ulp_start(bp);
 		bnxt_reenable_sriov(bp);
Signature · 1 line
2.54.0
Acked-by on v1 1/1
#1 lore raw
Quoted 24 lines · Jakub Kicinski

This reverts commit 850d9248d2eac662f869c766a598c877690c74e5. This reapplies commit 325eb217e41f ("bnxt_en: bring back rtnl_lock() in the bnxt_open() path").

Breno reports a lockdep warning in bnxt. During FW reset the driver may end up calling netif_set_real_num_tx_queues() (if queue count changes), so calls to bnxt_open() still require rtnl_lock.

net/sched/sch_generic.c:1416 suspicious rcu_dereference_protected() usage!

   dev_qdisc_change_real_num_tx+0x54/0xe0
   netif_set_real_num_tx_queues+0x4ed/0xa80
   __bnxt_open_nic+0x9cb/0x3490
   bnxt_open+0x1cb/0x370
   bnxt_fw_reset_task+0x80d/0x1e80
   process_scheduled_works+0x9c1/0x13b0

The reverted commit was just an optimization / experiment so let's go back to taking the lock.

Reported-by Breno Leitao <[email protected]>
Link https://lore.kernel.org/[email protected] open here
Fixes 850d9248d2ea ("Revert "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"")
Signed-off-by Jakub Kicinski <[email protected]>
Quoted 6 lines

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
---

Acked-by Stanislav Fomichev <[email protected]>

(Let's do a full revert for now and we can revisit once we sort out netdev locks for qdisc paths)

maintainer Reviewed-by on v1 1/1
#2 lore raw
In reply to Jakub Kicinski
Quoted 18 lines · Jakub Kicinski

This reverts commit 850d9248d2eac662f869c766a598c877690c74e5. This reapplies commit 325eb217e41f ("bnxt_en: bring back rtnl_lock() in the bnxt_open() path").

Breno reports a lockdep warning in bnxt. During FW reset the driver may end up calling netif_set_real_num_tx_queues() (if queue count changes), so calls to bnxt_open() still require rtnl_lock.

net/sched/sch_generic.c:1416 suspicious rcu_dereference_protected() usage!

   dev_qdisc_change_real_num_tx+0x54/0xe0
   netif_set_real_num_tx_queues+0x4ed/0xa80
   __bnxt_open_nic+0x9cb/0x3490
   bnxt_open+0x1cb/0x370
   bnxt_fw_reset_task+0x80d/0x1e80
   process_scheduled_works+0x9c1/0x13b0
Quoted 7 lines

The reverted commit was just an optimization / experiment so let's go back to taking the lock.

Reported-by Breno Leitao <[email protected]>
Link https://lore.kernel.org/[email protected] open here
Fixes 850d9248d2ea ("Revert "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"")
Signed-off-by Jakub Kicinski <[email protected]>
Reviewed-by Michael Chan <[email protected]>

Confirmed this is a full revert/full re-apply.

Reviewed-by on v1 1/1
#3 lore raw
In reply to Jakub Kicinski
Quoted 17 lines · Jakub Kicinski

This reverts commit 850d9248d2eac662f869c766a598c877690c74e5. This reapplies commit 325eb217e41f ("bnxt_en: bring back rtnl_lock() in the bnxt_open() path").

Breno reports a lockdep warning in bnxt. During FW reset the driver may end up calling netif_set_real_num_tx_queues() (if queue count changes), so calls to bnxt_open() still require rtnl_lock.

net/sched/sch_generic.c:1416 suspicious rcu_dereference_protected() usage!

   dev_qdisc_change_real_num_tx+0x54/0xe0
   netif_set_real_num_tx_queues+0x4ed/0xa80
   __bnxt_open_nic+0x9cb/0x3490
   bnxt_open+0x1cb/0x370
   bnxt_fw_reset_task+0x80d/0x1e80
   process_scheduled_works+0x9c1/0x13b0
Quoted 7 lines

The reverted commit was just an optimization / experiment so let's go back to taking the lock.

Reported-by Breno Leitao <[email protected]>
Link https://lore.kernel.org/[email protected] open here
Fixes 850d9248d2ea ("Revert "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"")
Signed-off-by Jakub Kicinski <[email protected]>
Reviewed-by Breno Leitao <[email protected]>
CI Applied
#4 lore raw
In reply to Jakub Kicinski
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 9 lines · On Wed, 3 Jun 2026 12:58:45 -0700 you wrote:

This reverts commit 850d9248d2eac662f869c766a598c877690c74e5. This reapplies commit 325eb217e41f ("bnxt_en: bring back rtnl_lock() in the bnxt_open() path").

Breno reports a lockdep warning in bnxt. During FW reset the driver may end up calling netif_set_real_num_tx_queues() (if queue count changes), so calls to bnxt_open() still require rtnl_lock.

[...]

Here is the summary with links:
- [net] Reapply "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"

    https://git.kernel.org/netdev/net/c/b6197b386677

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Reviewed-by on v1 1/1
#5 lore raw
In reply to Jakub Kicinski
Quoted 47 lines

-----Original Message-----
From: Jakub Kicinski <[email protected]>
Sent: Wednesday, June 3, 2026 9:59 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Jakub Kicinski <[email protected]>; Breno Leitao <[email protected]>; [email protected]; [email protected]; [email protected]; Loktionov, Aleksandr <[email protected]>
Subject: [PATCH net] Reapply "bnxt_en: bring back rtnl_lock() in the bnxt_open() path"

This reverts commit 850d9248d2eac662f869c766a598c877690c74e5. This reapplies commit 325eb217e41f ("bnxt_en: bring back rtnl_lock() in the bnxt_open() path").

Breno reports a lockdep warning in bnxt. During FW reset the driver may end up calling netif_set_real_num_tx_queues() (if queue count changes), so calls to bnxt_open() still require rtnl_lock.

net/sched/sch_generic.c:1416 suspicious rcu_dereference_protected()
usage!

   dev_qdisc_change_real_num_tx+0x54/0xe0
   netif_set_real_num_tx_queues+0x4ed/0xa80
   __bnxt_open_nic+0x9cb/0x3490
   bnxt_open+0x1cb/0x370
   bnxt_fw_reset_task+0x80d/0x1e80
   process_scheduled_works+0x9c1/0x13b0

The reverted commit was just an optimization / experiment so let's go back to taking the lock.

Reported-by Breno Leitao <[email protected]>
Link https://lore.kernel.org/[email protected] open here
Fixes 850d9248d2ea ("Revert "bnxt_en: bring back rtnl_lock() in the

bnxt_open() path"")

Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 36 ++++++++++++++++++----

...

Quoted 5 lines
drivers/net/ethernet/broadcom/bnxt/bnxt.c +0 -1
 	if (!err) {
 		bnxt_ulp_start(bp);
 		bnxt_reenable_sriov(bp);
--

2.54.0

Reviewed-by Aleksandr Loktionov <[email protected]>