[PATCH net v4] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[PATCH net v4] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[Bryan Cochrane]

netif_close() must be called with RTNL held in addition to the netdev
instance lock: __dev_close_many() asserts it, and the NETDEV_GOING_DOWN
and NETDEV_DOWN notifier chains it runs rely on it for serialization.

Commit 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
replaced rtnl_lock() with netdev_lock() in bnxt_fw_reset_task() and
bnxt_fw_reset(). That is sufficient for the paths that only tear the
NIC down internally with __bnxt_close_nic(), but the abort paths close
the netdev via bnxt_fw_reset_abort() -> netif_close(), and the error
path in bnxt_fw_reset() calls netif_close() directly. Those now run
without RTNL and trigger a WARN_ONCE() cascade when a firmware reset is
aborted:

  RTNL: assertion failed at net/core/dev.c (1740)
  WARNING: net/core/dev.c:1740 at __dev_close_many+0x1be/0x1f0
  Workqueue: bnxt_pf_wq bnxt_fw_reset_task [bnxt_en]
  Call Trace:
   netif_close_many+0xb5/0x190
   netif_close+0xa2/0xd0
   bnxt_fw_reset_task.cold+0x8d/0x213 [bnxt_en]
   process_one_work+0x19d/0x3a0
   worker_thread+0x1af/0x320
   kthread+0xe3/0x120

followed by the same assertion from call_netdevice_notifiers_info(),
fib_rules_event(), inetdev_event(), rtnl_fill_ifinfo(),
devlink_compat_phys_port_name_get() and ip_mc_down().

This was observed on a machine in real use with a BCM57504 (4 PFs,
FW 236.1.153.0) running v7.1.6: the firmware crashed, announced a
"Fatal firmware reset event" and then failed to come back within the
advertised max wait, so bnxt_fw_reset_task() aborted the reset on all
ports:

  bnxt_en 0000:c1:00.1 eno16905np1: Firmware reset aborted
  bnxt_en 0000:c1:00.1 eno16905np1: fw_health_status 0x448100

Take RTNL (before the netdev instance lock, matching the OPENING state
which already uses rtnl_trylock()) in the fw_reset_abort path of
bnxt_fw_reset_task() and route the POLL_VF abort through that same
path.

In bnxt_fw_reset(), only the bnxt_get_registered_vfs() error path needs
RTNL: drop the instance lock, reacquire RTNL and the instance lock via
bnxt_rtnl_lock_sp(), then clear BNXT_STATE_IN_FW_RESET and close the
device synchronously. The normal reset flow, which may run on many VFs
at the same time, keeps running without RTNL.

Because bnxt_fw_reset() and bnxt_fw_exception() can also be entered
from devlink process context via bnxt_fw_recover(), guard the
BNXT_STATE_IN_SP_TASK bit manipulation in bnxt_[rtnl_]lock_sp() and
bnxt_[rtnl_]unlock_sp() with current_work() == &bp->sp_task. Otherwise
a concurrent devlink health recover backing off during the unlock
window would set BNXT_STATE_IN_SP_TASK with no sp_task running to clear
it, causing the subsequent __bnxt_close_nic() busy-wait to hang.

The bug was found by inspecting the kernel log of the affected machine
and the driver source; the fix was prepared with the help of an AI
coding assistant (Gemini based) and reviewed by the submitter. It is
build tested (allmodconfig with W=1, plus the same change on top of
v7.1.6 built as a module for the affected BCM57504 host). The aborted
reset path itself only runs when the firmware fails to return from a
reset, which could not be reproduced on demand.

Fixes: 004b5008016a ("eth: bnxt: remove most dependencies on RTNL")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan Cochrane <bcochrane@google.com>
---
v4:
 - Return to the synchronous close in bnxt_fw_reset() from v2, using
   netdev_unlock() + bnxt_rtnl_lock_sp() as suggested by Michael Chan,
   and guard BNXT_STATE_IN_SP_TASK in bnxt_[rtnl_]lock_sp() and
   bnxt_[rtnl_]unlock_sp() with current_work() == &bp->sp_task. Deferring
   the close to bnxt_fw_reset_task() in v3 still hung when entered via
   bnxt_fw_recover(), raced with bnxt_hwrm_if_change() clearing
   fw_reset_state, and marked the reporter SEVERITY_FATAL (reported by
   the netdev Sashiko review).
 - Omitted Michael's Reviewed-by and Stanislav's Acked-by from v2 due to
   the new bnxt_in_sp_task() check.
