[PATCH net-next 0/4] bnge: add more functionality

[PATCH net-next 0/4] bnge: add more functionality

[Vikas Gupta]

Hi,

This patch series adds a few functionality for bnge driver:

Patch 1: Makes bnge_shutdown() functional by properly unregistering the
         netdevice and freeing associated hardware queues and interrupts.
Patch 2: Implements ndo_set_rx_mode_async().

Patch 3: Adds a dedicated HWRM (Hardware Resource Management) command
         sequence to handle explicit interface down and up transitions cleanly.

Thanks,
Vikas

Vikas Gupta (4):
  bnge: add steps in bnge_shutdown()
  bnge: refactor rx mode helpers to accept explicit address lists
  bnge: add ndo_set_rx_mode_async support
  bnge: send hwrm for interface down/up transitions

 .../net/ethernet/broadcom/bnge/bnge_core.c    |  24 ++++
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 123 ++++++++++++++----
 .../net/ethernet/broadcom/bnge/bnge_netdev.h  |   2 +
 3 files changed, 126 insertions(+), 23 deletions(-)

-- 
2.47.1

[PATCH net-next 1/4] bnge: add steps in bnge_shutdown()

[Vikas Gupta]

Since the driver now implements ndo_open and ndo_close with full link
management, the shutdown callback must mirror the close sequence to
ensure the device is brought down cleanly.

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
Reviewed-by: Rahul Gupta <rahul-rg.gupta@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_core.c    | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
index 68b74eb2c3a2..9846d29b4fe3 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
@@ -5,12 +5,14 @@
 #include <linux/crash_dump.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/rtnetlink.h>
 
 #include "bnge.h"
 #include "bnge_devlink.h"
 #include "bnge_hwrm.h"
 #include "bnge_hwrm_lib.h"
 #include "bnge_link.h"
