Re: [PATCH net-next 4/4] bnge: send hwrm for interface down/up transitions
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-29 02:09:15
Also in:
lkml
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c index e67536a8c4304..220bdcced0db9 100644 --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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'?
+} +
[ ... ]
quoted hunk ↗ jump to hunk
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?