v3: https://lore.kernel.org/netdev/20261104045534.79783-1-bcochrane@google.com/
 - Do not drop and retake the locks in bnxt_fw_reset(). Route the
   bnxt_get_registered_vfs() failure through the bnxt_fw_reset_task()
   abort path instead; this also restarts the ULP after that abort. The
   v2 unlock/relock window let a concurrent devlink health recover set
   BNXT_STATE_IN_SP_TASK via bnxt_unlock_sp() and hang the close
   (reported by the netdev Sashiko review).
v2: https://lore.kernel.org/netdev/20261103042210.2984373-1-bcochrane@google.com/
 - Keep bnxt_lock_sp() in bnxt_fw_reset() and take RTNL only on the
   bnxt_get_registered_vfs() error path, to avoid RTNL contention when
   many VFs recover at the same time (Michael Chan).
 - Dropped Stanislav's Acked-by because of the change above.
v1: https://lore.kernel.org/netdev/20261029032832.1511180-1-bcochrane@google.com/
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 31 ++++++++++++++++-------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 33c4cacc8ffe..7a2b96f56464 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14575,35 +14575,44 @@ static void bnxt_timer(struct timer_list *t)
 	mod_timer(&bp->timer, jiffies + bp->current_interval);
 }
 
+static bool bnxt_in_sp_task(struct bnxt *bp)
+{
+	return current_work() == &bp->sp_task;
+}
+
 static void bnxt_lock_sp(struct bnxt *bp)
 {
-	/* We are called from bnxt_sp_task which has BNXT_STATE_IN_SP_TASK
-	 * set.  If the device is being closed, bnxt_close() may be holding
+	/* If we are called from bnxt_sp_task, BNXT_STATE_IN_SP_TASK is set.
+	 * If the device is being closed, bnxt_close() may be holding
 	 * netdev instance lock and waiting for BNXT_STATE_IN_SP_TASK to clear.
 	 * So we must clear BNXT_STATE_IN_SP_TASK before holding netdev
 	 * instance lock.
 	 */
-	clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
+	if (bnxt_in_sp_task(bp))
+		clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
 	netdev_lock(bp->dev);
 }
 
 static void bnxt_unlock_sp(struct bnxt *bp)
 {
-	set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
+	if (bnxt_in_sp_task(bp))
+		set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
 	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);
+	if (bnxt_in_sp_task(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);
+	if (bnxt_in_sp_task(bp))
+		set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
 	netdev_unlock(bp->dev);
 	rtnl_unlock();
 }
@@ -14860,9 +14869,12 @@ void bnxt_fw_reset(struct bnxt *bp)
 		if (n < 0) {
 			netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
 				   n);
+			netdev_unlock(bp->dev);
+			bnxt_rtnl_lock_sp(bp);
 			clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
 			netif_close(bp->dev);
-			goto fw_reset_exit;
+			bnxt_rtnl_unlock_sp(bp);
+			return;
 		} else if (n > 0) {
 			u16 vf_tmo_dsecs = n * 10;
 
@@ -15547,9 +15559,8 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 		bp->fw_reset_timestamp = jiffies;
 		netdev_lock(bp->dev);
 		if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) {
-			bnxt_fw_reset_abort(bp, rc);
 			netdev_unlock(bp->dev);
-			goto ulp_start;
+			goto fw_reset_abort;
 		}
 		bnxt_fw_reset_close(bp);
 		if (bp->fw_cap & BNXT_FW_CAP_ERR_RECOVER_RELOAD) {
@@ -15682,9 +15693,11 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 		netdev_err(bp->dev, "fw_health_status 0x%x\n", sts);
 	}
 fw_reset_abort:
+	rtnl_lock();
 	netdev_lock(bp->dev);
 	bnxt_fw_reset_abort(bp, rc);
 	netdev_unlock(bp->dev);
+	rtnl_unlock();
 ulp_start:
 	if (!rc)
 		bnxt_ulp_start(bp);

base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
-- 
2.56.0.542.g3ef89b7020-goog

Re: [PATCH net v4] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[Michael Chan]

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

On Sun, Nov 8, 2026 at 2:29 PM Bryan Cochrane <bcochrane@google.com> wrote:
> v4:
>  - Return to the synchronous close in bnxt_fw_reset() from v2, using
>    netdev_unlock() + bnxt_rtnl_lock_sp() as suggested by Michael Chan,
>    and guard BNXT_STATE_IN_SP_TASK in bnxt_[rtnl_]lock_sp() and
>    bnxt_[rtnl_]unlock_sp() with current_work() == &bp->sp_task. Deferring
>    the close to bnxt_fw_reset_task() in v3 still hung when entered via
>    bnxt_fw_recover(), raced with bnxt_hwrm_if_change() clearing
>    fw_reset_state, and marked the reporter SEVERITY_FATAL (reported by
>    the netdev Sashiko review).

v4 looks good to me.  Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

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

