[PATCH net-next 0/7] bnge: Support async events while down

[PATCH net-next 0/7] bnge: Support async events while down

[Bhargava Marreddy]

Hi,

This series lets the bnge driver keep processing firmware async events
while the interface is administratively down, by keeping one notification
queue (NQ0) active across close/open instead of tearing it down and
rebuilding it every time the interface is brought down and up.

This persistent NQ0 setup lays the groundwork for additional async events
in future patches.

Bhargava Marreddy (7):
  bnge: Extract bnapi allocation and cleanup into helpers
  bnge: Extract per-NQ ring allocation and init helpers
  bnge: Move bnapi and ring_grp allocation to probe/remove
  bnge: Drop obsolete bn->bnapi NULL checks in open/close paths
  bnge: Quiesce NQ0 around ring teardown in bnge_free_core()
  bnge: Create NQ0 during probe and keep active across open/close
  bnge: Process async events while administratively down

 .../net/ethernet/broadcom/bnge/bnge_ethtool.c |   2 +-
 .../net/ethernet/broadcom/bnge/bnge_link.c    |   3 +
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 483 ++++++++++++++----
 .../net/ethernet/broadcom/bnge/bnge_netdev.h  |  11 +
 .../net/ethernet/broadcom/bnge/bnge_rmem.c    |  26 +-
 .../net/ethernet/broadcom/bnge/bnge_rmem.h    |   4 +-
 .../net/ethernet/broadcom/bnge/bnge_txrx.c    |  15 +
 7 files changed, 440 insertions(+), 104 deletions(-)

-- 
2.47.3

[PATCH net-next 1/7] bnge: Extract bnapi allocation and cleanup into helpers

[Bhargava Marreddy]

Factor bn->bnapi allocation and free logic out of bnge_{alloc,free}_core()
into new bnge_{alloc,free}_bnapi_mem() helpers.

This is a pure refactor with no functional changes, preparing the driver to
move bnapi allocation out of the open/close path.

Signed-off-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
Reviewed-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Rajashekar Hudumula <rajashekar.hudumula@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 29 +++++++++++++++----
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index a4288f0258f8..dbadbf76a3a9 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -1195,6 +1195,12 @@ static int bnge_init_ring_grps(struct bnge_net *bn)
 	return 0;
 }
 
+static void bnge_free_bnapi_mem(struct bnge_net *bn)
+{
+	kfree(bn->bnapi);
+	bn->bnapi = NULL;
+}
+
 static void bnge_free_core(struct bnge_net *bn)
 {
 	bnge_free_vnic_attributes(bn);
@@ -1211,15 +1217,13 @@ static void bnge_free_core(struct bnge_net *bn)
 	bn->tx_ring = NULL;
 	kfree(bn->rx_ring);
 	bn->rx_ring = NULL;
-	kfree(bn->bnapi);
-	bn->bnapi = NULL;
+	bnge_free_bnapi_mem(bn);
 }
 
