Re: [PATCH net-next v14 01/10] net: phy: aquantia: fix system interface type not updated in forced mode

[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.
---
net: phy: aquantia: fix system interface type not updated in forced mode

This drops the phydev->autoneg == AUTONEG_DISABLE test from the early
return in aqr_gen1_read_status(), so that MDIO_PHYXS_VEND_IF_STATUS is
decoded into phydev->interface whenever the link is up. The commit is
marked for stable and carries Fixes: 110a2432c520 ("net: phy: aquantia:
add downshift support").

> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e95d391a6dc23..b720e4e9028cc 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -544,7 +544,7 @@ static int aqr_gen1_read_status(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
> +	if (!phydev->link)
>  		return 0;
>  
>  	/* The status register is not immediately correct on line side link up.

[Severity: Medium]
This early return was also gating the rest of aqr_gen1_read_status(), not
just the MDIO_PHYXS_VEND_IF_STATUS decode. With the autoneg test gone, the
tail of the function now runs in forced mode too:

drivers/net/phy/aquantia/aquantia_main.c:aqr_gen1_read_status() {
	...
	phydev->interface = aqr_translate_interface(phydev, interface);

	/* Read rate from vendor register */
	return aqr_gen1_read_rate(phydev);
}

aqr_gen1_read_rate() overwrites both speed and duplex unconditionally from
the autoneg MMD vendor register:

drivers/net/phy/aquantia/aquantia_main.c:aqr_gen1_read_rate() {
	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1);
	if (val < 0)
		return val;

	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
		phydev->duplex = DUPLEX_FULL;
	else
		phydev->duplex = DUPLEX_HALF;

	switch (FIELD_GET(MDIO_AN_TX_VEND_STATUS1_RATE_MASK, val)) {
	case MDIO_AN_TX_VEND_STATUS1_10BASET:
		phydev->speed = SPEED_10;
	...
	default:
		phydev->speed = SPEED_UNKNOWN;
	}
}

In forced mode the speed and duplex were just established from the PMA by
aqr_read_status() -> genphy_c45_read_status() -> genphy_c45_read_pma(),
which reads the PMA MDIO_CTRL1 speed select and sets DUPLEX_FULL. Is
MDIO_AN_TX_VEND_STATUS1 still maintained by the firmware after
genphy_c45_an_disable_aneg() (called from genphy_c45_pma_setup_forced()
and aqr105_setup_forced())?

If it is not, a zeroed or stale rate field decodes as SPEED_10 with
DUPLEX_HALF, and an unrecognised code as SPEED_UNKNOWN, so a forced
1000BASE-T full duplex link would be reported to phylink at the wrong
rate and duplex. There is no validity qualifier and no "leave unchanged"
path in aqr_gen1_read_rate().

On gen2 parts a wrong or unknown speed also feeds the rate matching
lookup, which silently keeps the previous value when nothing matches:

drivers/net/phy/aquantia/aquantia_main.c:aqr_gen2_read_status() {
	for (i = 0; i < AQR_NUM_GLOBAL_CFG; i++) {
		struct aqr_global_syscfg *syscfg = &priv->global_cfg[i];

		if (syscfg->speed != phydev->speed)
			continue;
		...
			phydev->rate_matching = RATE_MATCH_PAUSE;
	...
}

The guard being removed came from the Fixes: commit 110a2432c520, where it
was added to keep the autoneg vendor rate handling out of the forced path.
Would it work to move the AUTONEG_DISABLE check onto the
aqr_gen1_read_rate() call instead of deleting it, so the interface decode
is always done but speed and duplex keep coming from the PMA in forced
mode? Alternatively, could the commit message state that
MDIO_AN_TX_VEND_STATUS1 is valid with autoneg disabled, since the message
currently only argues this for MDIO_PHYXS_VEND_IF_STATUS:

    MDIO_PHYXS_VEND_IF_STATUS is set by the PHY firmware based on the
    negotiated link speed, not based on whether autoneg was used to reach
    it.

This matters for the stable backport since the change is larger than the
one described.

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic