Re: [PATCH net-next v20 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: stale 2.5G LPA and pause bug in
  asix_ax88279_read_status()
--

commit 15d79caef25783090283820b8fda304017ab1973
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, AX88179A, and
AX88279 PHYs, including status reading and auto-negotiation configuration
for the 2.5G AX88279 PHY.

> 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;

[Severity: Medium]
Does updating phydev->lp_advertising and phydev->duplex after
genphy_read_status() leave ETHTOOL_LINK_MODE_2500baseT_Full_BIT stale on
link down and prevent pause resolution on 2.5G links?

When a 2.5G link drops, genphy_read_status() -> genphy_read_lpa() only
clears Clause 22 10/100/1000 link modes in phydev->lp_advertising.
Returning early on !phydev->link skips clearing
ETHTOOL_LINK_MODE_2500baseT_Full_BIT, leaving it reported via ethtool while
the link is down.

Also, genphy_read_status() runs phy_resolve_aneg_linkmode() before
ETHTOOL_LINK_MODE_2500baseT_Full_BIT is set in phydev->lp_advertising and
before phydev->duplex is set to DUPLEX_FULL.  When 2500baseT_Full is the
only common speed mode, phy_resolve_aneg_linkmode() leaves phydev->duplex
as DUPLEX_UNKNOWN, so phy_resolve_aneg_pause() does not set phydev->pause
or phydev->asym_pause:

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 afterwards without calling
phy_resolve_aneg_pause() leaves negotiated pause flow control disabled.
In addition, if phy_read_mmd() fails with a negative error, that error is
silently ignored rather than returned.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-ax88179a-v20-0-37b5a1189983@birger-koblitz.de?part=13