Re: [PATCH net-next v21 13/13] ax88796b: Add support for AX88772D, AX88179A and AX88279

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net: phy: ax88796b: broken 2.5G LPA and pause in
  asix_ax88279_read_status()
--

commit bae79c8bab2ee5c87d5267f24efcefa71548bd4c
Author: Birger Koblitz <mail@birger-koblitz.de>

ax88796b: Add support for AX88772D, AX88179A and AX88279

This commit adds PHY driver support for the ASIX AX88772D (100Mbps),
AX88179A (1Gbps), and AX88279 (2.5Gbps) internal Ethernet PHYs.

> diff --git a/drivers/net/phy/ax88796b.c b/drivers/net/phy/ax88796b.c
> index f20ddf6491499..0df44e0e236ab 100644
> --- a/drivers/net/phy/ax88796b.c
> +++ b/drivers/net/phy/ax88796b.c

[ ... ]

> @@ -83,6 +96,107 @@ static int asix_ax88772a_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int asix_ax88279_read_status(struct phy_device *phydev)
> +{
> +	int ret, val;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (!phydev->link)
> +		return 0;
> +
> +	/* Read actual speed from vendor register */
> +	val = phy_read(phydev, AX_CTRL_STATUS);
> +	if (val < 0)
> +		return val;
> +
> +	switch (val & AX_CTRL_STATUS_SPEED_MASK) {
> +	case AX_CTRL_STATUS_SPEED_2500:
> +		phydev->speed = SPEED_2500;
> +		break;
> +	case AX_CTRL_STATUS_SPEED_1000:
> +		phydev->speed = SPEED_1000;
> +		break;
> +	case AX_CTRL_STATUS_SPEED_100:
> +		phydev->speed = SPEED_100;
> +		break;
> +	case AX_CTRL_STATUS_SPEED_10:
> +		phydev->speed = SPEED_10;
> +	}
> +
> +	/* MDIO_AN_10GBT_STAT_LP2_5G is broken, but we can deduce that
> +	 * the link-partner advertised 2500M if remotely AN succceded
> +	 * for link speed > 1000M and we locally have a link speed of
> +	 * 2500M
> +	 */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->lp_advertising);
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
> +	if (val >= 0 && val & MDIO_AN_10GBT_STAT_REMOK) {
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +				 phydev->lp_advertising,
> +				 phydev->speed == SPEED_2500);
> +	}
> +	/* Only supports full duplex */
> +	phydev->duplex = DUPLEX_FULL;
> +
> +	/* PHY switches interface between 2.5GBit and slower modes */
> +	if (phydev->speed == SPEED_2500)
> +		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> +	else
> +		phydev->interface = PHY_INTERFACE_MODE_SGMII;
> +
> +	return 0;
> +}

[Severity: Medium]
Does updating phydev->lp_advertising and phydev->duplex after
genphy_read_status() leave pause resolution and 2.5G link-partner
advertising broken?

genphy_read_status() resets phydev->duplex to DUPLEX_UNKNOWN and
phydev->pause / phydev->asym_pause to false, then calls
phy_resolve_aneg_linkmode().  When 2500baseT_Full is the only common
advertised speed mode (for example, when local advertisement is restricted
to 2500baseT/Full via ethtool), ETHTOOL_LINK_MODE_2500baseT_Full_BIT is not
yet set in phydev->lp_advertising during genphy_read_status(), so
phy_caps_lookup_by_linkmode() fails and phydev->duplex remains
DUPLEX_UNKNOWN when phy_resolve_aneg_pause() runs:

drivers/net/phy/phy-core.c:phy_resolve_aneg_pause() {
	if (phydev->duplex == DUPLEX_FULL) {
		phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
						  phydev->lp_advertising);
		phydev->asym_pause = linkmode_test_bit(
			ETHTOOL_LINK_MODE_Asym_Pause_BIT,
			phydev->lp_advertising);
	}
}

Setting phydev->duplex = DUPLEX_FULL later without calling
phy_resolve_aneg_pause(phydev) leaves negotiated pause flow control
disabled.

Also, returning early on !phydev->link before clearing
ETHTOOL_LINK_MODE_2500baseT_Full_BIT leaves that bit stale in
phydev->lp_advertising when a 2.5G link drops, because genphy_read_lpa()
only clears Clause 22 MII_LPA and MII_STAT1000 bits.  Finally, if
phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT) returns a negative
error, the error is ignored rather than returned to
phy_check_link_status().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-ax88179a-v21-0-c2d34410bcbe@birger-koblitz.de?part=13