Re: [PATCH net-next 5/7] bnge: Quiesce NQ0 around ring teardown in bnge_free_core()
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-04 10:18:52
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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); +} +
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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