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,