Re: [net-next,v6,02/10] bng_en: query PHY capabilities and report link status

[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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
bng_en: query PHY capabilities and report link status

This commit queries PHY capabilities and supported speeds from firmware,
retrieves current link state (speed, duplex, pause, FEC), logs the
information, and seeds initial link state during probe.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_link.c b/drivers/net/ethernet/broadcom/bnge/bnge_link.c
> new file mode 100644
> index 000000000000..0c76cbc794f7
> --- /dev/null
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_link.c

[ ... ]

> +void bnge_init_ethtool_link_settings(struct bnge_net *bn)
> +{
> +	struct bnge_ethtool_link_info *elink_info = &bn->eth_link_info;
> +	struct bnge_link_info *link_info;
> +	struct bnge_dev *bd = bn->bd;
> +
> +	link_info = &bd->link_info;
> +
> +	if (BNGE_AUTO_MODE(link_info->auto_mode)) {
> +		elink_info->autoneg = BNGE_AUTONEG_SPEED;
> +		if (link_info->auto_pause_setting &
> +		    PORT_PHY_CFG_REQ_AUTO_PAUSE_AUTONEG_PAUSE)
> +			elink_info->autoneg |= BNGE_AUTONEG_FLOW_CTRL;
> +		bnge_set_auto_speed(bn);
> +	} else {
> +		bnge_set_force_speed(bn);
> +		elink_info->req_duplex = link_info->duplex_setting;
> +	}

Does the forced-speed (else) branch need to clear elink_info->autoneg?

When called only at probe time on a zero-initialized struct, leaving
autoneg uninitialized is harmless. However, commit f833dbb3f7ab
('bng_en: add support for link async events') in this same series adds
a re-init call path from bnge_sp_task() in response to
BNGE_LINK_CFG_CHANGE_SP_EVENT async firmware events.

If the PHY transitions from auto-negotiation to forced-speed mode (e.g.,
via BMC/management configuration change), the stale BNGE_AUTONEG_SPEED
and BNGE_AUTONEG_FLOW_CTRL bits in elink_info->autoneg persist. This
causes:

bnge_hwrm_set_link_common() to send auto-negotiation configuration to
firmware instead of forced-speed:

    if (elink_info->autoneg & BNGE_AUTONEG_SPEED) {
        req->auto_mode |= PORT_PHY_CFG_REQ_AUTO_MODE_SPEED_MASK;
        ...

bnge_hwrm_set_pause_common() to configure auto-negotiation pause
instead of forced pause:

    if (bn->eth_link_info.autoneg & BNGE_AUTONEG_FLOW_CTRL) {
        req->auto_pause = PORT_PHY_CFG_REQ_AUTO_PAUSE_AUTONEG_PAUSE;
        ...

bnge_update_phy_setting() to make incorrect speed-change decisions:

    if (!(elink_info->autoneg & BNGE_AUTONEG_SPEED)) {
        if (BNGE_AUTO_MODE(link_info->auto_mode))
            update_link = true;

The bnxt reference driver (bnxt_init_ethtool_link_settings) has the same
pattern, but this doesn't excuse introducing it in a new driver.

No fix was found later in the series (checked through 2d7643a78ddb).

> +	if (elink_info->autoneg & BNGE_AUTONEG_FLOW_CTRL)
> +		elink_info->req_flow_ctrl =
> +			link_info->auto_pause_setting & BNGE_LINK_PAUSE_BOTH;
> +	else
> +		elink_info->req_flow_ctrl = link_info->force_pause_setting;
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index edbb42efb70b..db3a19bbf75a 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2532,6 +2562,14 @@ static int bnge_open_core(struct bnge_net *bn)
>
> 	bnge_enable_napi(bn);
>
> +	rc = bnge_update_phy_setting(bn);
> +	if (rc) {
> +		netdev_warn(bn->netdev, "failed to update PHY settings (rc: %d)\n",
> +			    rc);
> +		bd->link_info.phy_retry = true;
> +		bd->link_info.phy_retry_expires = jiffies + 5 * HZ;
> +	}
> +

Does stale phy_retry state from a previous open/close cycle cause spurious
warnings?

When bnge_update_phy_setting() fails during bnge_open_core(),
bd->link_info.phy_retry is set to true and phy_retry_expires is set to
jiffies + 5*HZ. Neither bnge_close_core() nor bnge_close() clears
phy_retry.

If the interface is closed and then reopened with a successful
bnge_update_phy_setting() call (rc=0), the stale phy_retry remains true
with an expired phy_retry_expires. On the next timer tick, bnge_timer()
sees:

    if (bd->link_info.phy_retry) {
        if (time_after(jiffies, bd->link_info.phy_retry_expires)) {
            bd->link_info.phy_retry = false;
            netdev_warn(bn->netdev, "failed to update PHY settings after maximum retries.\n");

and logs a spurious warning even though the PHY update succeeded on reopen.

No later fix was found in the series (searched git range
f342d5af0695..2d7643a78ddb).

> 	set_bit(BNGE_STATE_OPEN, &bd->state);
>
> 	bnge_enable_int(bn);

[ ... ]
-- 
pw-bot: cr