Re: [PATCH net v2 1/4] net: phy: as21xxx: handle corner case with link and autoneg complete

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: phy: as21xxx: handle corner case
  with link and autoneg complete" describes the wrong condition.

--

net: phy: as21xxx: handle corner case with link and autoneg complete

This commit makes as21xxx_read_link() store the autoneg complete bit
from MDIO_STAT1 in phydev->autoneg_complete. When autoneg is enabled
but has not completed, it reports the link as down. This is the same
approach genphy_update_link() takes.

> Add missing case in custom read_link, when autoneg is started, autoneg
> complete bit is reset but link is still not up.

[Severity: Low]
Is this the condition the new code actually handles?

If the link is not up, MDIO_STAT1_LSTATUS is already clear. That means
phydev->link is already 0 before the new check runs:

	phydev->link = !!(status & MDIO_STAT1_LSTATUS);
	phydev->autoneg_complete = !!(status & MDIO_AN_STAT1_COMPLETE);
	...
	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
		phydev->link = 0;

The check only changes anything when MDIO_STAT1_LSTATUS is still set
and MDIO_AN_STAT1_COMPLETE has already been cleared. The in-code
comment, copied from genphy_update_link(), describes it that way:

	/* Consider the case that autoneg was started and "aneg complete"
	 * bit has been reset, but "link up" bit not yet.
	 */

Could the commit message be reworded to match? The case being handled
is that the link up bit has not been reset yet, not that the link is
not up.

It could also help stable backporters to state the user-visible effect.
Without this check, as21xxx_read_status() can move from link down to
link up while genphy_c45_read_lpa() is taking its
!MDIO_AN_STAT1_COMPLETE branch:

	if (!(val & MDIO_AN_STAT1_COMPLETE)) {
		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
				   phydev->lp_advertising);
		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
		mii_adv_mod_linkmode_adv_t(phydev->lp_advertising, 0);
		...
		return 0;
	}

In that case phy_resolve_aneg_linkmode() resolves speed and duplex
after the base page and 10G link partner bits have been cleared. The
link can then be reported up with an unknown or stale speed.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022222955.698722-1-ansuelsmth%40gmail.com