Re: [PATCH net-next v4 5/5] bnge: create NQ0 during probe and keep it active across open/close

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · 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] bnge: race between bnge_napi_poll() and bnge_shutdown_nic() on
  NQ0

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  more callers that hit it.

--

bnge: create NQ0 during probe and keep it active across open/close

This commit creates NQ0 and its MSI-X vector once at probe and keeps them
until remove, so firmware async events and HWRM completions are still
received while the interface is down. While the interface is up NQ0 is
serviced by NAPI, and while it is down it is serviced by a new nq0_work
worker. The BNGE_STATE_NQ0_NAPI bit selects which one is used, and it is
switched in open and stop.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 400ada1023ed6..ab917c0f81b1f 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2963,11 +3057,32 @@ static int bnge_hwrm_if_change(struct bnge_dev *bd, bool up)
>  	return bnge_hwrm_req_send(bd, req);
>  }
>  
> +/* Hand NQ0 from nq0_work to NAPI. The caller keeps NQ0's IRQ disabled; it is
> + * re-enabled here.
> + */
> +static void bnge_nq0_wq_to_napi(struct bnge_net *bn)
> +{
> +	struct bnge_nq_ring_info *nqr = &bn->nq0->nq_ring;
> +	struct bnge_napi *bnapi = bn->nq0;
> +	struct bnge_dev *bd = bn->bd;
> +
> +	bnapi->in_reset = false;
> +	bnapi->tx_fault = 0;
> +	napi_enable_locked(&bnapi->napi);
> +	set_bit(BNGE_STATE_NQ0_NAPI, &bn->state);
> +	BNGE_DB_NQ_ARM(bd, &nqr->nq_db, nqr->nq_raw_cons);
> +	enable_irq(bd->irq_tbl[nqr->ring_struct.map_idx].vector);
> +}

[ ... ]

> @@ -3017,6 +3141,7 @@ static int bnge_open_core(struct bnge_net *bn)
>  	set_bit(BNGE_STATE_STATS_ENABLE, &bn->state);
>  
>  	bnge_enable_int(bn);
> +	bnge_nq0_wq_to_napi(bn);

[Severity: High]
This isn't a bug introduced by this patch, but the patch adds more callers
that run into it. On 32-bit builds, bnge_writeq() takes bd->db_lock with a
plain spin_lock() and leaves BH enabled:

drivers/net/ethernet/broadcom/bnge/bnge.h:bnge_writeq() {
#if BITS_PER_LONG == 32
	spin_lock(&bd->db_lock);
	lo_hi_writeq(val, addr);
	spin_unlock(&bd->db_lock);
    ...
}

bnge_writeq_relaxed() in bnge_txrx.h does the same thing. Softirq code
takes the same lock too: bnge_napi_poll() takes it through BNGE_DB_NQ(),
BNGE_DB_NQ_ARM() and the CQ doorbells, and the xmit path takes it as well.

Here bnge_nq0_wq_to_napi() writes BNGE_DB_NQ_ARM() from process context.
By that point bnge_enable_int() has already armed NQ1..n with their NAPIs
enabled. The close path has the same ordering:

bnge_close_core()
  bnge_nq0_napi_to_wq()
    bnge_nq0_poll()
      BNGE_DB_NQ_ARM()
        bnge_writeq()
  bnge_disable_napi()    <- NAPI for NQ1..n is only disabled here

Can this self-deadlock on a 32-bit kernel? Suppose an interrupt for one of
the other NQs lands on the same CPU while process context holds db_lock.
irq_exit() can then run NET_RX softirq:

process context               same CPU, softirq from irq_exit()
bnge_nq0_wq_to_napi()
  bnge_writeq()
    spin_lock(&bd->db_lock)
                              bnge_napi_poll()
                                BNGE_DB_NQ_ARM()
                                  bnge_writeq()
                                    spin_lock(&bd->db_lock)  <- spins

With lockdep enabled, this should also be reported as an inconsistent
{SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock state once traffic flows after the
first open.

The existing process-context callers bnge_enable_int() and
bnge_disable_int() already followed this pattern before this patch.

Would it make sense to use spin_lock_bh() or spin_lock_irqsave() in
bnge_writeq() and bnge_writeq_relaxed()? Disabling BH around the
process-context doorbell writes would also work.

[ ... ]

> @@ -3243,6 +3402,8 @@ static void bnge_close_core(struct bnge_net *bn)
>  
>  	timer_delete_sync(&bn->timer);
>  	bnge_shutdown_nic(bn);
> +
> +	bnge_nq0_napi_to_wq(bn);
>  	bnge_disable_napi(bn);
>  
>  	/* Save ring stats before shutdown */

[Severity: High]
Does this ordering leave a window where NQ0's NAPI is still running
against rings that firmware is tearing down?

bnge_shutdown_nic() calls bnge_hwrm_resource_free(bn, 1), which calls
bnge_hwrm_ring_free(bn, 1). At that point NQ0 is still in NAPI mode:
BNGE_STATE_NQ0_NAPI is set, vector0 is enabled and NAPI is enabled.

On the close path, bnge_hwrm_tx_ring_free(), bnge_hwrm_rx_ring_free()
and bnge_hwrm_rx_agg_ring_free() point cmpl_ring at the L2 completion
ring. __hwrm_send_ctx() therefore waits until bnge_napi_poll() reaches
bnge_hwrm_update_token() through __bnge_poll_work() and
bnge_hwrm_handler().

The waiter can wake up as soon as the token becomes
BNGE_HWRM_COMPLETE. bnge_napi_poll() may still be inside its loop and
may not yet have updated nqr->nq_raw_cons or written the NQ doorbell.
bnge_hwrm_ring_free() then goes straight to bnge_disable_int_sync() and
bnge_hwrm_cp_ring_free():

CPU0 (ndo_stop)                  CPU1 (NET_RX softirq)
bnge_hwrm_ring_free()            bnge_napi_poll() on NQ0
  rx/agg ring free                 __bnge_poll_work()
    waits for token                  bnge_hwrm_update_token()
  <- wakes up                      ...
  bnge_disable_int_sync()
    bnge_disable_int()
      reads nqr->nq_raw_cons
                                   nqr->nq_raw_cons = raw_cons
                                   BNGE_DB_NQ_ARM()
      bnge_db_nq() with old cons
    synchronize_irq()
  bnge_hwrm_cp_ring_free()

synchronize_irq() waits only for the hardirq handler, not for the
softirq poll. Nothing here orders the two doorbell writers, so one of
two things can happen:

- The mask write goes out with a stale consumer index after the poll
  has advanced it.
- The poll re-arms NQ0 after it was masked. This happens while NQ0's
  completion rings and stat contexts are being freed in firmware, and
  while vector0 is still routed to NAPI.

Before this patch, NQ0 was freed in firmware on close and re-created
on open, so any state this race left on the NQ was discarded. Now NQ0
persists. The same NQ also carries the async events and HWRM
completions that the worker mode depends on while the interface is
down.

Is the bnge_nq0_poll() re-arm in bnge_nq0_napi_to_wq() meant to repair
NQ0's doorbell state after this race? If not, would it be better to
quiesce NQ0's NAPI, and the other NQs' NAPIs, after the TX/RX/AGG ring
frees but before bnge_disable_int_sync() and the completion ring frees?
That would replace doing it only after bnge_shutdown_nic() has
returned.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111190011.53437-1-bhargava.marreddy%40broadcom.com