-static int bnge_alloc_core(struct bnge_net *bn)
+static int bnge_alloc_bnapi_mem(struct bnge_net *bn)
 {
 	struct bnge_dev *bd = bn->bd;
-	int i, j, size, arr_size;
-	int rc = -ENOMEM;
+	int i, size, arr_size;
 	void *bnapi;
 
 	arr_size = L1_CACHE_ALIGN(sizeof(struct bnge_napi *) *
@@ -1227,7 +1231,7 @@ static int bnge_alloc_core(struct bnge_net *bn)
 	size = L1_CACHE_ALIGN(sizeof(struct bnge_napi));
 	bnapi = kzalloc(arr_size + size * bd->nq_nr_rings, GFP_KERNEL);
 	if (!bnapi)
-		return rc;
+		return -ENOMEM;
 
 	bn->bnapi = bnapi;
 	bnapi += arr_size;
@@ -1241,6 +1245,19 @@ static int bnge_alloc_core(struct bnge_net *bn)
 		nqr->ring_struct.ring_mem.flags = BNGE_RMEM_RING_PTE_FLAG;
 	}
 
+	return 0;
+}
+
+static int bnge_alloc_core(struct bnge_net *bn)
+{
+	struct bnge_dev *bd = bn->bd;
+	int i, j, rc;
+
+	rc = bnge_alloc_bnapi_mem(bn);
+	if (rc)
+		return rc;
+
+	rc = -ENOMEM;
 	bn->rx_ring = kzalloc_objs(struct bnge_rx_ring_info, bd->rx_nr_rings);
 	if (!bn->rx_ring)
 		goto err_free_core;
-- 
2.47.3

[PATCH net-next 2/7] bnge: Extract per-NQ ring allocation and init helpers

[Bhargava Marreddy]

Factor the per-NQ loop body in bnge_hwrm_ring_alloc() and
bnge_init_ring_struct() into standalone bnge_hwrm_nq_ring_alloc() and
bnge_init_nq_ring_struct() helpers.

This is a pure refactor with no functional changes, preparing NQ0 setup
to run independently during probe rather than through the open path loop.

Signed-off-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
Reviewed-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 56 +++++++++++--------
 .../net/ethernet/broadcom/bnge/bnge_rmem.c    | 25 ++++++---
 .../net/ethernet/broadcom/bnge/bnge_rmem.h    |  4 +-
 3 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index dbadbf76a3a9..6c1cb3aab5a8 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -1909,6 +1909,37 @@ static int bnge_hwrm_rx_ring_alloc(struct bnge_net *bn,
 	return 0;
 }
 
+static int bnge_hwrm_nq_ring_alloc(struct bnge_net *bn, int index)
+{
+	struct bnge_napi *bnapi = bn->bnapi[index];
+	struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;
+	struct bnge_ring_struct *ring = &nqr->ring_struct;
+	u32 type = HWRM_RING_ALLOC_NQ;
+	struct bnge_dev *bd = bn->bd;
+	u32 map_idx = ring->map_idx;
+	unsigned int vector;
+	int rc;
+
+	vector = bd->irq_tbl[map_idx].vector;
+	disable_irq_nosync(vector);
+	rc = hwrm_ring_alloc_send_msg(bn, ring, type, map_idx);
+	if (rc) {
+		enable_irq(vector);
+		return rc;
+	}
+	bnge_set_db(bn, &nqr->nq_db, type, map_idx, ring->fw_ring_id);
+	bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
+	enable_irq(vector);
+	bn->grp_info[index].nq_fw_ring_id = (u16)ring->fw_ring_id;
+	if (!index) {
+		rc = bnge_hwrm_set_async_event_cr(bd, ring->fw_ring_id);
+		if (rc)
+			netdev_warn(bn->netdev, "Failed to set async event completion ring.\n");
+	}
+
+	return 0;
+}
+
 static int bnge_hwrm_ring_alloc(struct bnge_net *bn)
 {
 	struct bnge_dev *bd = bn->bd;
@@ -1917,30 +1948,9 @@ static int bnge_hwrm_ring_alloc(struct bnge_net *bn)
 
 	agg_rings = !!(bnge_is_agg_reqd(bd));
 	for (i = 0; i < bd->nq_nr_rings; i++) {
-		struct bnge_napi *bnapi = bn->bnapi[i];
-		struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;
-		struct bnge_ring_struct *ring = &nqr->ring_struct;
-		u32 type = HWRM_RING_ALLOC_NQ;
-		u32 map_idx = ring->map_idx;
-		unsigned int vector;
-
-		vector = bd->irq_tbl[map_idx].vector;
-		disable_irq_nosync(vector);
-		rc = hwrm_ring_alloc_send_msg(bn, ring, type, map_idx);
-		if (rc) {
-			enable_irq(vector);
+		rc = bnge_hwrm_nq_ring_alloc(bn, i);
+		if (rc)
 			goto err_out;
-		}
-		bnge_set_db(bn, &nqr->nq_db, type, map_idx, ring->fw_ring_id);
-		bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
-		enable_irq(vector);
-		bn->grp_info[i].nq_fw_ring_id = (u16)ring->fw_ring_id;
-
-		if (!i) {
-			rc = bnge_hwrm_set_async_event_cr(bd, ring->fw_ring_id);
-			if (rc)
-				netdev_warn(bn->netdev, "Failed to set async event completion ring.\n");
-		}
 	}
 
 	for (i = 0; i < bd->tx_nr_rings; i++) {
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
index b066ee887a09..e0ddb2800c54 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
@@ -423,6 +423,21 @@ int bnge_alloc_ctx_mem(struct bnge_dev *bd)
 	return 0;
 }
 
+void bnge_init_nq_ring_struct(struct bnge_net *bn,
+			      struct bnge_nq_ring_info *nqr)
+{
+	struct bnge_ring_mem_info *rmem;
+	struct bnge_ring_struct *ring;
+
+	ring = &nqr->ring_struct;
+	rmem = &ring->ring_mem;
+	rmem->nr_pages = bn->cp_nr_pages;
+	rmem->page_size = HW_CMPD_RING_SIZE;
+	rmem->pg_arr = (void **)nqr->desc_ring;
+	rmem->dma_arr = nqr->desc_mapping;
+	rmem->vmem_size = 0;
+}
+
 void bnge_init_ring_struct(struct bnge_net *bn)
 {
 	struct bnge_dev *bd = bn->bd;
@@ -431,19 +446,11 @@ void bnge_init_ring_struct(struct bnge_net *bn)
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 		struct bnge_ring_mem_info *rmem;
-		struct bnge_nq_ring_info *nqr;
 		struct bnge_rx_ring_info *rxr;
 		struct bnge_tx_ring_info *txr;
 		struct bnge_ring_struct *ring;
 
-		nqr = &bnapi->nq_ring;
-		ring = &nqr->ring_struct;
-		rmem = &ring->ring_mem;
-		rmem->nr_pages = bn->cp_nr_pages;
-		rmem->page_size = HW_CMPD_RING_SIZE;
-		rmem->pg_arr = (void **)nqr->desc_ring;
-		rmem->dma_arr = nqr->desc_mapping;
-		rmem->vmem_size = 0;
+		bnge_init_nq_ring_struct(bn, &bnapi->nq_ring);
 
 		rxr = bnapi->rx_ring;
 		if (!rxr)
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.h b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.h
index bb0c79a1ee60..a5a1d346ac93 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.h
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.h
@@ -7,6 +7,7 @@
 struct bnge_ctx_mem_type;
 struct bnge_dev;
 struct bnge_net;
+struct bnge_nq_ring_info;
 
 #define PTU_PTE_VALID             0x1UL
 #define PTU_PTE_LAST              0x2UL
@@ -198,5 +199,6 @@ void bnge_free_ring(struct bnge_dev *bd, struct bnge_ring_mem_info *rmem);
 int bnge_alloc_ctx_mem(struct bnge_dev *bd);
 void bnge_free_ctx_mem(struct bnge_dev *bd);
 void bnge_init_ring_struct(struct bnge_net *bn);
-
+void bnge_init_nq_ring_struct(struct bnge_net *bn,
+			      struct bnge_nq_ring_info *nqr);
 #endif /* _BNGE_RMEM_H_ */
-- 
2.47.3

[PATCH net-next 3/7] bnge: Move bnapi and ring_grp allocation to probe/remove

[Bhargava Marreddy]

Allocate bn->bnapi and bn->grp_info during probe and free them during
remove instead of reallocating on every open/close.

To support this shift:
- Pass an irq_re_init flag to bnge_init_ring_grps() so open can reset
  per-group state without reallocating bn->grp_info.
- Have bnge_free_core() clear rx_ring/tx_ring back-pointers via the new
  bnge_clear_bnapi_queues() rather than freeing bn->bnapi.
- Replace "!bn->bnapi" checks in bnge_get_queue_stats_rx() and
  bnge_get_ethtool_stats() with !netif_running(dev) checks to avoid
  reading sw_stats memory freed on close.

Signed-off-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
Reviewed-by: Rajashekar Hudumula <rajashekar.hudumula@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
Reviewed-by: Kiran Kella <kiran.kella@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_ethtool.c |   2 +-
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 108 ++++++++++++------
 2 files changed, 73 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
index 2467e44de291..ad9956bc4408 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
@@ -346,7 +346,7 @@ static void bnge_get_ethtool_stats(struct net_device *dev,
 	u32 tpa_stats;
 	u32 i, j = 0;
 
-	if (!bn->bnapi) {
+	if (!netif_running(dev)) {
 		j += bnge_get_num_ring_stats(bd);
 		goto skip_ring_stats;
 	}
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index 6c1cb3aab5a8..dcf04d391570 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -1176,20 +1176,26 @@ static void bnge_free_ring_grps(struct bnge_net *bn)
 	bn->grp_info = NULL;
 }
 
-static int bnge_init_ring_grps(struct bnge_net *bn)
+static int bnge_init_ring_grps(struct bnge_net *bn, bool irq_re_init)
 {
 	struct bnge_dev *bd = bn->bd;
 	int i;
 
-	bn->grp_info = kzalloc_objs(struct bnge_ring_grp_info, bd->nq_nr_rings);
-	if (!bn->grp_info)
-		return -ENOMEM;
+	if (irq_re_init) {
+		bn->grp_info = kzalloc_objs(struct bnge_ring_grp_info,
+					    bd->nq_nr_rings);
+		if (!bn->grp_info)
+			return -ENOMEM;
+	}
+
 	for (i = 0; i < bd->nq_nr_rings; i++) {
-		bn->grp_info[i].fw_stats_ctx = INVALID_HW_RING_ID;
 		bn->grp_info[i].fw_grp_id = INVALID_HW_RING_ID;
 		bn->grp_info[i].rx_fw_ring_id = INVALID_HW_RING_ID;
 		bn->grp_info[i].agg_fw_ring_id = INVALID_HW_RING_ID;
 		bn->grp_info[i].nq_fw_ring_id = INVALID_HW_RING_ID;
+
+		if (irq_re_init)
+			bn->grp_info[i].fw_stats_ctx = INVALID_HW_RING_ID;
 	}
 
 	return 0;
@@ -1201,25 +1207,6 @@ static void bnge_free_bnapi_mem(struct bnge_net *bn)
 	bn->bnapi = NULL;
 }
 
-static void bnge_free_core(struct bnge_net *bn)
-{
-	bnge_free_vnic_attributes(bn);
-	bnge_free_tx_rings(bn);
-	bnge_free_rx_rings(bn);
-	bnge_free_nq_tree(bn);
-	bnge_free_nq_arrays(bn);
-	bnge_free_ring_stats(bn);
-	bnge_free_ring_grps(bn);
-	bnge_free_vnics(bn);
-	kfree(bn->tx_ring_map);
-	bn->tx_ring_map = NULL;
-	kfree(bn->tx_ring);
-	bn->tx_ring = NULL;
-	kfree(bn->rx_ring);
-	bn->rx_ring = NULL;
-	bnge_free_bnapi_mem(bn);
-}
-
 static int bnge_alloc_bnapi_mem(struct bnge_net *bn)
 {
 	struct bnge_dev *bd = bn->bd;
@@ -1248,16 +1235,52 @@ static int bnge_alloc_bnapi_mem(struct bnge_net *bn)
 	return 0;
 }
 
-static int bnge_alloc_core(struct bnge_net *bn)
+static void bnge_clear_bnapi_queues(struct bnge_net *bn)
 {
 	struct bnge_dev *bd = bn->bd;
-	int i, j, rc;
+	int i;
 
-	rc = bnge_alloc_bnapi_mem(bn);
-	if (rc)
-		return rc;
+	if (!bn->bnapi)
+		return;
+
+	for (i = 0; i < bd->nq_nr_rings; i++) {
+		struct bnge_napi *bnapi = bn->bnapi[i];
+		int j;
+
+		if (!bnapi)
+			continue;
+
+		bnapi->rx_ring = NULL;
+		for (j = 0; j < BNGE_MAX_TXR_PER_NAPI; j++)
+			bnapi->tx_ring[j] = NULL;
+	}
+}
+
+static void bnge_free_core(struct bnge_net *bn)
+{
+	bnge_free_vnic_attributes(bn);
+	bnge_free_tx_rings(bn);
+	bnge_free_rx_rings(bn);
+	bnge_free_nq_tree(bn);
+	bnge_free_nq_arrays(bn);
+	bnge_free_ring_stats(bn);
+	bnge_free_vnics(bn);
+
+	kfree(bn->tx_ring_map);
+	bn->tx_ring_map = NULL;
+	kfree(bn->tx_ring);
+	bn->tx_ring = NULL;
+	kfree(bn->rx_ring);
+	bn->rx_ring = NULL;
+
+	bnge_clear_bnapi_queues(bn);
+}
+
+static int bnge_alloc_core(struct bnge_net *bn)
+{
+	struct bnge_dev *bd = bn->bd;
+	int i, j, rc = -ENOMEM;
 
-	rc = -ENOMEM;
 	bn->rx_ring = kzalloc_objs(struct bnge_rx_ring_info, bd->rx_nr_rings);
 	if (!bn->rx_ring)
 		goto err_free_core;
@@ -1393,6 +1416,7 @@ static void bnge_init_nq_tree(struct bnge_net *bn)
 		struct bnge_nq_ring_info *nqr = &bn->bnapi[i]->nq_ring;
 		struct bnge_ring_struct *ring = &nqr->ring_struct;
 
+		nqr->nq_raw_cons = 0;
 		ring->fw_ring_id = INVALID_HW_RING_ID_32BIT;
 		for (j = 0; j < nqr->cp_ring_count; j++) {
 			struct bnge_cp_ring_info *cpr = &nqr->cp_ring_arr[j];
@@ -2834,9 +2858,7 @@ static int bnge_init_nic(struct bnge_net *bn)
 
 	bnge_init_tx_rings(bn);
 
-	rc = bnge_init_ring_grps(bn);
-	if (rc)
-		goto err_free_rx_ring_pair_bufs;
+	bnge_init_ring_grps(bn, false);
 
 	bnge_init_vnics(bn);
 
@@ -2847,7 +2869,6 @@ static int bnge_init_nic(struct bnge_net *bn)
 
 err_free_ring_grps:
 	bnge_free_ring_grps(bn);
-err_free_rx_ring_pair_bufs:
 	bnge_free_rx_ring_pair_bufs(bn);
 	return rc;
 }
@@ -3222,7 +3243,7 @@ static void bnge_get_queue_stats_rx(struct net_device *dev, int i,
 	struct bnge_nq_ring_info *nqr;
 	u64 *sw;
 
-	if (!bn->bnapi)
+	if (!netif_running(dev))
 		return;
 
 	nqr = &bn->bnapi[i]->nq_ring;
@@ -3544,14 +3565,27 @@ int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
 	spin_lock_init(&bn->stats_lock);
 
 	netdev->request_ops_lock = true;
+
+	rc = bnge_alloc_bnapi_mem(bn);
+	if (rc)
+		goto err_free_port_stats;
+
+	rc = bnge_init_ring_grps(bn, true);
+	if (rc)
+		goto err_free_bnapi_mem;
+
 	rc = register_netdev(netdev);
 	if (rc) {
 		dev_err(bd->dev, "Register netdev failed rc: %d\n", rc);
-		goto err_free_port_stats;
+		goto err_free_ring_grps;
 	}
 
 	return 0;
 
+err_free_ring_grps:
+	bnge_free_ring_grps(bn);
+err_free_bnapi_mem:
+	bnge_free_bnapi_mem(bn);
 err_free_port_stats:
 	bnge_free_port_stats(bn);
 err_free_workq:
@@ -3576,6 +3610,8 @@ void bnge_netdev_free(struct bnge_dev *bd)
 	destroy_workqueue(bn->bnge_pf_wq);
 
 	bnge_free_port_stats(bn);
+	bnge_free_ring_grps(bn);
+	bnge_free_bnapi_mem(bn);
 
 	free_netdev(netdev);
 	bd->netdev = NULL;
-- 
2.47.3

Re: [PATCH net-next 3/7] bnge: Move bnapi and ring_grp allocation to probe/remove

[netdev-bot+sashiko]

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

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

- [High] bnge_init_nic()'s error path frees probe-owned memory,
  permanently NULLing bn->grp_info and causing a NULL-pointer write on…

--

bnge: Move bnapi and ring_grp allocation to probe/remove

This moves the bn->bnapi and bn->grp_info allocations from the open path
into bnge_netdev_alloc(), frees them in bnge_netdev_free(), and adds an
irq_re_init flag to bnge_init_ring_grps() so that open only resets the
per-group state.  bnge_free_core() now clears the bnapi queue
back-pointers instead of freeing bn->bnapi.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 6c1cb3aab5a8c..dcf04d391570b 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -1176,20 +1176,26 @@ static void bnge_free_ring_grps(struct bnge_net *bn)
>  	bn->grp_info = NULL;
>  }
>  
> -static int bnge_init_ring_grps(struct bnge_net *bn)
> +static int bnge_init_ring_grps(struct bnge_net *bn, bool irq_re_init)
>  {
>  	struct bnge_dev *bd = bn->bd;
>  	int i;
>  
> -	bn->grp_info = kzalloc_objs(struct bnge_ring_grp_info, bd->nq_nr_rings);
> -	if (!bn->grp_info)
> -		return -ENOMEM;
> +	if (irq_re_init) {
> +		bn->grp_info = kzalloc_objs(struct bnge_ring_grp_info,
> +					    bd->nq_nr_rings);
> +		if (!bn->grp_info)
> +			return -ENOMEM;
> +	}
> +
>  	for (i = 0; i < bd->nq_nr_rings; i++) {
> -		bn->grp_info[i].fw_stats_ctx = INVALID_HW_RING_ID;
>  		bn->grp_info[i].fw_grp_id = INVALID_HW_RING_ID;

[Severity: High]
With irq_re_init == false, which is the only value the open path passes,
the allocation branch is skipped and this loop writes bn->grp_info[i]
unconditionally.  Can bn->grp_info be NULL when we get here?

See the error path left behind in bnge_init_nic() below.

>  		bn->grp_info[i].rx_fw_ring_id = INVALID_HW_RING_ID;
>  		bn->grp_info[i].agg_fw_ring_id = INVALID_HW_RING_ID;
>  		bn->grp_info[i].nq_fw_ring_id = INVALID_HW_RING_ID;
> +
> +		if (irq_re_init)
> +			bn->grp_info[i].fw_stats_ctx = INVALID_HW_RING_ID;
>  	}
>  
>  	return 0;
> @@ -1201,25 +1207,6 @@ static void bnge_free_bnapi_mem(struct bnge_net *bn)
>  	bn->bnapi = NULL;
>  }
>  
> -static void bnge_free_core(struct bnge_net *bn)
> -{
> -	bnge_free_vnic_attributes(bn);
> -	bnge_free_tx_rings(bn);
> -	bnge_free_rx_rings(bn);
> -	bnge_free_nq_tree(bn);
> -	bnge_free_nq_arrays(bn);
> -	bnge_free_ring_stats(bn);
> -	bnge_free_ring_grps(bn);
> -	bnge_free_vnics(bn);
> -	kfree(bn->tx_ring_map);
> -	bn->tx_ring_map = NULL;
> -	kfree(bn->tx_ring);
> -	bn->tx_ring = NULL;
> -	kfree(bn->rx_ring);
> -	bn->rx_ring = NULL;
> -	bnge_free_bnapi_mem(bn);
> -}
> -
>  static int bnge_alloc_bnapi_mem(struct bnge_net *bn)
>  {
>  	struct bnge_dev *bd = bn->bd;

[ ... ]

> @@ -2834,9 +2858,7 @@ static int bnge_init_nic(struct bnge_net *bn)
>  
>  	bnge_init_tx_rings(bn);
>  
> -	rc = bnge_init_ring_grps(bn);
> -	if (rc)
> -		goto err_free_rx_ring_pair_bufs;
> +	bnge_init_ring_grps(bn, false);
>  
>  	bnge_init_vnics(bn);
>  
> @@ -2847,7 +2869,6 @@ static int bnge_init_nic(struct bnge_net *bn)
>  
>  err_free_ring_grps:
>  	bnge_free_ring_grps(bn);
> -err_free_rx_ring_pair_bufs:
>  	bnge_free_rx_ring_pair_bufs(bn);
>  	return rc;
>  }

[Severity: High]
Now that bn->grp_info belongs to probe/remove, should this
bnge_free_ring_grps() call still be here?

bnge_init_nic() reaches this label when bnge_init_chip() fails, which is
reachable through bnge_hwrm_stat_ctx_alloc(), bnge_hwrm_ring_alloc(),
bnge_hwrm_vnic_alloc() or bnge_setup_vnic() returning an error, and
bnge_free_ring_grps() does:

	kfree(bn->grp_info);
	bn->grp_info = NULL;

The unwind in bnge_open_core() (err_free_irq -> bnge_free_irq ->
bnge_del_napi -> bnge_free_core -> err_if_change) does not reallocate it,
and bnge_free_core() no longer touches grp_info at all after this patch,
so the netdev stays registered with bn->grp_info == NULL.

A second "ip link set <dev> up" then goes:

bnge_open_core()
  bnge_alloc_core()		/* does not touch grp_info */
  bnge_init_nic()
    bnge_init_ring_grps(bn, false)
      /* irq_re_init == false, allocation skipped */
      bn->grp_info[i].fw_grp_id = INVALID_HW_RING_ID;   /* NULL deref */

Does that write oops?  Even if it were guarded, both
bnge_hwrm_stat_ctx_alloc() and hwrm_ring_alloc_send_msg() in
bnge_hwrm_lib.c dereference bn->grp_info:

		grp_info = &bn->grp_info[ring->grp_idx];
		req->stat_ctx_id = cpu_to_le32(grp_info->fw_stats_ctx);

so the driver would also lose its mirror of the programmed firmware
stat_ctx_id / rx_ring_id / nq_ring_id associations.

Would dropping the bnge_free_ring_grps() call from this error path, so
only bnge_free_rx_ring_pair_bufs() remains, match the new ownership
model?

For reference, this is not fixed later in the series: at c51d67eb
bnge_init_nic() still has the err_free_ring_grps label calling
bnge_free_ring_grps(), bnge_init_ring_grps() still dereferences
bn->grp_info unguarded, and "bnge: Create NQ0 during probe and keep
active across open/close" adds another use of the same pointer in
bnge_free_nq0():

	bn->grp_info[0].nq_fw_ring_id = INVALID_HW_RING_ID;

[ ... ]

> @@ -3544,14 +3565,27 @@ int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
>  	spin_lock_init(&bn->stats_lock);
>  
>  	netdev->request_ops_lock = true;
> +
> +	rc = bnge_alloc_bnapi_mem(bn);
> +	if (rc)
> +		goto err_free_port_stats;
> +
> +	rc = bnge_init_ring_grps(bn, true);
> +	if (rc)
> +		goto err_free_bnapi_mem;
> +
>  	rc = register_netdev(netdev);
>  	if (rc) {
>  		dev_err(bd->dev, "Register netdev failed rc: %d\n", rc);
> -		goto err_free_port_stats;
> +		goto err_free_ring_grps;
>  	}
>  
>  	return 0;
>  
> +err_free_ring_grps:
> +	bnge_free_ring_grps(bn);
> +err_free_bnapi_mem:
> +	bnge_free_bnapi_mem(bn);
>  err_free_port_stats:
>  	bnge_free_port_stats(bn);
>  err_free_workq:

[ ... ]

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

[PATCH net-next 4/7] bnge: Drop obsolete bn->bnapi NULL checks in open/close paths

[Bhargava Marreddy]

Since bn->bnapi is now allocated during probe in bnge_netdev_alloc() and
freed in bnge_netdev_free(), it remains non-NULL for the lifetime of the
open/close path helpers.

Remove the dead "if (!bn->bnapi)" guards in bnge_free_ring_stats(),
bnge_clear_bnapi_queues(), bnge_disable_int(), and bnge_hwrm_ring_free().

Signed-off-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
Reviewed-by: Rajashekar Hudumula <rajashekar.hudumula@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnge/bnge_netdev.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index dcf04d391570..dc3d51f31e5e 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -85,9 +85,6 @@ static void bnge_free_ring_stats(struct bnge_net *bn)
 	struct bnge_dev *bd = bn->bd;
 	int i;
 
-	if (!bn->bnapi)
-		return;
-
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 		struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;
@@ -1240,9 +1237,6 @@ static void bnge_clear_bnapi_queues(struct bnge_net *bn)
 	struct bnge_dev *bd = bn->bd;
 	int i;
 
-	if (!bn->bnapi)
-		return;
-
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 		int j;
@@ -2371,9 +2365,6 @@ static void bnge_disable_int(struct bnge_net *bn)
 	struct bnge_dev *bd = bn->bd;
 	int i;
 
-	if (!bn->bnapi)
-		return;
-
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 		struct bnge_nq_ring_info *nqr;
@@ -2565,9 +2556,6 @@ static void bnge_hwrm_ring_free(struct bnge_net *bn, bool close_path)
 	struct bnge_dev *bd = bn->bd;
 	int i;
 
-	if (!bn->bnapi)
-		return;
-
 	for (i = 0; i < bd->tx_nr_rings; i++)
 		bnge_hwrm_tx_ring_free(bn, &bn->tx_ring[i], close_path);
 
-- 
2.47.3

Re: [PATCH net-next 4/7] bnge: Drop obsolete bn->bnapi NULL checks in open/close paths

[netdev-bot+sashiko]

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

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

- [High] NULL pointer dereference on a retried interface open after a
  failed open: bn->grp_info is now allocated once during probe…

--

bnge: Drop obsolete bn->bnapi NULL checks in open/close paths

This removes the "if (!bn->bnapi)" guards from bnge_free_ring_stats(),
bnge_clear_bnapi_queues(), bnge_disable_int() and bnge_hwrm_ring_free(),
on the grounds that bn->bnapi now lives from bnge_netdev_alloc() until
bnge_netdev_free().

One question below about the sibling allocation, bn->grp_info, which was
moved to probe lifetime by the same series but is still freed by the open
error path.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index dcf04d391570b..dc3d51f31e5ef 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -85,9 +85,6 @@ static void bnge_free_ring_stats(struct bnge_net *bn)
>  	struct bnge_dev *bd = bn->bd;
>  	int i;
>  
> -	if (!bn->bnapi)
> -		return;
> -
>  	for (i = 0; i < bd->nq_nr_rings; i++) {
>  		struct bnge_napi *bnapi = bn->bnapi[i];
>  		struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;

[Severity: High]
The reasoning here applies equally to bn->grp_info, which the same series
moved to probe lifetime via bnge_init_ring_grps(bn, true) in
bnge_netdev_alloc(), with the matching free in bnge_netdev_free().  But the
open path still frees it on error.  Can a retried open dereference a NULL
bn->grp_info?

bnge_init_nic():

	bnge_init_ring_grps(bn, false);

	bnge_init_vnics(bn);

	rc = bnge_init_chip(bn);
	if (rc)
		goto err_free_ring_grps;
	return rc;

err_free_ring_grps:
	bnge_free_ring_grps(bn);
	bnge_free_rx_ring_pair_bufs(bn);
	return rc;

and bnge_free_ring_grps() drops the probe-time allocation entirely:

static void bnge_free_ring_grps(struct bnge_net *bn)
{
	kfree(bn->grp_info);
	bn->grp_info = NULL;
}

bnge_init_chip() can fail on any of its firmware calls
(bnge_hwrm_stat_ctx_alloc(), bnge_hwrm_ring_alloc(),
bnge_hwrm_vnic_alloc(), bnge_hwrm_set_vnic_filter(), ...), for example on a
busy or timing-out firmware.  bnge_open_core() then unwinds through
err_free_irq -> bnge_free_irq() -> bnge_del_napi() -> bnge_free_core() ->
bnge_hwrm_if_change(bd, false) and returns, with the netdev still
registered and bn->grp_info left NULL.

Is there anything that reallocates it before the next open?  The only
allocation site is bnge_init_ring_grps() under irq_re_init, and the open
path calls it with irq_re_init == false, so the loop writes through the
NULL pointer:

	if (irq_re_init) {
		bn->grp_info = kzalloc_objs(struct bnge_ring_grp_info,
					    bd->nq_nr_rings);
		if (!bn->grp_info)
			return -ENOMEM;
	}

	for (i = 0; i < bd->nq_nr_rings; i++) {
		bn->grp_info[i].fw_grp_id = INVALID_HW_RING_ID;

bd->nq_nr_rings is at least 1, so a second "ip link set <dev> up" after a
failed open looks like it oopses on bn->grp_info[0].fw_grp_id.

Should the err_free_ring_grps label in bnge_init_nic() simply stop freeing
grp_info now that its lifetime is probe to remove?

This also appears to still be the case at the end of the series (commit
c51d67eb0c2d), where bnge_free_nq0() dereferences bn->grp_info[0] during
teardown and would add a second crash path after such a failed open.

[ ... ]

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

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

[Bhargava Marreddy]

Add bnge_quiesce_nq0() and bnge_resume_nq0() helpers to temporarily disable
and re-enable NQ0 NAPI. Use these helpers to bracket ring teardown in
bnge_free_core() and ring rebuild in bnge_alloc_core().

Both helpers currently no-op, since no bnge_napi has BNGE_NAPI_FLAG_NQ0 set
yet. The next patch creates NQ0 during probe and sets this flag, at which
point these helpers become load-bearing: because NQ0 will remain active
while administratively down, its NAPI is not disabled by the standard close
paths, so temporarily quiescing it during core resource free/rebuild windows
is required to prevent stray completions from accessing uninitialized or
freed memory.

Signed-off-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
Reviewed-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Rajashekar Hudumula <rajashekar.hudumula@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 70 ++++++++++++++-----
 .../net/ethernet/broadcom/bnge/bnge_netdev.h  | 10 +++
 2 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index dc3d51f31e5e..10d54568f2c5 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -442,6 +442,25 @@ static void bnge_sp_task(struct work_struct *work)
 	netdev_unlock(bn->netdev);
 }
 
+static void bnge_db_nq_arm(struct bnge_net *bn,
+			   struct bnge_db_info *db, u32 idx)
+{
+	bnge_writeq(bn->bd, db->db_key64 | DBR_TYPE_NQ_ARM |
+		    DB_RING_IDX(db, idx), db->doorbell);
+}
+
+static void bnge_db_nq(struct bnge_net *bn, struct bnge_db_info *db, u32 idx)
+{
+	bnge_writeq(bn->bd, db->db_key64 | DBR_TYPE_NQ_MASK |
+		    DB_RING_IDX(db, idx), db->doorbell);
+}
+
+static void bnge_db_cq(struct bnge_net *bn, struct bnge_db_info *db, u32 idx)
+{
+	bnge_writeq(bn->bd, db->db_key64 | DBR_TYPE_CQ_ARMALL |
+		    DB_RING_IDX(db, idx), db->doorbell);
+}
+
 static void bnge_free_nq_desc_arr(struct bnge_nq_ring_info *nqr)
 {
 	struct bnge_ring_struct *ring = &nqr->ring_struct;
@@ -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);
+}
+
 static int alloc_one_cp_ring(struct bnge_net *bn,
 			     struct bnge_cp_ring_info *cpr)
 {
@@ -1253,6 +1300,7 @@ static void bnge_clear_bnapi_queues(struct bnge_net *bn)
 static void bnge_free_core(struct bnge_net *bn)
 {
 	bnge_free_vnic_attributes(bn);
+	bnge_quiesce_nq0(bn);
 	bnge_free_tx_rings(bn);
 	bnge_free_rx_rings(bn);
 	bnge_free_nq_tree(bn);
@@ -1268,6 +1316,7 @@ static void bnge_free_core(struct bnge_net *bn)
 	bn->rx_ring = NULL;
 
 	bnge_clear_bnapi_queues(bn);
+	bnge_resume_nq0(bn);
 }
 
 static int bnge_alloc_core(struct bnge_net *bn)
@@ -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;
 
@@ -1372,25 +1423,6 @@ u32 bnge_cp_ring_for_tx(struct bnge_tx_ring_info *txr)
 	return txr->tx_cpr->ring_struct.fw_ring_id;
 }
 
-static void bnge_db_nq_arm(struct bnge_net *bn,
-			   struct bnge_db_info *db, u32 idx)
-{
-	bnge_writeq(bn->bd, db->db_key64 | DBR_TYPE_NQ_ARM |
-		    DB_RING_IDX(db, idx), db->doorbell);
-}
-
-static void bnge_db_nq(struct bnge_net *bn, struct bnge_db_info *db, u32 idx)
-{
-	bnge_writeq(bn->bd, db->db_key64 | DBR_TYPE_NQ_MASK |
-		    DB_RING_IDX(db, idx), db->doorbell);
-}
-
-static void bnge_db_cq(struct bnge_net *bn, struct bnge_db_info *db, u32 idx)
-{
-	bnge_writeq(bn->bd, db->db_key64 | DBR_TYPE_CQ_ARMALL |
-		    DB_RING_IDX(db, idx), db->doorbell);
-}
-
 static int bnge_cp_num_to_irq_num(struct bnge_net *bn, int n)
 {
 	struct bnge_napi *bnapi = bn->bnapi[n];
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
index 476b5bab96fe..4d84f109ad5f 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
@@ -539,6 +539,14 @@ struct bnge_tx_ring_info {
 	struct bnge_ring_struct	tx_ring_struct;
 };
 
+#define BNGE_NQ0_NAPI_IDX	0
+
+enum bnge_napi_flag {
+	BNGE_NAPI_FLAG_NQ0,
+};
+
+#define BNGE_NQ0_NAPI(bnapi)	(test_bit(BNGE_NAPI_FLAG_NQ0, &(bnapi)->flags))
+
 struct bnge_napi {
 	struct napi_struct		napi;
 	struct bnge_net			*bn;
@@ -555,6 +563,8 @@ struct bnge_napi {
 #define BNGE_TX_CMP_EVENT		0x10
 	bool				in_reset;
 	bool				tx_fault;
+
+	unsigned long			flags;
 };
 
 #define INVALID_STATS_CTX_ID	-1
-- 
2.47.3

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

[PATCH net-next 6/7] bnge: Create NQ0 during probe and keep active across open/close

[Bhargava Marreddy]

Set up NQ0 via bnge_setup_nq0() during probe and free it via bnge_free_nq0()
at remove. Skip NQ0 in open/close ring, IRQ, and NAPI helpers once
BNGE_NAPI_FLAG_NQ0 is set.

Also guard bnge_napi_poll() against stale completions for rings freed by
bnge_free_nq_tree().

The previous patch already added the bnge_quiesce_nq0()/bnge_resume_nq0()
guard around bnge_free_core()'s teardown and bnge_alloc_core()'s rebuild,
so NQ0 is safe to leave active across close/open from the moment
BNGE_NAPI_FLAG_NQ0 is set below.

Signed-off-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
Reviewed-by: Vikas Gupta <vikas.gupta@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 237 +++++++++++++++++-
 .../net/ethernet/broadcom/bnge/bnge_netdev.h  |   1 +
 .../net/ethernet/broadcom/bnge/bnge_rmem.c    |   3 +-
 .../net/ethernet/broadcom/bnge/bnge_txrx.c    |  15 ++
 4 files changed, 244 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index 10d54568f2c5..d11c404acd4e 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -527,6 +527,9 @@ static void bnge_free_nq_arrays(struct bnge_net *bn)
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
+
 		bnge_free_nq_desc_arr(&bnapi->nq_ring);
 	}
 }
@@ -539,6 +542,9 @@ static int bnge_alloc_nq_arrays(struct bnge_net *bn)
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
+
 		rc = bnge_alloc_nq_desc_arr(&bnapi->nq_ring, bn->cp_nr_pages);
 		if (rc)
 			goto err_free_nq_arrays;
@@ -564,7 +570,8 @@ static void bnge_free_nq_tree(struct bnge_net *bn)
 		nqr = &bnapi->nq_ring;
 		ring = &nqr->ring_struct;
 
-		bnge_free_ring(bd, &ring->ring_mem);
+		if (!BNGE_NQ0_NAPI(bnapi))
+			bnge_free_ring(bd, &ring->ring_mem);
 
 		if (!nqr->cp_ring_arr)
 			continue;
@@ -592,6 +599,9 @@ static void bnge_quiesce_nq0(struct bnge_net *bn)
 	if (!BNGE_NQ0_NAPI(bnapi))
 		return;
 
+	if (test_and_set_bit(BNGE_NAPI_FLAG_NQ0_QUIESCED, &bnapi->flags))
+		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);
@@ -606,6 +616,9 @@ static void bnge_resume_nq0(struct bnge_net *bn)
 	if (!BNGE_NQ0_NAPI(bnapi))
 		return;
 
+	if (!test_and_clear_bit(BNGE_NAPI_FLAG_NQ0_QUIESCED, &bnapi->flags))
+		return;
+
 	napi_enable_locked(&bnapi->napi);
 	bnge_db_nq_arm(bn, &nqr->nq_db, nqr->nq_raw_cons);
 }
@@ -658,11 +671,13 @@ static int bnge_alloc_nq_tree(struct bnge_net *bn)
 		nqr->bnapi = bnapi;
 		ring = &nqr->ring_struct;
 
-		rc = bnge_alloc_ring(bd, &ring->ring_mem);
-		if (rc)
-			goto err_free_nq_tree;
+		if (!BNGE_NQ0_NAPI(bnapi)) {
+			rc = bnge_alloc_ring(bd, &ring->ring_mem);
+			if (rc)
+				goto err_free_nq_tree;
 
-		ring->map_idx = ulp_msix + i;
+			ring->map_idx = ulp_msix + i;
+		}
 
 		if (i < bd->rx_nr_rings) {
 			cp_count++;
@@ -1236,10 +1251,14 @@ static int bnge_init_ring_grps(struct bnge_net *bn, bool irq_re_init)
 		bn->grp_info[i].fw_grp_id = INVALID_HW_RING_ID;
 		bn->grp_info[i].rx_fw_ring_id = INVALID_HW_RING_ID;
 		bn->grp_info[i].agg_fw_ring_id = INVALID_HW_RING_ID;
-		bn->grp_info[i].nq_fw_ring_id = INVALID_HW_RING_ID;
 
 		if (irq_re_init)
 			bn->grp_info[i].fw_stats_ctx = INVALID_HW_RING_ID;
+
+		if (BNGE_NQ0_NAPI(bn->bnapi[i]))
+			continue;
+
+		bn->grp_info[i].nq_fw_ring_id = INVALID_HW_RING_ID;
 	}
 
 	return 0;
@@ -1442,8 +1461,11 @@ static void bnge_init_nq_tree(struct bnge_net *bn)
 		struct bnge_nq_ring_info *nqr = &bn->bnapi[i]->nq_ring;
 		struct bnge_ring_struct *ring = &nqr->ring_struct;
 
-		nqr->nq_raw_cons = 0;
-		ring->fw_ring_id = INVALID_HW_RING_ID_32BIT;
+		if (!BNGE_NQ0_NAPI(bn->bnapi[i])) {
+			nqr->nq_raw_cons = 0;
+			ring->fw_ring_id = INVALID_HW_RING_ID_32BIT;
+		}
+
 		for (j = 0; j < nqr->cp_ring_count; j++) {
 			struct bnge_cp_ring_info *cpr = &nqr->cp_ring_arr[j];
 
@@ -1970,6 +1992,9 @@ static int bnge_hwrm_nq_ring_alloc(struct bnge_net *bn, int index)
 	unsigned int vector;
 	int rc;
 
+	if (BNGE_NQ0_NAPI(bnapi))
+		return 0;
+
 	vector = bd->irq_tbl[map_idx].vector;
 	disable_irq_nosync(vector);
 	rc = hwrm_ring_alloc_send_msg(bn, ring, type, map_idx);
@@ -2402,6 +2427,9 @@ static void bnge_disable_int(struct bnge_net *bn)
 		struct bnge_nq_ring_info *nqr;
 		struct bnge_ring_struct *ring;
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
+
 		nqr = &bnapi->nq_ring;
 		ring = &nqr->ring_struct;
 
@@ -2417,9 +2445,10 @@ static void bnge_disable_int_sync(struct bnge_net *bn)
 
 	bnge_disable_int(bn);
 	for (i = 0; i < bd->nq_nr_rings; i++) {
-		int map_idx = bnge_cp_num_to_irq_num(bn, i);
+		if (BNGE_NQ0_NAPI(bn->bnapi[i]))
+			continue;
 
-		synchronize_irq(bd->irq_tbl[map_idx].vector);
+		synchronize_irq(bd->irq_tbl[bnge_cp_num_to_irq_num(bn, i)].vector);
 	}
 }
 
@@ -2432,6 +2461,9 @@ static void bnge_enable_int(struct bnge_net *bn)
 		struct bnge_napi *bnapi = bn->bnapi[i];
 		struct bnge_nq_ring_info *nqr;
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
+
 		nqr = &bnapi->nq_ring;
 		bnge_db_nq_arm(bn, &nqr->nq_db, nqr->nq_raw_cons);
 	}
@@ -2448,6 +2480,8 @@ static void bnge_disable_napi(struct bnge_net *bn)
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
 		napi_disable_locked(&bnapi->napi);
 	}
 }
