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

[Jakub Kicinski <kuba@kernel.org>]

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?