Re: [PATCH net v4] bnxt_en: Take RTNL before netif_close() when aborting FW reset

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds a bnxt_in_sp_task() guard to
  bnxt_lock_sp()/bnxt_rtnl_lock_sp().
- [Medium] bnxt_en: transient BNXT_STATE_IN_FW_RESET exposed in
  bnxt_fw_reset() unlock window
- [Low] The commit message understates what the bnxt_in_sp_task() guard
  fixes, and the Fixes: tag scope does not match it.

--

bnxt_en: Take RTNL before netif_close() when aborting FW reset

This patch takes RTNL before netif_close() in two places: the firmware
reset abort paths of bnxt_fw_reset_task(), and the
bnxt_get_registered_vfs() error path of bnxt_fw_reset(). It also adds a
current_work() == &bp->sp_task check around the BNXT_STATE_IN_SP_TASK
updates in bnxt_[rtnl_]lock_sp() and bnxt_[rtnl_]unlock_sp(), so callers
running in devlink context leave the bit alone.

> Because bnxt_fw_reset() and bnxt_fw_exception() can also be entered
> from devlink process context via bnxt_fw_recover(), guard the
> BNXT_STATE_IN_SP_TASK bit manipulation in bnxt_[rtnl_]lock_sp() and
> bnxt_[rtnl_]unlock_sp() with current_work() == &bp->sp_task. Otherwise
> a concurrent devlink health recover backing off during the unlock
> window would set BNXT_STATE_IN_SP_TASK with no sp_task running to clear
> it, causing the subsequent __bnxt_close_nic() busy-wait to hang.

[Severity: Low]
Does this paragraph describe everything the guard fixes? Before this
patch, every devlink health recover went through:

bnxt_fw_recover()->__bnxt_fw_recover()->bnxt_fw_reset()->bnxt_unlock_sp()

bnxt_unlock_sp() did an unconditional set_bit(BNXT_STATE_IN_SP_TASK),
and no sp_task was left to clear it. No unlock window was needed for
this.

If the reset was then aborted, the following chain would spin forever
in the bnxt_drv_busy() loop:

bnxt_fw_reset_abort()->netif_close()->bnxt_close()->__bnxt_close_nic()

Before 004b5008016a, bnxt_rtnl_lock_sp() and bnxt_rtnl_unlock_sp()
already did the same unconditional clear_bit()/set_bit(). So this hang
looks older than the commit named in the Fixes: tag.

Could this part be described separately, or split out with its own
Fixes: tag so that older stable trees get it too?

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 33c4cacc8ffe3..7a2b96f564647 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14575,35 +14575,44 @@ static void bnxt_timer(struct timer_list *t)
>  	mod_timer(&bp->timer, jiffies + bp->current_interval);
>  }
>  
> +static bool bnxt_in_sp_task(struct bnxt *bp)
> +{
> +	return current_work() == &bp->sp_task;
> +}
> +
>  static void bnxt_lock_sp(struct bnxt *bp)
>  {
> -	/* We are called from bnxt_sp_task which has BNXT_STATE_IN_SP_TASK
> -	 * set.  If the device is being closed, bnxt_close() may be holding
> +	/* If we are called from bnxt_sp_task, BNXT_STATE_IN_SP_TASK is set.
> +	 * If the device is being closed, bnxt_close() may be holding
>  	 * netdev instance lock and waiting for BNXT_STATE_IN_SP_TASK to clear.
>  	 * So we must clear BNXT_STATE_IN_SP_TASK before holding netdev
>  	 * instance lock.
>  	 */
> -	clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
> +	if (bnxt_in_sp_task(bp))
> +		clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
>  	netdev_lock(bp->dev);
>  }

[Severity: High]
Can this guard cause a deadlock between devl_lock and the
BNXT_STATE_IN_SP_TASK busy-wait in __bnxt_close_nic()?

Before this patch, a devlink-initiated recover always cleared
BNXT_STATE_IN_SP_TASK in bnxt_lock_sp(), so it never waited on a running
sp_task. With the guard, a caller in devlink process context leaves the
bit set and does wait for a concurrent sp_task. The same applies to
bnxt_rtnl_lock_sp().

Thread A runs devlink health recover. The HEALTH_REPORTER_RECOVER doit
already holds devl_lock:

devlink_health_reporter_recover()
  bnxt_fw_recover()
    __bnxt_fw_recover()
      bnxt_fw_reset() or bnxt_fw_exception()
        bnxt_lock_sp()        <- BNXT_STATE_IN_SP_TASK left alone
        ...
        __bnxt_close_nic()
            clear_bit(BNXT_STATE_OPEN, &bp->state);
            smp_mb__after_atomic();
            while (bnxt_drv_busy(bp))
                msleep(20);