+#include "bnge_resc.h"
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION(DRV_SUMMARY);
@@ -406,12 +408,34 @@ static void bnge_remove_one(struct pci_dev *pdev)
 
 static void bnge_shutdown(struct pci_dev *pdev)
 {
+	struct bnge_dev *bd = pci_get_drvdata(pdev);
+	struct net_device *dev;
+
+	dev = bd ? bd->netdev : NULL;
+	if (!dev)
+		return;
+
+	rtnl_lock();
+	netdev_lock(dev);
+
+	if (netif_running(dev))
+		netif_close(dev);
+
+	if (bnge_hwrm_func_drv_unrgtr(bd)) {
+		pcie_flr(pdev);
+		goto shutdown_exit;
+	}
+	bnge_free_irqs(bd);
 	pci_disable_device(pdev);
 
 	if (system_state == SYSTEM_POWER_OFF) {
 		pci_wake_from_d3(pdev, 0);
 		pci_set_power_state(pdev, PCI_D3hot);
 	}
+
+shutdown_exit:
+	netdev_unlock(dev);
+	rtnl_unlock();
 }
 
 static struct pci_driver bnge_driver = {
-- 
2.47.1

[PATCH net-next 2/4] bnge: refactor rx mode helpers to accept explicit address lists

[Vikas Gupta]

Rename bnge_cfg_def_vnic() to bnge_cfg_rx_mode() and update
bnge_mc_list_updated() and bnge_uc_list_updated() to accept
explicit netdev_hw_addr_list pointers rather than deriving
them from the netdev.

Add a snapshot parameter to bnge_cfg_rx_mode() to skip
netif_addr_lock_bh() when the caller provides a pre-snapshotted
list. On the open path (snapshot=false), the live netdev UC list
is passed and the addr lock is taken as before.

Handle -EAGAIN from bnge_hwrm_set_vnic_filter() on the open path by
scheduling a retry via netif_rx_mode_schedule_retry() rather than
failing
the open.

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
Reviewed-by: Rahul Gupta <rahul-rg.gupta@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 47 +++++++++++--------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index 6f7ef506d4e1..1e3cdaeaa03d 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -2144,16 +2144,16 @@ static int bnge_hwrm_set_vnic_filter(struct bnge_net *bn, u16 vnic_id, u16 idx,
 	return rc;
 }
 
-static bool bnge_mc_list_updated(struct bnge_net *bn, u32 *rx_mask)
+static bool bnge_mc_list_updated(struct bnge_net *bn, u32 *rx_mask,
+				 const struct netdev_hw_addr_list *mc)
 {
 	struct bnge_vnic_info *vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
-	struct net_device *dev = bn->netdev;
 	struct netdev_hw_addr *ha;
 	int mc_count = 0, off = 0;
 	bool update = false;
 	u8 *haddr;
 
-	netdev_for_each_mc_addr(ha, dev) {
+	netdev_hw_addr_list_for_each(ha, mc) {
 		if (mc_count >= BNGE_MAX_MC_ADDRS) {
 			*rx_mask |= CFA_L2_SET_RX_MASK_REQ_MASK_ALL_MCAST;
 			vnic->mc_list_count = 0;
@@ -2177,17 +2177,17 @@ static bool bnge_mc_list_updated(struct bnge_net *bn, u32 *rx_mask)
 	return update;
 }
 
-static bool bnge_uc_list_updated(struct bnge_net *bn)
+static bool bnge_uc_list_updated(struct bnge_net *bn,
+				 const struct netdev_hw_addr_list *uc)
 {
 	struct bnge_vnic_info *vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
-	struct net_device *dev = bn->netdev;
 	struct netdev_hw_addr *ha;
 	int off = 0;
 
-	if (netdev_uc_count(dev) != (vnic->uc_filter_count - 1))
+	if (netdev_hw_addr_list_count(uc) != (vnic->uc_filter_count - 1))
 		return true;
 
-	netdev_for_each_uc_addr(ha, dev) {
+	netdev_hw_addr_list_for_each(ha, uc) {
 		if (!ether_addr_equal(ha->addr, vnic->uc_list + off))
 			return true;
 
@@ -2201,7 +2201,8 @@ static bool bnge_promisc_ok(struct bnge_net *bn)
 	return true;
 }
 
-static int bnge_cfg_def_vnic(struct bnge_net *bn)
+static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
+			    bool snapshot)
 {
 	struct bnge_vnic_info *vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
 	struct net_device *dev = bn->netdev;
@@ -2210,9 +2211,7 @@ static int bnge_cfg_def_vnic(struct bnge_net *bn)
 	int i, off = 0, rc;
 	bool uc_update;
 
-	netif_addr_lock_bh(dev);
-	uc_update = bnge_uc_list_updated(bn);
-	netif_addr_unlock_bh(dev);
+	uc_update = bnge_uc_list_updated(bn, uc);
 
 	if (!uc_update)
 		goto skip_uc;
@@ -2226,22 +2225,28 @@ static int bnge_cfg_def_vnic(struct bnge_net *bn)
 
 	vnic->uc_filter_count = 1;
 
-	netif_addr_lock_bh(dev);
-	if (netdev_uc_count(dev) > (BNGE_MAX_UC_ADDRS - 1)) {
+	if (!snapshot)
+		netif_addr_lock_bh(dev);
+	if (netdev_hw_addr_list_count(uc) > (BNGE_MAX_UC_ADDRS - 1)) {
 		vnic->rx_mask |= CFA_L2_SET_RX_MASK_REQ_MASK_PROMISCUOUS;
 	} else {
-		netdev_for_each_uc_addr(ha, dev) {
+		netdev_hw_addr_list_for_each(ha, uc) {
 			memcpy(vnic->uc_list + off, ha->addr, ETH_ALEN);
 			off += ETH_ALEN;
 			vnic->uc_filter_count++;
 		}
 	}
-	netif_addr_unlock_bh(dev);
+	if (!snapshot)
+		netif_addr_unlock_bh(dev);
 
 	for (i = 1, off = 0; i < vnic->uc_filter_count; i++, off += ETH_ALEN) {
 		rc = bnge_hwrm_set_vnic_filter(bn, 0, i, vnic->uc_list + off);
 		if (rc) {
-			netdev_err(dev, "HWRM vnic filter failure rc: %d\n", rc);
+			if (rc == -EAGAIN)
+				netdev_warn(dev, "FW busy while setting vnic filter, will retry\n");
+			else
+				netdev_err(dev, "HWRM vnic filter failure rc: %d\n",
+					   rc);
 			vnic->uc_filter_count = i;
 			return rc;
 		}
@@ -2695,13 +2700,17 @@ static int bnge_init_chip(struct bnge_net *bn)
 	} else if (bn->netdev->flags & IFF_MULTICAST) {
 		u32 mask = 0;
 
-		bnge_mc_list_updated(bn, &mask);
+		bnge_mc_list_updated(bn, &mask, &bn->netdev->mc);
 		vnic->rx_mask |= mask;
 	}
 
-	rc = bnge_cfg_def_vnic(bn);
-	if (rc)
+	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, false);
+	if (rc == -EAGAIN) {
+		netif_rx_mode_schedule_retry(bn->netdev);
+		rc = 0;
+	} else if (rc) {
 		goto err_out;
+	}
 	return 0;
 
 err_out:
-- 
2.47.1

[PATCH net-next 3/4] bnge: add ndo_set_rx_mode_async support

[Vikas Gupta]

Register bnge_set_rx_mode() as ndo_set_rx_mode_async to handle
unicast, multicast, broadcast, and promiscuous filter updates via
CFA_L2_SET_RX_MASK. The async variant receives pre-snapshotted address
lists from the kernel, allowing the driver to issue sleepable HWRM
firmware commands without holding the addr lock.

Move uc_update detection to the caller so the async path can compute
it directly from the snapshotted UC list before calling
bnge_cfg_rx_mode().

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
Reviewed-by: Rahul Gupta <rahul-rg.gupta@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 50 +++++++++++++++++--
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index 1e3cdaeaa03d..e67536a8c430 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -2202,16 +2202,13 @@ static bool bnge_promisc_ok(struct bnge_net *bn)
 }
 
 static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
-			    bool snapshot)
+			    bool uc_update, bool snapshot)
 {
 	struct bnge_vnic_info *vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
 	struct net_device *dev = bn->netdev;
 	struct bnge_dev *bd = bn->bd;
 	struct netdev_hw_addr *ha;
 	int i, off = 0, rc;
-	bool uc_update;
-
-	uc_update = bnge_uc_list_updated(bn, uc);
 
 	if (!uc_update)
 		goto skip_uc;
@@ -2272,6 +2269,48 @@ static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
 	return rc;
 }
 
+static int bnge_set_rx_mode(struct net_device *dev,
+			    struct netdev_hw_addr_list *uc,
+			    struct netdev_hw_addr_list *mc)
+{
+	struct bnge_net *bn = netdev_priv(dev);
+	struct bnge_vnic_info *vnic;
+	bool mc_update = false;
+	bool uc_update;
+	u32 mask;
+
+	if (!test_bit(BNGE_STATE_OPEN, &bn->bd->state))
+		return 0;
+
+	vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
+	mask = vnic->rx_mask;
+	mask &= ~(CFA_L2_SET_RX_MASK_REQ_MASK_PROMISCUOUS |
+		  CFA_L2_SET_RX_MASK_REQ_MASK_MCAST |
+		  CFA_L2_SET_RX_MASK_REQ_MASK_ALL_MCAST |
+		  CFA_L2_SET_RX_MASK_REQ_MASK_BCAST);
+
+	if (dev->flags & IFF_PROMISC)
+		mask |= CFA_L2_SET_RX_MASK_REQ_MASK_PROMISCUOUS;
+
+	uc_update = bnge_uc_list_updated(bn, uc);
+
+	if (dev->flags & IFF_BROADCAST)
+		mask |= CFA_L2_SET_RX_MASK_REQ_MASK_BCAST;
+	if (dev->flags & IFF_ALLMULTI) {
+		mask |= CFA_L2_SET_RX_MASK_REQ_MASK_ALL_MCAST;
+		vnic->mc_list_count = 0;
+	} else if (dev->flags & IFF_MULTICAST) {
+		mc_update = bnge_mc_list_updated(bn, &mask, mc);
+	}
+
+	if (mask != vnic->rx_mask || uc_update || mc_update) {
+		vnic->rx_mask = mask;
+		return bnge_cfg_rx_mode(bn, uc, uc_update, true);
+	}
+
+	return 0;
+}
+
 static void bnge_disable_int(struct bnge_net *bn)
 {
 	struct bnge_dev *bd = bn->bd;
@@ -2704,7 +2743,7 @@ static int bnge_init_chip(struct bnge_net *bn)
 		vnic->rx_mask |= mask;
 	}
 
-	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, false);
+	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, true, false);
 	if (rc == -EAGAIN) {
 		netif_rx_mode_schedule_retry(bn->netdev);
 		rc = 0;
@@ -3204,6 +3243,7 @@ static const struct net_device_ops bnge_netdev_ops = {
 	.ndo_stop		= bnge_close,
 	.ndo_start_xmit		= bnge_start_xmit,
 	.ndo_get_stats64	= bnge_get_stats64,
+	.ndo_set_rx_mode_async	= bnge_set_rx_mode,
 	.ndo_features_check	= bnge_features_check,
 };
 
-- 
2.47.1

[PATCH net-next 4/4] bnge: send hwrm for interface down/up transitions

[Vikas Gupta]

Firmware expects HWRM_FUNC_DRV_IF_CHANGE on interface down/up
transitions to coordinate resource management.
Add bnge_hwrm_if_change() to send this notification.

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Dharmender Garg <dharmender.garg@broadcom.com>
Reviewed-by: Rahul Gupta <rahul-rg.gupta@broadcom.com>
---
 .../net/ethernet/broadcom/bnge/bnge_netdev.c  | 32 +++++++++++++++++--
 .../net/ethernet/broadcom/bnge/bnge_netdev.h  |  2 ++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
index e67536a8c430..220bdcced0db 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
@@ -20,6 +20,7 @@
 #include <net/page_pool/helpers.h>
 
 #include "bnge.h"
+#include "bnge_hwrm.h"
 #include "bnge_hwrm_lib.h"
 #include "bnge_ethtool.h"
 #include "bnge_rmem.h"
@@ -2861,6 +2862,24 @@ static void bnge_tx_enable(struct bnge_net *bn)
 		netif_carrier_on(bn->netdev);
 }
 
+static int bnge_hwrm_if_change(struct bnge_dev *bd, bool up)
+{
+	struct hwrm_func_drv_if_change_input *req;
+	int rc;
+
+	if (!(bd->fw_cap & BNGE_FW_CAP_IF_CHANGE))
+		return 0;
+
+	rc = bnge_hwrm_req_init(bd, req, HWRM_FUNC_DRV_IF_CHANGE);
+	if (rc)
+		return rc;
+
+	if (up)
+		req->flags = cpu_to_le32(FUNC_DRV_IF_CHANGE_REQ_FLAGS_UP);
+
+	return bnge_hwrm_req_send(bd, req);
+}
+
 static int bnge_open_core(struct bnge_net *bn)
 {
 	struct bnge_dev *bd = bn->bd;
@@ -2868,16 +2887,22 @@ static int bnge_open_core(struct bnge_net *bn)
 
 	netif_carrier_off(bn->netdev);
 
+	rc = bnge_hwrm_if_change(bd, true);
+	if (rc) {
+		netdev_err(bn->netdev, "bnge_hwrm_if_change err: %d\n", rc);
+		return rc;
+	}
+
 	rc = bnge_reserve_rings(bd);
 	if (rc) {
 		netdev_err(bn->netdev, "bnge_reserve_rings err: %d\n", rc);
-		return rc;
+		goto err_if_change;
 	}
 
 	rc = bnge_alloc_core(bn);
 	if (rc) {
 		netdev_err(bn->netdev, "bnge_alloc_core err: %d\n", rc);
-		return rc;
+		goto err_if_change;
 	}
 
 	bnge_init_napi(bn);
@@ -2924,6 +2949,8 @@ static int bnge_open_core(struct bnge_net *bn)
 err_del_napi:
 	bnge_del_napi(bn);
 	bnge_free_core(bn);
+err_if_change:
+	bnge_hwrm_if_change(bd, false);
 	return rc;
 }
 
@@ -3154,6 +3181,7 @@ static int bnge_close(struct net_device *dev)
 
 	bnge_close_core(bn);
 	bnge_hwrm_shutdown_link(bn->bd);
+	bnge_hwrm_if_change(bn->bd, false);
 	bn->sp_event = 0;
 
 	return 0;
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
index d177919c2e11..a1a939496ebd 100644
--- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
+++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
@@ -275,6 +275,8 @@ enum bnge_net_flag {
 	BNGE_FLAG_PORT_STATS_EXT	= BIT(1),
 };
 
+#define BNGE_FW_IF_RETRY	10
+
 enum bnge_sp_event {
 	BNGE_LINK_CHNG_SP_EVENT,
 	BNGE_LINK_SPEED_CHNG_SP_EVENT,
-- 
2.47.1

Re: [PATCH net-next 2/4] bnge: refactor rx mode helpers to accept explicit address lists

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
bnge: refactor rx mode helpers to accept explicit address lists

This patch renames bnge_cfg_def_vnic() to bnge_cfg_rx_mode(), threads
explicit netdev_hw_addr_list pointers through the mc/uc helpers, adds a
snapshot flag to skip netif_addr_lock_bh(), and treats -EAGAIN from
bnge_hwrm_set_vnic_filter() on the open path as a scheduled retry
instead of an open failure.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 6f7ef506d4e1..1e3cdaeaa03d 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2201,7 +2201,8 @@ static bool bnge_promisc_ok(struct bnge_net *bn)
>  	return true;
>  }
>  
> -static int bnge_cfg_def_vnic(struct bnge_net *bn)
> +static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
> +			    bool snapshot)
>  {

[ ... ]

> @@ -2226,22 +2225,28 @@ static int bnge_cfg_def_vnic(struct bnge_net *bn)

[ ... ]

>  	for (i = 1, off = 0; i < vnic->uc_filter_count; i++, off += ETH_ALEN) {
>  		rc = bnge_hwrm_set_vnic_filter(bn, 0, i, vnic->uc_list + off);
>  		if (rc) {
> -			netdev_err(dev, "HWRM vnic filter failure rc: %d\n", rc);
> +			if (rc == -EAGAIN)
> +				netdev_warn(dev, "FW busy while setting vnic filter, will retry\n");
> +			else
> +				netdev_err(dev, "HWRM vnic filter failure rc: %d\n",
> +					   rc);
>  			vnic->uc_filter_count = i;
>  			return rc;
>  		}
>  	}

[Low]
The commit message says the -EAGAIN retry handles the case coming from
bnge_hwrm_set_vnic_filter(), but bnge_cfg_rx_mode() can also return
-EAGAIN from the trailing bnge_hwrm_cfa_l2_set_rx_mask() calls (via
HWRM_ERR_CODE_BUSY translated in bnge_map_hwrm_error()).

In that second path the driver still logs:

    netdev_err(dev, "HWRM cfa l2 rx mask failure rc: %d\n", rc);

while the caller in bnge_init_chip() silently converts the same rc to 0
and schedules a retry.

Is the intent to retry only vnic-filter -EAGAINs, or any -EAGAIN out of
bnge_cfg_rx_mode()?  If the broader scope is intentional, would it be
worth aligning the log message (and the commit description) so the
netdev_err isn't emitted for a case the caller treats as a soft retry?

> @@ -2695,13 +2700,17 @@ static int bnge_init_chip(struct bnge_net *bn)
>  	} else if (bn->netdev->flags & IFF_MULTICAST) {
>  		u32 mask = 0;
>  
> -		bnge_mc_list_updated(bn, &mask);
> +		bnge_mc_list_updated(bn, &mask, &bn->netdev->mc);
>  		vnic->rx_mask |= mask;
>  	}
>  
> -	rc = bnge_cfg_def_vnic(bn);
> -	if (rc)
> +	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, false);
> +	if (rc == -EAGAIN) {
> +		netif_rx_mode_schedule_retry(bn->netdev);
> +		rc = 0;
> +	} else if (rc) {
>  		goto err_out;
> +	}
>  	return 0;

