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

[PATCH net v3] 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.

bnxt_fw_reset() cannot take RTNL in its bnxt_get_registered_vfs() error
path because it already holds the instance lock, and dropping that lock
to reorder would let a concurrent devlink health recover enter
bnxt_fw_reset() and set BNXT_STATE_IN_SP_TASK through bnxt_unlock_sp(),
which the subsequent close would then wait on forever. Instead, mark
the reset as aborted and let bnxt_fw_reset_task() perform the abort via
the same fw_reset_abort path. The normal reset flow, which may run on
many VFs at the same time, keeps running without RTNL, and the ULP is
restarted after this abort like after the other aborts.

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>
---
v3:
 - 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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 33c4cacc8ffe..f48e67670ccf 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14860,8 +14860,8 @@ void bnxt_fw_reset(struct bnxt *bp)
 		if (n < 0) {
 			netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
 				   n);
-			clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
-			netif_close(bp->dev);
+			bp->fw_reset_state = BNXT_FW_RESET_STATE_ABORT;
+			bnxt_queue_fw_reset_work(bp, 0);
 			goto fw_reset_exit;
 		} else if (n > 0) {
 			u16 vf_tmo_dsecs = n * 10;
@@ -15524,6 +15524,8 @@ static void bnxt_fw_reset_task(struct work_struct *work)
 	}
 
 	switch (bp->fw_reset_state) {
+	case BNXT_FW_RESET_STATE_ABORT:
+		goto fw_reset_abort;
 	case BNXT_FW_RESET_STATE_POLL_VF: {
 		int n = bnxt_get_registered_vfs(bp);
 		int tmo;
@@ -15547,9 +15549,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 +15683,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: 787268686c64be60023743b527645a907e38ccca
-- 
2.56.0.398.g882cbad275-goog

Re: [PATCH net v3] 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: 2 · Low: 0

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] Permanent hang while holding RTNL and the netdev instance lock.
- [Medium] The patch defers the bnxt_get_registered_vfs() failure in
  bnxt_fw_reset() to bnxt_fw_reset_task().
- [Medium] The patch turns a synchronous abort into an asynchronous
  request carried only in bp->fw_reset_state.

--

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

This patch takes RTNL around bnxt_fw_reset_abort() in bnxt_fw_reset_task()
and sends the POLL_VF ABORT_ERR case through that same path. It also
changes the bnxt_get_registered_vfs() failure path in bnxt_fw_reset(): that
path now sets fw_reset_state to BNXT_FW_RESET_STATE_ABORT and leaves the
netif_close() to bnxt_fw_reset_task().

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 33c4cacc8ffe3..f48e67670ccfb 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14860,8 +14860,8 @@ void bnxt_fw_reset(struct bnxt *bp)
>  		if (n < 0) {
>  			netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
>  				   n);
> -			clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
> -			netif_close(bp->dev);
> +			bp->fw_reset_state = BNXT_FW_RESET_STATE_ABORT;
> +			bnxt_queue_fw_reset_work(bp, 0);
>  			goto fw_reset_exit;

[Severity: High]
Can this hang forever, holding rtnl_mutex and the instance lock, when
bnxt_fw_reset() is entered from devlink health recover?

bnxt_fw_recover() runs in devlink netlink process context, not from
bnxt_sp_task(). With BNXT_STATE_FW_NON_FATAL_COND set, the path is:

bnxt_fw_recover()
  __bnxt_fw_recover()
    bnxt_fw_reset()
      bnxt_lock_sp()              /* clears BNXT_STATE_IN_SP_TASK */
      bnxt_get_registered_vfs()   /* returns < 0 */
      bnxt_queue_fw_reset_work(bp, 0)
      bnxt_unlock_sp()            /* set_bit(BNXT_STATE_IN_SP_TASK) */

No sp_task is running, so nothing clears BNXT_STATE_IN_SP_TASK after this.

fw_reset_task then runs on bnxt_pf_wq:

bnxt_fw_reset_task()
  rtnl_lock(); netdev_lock(bp->dev);
  bnxt_fw_reset_abort()
    netif_close()
      bnxt_close()
        bnxt_close_nic()
          __bnxt_close_nic()
            while (bnxt_drv_busy(bp))
                msleep(20);

bnxt_drv_busy() checks BNXT_STATE_IN_SP_TASK, and this loop has no
timeout. For a PF, bnxt_sp_task() goes on the same ordered bnxt_pf_wq, so
it can't run behind the blocked fw_reset_task and clear the bit.

A second devlink health recover can't break the loop either.
bnxt_fw_reset_abort() has already set SEVERITY_FATAL before calling
netif_close(), so bnxt_fw_recover() returns -ENODEV.

Before this patch, the same branch called netif_close() inline. At that
point bnxt_lock_sp() had cleared BNXT_STATE_IN_SP_TASK, so the busy loop
exited.

The commit message says that dropping the lock would let "a concurrent
devlink health recover enter bnxt_fw_reset() and set BNXT_STATE_IN_SP_TASK
through bnxt_unlock_sp(), which the subsequent close would then wait on
forever".

Can the same hang still happen when the devlink health recover caller is
itself the bnxt_fw_reset() call that defers the close?

[Severity: Medium]
Is the deferred abort safe against a concurrent bnxt_close()?

After this change, bnxt_fw_reset() drops the instance lock while the
netdev is still open and BNXT_STATE_IN_FW_RESET is set. The pending abort
is recorded only in bp->fw_reset_state.

bnxt_hwrm_if_change(), called from bnxt_close(), always reads and resets
that state:

bnxt_hwrm_if_change() {
    ...
	fw_reset = (bp->fw_reset_state == BNXT_FW_RESET_STATE_ABORT);
	bp->fw_reset_state = 0;
    ...
}

A user close (ip link set down) can run before fw_reset_task reads
fw_reset_state. For example, work for other PFs may be queued ahead of it
on the single-threaded bnxt_pf_wq. The close path then sets
BNXT_STATE_ABORT_ERR and resets fw_reset_state to 0.

When bnxt_fw_reset_task() runs, it sees BNXT_STATE_IN_FW_RESET set and
fw_reset_state == 0. No case in the switch matches, and there is no
default, so it returns. bnxt_fw_reset_abort() never runs, and
BNXT_STATE_IN_FW_RESET is never cleared.

After that, bnxt_open() -> bnxt_reinit_after_abort() fails with:

	if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
		return -EBUSY;

As a result, the port can't be opened, and later firmware resets and
health checks skip it. The stopped ULP is also never restarted, until the
driver is reloaded.

The other in-progress reset states such as POLL_VF have a similar
exposure. This path, however, used to be fully synchronous.

>  		} else if (n > 0) {

[ ... ]

> @@ -15524,6 +15524,8 @@ static void bnxt_fw_reset_task(struct work_struct *work)
>  	}
>  
>  	switch (bp->fw_reset_state) {
> +	case BNXT_FW_RESET_STATE_ABORT:
> +		goto fw_reset_abort;

[Severity: Medium]
With bnxt_fw_reset() now setting fw_reset_state to
BNXT_FW_RESET_STATE_ABORT before this runs, will bnxt_fw_reset_abort()
report a fatal firmware error for a func_qcfg failure?

bnxt_fw_reset_abort() {
	clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
	if (bp->fw_reset_state != BNXT_FW_RESET_STATE_POLL_VF)
		bnxt_dl_health_fw_status_update(bp, false);
    ...
}

The POLL_VF exemption no longer applies on this path. As a result,
bnxt_dl_health_fw_status_update(bp, false) sets SEVERITY_FATAL and
REMEDY_POWER_CYCLE_DEVICE, and moves the devlink fw reporter to
DEVLINK_HEALTH_REPORTER_STATE_ERROR.

This is the same kind of failure as the POLL_VF n < 0 abort in
bnxt_fw_reset_task(): a bnxt_hwrm_func_qcfg() failure before any reset
step has run. That path skips the health update, and so did the old
inline abort in bnxt_fw_reset().

Once severity is SEVERITY_FATAL, bnxt_fw_recover() refuses manual
recovery until a new firmware event resets the severity:

	if (bp->fw_health->severity == SEVERITY_FATAL)
		return -ENODEV;

There is also a secondary effect. bnxt_dl_health_fw_status_update() calls
devlink_health_reporter_state_update(fw_health->fw_reporter, state)
without checking fw_reporter for NULL.

__bnxt_dl_reporter_create() can leave fw_reporter NULL, and
__bnxt_fw_recover() can still reach bnxt_fw_reset() in that case. The
other abort paths already carry this NULL dereference risk, and this
patch adds another way to reach it.

The commit message doesn't mention this behavior change. Was it intended?

>  	case BNXT_FW_RESET_STATE_POLL_VF: {
>  		int n = bnxt_get_registered_vfs(bp);
>  		int tmo;

[ ... ]

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