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