[Medium]
When bnge_hwrm_set_vnic_filter() returns -EAGAIN, bnge_cfg_rx_mode()
returns immediately from inside the uc-filter loop:

    for (i = 1, off = 0; i < vnic->uc_filter_count; i++, off += ETH_ALEN) {
        rc = bnge_hwrm_set_vnic_filter(bn, 0, i, vnic->uc_list + off);
        if (rc) {
            ...
            vnic->uc_filter_count = i;
            return rc;
        }
    }

so control never reaches the skip_uc: label and
bnge_hwrm_cfa_l2_set_rx_mask() is never called.  The vnic->rx_mask bits
assembled just above (IFF_BROADCAST / IFF_MULTICAST / IFF_PROMISC /
ALL_MCAST) are therefore not pushed to firmware.

bnge_init_chip() then converts the -EAGAIN to rc = 0 and calls
netif_rx_mode_schedule_retry(), so the open succeeds and the interface
comes up IFF_UP.

The retry timer eventually fires netif_rx_mode_run(), which dispatches
via ops->ndo_set_rx_mode_async / ops->ndo_set_rx_mode.  At this commit,
bnge_netdev_ops registers neither callback, so the retry is a no-op and
the device stays with no broadcast / multicast / promisc filters
programmed until something else re-triggers rx-mode configuration.

