Re: [PATCH net-next 5/7] bnge: Quiesce NQ0 around ring teardown in bnge_free_core()

[netdev-bot+sashiko]

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

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

- [Low] Claim: in bnge_alloc_core() bnge_resume_nq0() runs before the 'if
  (rc) goto err_free_core;' check, so on a bnge_alloc_nq_tree() failure…

--

bnge: Quiesce NQ0 around ring teardown in bnge_free_core()

This adds bnge_quiesce_nq0() and bnge_resume_nq0() helpers that mask and
re-arm the NQ0 doorbell and disable/enable its NAPI, and uses them to
bracket ring teardown in bnge_free_core() and the NQ tree rebuild in
bnge_alloc_core().  Both helpers are inert in this commit because no
bnge_napi has BNGE_NAPI_FLAG_NQ0 set yet.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index dc3d51f31e5ef..10d54568f2c56 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -563,6 +582,34 @@ static void bnge_free_nq_tree(struct bnge_net *bn)
>  	}
>  }
>  
> +static void bnge_quiesce_nq0(struct bnge_net *bn)
> +{
> +	struct bnge_napi *bnapi = bn->bnapi[BNGE_NQ0_NAPI_IDX];
> +	struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;
> +	struct bnge_ring_struct *ring;
> +	struct bnge_dev *bd = bn->bd;
> +
> +	if (!BNGE_NQ0_NAPI(bnapi))
> +		return;
> +
> +	ring = &nqr->ring_struct;
> +	bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
> +	synchronize_irq(bd->irq_tbl[ring->map_idx].vector);
> +	napi_disable_locked(&bnapi->napi);
> +}
> +
> +static void bnge_resume_nq0(struct bnge_net *bn)
> +{
> +	struct bnge_napi *bnapi = bn->bnapi[BNGE_NQ0_NAPI_IDX];
> +	struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;
> +
> +	if (!BNGE_NQ0_NAPI(bnapi))
> +		return;
> +
> +	napi_enable_locked(&bnapi->napi);
> +	bnge_db_nq_arm(bn, &nqr->nq_db, nqr->nq_raw_cons);
> +}
> +

[ ... ]

> @@ -1345,7 +1394,9 @@ static int bnge_alloc_core(struct bnge_net *bn)
>  	if (rc)
>  		goto err_free_core;
>  
> +	bnge_quiesce_nq0(bn);
>  	rc = bnge_alloc_nq_tree(bn);
> +	bnge_resume_nq0(bn);
>  	if (rc)
>  		goto err_free_core;

[Severity: Low]
Is the placement of bnge_resume_nq0() before the rc check here
intentional?

On a bnge_alloc_nq_tree() failure the internal cleanup has already run
bnge_free_nq_tree(), and only after that does bnge_alloc_core() re-enable
NQ0 NAPI and write the NQ_ARM doorbell:

	bnge_quiesce_nq0(bn);
	rc = bnge_alloc_nq_tree(bn);
	bnge_resume_nq0(bn);	/* arms NQ0 even when rc != 0 */
	if (rc)
		goto err_free_core;

At this commit bnge_free_nq_tree() frees the NQ ring memory for every
index without exception:

drivers/net/ethernet/broadcom/bnge/bnge_netdev.c:bnge_free_nq_tree() {
	...
	nqr = &bnapi->nq_ring;
	ring = &nqr->ring_struct;

	bnge_free_ring(bd, &ring->ring_mem);
	...
	kfree(nqr->cp_ring_arr);
	nqr->cp_ring_arr = NULL;
	nqr->cp_ring_count = 0;
}

Tracing this to the end of the series, the exposure appears to go away:
bnge_free_nq_tree() gains an "if (!BNGE_NQ0_NAPI(bnapi))" guard around
bnge_free_ring(), bnge_free_nq_arrays() skips NQ0 as well, and
bnge_napi_poll() gains a guard for the one piece of NQ0 state that this
error path still clears:

drivers/net/ethernet/broadcom/bnge/bnge_txrx.c:bnge_napi_poll() {
	...
	if (unlikely(!nqr->cp_ring_arr ||
		     idx >= nqr->cp_ring_count)) {
		raw_cons = NEXT_RAW_CMP(raw_cons);
		continue;
	}
	...
}

Would it still be worth moving bnge_resume_nq0() after the rc check, or
at least noting in the commit message that arming NQ0 on the failure path
is deliberate and relies on those later guards?

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