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

[Bryan Cochrane]

On Tue, 3 Nov 2026, <netdev-bot> wrote:
> - [High] New deadlock in bnxt_fw_reset().
[...]
> CPU1
>       rtnl_lock();
>       netdev_lock(bp->dev);
>       netif_close(bp->dev)
>         bnxt_close()->bnxt_close_nic()->__bnxt_close_nic()
>           while (bnxt_drv_busy(bp))
>               msleep(20);
>
> bnxt_drv_busy() tests BNXT_STATE_IN_SP_TASK. Normally only the sp_task
> clears that bit, and the sp_task is the thread stuck in this loop.
>
> Would this spin forever with rtnl_mutex and the instance lock held?

Yes, this is a valid finding, thanks. bnxt_fw_recover() can enter
bnxt_fw_reset() from the devlink health recover path as soon as the
reporter is in the error state, and bnxt_unlock_sp() then sets
BNXT_STATE_IN_SP_TASK although the caller is not the sp_task. Before v2
the instance lock was held across netif_close(), so this could not
interleave; the unlock/relock window in v2 makes it reachable, and the
thread that would spin in __bnxt_close_nic() is the sp_task itself, so
nothing would ever clear the bit.

v3 will not drop and retake the locks in bnxt_fw_reset() at all. The
bnxt_get_registered_vfs() failure sets fw_reset_state to
BNXT_FW_RESET_STATE_ABORT and queues bnxt_fw_reset_task(), which then
performs the abort through the fw_reset_abort path that already takes
RTNL before the instance lock. bnxt_fw_reset() keeps the instance lock
from bnxt_lock_sp() to bnxt_unlock_sp() as before, with no netif_close()
in it.

The unconditional set_bit(BNXT_STATE_IN_SP_TASK) in bnxt_unlock_sp()
when bnxt_fw_reset()/bnxt_fw_exception() are entered via devlink is
pre-existing and not specific to this fix; I'd rather address it in a
separate patch.

> [Severity: Medium]
> This is a pre-existing issue, but this path returns without calling
> bnxt_ulp_start(). Nothing undoes the bnxt_ulp_stop() at the top of
> bnxt_fw_reset(). Does this leave the RoCE/aux driver suspended after the
> abort?
[...]
> Since this path is being rewritten anyway, could the ULP be restarted here
> too?

Agreed. With the abort routed through bnxt_fw_reset_task() in v3, rc is
0 at the ulp_start label and bnxt_ulp_start() runs, the same as for the
POLL_VF abort.

> [Severity: High]
> This isn't a bug introduced by this patch, but commit 004b5008016a seems
> to have left one more netif_close() call without RTNL that this patch does
> not cover. bnxt_vf_reps_destroy() holds only the instance lock:
[...]
> If this patch is backported to stable on its own, this site from the same
> Fixes commit would still be there.

Agreed that bnxt_open_nic() -> netif_close() under bnxt_vf_reps_destroy()
has the same problem, but it is a different call path (devlink eswitch
mode change and SR-IOV disable) with its own locking context, so I'd
prefer to keep it out of this fix and handle it separately. The two
fixes can be backported independently.

> [Severity: High]
> This is also a pre-existing issue, but the same netif_close() without RTNL
> looks reachable through the TPH queue restart path. bnxt_tph_update()
> runs from bnxt_sp_task() under bnxt_lock_sp(), which takes only the
> instance lock:
[...]
> Can this trigger the same ASSERT_RTNL() warnings when the HWRM calls fail
> on unhealthy firmware?