@@ -2464,6 +2498,8 @@ static void bnge_enable_napi(struct bnge_net *bn)
 		bnapi->in_reset = false;
 		bnapi->tx_fault = 0;
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
 		napi_enable_locked(&bnapi->napi);
 	}
 }
@@ -2612,6 +2648,9 @@ static void bnge_hwrm_ring_free(struct bnge_net *bn, bool close_path)
 		for (j = 0; j < nqr->cp_ring_count && nqr->cp_ring_arr; j++)
 			bnge_hwrm_cp_ring_free(bn, &nqr->cp_ring_arr[j]);
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
+
 		ring = &nqr->ring_struct;
 		if (ring->fw_ring_id != INVALID_HW_RING_ID_32BIT) {
 			hwrm_ring_free_send_msg(bn, ring,
@@ -2673,6 +2712,9 @@ static void bnge_free_irq(struct bnge_net *bn)
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		int map_idx = bnge_cp_num_to_irq_num(bn, i);
 
+		if (BNGE_NQ0_NAPI(bn->bnapi[i]))
+			continue;
+
 		irq = &bd->irq_tbl[map_idx];
 		if (irq->requested) {
 			if (irq->have_cpumask) {
@@ -2701,6 +2743,9 @@ static int bnge_request_irq(struct bnge_net *bn)
 		int map_idx = bnge_cp_num_to_irq_num(bn, i);
 		struct bnge_irq *irq = &bd->irq_tbl[map_idx];
 
+		if (BNGE_NQ0_NAPI(bn->bnapi[i]))
+			continue;
+
 		rc = request_irq(irq->vector, irq->handler, 0, irq->name,
 				 bn->bnapi[i]);
 		if (rc)
@@ -2840,6 +2885,10 @@ static void bnge_init_napi(struct bnge_net *bn)
 
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		bnapi = bn->bnapi[i];
+
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
+
 		netif_napi_add_config_locked(bn->netdev, &bnapi->napi,
 					     bnge_napi_poll, bnapi->index);
 	}
@@ -2858,6 +2907,9 @@ static void bnge_del_napi(struct bnge_net *bn)
 	for (i = 0; i < bd->nq_nr_rings; i++) {
 		struct bnge_napi *bnapi = bn->bnapi[i];
 
+		if (BNGE_NQ0_NAPI(bnapi))
+			continue;
+
 		__netif_napi_del_locked(&bnapi->napi);
 	}
 
@@ -3237,6 +3289,8 @@ static void bnge_close_core(struct bnge_net *bn)
 	clear_bit(BNGE_STATE_STATS_ENABLE, &bn->state);
 	spin_unlock_bh(&bn->stats_lock);
 
+	bnge_quiesce_nq0(bn);
+
 	bnge_free_all_rings_bufs(bn);
 	bnge_free_irq(bn);
 	bnge_del_napi(bn);
@@ -3467,6 +3521,159 @@ static void bnge_init_ring_params(struct bnge_net *bn)
 	bn->netdev->cfg->hds_thresh = max(BNGE_DEFAULT_RX_COPYBREAK, rx_size);
 }
 
+static void bnge_free_nq0(struct bnge_net *bn)
+{
+	struct bnge_nq_ring_info *nqr;
+	struct bnge_ring_struct *ring;
+	struct bnge_dev *bd = bn->bd;
+	struct bnge_napi *bnapi;
+	struct bnge_irq *irq;
+
+	bnapi = bn->bnapi[BNGE_NQ0_NAPI_IDX];
+	nqr = &bnapi->nq_ring;
+	ring = &nqr->ring_struct;
+	irq = &bd->irq_tbl[ring->map_idx];
+
+	if (!BNGE_NQ0_NAPI(bnapi)) {
+		/* A previous bnge_setup_nq0() could have failed
+		 * leaving behind an active irq.
+		 */
+		goto free_irq;
+	}
+
+	clear_bit(BNGE_NAPI_FLAG_NQ0, &bnapi->flags);
+	clear_bit(BNGE_NAPI_FLAG_NQ0_QUIESCED, &bnapi->flags);
+
+	/* Unlike the other NQs, NQ0's NAPI is left enabled by bnge_disable_napi()
+	 * so it can keep processing async events while the interface is
+	 * administratively down. It is explicitly disabled below, or was never
+	 * enabled if netdev was never opened (netif_napi_add default).
+	 */
+	bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
+	synchronize_irq(irq->vector);
+
+	hwrm_ring_free_send_msg(bn, ring,
+				RING_FREE_REQ_RING_TYPE_NQ,
+				INVALID_HW_RING_ID);
+	ring->fw_ring_id = INVALID_HW_RING_ID;
+	bn->grp_info[0].nq_fw_ring_id = INVALID_HW_RING_ID;
+
+free_irq:
+	if (irq->requested) {
+		if (irq->have_cpumask) {
+			irq_set_affinity_hint(irq->vector, NULL);
+			free_cpumask_var(irq->cpu_mask);
+			irq->have_cpumask = 0;
+		}
+		free_irq(irq->vector, bnapi);
+		irq->requested = 0;
+
+		netdev_lock(bn->netdev);
+		napi_disable_locked(&bnapi->napi);
+		__netif_napi_del_locked(&bnapi->napi);
+		netdev_unlock(bn->netdev);
+
+		/* We called __netif_napi_del_locked(), we need
+		 * grace period before freeing napi structures.
+		 */
+		synchronize_net();
+	}
+
+	bnge_free_ring(bd, &ring->ring_mem);
+	bnge_free_nq_desc_arr(nqr);
+}
+
+static int bnge_setup_nq0(struct bnge_net *bn)
+{
+	struct bnge_nq_ring_info *nqr;
+	struct bnge_ring_struct *ring;
+	struct bnge_dev *bd = bn->bd;
+	struct bnge_napi *bnapi;
+	struct bnge_irq *irq;
+	int map_idx, rc;
+
+	bnapi = bn->bnapi[BNGE_NQ0_NAPI_IDX];
+	if (BNGE_NQ0_NAPI(bnapi))
+		return 0;
+
+	nqr = &bnapi->nq_ring;
+	ring = &nqr->ring_struct;
+	rc = bnge_alloc_nq_desc_arr(&bnapi->nq_ring, bn->cp_nr_pages);
+	if (rc)
+		return -ENOMEM;
+
+	bnge_init_nq_ring_struct(bn, nqr);
+	rc = bnge_alloc_ring(bd, &ring->ring_mem);
+	if (rc)
+		goto err_free_nq_desc_arr;
+
+	map_idx = bnge_aux_get_msix(bd);
+	ring->map_idx = map_idx;
+	irq = &bd->irq_tbl[map_idx];
+	irq->handler = bnge_msix;
+
+	netdev_lock(bn->netdev);
+	netif_napi_add_config_locked(bn->netdev, &bnapi->napi,
+				     bnge_napi_poll, bnapi->index);
+	netdev_unlock(bn->netdev);
+
+	snprintf(irq->name, sizeof(bd->irq_tbl[0].name), "%s-%s-%d", "bnge",
+		 "nq", map_idx);
+	rc = request_irq(irq->vector, irq->handler, 0, irq->name, bnapi);
+	if (rc)
+		goto err_del_napi;
+
+	netdev_lock(bn->netdev);
+	netif_napi_set_irq_locked(&bnapi->napi, irq->vector);
+	netdev_unlock(bn->netdev);
+	irq->requested = 1;
+
+	if (zalloc_cpumask_var(&irq->cpu_mask, GFP_KERNEL)) {
+		int numa_node = dev_to_node(&bd->pdev->dev);
+
+		irq->have_cpumask = 1;
+		cpumask_set_cpu(cpumask_local_spread(BNGE_NQ0_NAPI_IDX, numa_node),
+				irq->cpu_mask);
+		rc = irq_set_affinity_hint(irq->vector, irq->cpu_mask);
+		if (rc) {
+			netdev_warn(bn->netdev,
+				    "Set affinity failed, IRQ = %d\n",
+				    irq->vector);
+			goto err_free_irq;
+		}
+	}
+
+	rc = bnge_hwrm_nq_ring_alloc(bn, BNGE_NQ0_NAPI_IDX);
+	if (rc)
+		goto err_free_irq;
+
+	netdev_lock(bn->netdev);
+	napi_enable_locked(&bnapi->napi);
+	netdev_unlock(bn->netdev);
+	bnge_db_nq_arm(bn, &nqr->nq_db, nqr->nq_raw_cons);
+
+	set_bit(BNGE_NAPI_FLAG_NQ0, &bnapi->flags);
+
+	return 0;
+
+err_free_irq:
+	if (irq->have_cpumask) {
+		irq_set_affinity_hint(irq->vector, NULL);
+		free_cpumask_var(irq->cpu_mask);
+		irq->have_cpumask = 0;
+	}
+	free_irq(irq->vector, bnapi);
+	irq->requested = 0;
+err_del_napi:
+	netdev_lock(bn->netdev);
+	__netif_napi_del_locked(&bnapi->napi);
+	netdev_unlock(bn->netdev);
+	bnge_free_ring(bd, &ring->ring_mem);
+err_free_nq_desc_arr:
+	bnge_free_nq_desc_arr(nqr);
+	return rc;
+}
+
 int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
 {
 	struct net_device *netdev;
@@ -3594,14 +3801,20 @@ int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
 	if (rc)
 		goto err_free_bnapi_mem;
 
+	rc = bnge_setup_nq0(bn);
+	if (rc)
+		goto err_free_ring_grps;
+
 	rc = register_netdev(netdev);
 	if (rc) {
 		dev_err(bd->dev, "Register netdev failed rc: %d\n", rc);
-		goto err_free_ring_grps;
+		goto err_free_nq0;
 	}
 
 	return 0;
 
+err_free_nq0:
+	bnge_free_nq0(bn);
 err_free_ring_grps:
 	bnge_free_ring_grps(bn);
 err_free_bnapi_mem:
@@ -3624,6 +3837,8 @@ void bnge_netdev_free(struct bnge_dev *bd)
 
 	unregister_netdev(netdev);
 
+	bnge_free_nq0(bn);
+
 	timer_shutdown_sync(&bn->timer);
 	cancel_work_sync(&bn->sp_task);
 	bn->sp_event = 0;
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
index 4d84f109ad5f..e2fb15fd62b6 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
@@ -543,6 +543,7 @@ struct bnge_tx_ring_info {
 
 enum bnge_napi_flag {
 	BNGE_NAPI_FLAG_NQ0,
+	BNGE_NAPI_FLAG_NQ0_QUIESCED,
 };
 
 #define BNGE_NQ0_NAPI(bnapi)	(test_bit(BNGE_NAPI_FLAG_NQ0, &(bnapi)->flags))
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
index e0ddb2800c54..22fd4eeb3213 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_rmem.c
@@ -450,7 +450,8 @@ void bnge_init_ring_struct(struct bnge_net *bn)
 		struct bnge_tx_ring_info *txr;
 		struct bnge_ring_struct *ring;
 
-		bnge_init_nq_ring_struct(bn, &bnapi->nq_ring);
+		if (!BNGE_NQ0_NAPI(bnapi))
+			bnge_init_nq_ring_struct(bn, &bnapi->nq_ring);
 
 		rxr = bnapi->rx_ring;
 		if (!rxr)
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_txrx.c b/drivers/net/ethernet/broadcom/bnge/bnge_txrx.c
index 7d45e057f2e8..dcec6fdced30 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_txrx.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_txrx.c
@@ -1375,6 +1375,21 @@ int bnge_napi_poll(struct napi_struct *napi, int budget)
 				break;
 
 			idx = BNGE_NQ_HDL_IDX(idx);
+
+			/* NQ0 keeps running while administratively down to
+			 * process async events, but its cp_ring_arr is torn
+			 * down (and cp_ring_count zeroed) by
+			 * bnge_free_nq_tree() while down. Guard against a
+			 * stray/late CQ notification arriving in that state
+			 * instead of dereferencing a freed or out-of-range
+			 * cp_ring_arr.
+			 */
+			if (unlikely(!nqr->cp_ring_arr ||
+				     idx >= nqr->cp_ring_count)) {
+				raw_cons = NEXT_RAW_CMP(raw_cons);
+				continue;
+			}
+
 			cpr = &nqr->cp_ring_arr[idx];
 			cpr->had_nqe_notify = 1;
 			cpr->toggle = NQE_CN_TOGGLE(type);
-- 
2.47.3

Re: [PATCH net-next 6/7] bnge: Create NQ0 during probe and keep active across open/close

[netdev-bot+sashiko]

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

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

- [High] bn->grp_info is now probe-lifetime state (allocated only via
  bnge_init_ring_grps(bn, true) in bnge_netdev_alloc(); reopen passes…
- [High] bnge_setup_nq0() freezes NQ0's ring->map_idx at probe to
  bnge_aux_get_msix(bd) (== bd->aux_num_msix, call it A), allocates the…
- [Medium] Commit message omits two load-bearing behavioural changes and
  actively asserts the opposite.
- [Medium] bnge_close_core() calls bnge_quiesce_nq0() only *after*
  bnge_shutdown_nic().
- [Medium] bnge_setup_nq0() treats failure to install the optional IRQ
  affinity *hint* as fatal, so on CONFIG_SMP=n kernels every bnge probe…
- [Medium] The new unconditional NQ0 setup dereferences bn->bnapi[0] and
  bd->irq_tbl[bnge_aux_get_msix(bd)] without checking that at least one…
- [Medium] bnge_free_nq0() frees the NQ0 hardware ring and invalidates
  its ring ids while NQ0's NAPI is still enabled and pollable.
- [Low] bnge_free_nq0() contains two comments that contradict
  bnge_setup_nq0(), and the branch one of them justifies is both dead…

--

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

This moves NQ0 creation into probe via bnge_setup_nq0() and frees it in
bnge_free_nq0() at remove, and teaches the open/close ring, IRQ and NAPI
helpers to skip NQ0 once BNGE_NAPI_FLAG_NQ0 is set.  It also adds a guard
in bnge_napi_poll() for completions of rings freed by bnge_free_nq_tree().

A few questions about the new NQ0 lifetime below.

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

[ ... ]

> @@ -592,6 +599,9 @@ static void bnge_quiesce_nq0(struct bnge_net *bn)
>  	if (!BNGE_NQ0_NAPI(bnapi))
>  		return;
>  
> +	if (test_and_set_bit(BNGE_NAPI_FLAG_NQ0_QUIESCED, &bnapi->flags))
> +		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);
> @@ -606,6 +616,9 @@ static void bnge_resume_nq0(struct bnge_net *bn)
>  	if (!BNGE_NQ0_NAPI(bnapi))
>  		return;
>  
> +	if (!test_and_clear_bit(BNGE_NAPI_FLAG_NQ0_QUIESCED, &bnapi->flags))
> +		return;
> +
>  	napi_enable_locked(&bnapi->napi);
>  	bnge_db_nq_arm(bn, &nqr->nq_db, nqr->nq_raw_cons);
>  }

[Severity: Medium]
The changelog says:

    The previous patch already added the bnge_quiesce_nq0()/bnge_resume_nq0()
    guard around bnge_free_core()'s teardown and bnge_alloc_core()'s rebuild,
    so NQ0 is safe to leave active across close/open from the moment
    BNGE_NAPI_FLAG_NQ0 is set below.

That reads as though no further quiesce work was needed, but this patch also
adds the new BNGE_NAPI_FLAG_NQ0_QUIESCED bit and a second bnge_quiesce_nq0()
call in bnge_close_core().  Could the changelog describe both?

The new bit looks load bearing rather than cosmetic.  bnge_close_core() now
does:

    bnge_quiesce_nq0(bn);
    ...
    bnge_free_core(bn);        /* calls bnge_quiesce_nq0() again */

Without the test_and_set_bit() guard, would the second call reach
napi_disable_locked() on an already disabled NAPI and spin forever in:

    net/core/dev.c:napi_disable_locked() {
        while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
            usleep_range(20, 200);
            val = READ_ONCE(n->state);
        }
    }

since nothing clears SCHED/NPSVC for an already disabled NAPI?  A backport
or split that keeps the earlier bnge_free_core() quiesce but drops this bit
would then hang on every "ip link set down".

Separately, the quiesce issued in bnge_close_core() is matched by the
bnge_resume_nq0() at the tail of bnge_free_core(), i.e. NQ0's NAPI is only
re-enabled because bnge_free_core() happens to be the last statement of
bnge_close_core().  Is that cross-function pairing intentional?  An early
return in bnge_free_core(), or any close path that does not end there, would
leave NQ0's NAPI disabled for good and silently defeat the async-event
handling this series adds.

> @@ -658,11 +671,13 @@ static int bnge_alloc_nq_tree(struct bnge_net *bn)
>  		nqr->bnapi = bnapi;
>  		ring = &nqr->ring_struct;
>  
> -		rc = bnge_alloc_ring(bd, &ring->ring_mem);
> -		if (rc)
> -			goto err_free_nq_tree;
> +		if (!BNGE_NQ0_NAPI(bnapi)) {
> +			rc = bnge_alloc_ring(bd, &ring->ring_mem);
> +			if (rc)
> +				goto err_free_nq_tree;
>  
> -		ring->map_idx = ulp_msix + i;
> +			ring->map_idx = ulp_msix + i;
> +		}
>  
>  		if (i < bd->rx_nr_rings) {
>  			cp_count++;

[ ... ]

> @@ -2612,6 +2648,9 @@ static void bnge_hwrm_ring_free(struct bnge_net *bn, bool close_path)
>  		for (j = 0; j < nqr->cp_ring_count && nqr->cp_ring_arr; j++)
>  			bnge_hwrm_cp_ring_free(bn, &nqr->cp_ring_arr[j]);
>  
> +		if (BNGE_NQ0_NAPI(bnapi))
> +			continue;
> +
>  		ring = &nqr->ring_struct;
>  		if (ring->fw_ring_id != INVALID_HW_RING_ID_32BIT) {
>  			hwrm_ring_free_send_msg(bn, ring,
> @@ -2673,6 +2712,9 @@ static void bnge_free_irq(struct bnge_net *bn)
>  	for (i = 0; i < bd->nq_nr_rings; i++) {
>  		int map_idx = bnge_cp_num_to_irq_num(bn, i);
>  
> +		if (BNGE_NQ0_NAPI(bn->bnapi[i]))
> +			continue;
> +
>  		irq = &bd->irq_tbl[map_idx];
>  		if (irq->requested) {
>  			if (irq->have_cpumask) {
> @@ -2701,6 +2743,9 @@ static int bnge_request_irq(struct bnge_net *bn)
>  		int map_idx = bnge_cp_num_to_irq_num(bn, i);
>  		struct bnge_irq *irq = &bd->irq_tbl[map_idx];
>  
> +		if (BNGE_NQ0_NAPI(bn->bnapi[i]))
> +			continue;
> +
>  		rc = request_irq(irq->vector, irq->handler, 0, irq->name,
>  				 bn->bnapi[i]);
>  		if (rc)

[ ... ]

> @@ -3237,6 +3289,8 @@ static void bnge_close_core(struct bnge_net *bn)
>  	clear_bit(BNGE_STATE_STATS_ENABLE, &bn->state);
>  	spin_unlock_bh(&bn->stats_lock);
>  
> +	bnge_quiesce_nq0(bn);
> +
>  	bnge_free_all_rings_bufs(bn);
>  	bnge_free_irq(bn);
>  	bnge_del_napi(bn);

[Severity: Medium]
Is it safe to quiesce NQ0 only after bnge_shutdown_nic()?  In
bnge_close_core() the order is now:

    bnge_shutdown_nic(bn);
    bnge_disable_napi(bn);        /* skips NQ0 */
    ... stats ...
    bnge_quiesce_nq0(bn);

Because this patch makes bnge_disable_int()/bnge_disable_int_sync() and
bnge_disable_napi() skip NQ0, NQ0's doorbell stays armed, its IRQ is never
synchronized and its NAPI stays enabled across bnge_shutdown_nic() ->
bnge_hwrm_resource_free() -> bnge_hwrm_ring_free(), which still releases
NQ0's completion rings in firmware because the new "continue" sits after the
cp_ring_arr loop:

    for (j = 0; j < nqr->cp_ring_count && nqr->cp_ring_arr; j++)
        bnge_hwrm_cp_ring_free(bn, &nqr->cp_ring_arr[j]);

    if (BNGE_NQ0_NAPI(bnapi))
        continue;

That is the case the comment just above bnge_disable_int_sync() in
bnge_hwrm_ring_free() warns about:

    /* The completion rings are about to be freed.  After that the
     * IRQ doorbell will not work anymore.  So we need to disable
     * IRQ here.
     */

In that window, can an NQ0 interrupt still take a CQ notification for a ring
firmware has already released, set cpr->had_nqe_notify/toggle, run
__bnge_poll_work() and then write that ring's doorbell from
__bnge_poll_cqs_done()?

The stray-completion guard added to bnge_napi_poll() does not seem to cover
this window, since nqr->cp_ring_arr is still non-NULL and nqr->cp_ring_count
is unchanged until bnge_free_core() -> bnge_free_nq_tree() runs later.  Would
moving bnge_quiesce_nq0() ahead of bnge_shutdown_nic() close it?

[ ... ]

> @@ -3467,6 +3521,159 @@ static void bnge_init_ring_params(struct bnge_net *bn)
>  	bn->netdev->cfg->hds_thresh = max(BNGE_DEFAULT_RX_COPYBREAK, rx_size);
>  }
>  
> +static void bnge_free_nq0(struct bnge_net *bn)
> +{
> +	struct bnge_nq_ring_info *nqr;
> +	struct bnge_ring_struct *ring;
> +	struct bnge_dev *bd = bn->bd;
> +	struct bnge_napi *bnapi;
> +	struct bnge_irq *irq;
> +
> +	bnapi = bn->bnapi[BNGE_NQ0_NAPI_IDX];
> +	nqr = &bnapi->nq_ring;
> +	ring = &nqr->ring_struct;
> +	irq = &bd->irq_tbl[ring->map_idx];
> +
> +	if (!BNGE_NQ0_NAPI(bnapi)) {
> +		/* A previous bnge_setup_nq0() could have failed
> +		 * leaving behind an active irq.
> +		 */
> +		goto free_irq;
> +	}
> +
> +	clear_bit(BNGE_NAPI_FLAG_NQ0, &bnapi->flags);
> +	clear_bit(BNGE_NAPI_FLAG_NQ0_QUIESCED, &bnapi->flags);
> +
> +	/* Unlike the other NQs, NQ0's NAPI is left enabled by bnge_disable_napi()
> +	 * so it can keep processing async events while the interface is
> +	 * administratively down. It is explicitly disabled below, or was never
> +	 * enabled if netdev was never opened (netif_napi_add default).
> +	 */

[Severity: Low]
Are these two comments accurate?

For the second one, bnge_setup_nq0() always runs napi_enable_locked() before
set_bit(BNGE_NAPI_FLAG_NQ0, ...), and it runs from bnge_netdev_alloc() before
register_netdev(), so whenever the flag is set the NAPI is enabled whether or
not the netdev was ever opened.  Is the "was never enabled if netdev was
never opened" case reachable?

For the first one, which state can leave "an active irq" behind with the flag
clear?  bnge_setup_nq0() runs once, every failure after request_irq() goes
through err_free_irq/err_del_napi which call free_irq() and clear
irq->requested, and bnge_netdev_alloc() jumps to err_free_ring_grps rather
than err_free_nq0 when bnge_setup_nq0() fails:

    rc = bnge_setup_nq0(bn);
    if (rc)
        goto err_free_ring_grps;

So both callers of bnge_free_nq0() only reach it with BNGE_NAPI_FLAG_NQ0 set,
making the goto dead code.  If it ever did become reachable it would act on
the wrong resources, since irq is computed from ring->map_idx before the flag
test, and map_idx is only assigned after the descriptor and ring allocations
succeed, so it would be 0 from the zeroed bnapi, which is an aux/RoCE vector
whenever bnge_aux_get_msix(bd) > 0.

> +	bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
> +	synchronize_irq(irq->vector);
> +
> +	hwrm_ring_free_send_msg(bn, ring,
> +				RING_FREE_REQ_RING_TYPE_NQ,
> +				INVALID_HW_RING_ID);
> +	ring->fw_ring_id = INVALID_HW_RING_ID;

[Severity: Medium]
Can the firmware NQ be freed here while NQ0's NAPI is still enabled and
pollable?  Only bnge_db_nq() plus synchronize_irq() run before
hwrm_ring_free_send_msg(); napi_disable_locked() happens much later, and only
inside the "if (irq->requested)" block below.

synchronize_irq() waits for bnge_msix(), not for a poll that handler already
scheduled, and the poll re-arms the NQ on the way out:

    drivers/net/ethernet/broadcom/bnge/bnge_txrx.c:bnge_napi_poll() {
        ...
        nqr->nq_raw_cons = raw_cons;
        if (napi_complete_done(napi, work_done))
            BNGE_DB_NQ_ARM(bd, &nqr->nq_db, nqr->nq_raw_cons);
        ...
    }

With a poll still active, would hwrm_ring_free_send_msg() run concurrently
with code that keeps consuming NQ entries of the ring firmware just released,
writes NQ doorbells from the stale db_key/xid, and updates nqr->nq_raw_cons?
Async events dispatched from bnge_hwrm_handler() -> bnge_queue_sp_work() in
that window also land before the timer_shutdown_sync()/cancel_work_sync() in
bnge_netdev_free().

bnge_quiesce_nq0() already uses the other order, db_nq -> synchronize_irq ->
napi_disable_locked(), before touching any ring state.  Should bnge_free_nq0()
do the same before the HWRM free?

> +	bn->grp_info[0].nq_fw_ring_id = INVALID_HW_RING_ID;

[Severity: High]
Can bn->grp_info be NULL here?

bn->grp_info is now probe lifetime state: it is allocated only by
bnge_init_ring_grps(bn, true) from bnge_netdev_alloc(), and the open path
passes irq_re_init == false and dereferences it without reallocating.  But
bnge_init_nic()'s error path still frees it:

    err_free_ring_grps:
        bnge_free_ring_grps(bn);        /* kfree(bn->grp_info); bn->grp_info = NULL; */
        bnge_free_rx_ring_pair_bufs(bn);
        return rc;

After one failed "ip link set up" (any bnge_init_chip() HWRM failure such as
stat ctx alloc, ring alloc, vnic alloc or vnic filter) bn->grp_info stays NULL
for the life of the device while the netdev remains registered.  Then:

  - remove/unbind runs bnge_netdev_free() -> bnge_free_nq0(), and this new
    unconditional store writes through a NULL bn->grp_info

  - a later reopen calls bnge_init_ring_grps(bn, false), which skips the
    allocation and writes bn->grp_info[i].fw_grp_id through NULL

  - NQ0's preserved firmware ring id, the value hwrm_ring_alloc_send_msg()
    uses as req->nq_ring_id for HWRM_RING_ALLOC_CMPL, is lost, so the
    CQ-to-NQ0 association breaks

Should the open error path stop freeing grp_info now that it outlives open,
or should this store be guarded?

> +
> +free_irq:
> +	if (irq->requested) {
> +		if (irq->have_cpumask) {
> +			irq_set_affinity_hint(irq->vector, NULL);
> +			free_cpumask_var(irq->cpu_mask);
> +			irq->have_cpumask = 0;
> +		}
> +		free_irq(irq->vector, bnapi);
> +		irq->requested = 0;
> +
> +		netdev_lock(bn->netdev);
> +		napi_disable_locked(&bnapi->napi);
> +		__netif_napi_del_locked(&bnapi->napi);
> +		netdev_unlock(bn->netdev);
> +
> +		/* We called __netif_napi_del_locked(), we need
> +		 * grace period before freeing napi structures.
> +		 */
> +		synchronize_net();
> +	}
> +
> +	bnge_free_ring(bd, &ring->ring_mem);
> +	bnge_free_nq_desc_arr(nqr);
> +}
> +
> +static int bnge_setup_nq0(struct bnge_net *bn)
> +{
> +	struct bnge_nq_ring_info *nqr;
> +	struct bnge_ring_struct *ring;
> +	struct bnge_dev *bd = bn->bd;
> +	struct bnge_napi *bnapi;
> +	struct bnge_irq *irq;
> +	int map_idx, rc;
> +
> +	bnapi = bn->bnapi[BNGE_NQ0_NAPI_IDX];
> +	if (BNGE_NQ0_NAPI(bnapi))
> +		return 0;

[Severity: Medium]
Should this check that a network NQ exists before indexing bn->bnapi[0]?

bnge_alloc_irqs() accepts the equality case:

    if (irqs_demand < 0 || irqs_demand < aux_msix) {
        rc = -ENODEV;
        goto err_free_irqs;
    }
    ...
    rc = bnge_adjust_rings(bd, &bd->rx_nr_rings, &bd->tx_nr_rings,
                           irqs_demand - aux_msix, min == 1);

so when pci_alloc_irq_vectors() returns exactly aux_msix vectors,
bnge_adjust_rings() gets max == 0 and the shared branch of
bnge_fix_rings_count() does:

    *rx = min_t(u16, _rx, max);
    *tx = min_t(u16, _tx, max);

returning success with both counts zero, leaving bd->nq_nr_rings == 0.
bnge_alloc_bnapi_mem() then does a zero sized kzalloc(), which returns
ZERO_SIZE_PTR rather than NULL, and creates no bnapi[0] entry.  Does
bn->bnapi[BNGE_NQ0_NAPI_IDX] then read from ZERO_SIZE_PTR during probe?  The
bd->irq_tbl[bnge_aux_get_msix(bd)] access below would also be one past the
acquired vectors in that case.

> +
> +	nqr = &bnapi->nq_ring;
> +	ring = &nqr->ring_struct;
> +	rc = bnge_alloc_nq_desc_arr(&bnapi->nq_ring, bn->cp_nr_pages);
> +	if (rc)
> +		return -ENOMEM;
> +
> +	bnge_init_nq_ring_struct(bn, nqr);
> +	rc = bnge_alloc_ring(bd, &ring->ring_mem);
> +	if (rc)
> +		goto err_free_nq_desc_arr;
> +
> +	map_idx = bnge_aux_get_msix(bd);
> +	ring->map_idx = map_idx;
> +	irq = &bd->irq_tbl[map_idx];
> +	irq->handler = bnge_msix;

[Severity: High]
Is bnge_aux_get_msix(bd) stable enough to freeze NQ0's map_idx here for the
device lifetime?

NQ0's slot is captured once at probe (call it A), used as the firmware
logical_id by hwrm_ring_alloc_send_msg(), used to derive the NQ doorbell in
bnge_set_db(), and claimed with request_irq(irq_tbl[A].vector, ..., bnapi0),
irq_tbl[A].requested = 1 and irq_tbl[A].cpu_mask.  It is never revalidated,
because bnge_hwrm_nq_ring_alloc() returns early for NQ0 and
bnge_alloc_nq_tree() skips the map_idx assignment for NQ0, while all other
NQs recompute ring->map_idx = bnge_aux_get_msix(bd) + i on every open.

bd->aux_num_msix is recomputed on the open path when the aux device is not
registered:

    drivers/net/ethernet/broadcom/bnge/bnge_resc.c:bnge_reserve_rings() {
        if (!bnge_aux_registered(bd)) {
            ...
            resv_msix = hw_resc->resv_irqs - bd->nq_nr_rings;
            aux_msix = min_t(u16, resv_msix, aux_msix);
            bnge_aux_set_msix_num(bd, aux_msix);
        }
    }

If the new base B is smaller than A, does ring i == A - B get handed NQ0's
slot?  bnge_request_irq() only skips i == 0, so request_irq() would hit NQ0's
already registered vector without IRQF_SHARED, return -EBUSY, and the
err_free_irq path would run bnge_free_irq(), which sees NQ0's
requested/have_cpumask state, drops NQ0's affinity hint, frees NQ0's
cpu_mask, calls free_irq(vector, bn->bnapi[i]) with a mismatched dev_id
(__free_irq() only warns and leaves the action installed) and clears
irq->requested.

After that, bnge_free_nq0() at remove sees "if (irq->requested)" as false, so
it never frees the IRQ and never disables or deletes NQ0's NAPI, while
bnge_remove_one() goes on to bnge_free_irqs() and free_netdev().  Does that
leave an installed handler whose dev_id and napi_struct have been freed?

If the base grows instead, NQ0's frozen slot falls inside
[0, bd->aux_num_msix), which bnge_register_dev()/bnge_fill_msix_vecs() hand
to the RoCE driver as its own vectors while firmware still has NQ0 bound to
that logical_id and doorbell.  Should NQ0's vector be reserved explicitly, or
its map_idx re-derived on open?

[ ... ]

> +	if (zalloc_cpumask_var(&irq->cpu_mask, GFP_KERNEL)) {
> +		int numa_node = dev_to_node(&bd->pdev->dev);
> +
> +		irq->have_cpumask = 1;
> +		cpumask_set_cpu(cpumask_local_spread(BNGE_NQ0_NAPI_IDX, numa_node),
> +				irq->cpu_mask);
> +		rc = irq_set_affinity_hint(irq->vector, irq->cpu_mask);
> +		if (rc) {
> +			netdev_warn(bn->netdev,
> +				    "Set affinity failed, IRQ = %d\n",
> +				    irq->vector);
> +			goto err_free_irq;
> +		}
> +	}

[Severity: Medium]
Should a failure to install the optional affinity hint be fatal to probe?

On CONFIG_SMP=n the branch is always entered and the hint always fails:

    include/linux/cpumask.h
    static __always_inline bool zalloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
    {
        cpumask_clear(*mask);
        return true;
    }

    include/linux/interrupt.h
    static inline int irq_set_affinity_hint(unsigned int irq,
                                            const struct cpumask *m)
    {
        return -EINVAL;
    }

So bnge_setup_nq0() takes err_free_irq and returns -EINVAL, which
bnge_netdev_alloc() propagates up to bnge_probe_one(), and the device never
probes on uniprocessor builds.  The same pattern already exists in
bnge_request_irq(), where it only broke bringing the interface up; here it
moves the failure to probe.  Would a netdev_warn() without the goto be
enough?

> +
> +	rc = bnge_hwrm_nq_ring_alloc(bn, BNGE_NQ0_NAPI_IDX);
> +	if (rc)
> +		goto err_free_irq;

[ ... ]

>  int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
>  {
>  	struct net_device *netdev;
> @@ -3594,14 +3801,20 @@ int bnge_netdev_alloc(struct bnge_dev *bd, int max_irqs)
>  	if (rc)
>  		goto err_free_bnapi_mem;
>  
> +	rc = bnge_setup_nq0(bn);
> +	if (rc)
> +		goto err_free_ring_grps;
> +
>  	rc = register_netdev(netdev);
>  	if (rc) {
>  		dev_err(bd->dev, "Register netdev failed rc: %d\n", rc);
> -		goto err_free_ring_grps;
> +		goto err_free_nq0;
>  	}
>  
>  	return 0;
>  
> +err_free_nq0:
> +	bnge_free_nq0(bn);
>  err_free_ring_grps:
>  	bnge_free_ring_grps(bn);
>  err_free_bnapi_mem:

[ ... ]

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

[PATCH net-next 7/7] bnge: Process async events while administratively down

[Bhargava Marreddy]

Restructure bnge_sp_task() to skip only periodic stats via goto when the
interface is down, allowing it to fall through to async event handling.
Previously, clearing BNGE_STATE_OPEN caused an early return that dropped all
async events while down.

Additionally, add a netif_running() early return to bnge_report_link() to avoid
updating carrier state or printing link messages while closed, while still
updating bd->link_info safely in the background.

Signed-off-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
Reviewed-by: Rajashekar Hudumula <rajashekar.hudumula@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnge/bnge_link.c   | 3 +++
 drivers/net/ethernet/broadcom/bnge/bnge_netdev.c | 7 +++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_link.c b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
index 3b0dfcf27376..31e2ede2e537 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_link.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
@@ -402,6 +402,9 @@ static char *bnge_report_fec(struct bnge_link_info *link_info)
 
 void bnge_report_link(struct bnge_dev *bd)
 {
+	if (!netif_running(bd->netdev))
+		return;
+
 	if (BNGE_LINK_IS_UP(bd)) {
 		const char *signal = "";
 		const char *flow_ctrl;
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index d11c404acd4e..49e27cbc6394 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -394,10 +394,8 @@ static void bnge_sp_task(struct work_struct *work)
 	struct bnge_dev *bd = bn->bd;
 
 	netdev_lock(bn->netdev);
-	if (!test_bit(BNGE_STATE_OPEN, &bd->state)) {
-		netdev_unlock(bn->netdev);
-		return;
-	}
+	if (!test_bit(BNGE_STATE_OPEN, &bd->state))
+		goto async_evt;
 
 	if (test_and_clear_bit(BNGE_PERIODIC_STATS_SP_EVENT, &bn->sp_event)) {
 		bnge_hwrm_port_qstats(bd, 0);
@@ -405,6 +403,7 @@ static void bnge_sp_task(struct work_struct *work)
 		bnge_accumulate_all_stats(bd);
 	}
 
+async_evt:
 	if (test_and_clear_bit(BNGE_UPDATE_PHY_SP_EVENT, &bn->sp_event)) {
 		int rc;
 
-- 
2.47.3

Re: [PATCH net-next 7/7] bnge: Process async events while administratively down

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 3 (1) · Low: 0

- [High] bnge_sp_task(): replacing the early return with `goto async_evt`
  lets the entire link/PHY event block execute while BNGE_STATE_OPEN is…
- [High] Processing PHY events while administratively down inadvertently
  turns the hardware link back on.
- [Medium] The new `if (!netif_running(bd->netdev)) return;` in
  bnge_report_link() (bnge_link.c:405) silently drops a report that its…
- [Medium] bnge_close() ends with the plain, non-atomic store
  `bn->sp_event = 0;` (bnge_netdev.c:3307) while NQ0's NAPI and IRQ are…
- [Medium] A speed/PHY-configuration async event processed while the
  interface is closed silently overwrites the link and pause settings…

Pre-existing issues:
- [Medium] bnge_free_nq0() (bnge_netdev.c:3523-3583) sends HWRM_RING_FREE
  for NQ0 and invalidates ring->fw_ring_id before it disables the NAPI…

--

bnge: Process async events while administratively down

bnge_sp_task() now jumps over the periodic-stats block with a goto instead
of returning early when BNGE_STATE_OPEN is clear, so async link/PHY events
are handled while the interface is down.  bnge_report_link() gains an early
return when the netdev is not running.

A few questions about the new behaviour below.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_link.c b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> index 3b0dfcf273762..31e2ede2e537a 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> @@ -402,6 +402,9 @@ static char *bnge_report_fec(struct bnge_link_info *link_info)
>  
>  void bnge_report_link(struct bnge_dev *bd)
>  {
> +	if (!netif_running(bd->netdev))
> +		return;
> +

[Severity: Medium]
Can this early return make a link transition disappear entirely instead of
just deferring the message?

The only caller is edge-triggered.  bnge_update_link() commits the new
state and reports only when it changed:

	if (chng_link_state) {
		if (link_info->phy_link_status == BNGE_LINK_LINK)
			link_info->link_state = BNGE_LINK_STATE_UP;
		else
			link_info->link_state = BNGE_LINK_STATE_DOWN;
		if (link_state != link_info->link_state)
			bnge_report_link(bd);

bnge_hwrm_shutdown_link() relies on that, leaving a sentinel behind on
close:

	if (!rc) {
		/* Device is not obliged to link down in certain scenarios,
		 * even when forced. Setting the state unknown is consistent
		 * with driver startup and will force link state to be
		 * reported during subsequent open based on PORT_PHY_QCFG.
		 */
		bd->link_info.link_state = BNGE_LINK_STATE_UNKNOWN;
	}

With this patch, an async link event handled while down calls
bnge_update_link(bn, true), which replaces BNGE_LINK_STATE_UNKNOWN with
UP/DOWN and consumes the transition, while bnge_report_link() returns
immediately.  At the next open, bnge_open_core() ->
bnge_update_phy_setting() -> bnge_update_link() reads the same value, sees
no change, and never calls bnge_report_link().

Does that permanently lose the "NIC Link is Up, N Mbps ..." / "NIC Link is
Down" message and the FEC line for that transition, so whether link state
gets reported at all depends on whether an async event happened to arrive
while the port was down?  Carrier itself still gets fixed up by
bnge_tx_enable() via "if (BNGE_LINK_IS_UP(bn->bd)) netif_carrier_on()",
which means carrier and the log output are now driven by two different
paths.

>  	if (BNGE_LINK_IS_UP(bd)) {
>  		const char *signal = "";
>  		const char *flow_ctrl;
> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index d11c404acd4e2..49e27cbc63946 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -394,10 +394,8 @@ static void bnge_sp_task(struct work_struct *work)
>  	struct bnge_dev *bd = bn->bd;
>  
>  	netdev_lock(bn->netdev);
> -	if (!test_bit(BNGE_STATE_OPEN, &bd->state)) {
> -		netdev_unlock(bn->netdev);
> -		return;
> -	}
> +	if (!test_bit(BNGE_STATE_OPEN, &bd->state))
> +		goto async_evt;

[Severity: High]
Is the block below the async_evt label read-only with respect to the PHY?

For a speed-change event, bnge_sp_task() first calls
bnge_hwrm_phy_qcaps(bd), which refreshes the supported-speed mask:

	if (resp->supported_speeds2_auto_mode)
		link_info->support_auto_speeds2 =
			le16_to_cpu(resp->supported_speeds2_auto_mode);

and then calls bnge_update_link(bn, true), whose tail is:

	support_changed = bnge_support_speed_dropped(bn);
	if (support_changed && (bn->eth_link_info.autoneg & BNGE_AUTONEG_SPEED))
		rc = bnge_hwrm_set_link_setting(bn, true);

bnge_support_speed_dropped() returns true whenever the advertised mask
contains speeds that are no longer in support_auto_speeds2, which is what
happens when a transceiver is swapped for one supporting fewer speeds while
the port is administratively down.

bnge_hwrm_set_link_setting() -> bnge_hwrm_set_link_common() then builds an
HWRM_PORT_PHY_CFG with:

		req->flags |= cpu_to_le32(BNGE_PHY_FLAGS_RESTART_AUTO);
	...
	/* tell FW that the setting takes effect immediately */
	req->flags |= cpu_to_le32(PORT_PHY_CFG_REQ_FLAGS_RESET_PHY);

Does this undo the PORT_PHY_CFG_REQ_FLAGS_FORCE_LINK_DWN that bnge_close()
issued through bnge_hwrm_shutdown_link()?  Nothing re-asserts the force-down
afterwards, and since bnge_report_link() now returns early while not
running, the carrier stays off, so the link partner would see an active link
on a closed interface.

BNGE_PHY_CFG_ABLE(bd) is link_info.phy_enabled, a firmware capability set in
bnge_hwrm_phy_qcaps() and not cleared on close, so it does not block this.
Every other PHY-programming path is gated on netif_running() -
bnge_set_link_ksettings() and bnge_set_pauseparam() both do
"if (netif_running(dev))" before touching hardware.  Would it be better to
narrow the BNGE_STATE_OPEN gate to the read-only link-info refresh rather
than remove it for the whole block?

[Severity: Medium]
Related question about the same fall-through: further down, sp_task ends
with

	if (speed_chng || cfg_chng)
		bnge_init_ethtool_link_settings(bn);

bnge_init_ethtool_link_settings() rewrites elink_info->autoneg,
->advertising, ->req_duplex and ->req_flow_ctrl from the current hardware
auto_mode / duplex_setting / pause settings.

While the interface is down, bn->eth_link_info is the pending userspace
request: bnge_set_link_ksettings() and bnge_set_pauseparam() store into it
and only program hardware "if (netif_running(dev))", and
bnge_update_phy_setting() applies it at the next open.

If an async speed/config event arrives after "ethtool -s" or "ethtool -A"
was issued on a down interface, does this overwrite the queued request with
the hardware-derived values, so the next open sees nothing to change and the
user's setting is silently dropped?

[Severity: Medium]
One more on whether the events the fall-through wants actually survive
close.  bnge_close() ends with a plain store:

	bnge_close_core(bn);
	bnge_hwrm_shutdown_link(bn->bd);
	bnge_hwrm_if_change(bn->bd, false);
	bn->sp_event = 0;

At that point NQ0 is live again: bnge_close_core() -> bnge_free_core() ends
with bnge_resume_nq0(), which re-enables NQ0 NAPI and re-arms the doorbell,
and bnge_free_irq() skips the NQ0 vector.  bnge_hwrm_shutdown_link() forces
the port down, which itself generates a LINK_STATUS_CHANGE async event, and
the NAPI side records it with atomic bitops:

static void bnge_queue_sp_work(struct bnge_net *bn, unsigned int event)
{
	set_bit(event, &bn->sp_event);
	__bnge_queue_sp_work(bn);
}

Two questions here.  Is the plain word store racing with set_bit() from
softirq context on the same word, and can it clobber a bit set
concurrently?

And since bnge is ops-locked and bnge_sp_task() takes netdev_lock, the
queued worker cannot run until bnge_close() returns, i.e. after the wipe.
Does that mean the event the new async_evt path is meant to handle is
discarded before it can be observed, with nothing re-polling while down
because the periodic timer is stopped?  Would clearing only the bits that
are meaningless while down, using atomic bitops, be more consistent with
the goal of this patch?

>  
>  	if (test_and_clear_bit(BNGE_PERIODIC_STATS_SP_EVENT, &bn->sp_event)) {
>  		bnge_hwrm_port_qstats(bd, 0);
> @@ -405,6 +403,7 @@ static void bnge_sp_task(struct work_struct *work)
>  		bnge_accumulate_all_stats(bd);
>  	}
>  
> +async_evt:
>  	if (test_and_clear_bit(BNGE_UPDATE_PHY_SP_EVENT, &bn->sp_event)) {
>  		int rc;
>  

[Severity: High]
The first block that the new label falls into is the PHY retry, and
that one does not just read state - it programs the PHY.
bnge_sp_task() calls bnge_update_phy_setting(bn), whose tail is:

	if (!BNGE_LINK_IS_UP(bd))
		update_link = true;

	if (update_link)
		rc = bnge_hwrm_set_link_setting(bn, update_pause);

So the "link is currently down" case is precisely the case that sends
an HWRM_PORT_PHY_CFG to bring the link back up.  Running that while
BNGE_STATE_OPEN is clear looks like it undoes the
PORT_PHY_CFG_REQ_FLAGS_FORCE_LINK_DWN that bnge_close() issued via
bnge_hwrm_shutdown_link(), and nothing on this path re-asserts it
afterwards.  Combined with the other half of this patch -
bnge_report_link() returning early when !netif_running() - the carrier
stays off while the port is actually being brought up, so the link
partner sees an active link on an administratively down interface and
the driver logs nothing about it.

How does the submitter expect BNGE_UPDATE_PHY_SP_EVENT to be reachable
while down?  bd->link_info.phy_retry is only cleared on success here,
and BNGE_PHY_CFG_ABLE(bd)/link_info.phy_enabled is a firmware
capability that close does not clear, so a retry queued before the
state bit went away (for example a partially failed open, or a reset
that clears BNGE_STATE_OPEN without wiping bn->sp_event) would be
serviced here.  If that is possible at all, this block needs to stay
gated: every other PHY-programming path in the driver checks
netif_running() first (bnge_set_link_ksettings(),
bnge_set_pauseparam()).  Would it be cleaner to keep the
BNGE_STATE_OPEN check for the PHY retry and only let the read-only
link-info refresh run while down?

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the NQ0
persistence machinery this patch builds on: in bnge_free_nq0(), the
firmware ring is freed before the poller is disabled.

	bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
	synchronize_irq(irq->vector);

	hwrm_ring_free_send_msg(bn, ring,
				RING_FREE_REQ_RING_TYPE_NQ,
				INVALID_HW_RING_ID);
	ring->fw_ring_id = INVALID_HW_RING_ID;

and only later, inside the irq->requested block:

		free_irq(irq->vector, bnapi);
		irq->requested = 0;

		netdev_lock(bn->netdev);
		napi_disable_locked(&bnapi->napi);

synchronize_irq() waits for an in-flight handler but not for a NAPI
instance that handler already scheduled, so can bnge_napi_poll() still
write BNGE_DB_NQ()/BNGE_DB_NQ_ARM() using the doorbell key of a ring
firmware has already released?  The ring memory itself is freed after
napi_disable_locked() and synchronize_net(), so this looks limited to
doorbell writes against a freed firmware ring id on the removal and probe
error paths.  Would disabling and draining NAPI before sending
HWRM_RING_FREE be the right ordering?

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