Should this patch either fail the open on -EAGAIN, or be ordered after
the ndo_set_rx_mode_async wiring so the scheduled retry actually
re-invokes bnge_cfg_rx_mode()?  The follow-up commit
"bnge: add ndo_set_rx_mode_async support" (fad0f7d4f3b6) does add
.ndo_set_rx_mode_async = bnge_set_rx_mode, which closes the gap only
after this patch lands.
-- 
pw-bot: cr

Re: [PATCH net-next 4/4] bnge: send hwrm for interface down/up transitions

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
bnge: send hwrm for interface down/up transitions

This commit adds bnge_hwrm_if_change() and calls it on the open and close
paths so firmware is notified of interface up/down transitions via
HWRM_FUNC_DRV_IF_CHANGE.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index e67536a8c430..220bdcced0db 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2861,6 +2862,24 @@ static void bnge_tx_enable(struct bnge_net *bn)
>  		netif_carrier_on(bn->netdev);
>  }
>
> +static int bnge_hwrm_if_change(struct bnge_dev *bd, bool up)
> +{
> +	struct hwrm_func_drv_if_change_input *req;
> +	int rc;
> +
> +	if (!(bd->fw_cap & BNGE_FW_CAP_IF_CHANGE))
> +		return 0;
> +
> +	rc = bnge_hwrm_req_init(bd, req, HWRM_FUNC_DRV_IF_CHANGE);
> +	if (rc)
> +		return rc;
> +
> +	if (up)
> +		req->flags = cpu_to_le32(FUNC_DRV_IF_CHANGE_REQ_FLAGS_UP);
> +
> +	return bnge_hwrm_req_send(bd, req);
> +}

