Re: [PATCH net-next v8 03/10] bng_en: add ethtool link settings, get_link, and nway_reset

[Simon Horman <horms@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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
bng_en: add ethtool link settings, get_link, and nway_reset

Add get/set_link_ksettings, get_link, and nway_reset support. Report
supported, advertised, and link-partner speeds across NRZ, PAM4, and
PAM4-112 signaling modes. Enable lane count reporting.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge.h b/drivers/net/ethernet/broadcom/bnge/bnge.h
> index 83ee4749cc70..e9b83c0c64b6 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge.h
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge.h

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_link.c b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> index e5a7b48f6a3f..03beb5593069 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_link.c

[ ... ]

> +int bnge_set_link_ksettings(struct net_device *dev,
> +			    const struct ethtool_link_ksettings *lk_ksettings)
> +{
> +	const struct ethtool_link_settings *base = &lk_ksettings->base;
> +	struct bnge_ethtool_link_info *elink_info;
> +	struct bnge_net *bn = netdev_priv(dev);
> +	struct bnge_link_info *link_info;
> +	struct bnge_dev *bd = bn->bd;
> +	bool set_pause = false;
> +	int rc = 0;
> +
> +	elink_info = &bn->eth_link_info;
> +	link_info = &bd->link_info;
> +
> +	if (!BNGE_PHY_CFG_ABLE(bd))
> +		return -EOPNOTSUPP;
> +
> +	if (base->autoneg == AUTONEG_ENABLE) {
> +		bnge_set_ethtool_speeds(bn,
> +					lk_ksettings->link_modes.advertising);
> +		elink_info->autoneg |= BNGE_AUTONEG_SPEED;
> +		if (!elink_info->advertising && !elink_info->advertising_pam4) {
> +			elink_info->advertising =
> +				link_info->support_auto_speeds;
> +			elink_info->advertising_pam4 =
> +				link_info->support_pam4_auto_speeds;
> +		}

When autoneg is enabled but no speeds are advertised, does this code
correctly handle SPEEDS2 devices?

On a SPEEDS2 device (bd->phy_flags & BNGE_PHY_FL_SPEEDS2), the fallback
populates elink_info->advertising with support_auto_speeds and
elink_info->advertising_pam4 with support_pam4_auto_speeds.

However, for SPEEDS2 devices, the firmware expects speed masks encoded in
the SPEEDS2 format (using support_auto_speeds2), not the legacy format
(using support_auto_speeds or support_pam4_auto_speeds).

Looking at how this value is used in bnge_hwrm_set_link_common() in patch v2
(c367afb6b104):

    if (bd->phy_flags & BNGE_PHY_FL_SPEEDS2) {
        req->auto_link_speeds2_mask = elink_info->advertising;
        ...
    } else {
        req->auto_link_speed_mask = elink_info->advertising;
        req->auto_pam4_link_speed_mask = elink_info->advertising_pam4;
    }

The firmware will receive legacy NRZ speed bits in auto_link_speeds2_mask,
which it will interpret as SPEEDS2-encoded bits, potentially negotiating the
wrong link speed.

Compare with bnge_support_speed_dropped() in the same patch series
(c367afb6b104), which handles this case differently when support speeds
change:

    if (bd->phy_flags & BNGE_PHY_FL_SPEEDS2) {
        link_info->advertising = link_info->support_auto_speeds2;
        ...
    } else {
        link_info->advertising = link_info->support_auto_speeds;
        link_info->advertising_pam4 = link_info->support_pam4_auto_speeds;
    }

Should bnge_set_link_ksettings() check bd->phy_flags & BNGE_PHY_FL_SPEEDS2 and
use support_auto_speeds2 for the fallback on SPEEDS2 devices?

> +		/* any change to autoneg will cause link change, therefore the
> +		 * driver should put back the original pause setting in autoneg
> +		 */
> +		if (!(bd->phy_flags & BNGE_PHY_FL_NO_PAUSE))
> +			set_pause = true;
> +	} else {

[ ... ]