[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?