[Medium]
The commit message says this HWRM is added "to coordinate resource
management", but the response from firmware is never inspected here.

There is no bnge_hwrm_req_hold() around the send, so resp->flags is
discarded. On the up-path, firmware can set three bits in that response
(see include/linux/bnxt/hsi.h):

    FUNC_DRV_IF_CHANGE_RESP_FLAGS_RESC_CHANGE
    FUNC_DRV_IF_CHANGE_RESP_FLAGS_HOT_FW_RESET_DONE
    FUNC_DRV_IF_CHANGE_RESP_FLAGS_CAPS_CHANGE

For comparison, bnxt_hwrm_if_change() in
drivers/net/ethernet/broadcom/bnxt/bnxt.c reads the response and, on
RESC_CHANGE, calls bnxt_cancel_reservations() so that the following ring
reservation actually re-requests resources from firmware.

In bnge, bnge_need_reserve_rings() compares the driver's requested ring
counts against bd->hw_resc.resv_*. If firmware has re-partitioned the
function's resources but the driver still holds the old resv_* values,
that check returns false and bnge_reserve_rings() short-circuits without
talking to firmware, and bnge_open_core() proceeds through
bnge_alloc_core() with stale ring counts.

HOT_FW_RESET_DONE and CAPS_CHANGE similarly imply that firmware
capability caches and health registers need to be re-initialised, and
none of that handling appears to be added here.

Should the response flags be consumed here to actually coordinate with
firmware as the commit message describes?

[Low]
Also, bnxt_hwrm_if_change() retries the request in a loop while firmware
returns -EAGAIN (with a short sleep between attempts, bounded by a retry
count). Here the send is issued once, so a transient -EAGAIN from
firmware during a reset transition would cause bnge_open_core() to fail
immediately. Is that intentional?

