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:08:37
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. --- 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.
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 e67536a8c430..220bdcced0db 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); +}
[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;[ ... ]
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 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,