Thread A reaches __bnxt_close_nic() through bnxt_force_fw_reset() or
bnxt_fw_reset() -> bnxt_fw_reset_close(). It can also get there through
the new n < 0 path in bnxt_fw_reset():
bnxt_rtnl_lock_sp() -> netif_close() -> bnxt_close().

Thread B is bnxt_sp_task on the same bp. It set BNXT_STATE_IN_SP_TASK
and passed the BNXT_STATE_OPEN check before thread A cleared
BNXT_STATE_OPEN:

bnxt_sp_task()
    if (test_and_clear_bit(BNXT_FW_EXCEPTION_SP_EVENT, &bp->sp_event)) {
        if (!is_bnxt_fw_ok(bp))
            bnxt_devlink_health_fw_report(bp);
    }

bnxt_devlink_health_fw_report()->devlink_health_report()
    if (reporter->auto_dump) {
        devl_lock(devlink);   <- blocks, thread A holds it

While the heartbeat is stale, bnxt_timer() -> bnxt_fw_health_check()
queues BNXT_FW_EXCEPTION_SP_EVENT on every tick.
BNXT_FW_RESET_NOTIFY_SP_EVENT can reach the same report call.

auto_dump defaults to true for bnxt because it provides .dump. If the fw
reporter has auto_recover=false, devlink_health_report() goes on to the
auto_dump devl_lock() and blocks there.

Thread A waits for thread B to clear BNXT_STATE_IN_SP_TASK, and thread B
waits for thread A to drop devl_lock. Wouldn't devl_lock and the netdev
instance lock then stay held forever? On the new
bnxt_get_registered_vfs() error path in bnxt_fw_reset(), RTNL would be
stuck as well.

The window looks fairly wide. bnxt_ulp_stop() and the netdev_lock()
acquisition both run between thread A taking devl_lock and thread A
clearing BNXT_STATE_OPEN.

[ ... ]

> @@ -14860,9 +14869,12 @@ void bnxt_fw_reset(struct bnxt *bp)
>  		if (n < 0) {
>  			netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
>  				   n);
> +			netdev_unlock(bp->dev);
> +			bnxt_rtnl_lock_sp(bp);
>  			clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
>  			netif_close(bp->dev);
> -			goto fw_reset_exit;
> +			bnxt_rtnl_unlock_sp(bp);
> +			return;
>  		} else if (n > 0) {
>  			u16 vf_tmo_dsecs = n * 10;
>  

[Severity: Medium]
Does dropping the instance lock here expose a stale
BNXT_STATE_IN_FW_RESET to other threads?

By this point bnxt_fw_reset() has already decided to abort. But
BNXT_STATE_IN_FW_RESET is still set, and no fw_reset_task has been
queued for this reset.

Between netdev_unlock(bp->dev) and the netdev_lock() inside
bnxt_rtnl_lock_sp(), any waiter on the instance lock can get in and act
on that bit. A waiter that already holds RTNL is guaranteed to win,
because bnxt_rtnl_lock_sp() has to take RTNL first.

Several users of the bit look affected.

bnxt_close_nic() called by an RTNL holder, for example an ethtool
reconfiguration or bnxt_close():

    if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
        netdev_warn(bp->dev, "FW reset in progress during close, ...");
        set_bit(BNXT_STATE_ABORT_ERR, &bp->state);
    }

Normally the POLL_VF state of bnxt_fw_reset_task() consumes
BNXT_STATE_ABORT_ERR, but no fw_reset_task will run for this reset.
The bit is left set even though bnxt_fw_reset_close() never ran. A
reconfiguration path that closes and then reopens would then see
bnxt_open_nic() fail with -EIO.

bnxt_fw_exception() -> bnxt_force_fw_reset() from another context:

    if (!test_bit(BNXT_STATE_OPEN, &bp->state) ||
        test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
        return;

A fatal reset requested inside the window is silently skipped.

bnxt_io_error_detected():

    if (test_and_set_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
        netdev_err(bp->dev, "Firmware reset already in progress\n");
        abort = true;
    }

This returns PCI_ERS_RESULT_DISCONNECT. A PCIe error is a plausible
reason for bnxt_hwrm_func_qcfg() to fail inside
bnxt_get_registered_vfs() in the first place, so this race does not
look far-fetched.

Clearing BNXT_STATE_IN_FW_RESET before netdev_unlock() would close
this window. However, it would also let a new reset start before
netif_close() runs.

Would it be acceptable to take RTNL up front in bnxt_fw_reset() when
bp->pf.active_vfs is non-zero, so the instance lock never has to be
dropped? VFs, and PFs without active VFs, never reach this path, so
the RTNL contention concern from v2 should not apply to them.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108222942.1233295-1-bcochrane%40google.com