>  static int bnge_open_core(struct bnge_net *bn)
>  {
>  	struct bnge_dev *bd = bn->bd;

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
> index d177919c2e11..a1a939496ebd 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
> @@ -275,6 +275,8 @@ enum bnge_net_flag {
>  	BNGE_FLAG_PORT_STATS_EXT	= BIT(1),
>  };
>
> +#define BNGE_FW_IF_RETRY	10
> +

[Low]
BNGE_FW_IF_RETRY does not appear to be referenced anywhere in the tree.
Given that the sibling bnxt driver uses an identically-named constant to
bound its -EAGAIN retry loop around HWRM_FUNC_DRV_IF_CHANGE, was a retry
loop intended to be added here as well?

>  enum bnge_sp_event {
>  	BNGE_LINK_CHNG_SP_EVENT,
>  	BNGE_LINK_SPEED_CHNG_SP_EVENT,

Re: [PATCH net-next 1/4] bnge: add steps in bnge_shutdown()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net-next 1/4] bnge: add steps in bnge_shutdown() ---
commit 92ff52be8a6aa712b5331ce7118ada003154f2ee
Author: Vikas Gupta <vikas.gupta@broadcom.com>

bnge: add steps in bnge_shutdown()

This commit updates the shutdown callback to mirror the close sequence 
so the device is brought down cleanly during system shutdown.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> index 68b74eb2c3a29..9846d29b4fe3c 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
[ ... ]
> @@ -406,12 +408,34 @@ static void bnge_remove_one(struct pci_dev *pdev)
>  
>  static void bnge_shutdown(struct pci_dev *pdev)
>  {
[ ... ]
> +	if (bnge_hwrm_func_drv_unrgtr(bd)) {
> +		pcie_flr(pdev);
> +		goto shutdown_exit;
> +	}
> +	bnge_free_irqs(bd);

[Severity: Medium]
Will unconditionally freeing IRQs here trigger a kernel warning if the
RDMA auxiliary driver is currently bound?

When bnge_shutdown() calls bnge_free_irqs() without first tearing down
or unregistering the RDMA auxiliary device, the auxiliary driver may
still have active IRQ actions. This usually causes a WARN_ON for
irq_has_action() inside msi_domain_free_irqs() during system shutdown
or reboot.

Could the auxiliary device be cleanly shut down or removed before
freeing the interrupts?

>  	pci_disable_device(pdev);
>  
>  	if (system_state == SYSTEM_POWER_OFF) {
[ ... ]

Re: [PATCH net-next 2/4] bnge: refactor rx mode helpers to accept explicit address lists

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [2]: [PATCH net-next 2/4] bnge: refactor rx mode helpers to accept explicit address lists ---
commit 89173081c7d6c9342e8bf0ec4661f5150e6862c9
Author: Vikas Gupta <vikas.gupta@broadcom.com>
bnge: refactor rx mode helpers to accept explicit address lists

This patch refactors bnge_cfg_def_vnic() to bnge_cfg_rx_mode() and updates
the MC/UC list update functions to take explicit address lists. It adds a
snapshot parameter to optionally skip acquiring netif_addr_lock_bh(), and
schedules a retry if bnge_hwrm_set_vnic_filter() returns -EAGAIN.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 6f7ef506d4e18..1e3cdaeaa03dc 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> @@ -2144,16 +2144,16 @@ static int bnge_hwrm_set_vnic_filter(struct bnge_net *bn, u16 vnic_id, u16 idx,
>  	return rc;
>  }
>  
> -static bool bnge_mc_list_updated(struct bnge_net *bn, u32 *rx_mask)
> +static bool bnge_mc_list_updated(struct bnge_net *bn, u32 *rx_mask,
> +				 const struct netdev_hw_addr_list *mc)
>  {
>  	struct bnge_vnic_info *vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
> -	struct net_device *dev = bn->netdev;
>  	struct netdev_hw_addr *ha;
>  	int mc_count = 0, off = 0;
>  	bool update = false;
>  	u8 *haddr;
>  
> -	netdev_for_each_mc_addr(ha, dev) {
> +	netdev_hw_addr_list_for_each(ha, mc) {
>  		if (mc_count >= BNGE_MAX_MC_ADDRS) {

[ ... ]

> @@ -2201,7 +2201,8 @@ static bool bnge_promisc_ok(struct bnge_net *bn)
>  	return true;
>  }
>  
> -static int bnge_cfg_def_vnic(struct bnge_net *bn)
> +static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
> +			    bool snapshot)
>  {
>  	struct bnge_vnic_info *vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
>  	struct net_device *dev = bn->netdev;
> @@ -2210,9 +2211,7 @@ static int bnge_cfg_def_vnic(struct bnge_net *bn)
>  	int i, off = 0, rc;
>  	bool uc_update;
>  
> -	netif_addr_lock_bh(dev);
> -	uc_update = bnge_uc_list_updated(bn);
> -	netif_addr_unlock_bh(dev);
> +	uc_update = bnge_uc_list_updated(bn, uc);

[Severity: High]
Does this code introduce a data race or list corruption?

The function bnge_cfg_rx_mode() is called with the live unicast list, but the
lock is only acquired later in the function. Since this patch removed
netif_addr_lock_bh() around the call to bnge_uc_list_updated(), the list
traversal inside bnge_uc_list_updated() is completely unprotected.

If there are concurrent modifications to the unicast list (e.g., via
dev_uc_add()) while the interface is being initialized, won't this race
cause a crash on poisoned list pointers?

>  
>  	if (!uc_update)
>  		goto skip_uc;

[ ... ]

> @@ -2695,13 +2700,17 @@ static int bnge_init_chip(struct bnge_net *bn)
>  	} else if (bn->netdev->flags & IFF_MULTICAST) {
>  		u32 mask = 0;
>  
> -		bnge_mc_list_updated(bn, &mask);
> +		bnge_mc_list_updated(bn, &mask, &bn->netdev->mc);

[Severity: High]
This isn't a bug introduced by this patch, but does this traverse the live
multicast list without holding netif_addr_lock_bh()?

Concurrent network stack operations (like IGMP/MLD processing triggering
dev_mc_add()) can modify this list while the device is opening, potentially
leading to a race and crash.

>  		vnic->rx_mask |= mask;
>  	}
>  
> -	rc = bnge_cfg_def_vnic(bn);
> -	if (rc)
> +	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, false);
> +	if (rc == -EAGAIN) {
> +		netif_rx_mode_schedule_retry(bn->netdev);

[Severity: High]
If bnge_cfg_rx_mode() returns -EAGAIN, this handles the error by scheduling
a retry mechanism via netif_rx_mode_schedule_retry().

However, since this driver does not implement .ndo_set_rx_mode or
.ndo_set_rx_mode_async in bnge_netdev_ops, will the scheduled retry worker
(netif_rx_mode_run()) silently return without doing anything?

If so, won't this leave the device permanently without proper receive filters,
leading to dropped traffic?

> +		rc = 0;
> +	} else if (rc) {
>  		goto err_out;
> +	}
>  	return 0;
>  
>  err_out:

Re: [PATCH net-next 3/4] bnge: add ndo_set_rx_mode_async support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [3]: [PATCH net-next 3/4] bnge: add ndo_set_rx_mode_async support ---
commit 1fc0ed8b56a0fd5a87f788e357261a7f2693625b
Author: Vikas Gupta <vikas.gupta@broadcom.com>

bnge: add ndo_set_rx_mode_async support

This commit registers bnge_set_rx_mode() as ndo_set_rx_mode_async to
handle filter updates asynchronously. It computes UC list updates directly
from the snapshot, allowing sleepable HWRM commands without holding the
address lock.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 1e3cdaeaa03dc..e67536a8c4304 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2272,6 +2269,48 @@ static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
>  	return rc;
>  }
>  
> +static int bnge_set_rx_mode(struct net_device *dev,
> +			    struct netdev_hw_addr_list *uc,
> +			    struct netdev_hw_addr_list *mc)
> +{
> +	struct bnge_net *bn = netdev_priv(dev);
> +	struct bnge_vnic_info *vnic;

[ ... ]

> +	if (dev->flags & IFF_ALLMULTI) {
> +		mask |= CFA_L2_SET_RX_MASK_REQ_MASK_ALL_MCAST;
> +		vnic->mc_list_count = 0;
> +	} else if (dev->flags & IFF_MULTICAST) {
> +		mc_update = bnge_mc_list_updated(bn, &mask, mc);
> +	}
> +
> +	if (mask != vnic->rx_mask || uc_update || mc_update) {
> +		vnic->rx_mask = mask;
> +		return bnge_cfg_rx_mode(bn, uc, uc_update, true);
> +	}

[Severity: High]
If bnge_cfg_rx_mode() returns -EAGAIN, the network core schedules a retry 
timer. However, because vnic->rx_mask is eagerly updated here (and 
vnic->mc_list is mutated in-place inside bnge_mc_list_updated()), won't the 
subsequent retry falsely detect no state changes and return 0 without 
programming the hardware?

Could this leave the hardware filter state permanently desynchronized from 
the network stack?

> +
> +	return 0;
> +}

[ ... ]

> @@ -2704,7 +2743,7 @@ static int bnge_init_chip(struct bnge_net *bn)
>  		vnic->rx_mask |= mask;
>  	}
>  
> -	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, false);
> +	rc = bnge_cfg_rx_mode(bn, &bn->netdev->uc, true, false);
>  	if (rc == -EAGAIN) {
>  		netif_rx_mode_schedule_retry(bn->netdev);
>  		rc = 0;

[Severity: High]
This is a pre-existing issue, but does iterating the live netdev multicast
list earlier in bnge_init_chip() require holding netif_addr_lock_bh()?

Just above this call in bnge_init_chip(), the code evaluates:
bnge_mc_list_updated(bn, &mask, &bn->netdev->mc)

Since this happens without netif_addr_lock_bh() held, could it race with 
concurrent multicast updates (such as dev_mc_add() from IGMP/MLD timers), 
potentially leading to a use-after-free or infinite loop during list 
traversal?

Re: [PATCH net-next 4/4] bnge: send hwrm for interface down/up transitions

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [4]: [PATCH net-next 4/4] bnge: send hwrm for interface down/up transitions ---
commit d98cba7c642aee81ff9e1c8c6042d1e9dc822e87
Author: Vikas Gupta <vikas.gupta@broadcom.com>

bnge: send hwrm for interface down/up transitions

Firmware expects HWRM_FUNC_DRV_IF_CHANGE on interface down/up
transitions to coordinate resource management.
Add bnge_hwrm_if_change() to send this notification.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index e67536a8c4304..220bdcced0db9 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
[ ... ]
> @@ -2861,6 +2862,24 @@ static void bnge_tx_enable(struct bnge_net *bn)
>  		netif_carrier_on(bn->netdev);
>  }
>  
> +static int bnge_hwrm_if_change(struct bnge_dev *bd, bool up)
> +{
> +	struct hwrm_func_drv_if_change_input *req;
> +	int rc;
> +
> +	if (!(bd->fw_cap & BNGE_FW_CAP_IF_CHANGE))
> +		return 0;
> +
> +	rc = bnge_hwrm_req_init(bd, req, HWRM_FUNC_DRV_IF_CHANGE);
> +	if (rc)
> +		return rc;
> +
> +	if (up)
> +		req->flags = cpu_to_le32(FUNC_DRV_IF_CHANGE_REQ_FLAGS_UP);
> +
> +	return bnge_hwrm_req_send(bd, req);

[Severity: Medium]
Is a retry mechanism needed here for transient firmware errors?

This function directly returns the result of the HWRM send instead of executing
a retry loop. If the firmware returns a transient error (like -EAGAIN), the
network interface might spuriously fail to open.

In teardown paths, could a single un-retried failure result in asymmetric
driver and firmware lifecycle states, where the driver tears down but the
firmware remains 'UP'?

> +}
> +
[ ... ]
> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
> index d177919c2e117..a1a939496ebd3 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.h
> @@ -275,6 +275,8 @@ enum bnge_net_flag {
>  	BNGE_FLAG_PORT_STATS_EXT	= BIT(1),
>  };
>  
> +#define BNGE_FW_IF_RETRY	10

[Severity: Medium]
Should BNGE_FW_IF_RETRY be used in bnge_hwrm_if_change()?

This retry limit constant is defined here but doesn't appear to be used
anywhere in the implementation.

> +
>  enum bnge_sp_event {
>  	BNGE_LINK_CHNG_SP_EVENT,
>  	BNGE_LINK_SPEED_CHNG_SP_EVENT,

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential data race
during multicast list traversal in bnge_init_chip().

In drivers/net/ethernet/broadcom/bnge/bnge_netdev.c, bnge_init_chip() calls
bnge_mc_list_updated() and passes the live multicast list without acquiring
netif_addr_lock_bh():

bnge_init_chip()
    ...
    bnge_mc_list_updated(bn, &mask, &bn->netdev->mc);

Inside bnge_mc_list_updated(), the list is traversed:

    netdev_hw_addr_list_for_each(ha, mc) {
    ...

If dev->mc is concurrently modified (for example, via dev_mc_add() from a
softirq context handling IPv6 MLD/ND), could the list pointers be corrupted
during traversal? Should netif_addr_lock_bh() be held here?

Re: [PATCH net-next 0/4] bnge: add more functionality

[Jakub Kicinski]

On Fri, 24 Jul 2026 19:59:50 +0530 Vikas Gupta wrote:
> Patch 1: Makes bnge_shutdown() functional by properly unregistering the
>          netdevice and freeing associated hardware queues and interrupts.
> Patch 2: Implements ndo_set_rx_mode_async().
> 
> Patch 3: Adds a dedicated HWRM (Hardware Resource Management) command
>          sequence to handle explicit interface down and up transitions cleanly.

If you have to respin - there are 4 patches in the series.

Sashikos point at bugs in the EAGAIN flow handling in
ndo_set_rx_mode_async. Can you double check that?

https://sashiko.dev/#/patchset/20260724142954.3101980-2-vikas.gupta@broadcom.com
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724142954.3101980-2-vikas.gupta@broadcom.com

And the concern about IRQs being in use on shutdown perhaps.

[P.S. Ugh, looks like I wrote this yesterday but didn't it send? sorry]

Re: [PATCH net-next 0/4] bnge: add more functionality

[Vikas Gupta]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

> If you have to respin - there are 4 patches in the series.
>
> Sashikos point at bugs in the EAGAIN flow handling in
> ndo_set_rx_mode_async. Can you double check that?

 Yes, I need to generate a new spin to resolve the EAGAIN issue and
address the other comments.

>
> https://sashiko.dev/#/patchset/20260724142954.3101980-2-vikas.gupta@broadcom.com
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260724142954.3101980-2-vikas.gupta@broadcom.com
>
> And the concern about IRQs being in use on shutdown perhaps.

 Since the IRQ issue stems from the new shared bng_re RoCE driver, and
it lacks a bng_re_shutdown
 implementation to free the IRQs, should we implement this in the
current series, especially given that
 the bng_re driver implementation is currently minimal?

Thanks,
Vikas

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5465 bytes --]

Re: [PATCH net-next 0/4] bnge: add more functionality

[Jakub Kicinski]

On Thu, 30 Jul 2026 11:26:17 +0530 Vikas Gupta wrote:
> > And the concern about IRQs being in use on shutdown perhaps.  
> 
>  Since the IRQ issue stems from the new shared bng_re RoCE driver, and
> it lacks a bng_re_shutdown
>  implementation to free the IRQs, should we implement this in the
> current series, especially given that the bng_re driver
> implementation is currently minimal?

Doesn't look like the other patches will conflict, maybe I'm missing
something. If there's no conflict perhaps you could take patch 1 out 
of this series and post a combined patch series for bng and _re
scoped to the parts that need to change in both trees.
And we can pull that series into both networking and RDMA?

Note that you can post multiple series at once, as long as they don't
conflict and the sum(patches) <= 15.