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

[Bryan Cochrane]

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

On Mon, Nov 9, 2026 <netdev-bot+sashiko@xxxxxxxxxx> wrote:
> - [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.

Thanks, all three points are valid:

1. [High] Guarding clear_bit(BNXT_STATE_IN_SP_TASK) in
   bnxt_[rtnl_]lock_sp() creates an ABBA deadlock when devlink health
   recover runs with devl_lock held (auto_recover=false, auto_dump=true):
   a concurrent bnxt_sp_task entering bnxt_devlink_health_fw_report() ->
   devlink_health_report() blocks on devl_lock with BNXT_STATE_IN_SP_TASK
   set, while __bnxt_close_nic() in the recover path spins on
   bnxt_drv_busy().

2. [Medium] Dropping netdev_lock(bp->dev) before bnxt_rtnl_lock_sp() on
   the bnxt_get_registered_vfs() < 0 path exposes BNXT_STATE_IN_FW_RESET
   even though no fw_reset_task will run, which can leave
   BNXT_STATE_ABORT_ERR set on a concurrent close/reconfig, cause a
   concurrent bnxt_force_fw_reset() to bail out, or make
   bnxt_io_error_detected() return PCI_ERS_RESULT_DISCONNECT.

   Taking RTNL up front in bnxt_fw_reset() avoids dropping the instance
   lock altogether. Rather than testing bp->pf.active_vfs before locking
   (which could race with bnxt_sriov_configure(), since it drops RTNL
   and netdev_lock before bnxt_cfg_hw_sriov() increments active_vfs), v5
   takes bnxt_rtnl_lock_sp() when BNXT_PF(bp) is true and keeps
   bnxt_lock_sp() on VFs. VFs never reach the netif_close() error path
   and never contend on RTNL during recovery, while PFs already run on
   the single-threaded bnxt_pf_wq.

3. [Low] With RTNL taken up front on PFs in bnxt_fw_reset(), there is no
   unlock/relock window, so v5 drops the bnxt_in_sp_task() change
   entirely and keeps this patch scoped to the 004b5008016a RTNL
   regression. The pre-existing BNXT_STATE_IN_SP_TASK behavior when
   bnxt_fw_recover() calls bnxt_unlock_sp() predates 004b5008016a and
   can be addressed separately.

I will send v5 shortly.

pw-bot: cr

On Mon, 9 Nov 2026 at 15:21, <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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