Also pre-existing and known: the netif_close() in the bnxt_queue_start()
error path was added by commit 325eb217e41f ("bnxt_en: bring back
rtnl_lock() in the bnxt_open() path") precisely because that path does
not hold RTNL. It is outside the scope of this patch.

pw-bot: cr


On Tue, 3 Nov 2026 at 20:25, <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (2) · Medium: 0 (1) · 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] New deadlock in bnxt_fw_reset().
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the same Fixes
>   commit (004b5008016a) left another netif_close()-without-RTNL site…
> - [High] This problem wasn't introduced by this patch, but the same
>   netif_close()-without-RTNL bug is still reachable through the TPH…
> - [Medium] This problem wasn't introduced by this patch, but the
>   bnxt_get_registered_vfs() failure path in bnxt_fw_reset() leaves the…
>
> --
>
> bnxt_en: Take RTNL before netif_close() when aborting FW reset
>
> This patch takes RTNL before the netdev instance lock on the FW reset
> abort paths in bnxt_fw_reset_task(). It does the same on the
> bnxt_get_registered_vfs() failure path in bnxt_fw_reset(), so that
> netif_close() runs with RTNL held. The goal is to fix the ASSERT_RTNL()
> warnings introduced by commit 004b5008016a ("eth: bnxt: remove most
> dependencies on RTNL").
>
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 33c4cacc8ffe3..1ea1e8266eabf 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -14860,9 +14860,16 @@ void bnxt_fw_reset(struct bnxt *bp)
> >               if (n < 0) {
> >                       netdev_err(bp->dev, "Firmware reset aborted, rc = %d\n",
> >                                  n);
> > +                     /* netif_close() requires RTNL, which must be taken
> > +                      * before the netdev instance lock.
> > +                      */
> > +                     netdev_unlock(bp->dev);
> > +                     rtnl_lock();
> > +                     netdev_lock(bp->dev);
>
> [Severity: High]
> Can a concurrent devlink health recover hang the sp_task here while it
> holds RTNL?
>
> bnxt_sp_task() is not the only caller of bnxt_fw_reset(). When
> devlink_health_report() returns -ECANCELED,
> bnxt_devlink_health_fw_report() calls __bnxt_fw_recover() from the sp_task
> without the devlink lock.
>
> At that point the reporter is already in the error state. A userspace
> "devlink health recover" can then reach bnxt_fw_reset() through
> bnxt_fw_recover() at the same time.
>
> Before this patch the instance lock was held all the way through
> netif_close(). The new unlock/relock window seems to allow this ordering:
>
> CPU1 (bnxt_sp_task)
> bnxt_devlink_health_fw_report()
>   __bnxt_fw_recover()
>     bnxt_fw_reset()
>       bnxt_lock_sp()          /* clears IN_SP_TASK */
>       set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
>       bnxt_get_registered_vfs() returns < 0
>       netdev_unlock(bp->dev);
>
> CPU2 (devlink health recover)
> devlink_health_reporter_recover()
>   bnxt_fw_recover()
>     __bnxt_fw_recover()
>       bnxt_fw_reset()
>         bnxt_lock_sp()        /* now gets the instance lock */
>         IN_FW_RESET is set, nothing to do
>         bnxt_unlock_sp()
>           set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
>           netdev_unlock(bp->dev);
>
> CPU1
>       rtnl_lock();
>       netdev_lock(bp->dev);
>       netif_close(bp->dev)
>         bnxt_close()->bnxt_close_nic()->__bnxt_close_nic()
>           while (bnxt_drv_busy(bp))
>               msleep(20);
>
> bnxt_drv_busy() tests BNXT_STATE_IN_SP_TASK. Normally only the sp_task
> clears that bit, and the sp_task is the thread stuck in this loop.
>
> Would this spin forever with rtnl_mutex and the instance lock held?
>
> CPU2 can wait on netdev_lock for the whole time the func_qcfg HWRM call is
> timing out, so the window may be wide.
>
> The commit message says:
>
>     BNXT_STATE_IN_FW_RESET stays set across the lock switch, so a
>     concurrent close marks the reset aborted as it does today and a
>     concurrent recovery request backs off.
>
> The back-off path goes through bnxt_unlock_sp(). Isn't that the code that
> sets the bit causing the hang?
>
> It is older behavior that bnxt_unlock_sp() and bnxt_rtnl_unlock_sp() set
> IN_SP_TASK no matter who the caller is. The comment in bnxt_lock_sp()
> assumes the caller is always bnxt_sp_task(). The new window is what makes
> the problem reachable.
>
> >                       clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
> >                       netif_close(bp->dev);
> > -                     goto fw_reset_exit;
> > +                     bnxt_rtnl_unlock_sp(bp);
> > +                     return;
>
> [Severity: Medium]
> This is a pre-existing issue, but this path returns without calling
> bnxt_ulp_start(). Nothing undoes the bnxt_ulp_stop() at the top of
> bnxt_fw_reset(). Does this leave the RoCE/aux driver suspended after the
> abort?
>
> The old goto fw_reset_exit had the same problem. Neither bnxt_close() nor
> a later bnxt_open() resumes the ULP.
>
> bnxt_hwrm_if_change() only queues BNXT_RESTART_ULP_SP_EVENT when
> BNXT_STATE_FW_RESET_DET is set. That won't happen if the func_qcfg failure
> was transient.
>
> The matching abort for the POLL_VF state in bnxt_fw_reset_task() reaches
> ulp_start with rc == 0, so it does restart the ULP:
>
> ulp_start:
>         if (!rc)
>                 bnxt_ulp_start(bp);
>
> Since this path is being rewritten anyway, could the ULP be restarted here
> too?
>
> >               } else if (n > 0) {
> >                       u16 vf_tmo_dsecs = n * 10;
> >
>
> [ ... ]
>
> > @@ -15682,9 +15688,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();
>
> [Severity: High]
> This isn't a bug introduced by this patch, but commit 004b5008016a seems
> to have left one more netif_close() call without RTNL that this patch does
> not cover. bnxt_vf_reps_destroy() holds only the instance lock:
>
> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:bnxt_vf_reps_destroy() {
>     ...
>         netdev_lock(bp->dev);
>         if (netif_running(bp->dev)) {
>                 bnxt_close_nic(bp, false, false);
>                 closed = true;
>         }
>     ...
>                 bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>                 bnxt_open_nic(bp, false, false);
>     ...
> }
>
> bnxt_open_nic() calls netif_close() on any failure. That includes the
> immediate -EIO when BNXT_STATE_ABORT_ERR is set:
>
>         if (rc) {
>                 netdev_err(bp->dev, "nic open fail (rc: %x)\n", rc);
>                 netif_close(bp->dev);
>         }
>
> There are two callers, and both hold only the devlink instance lock:
>
> - bnxt_dl_eswitch_mode_set() for DEVLINK_ESWITCH_MODE_LEGACY
> - __bnxt_sriov_disable()
>
> Wouldn't this hit the same ASSERT_RTNL() in __dev_close_many()? Wouldn't
> it also run the NETDEV_GOING_DOWN/NETDEV_DOWN notifiers without RTNL?
>
> If this patch is backported to stable on its own, this site from the same
> Fixes commit would still be there.
>
> [Severity: High]
> This is also a pre-existing issue, but the same netif_close() without RTNL
> looks reachable through the TPH queue restart path. bnxt_tph_update()
> runs from bnxt_sp_task() under bnxt_lock_sp(), which takes only the
> instance lock:
>
> bnxt_sp_task()
>   bnxt_tph_update()
>     bnxt_lock_sp()
>     netdev_rx_queue_restart()
>       netdev_rx_queue_reconfig()   /* only asserts the instance lock */
>         bnxt_queue_start()
>
> bnxt_queue_start() closes the device on its error path. This happens if
> any of these fail: bnxt_hwrm_rx_ring_alloc(),
> bnxt_hwrm_cp_ring_alloc_p5(), bnxt_hwrm_rx_agg_ring_alloc() or
> bnxt_tx_queue_start().
>
> err_reset:
>         netdev_err(bp->dev, "Unexpected HWRM error during queue start rc: %d\n",
>                    rc);
>         napi_enable_locked(&bnapi->napi);
>         bnxt_db_nq_arm(bp, &cpr->cp_db, cpr->cp_raw_cons);
>         netif_close(dev);
>
> Can this trigger the same ASSERT_RTNL() warnings when the HWRM calls fail
> on unhealthy firmware?
>
> >  ulp_start:
> >       if (!rc)
> >               bnxt_ulp_start(bp);
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103042210.2984373-1-bcochrane%40google.com