Re: [PATCH net v2 2/4] net: phy: as21xxx: fix read_status speed handling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With autoneg enabled, as21xxx_read_status()
  (drivers/net/phy/as21xxx.c) now takes the duplex for 1000/100/10 links…
- [Medium] With autoneg enabled, as21xxx_read_status() now reads and
  decodes VEND1_SPEED_STATUS (MMD VEND1 0x4002) whenever the cached…
- [Low] The commit message does not match several behaviour changes in
  the patch.

--

net: phy: as21xxx: fix read_status speed handling

This commit reworks as21xxx_read_status() so that the speed always comes
from the vendor VEND1_SPEED_STATUS register, including when autoneg is
enabled. It also changes the switch to mask with VEND1_SPEED_MASK. The
generic link partner registers are now used only to resolve pause, through
phy_resolve_aneg_pause().

> With further test with 2.5G NIC it was discovered that
> phy_resolve_aneg_linkmode is not enough to detect speed higher that 1G
> when autoneg is enabled.

[ ... ]

> Rework the read_status code to always read the speed from the vendor
> register and parse the generic bit only for the pause frame.

[Severity: Low]
Could the commit message explain why phy_resolve_aneg_linkmode() was not
enough?

The removed genphy_c45_read_lpa() already read MDIO_AN_10GBT_STAT and set
the partner's 2.5G/5G/10G bits before resolution. The new code reads the
same register to fill lp_advertising:

    val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
    ...
    mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, val);

The message doesn't say whether that register is reliable on this PHY. If
it isn't, the partner modes reported to userspace would still be wrong.

The message also leaves out a few behaviour changes:

- In as21xxx_read_c22_lpa(), the base page partner advertisement now
  comes from the mapped C22 MII_LPA instead of C45 MDIO_AN_LPA. The
  partner Autoneg bit therefore comes from LPA_LPACK instead of
  MDIO_AN_STAT1_LPABLE.

- Forced 10/100 used to always report DUPLEX_FULL and now reports the
  BMCR_FULLDPLX bit. Only the v2 changelog below the --- line mentions
  this, and that part is dropped when the patch is applied.

- The default -EINVAL return in the vendor speed switch can now be
  reached with autoneg enabled.

Since the patch carries a Fixes: tag, could the commit message describe
these changes too?

> diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
> index 0db82da8dbdf3..519b78d3579d3 100644
> --- a/drivers/net/phy/as21xxx.c
> +++ b/drivers/net/phy/as21xxx.c

[ ... ]

> @@ -720,58 +729,73 @@ static int as21xxx_read_status(struct phy_device *phydev)
>  	phydev->asym_pause = 0;
>  
>  	if (phydev->autoneg == AUTONEG_ENABLE) {
> -		ret = genphy_c45_read_lpa(phydev);
> -		if (ret)
> -			return ret;
> +		if (!phydev->autoneg_complete) {
> +			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
> +							0);
> +			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
> +			mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
> +			return 0;
> +		}

[Severity: Medium]
Can phydev->autoneg_complete be stale when this guard checks it?

as21xxx_read_link() returns early when ANRESTART is set, before it updates
autoneg_complete:

    if (*bmcr & BMCR_ANRESTART) {
        phydev->link = 0;
        return 0;
    }

If the previous poll saw the link up, autoneg_complete is still 1, so
renegotiation skips this guard. One example is
_phy_start_aneg()->phy_config_aneg()->phy_check_link_status() running
before ANRESTART self-clears.

The removed genphy_c45_read_lpa() re-read MDIO_STAT1 on every call. The
code also falls through when AN complete is still set but LSTATUS has
dropped.

In both cases the code goes on to decode VEND1_SPEED_STATUS while the link
is down or renegotiating, which can cause two problems.

First, VEND1_SPEED_10 is encoded as 0, so a zero or unresolved register
value is reported as a 10 Mbps link. If the register lags behind link up,
the "why bother the PHY if nothing can have changed" shortcut keeps that
speed until the link drops.

Second, any value outside the six documented codes hits the default
-EINVAL. phy_state_machine() turns that into phy_error_precise() and
PHY_ERROR. Before this patch, that path could only be reached with autoneg
disabled.

Should this guard use a fresh AN status read, or should the vendor speed
read be skipped when phydev->link is 0? The later patches in the series
don't seem to change this code.

>  
>  		ret = as21xxx_read_c22_lpa(phydev);
>  		if (ret)
>  			return ret;
>  
> -		phy_resolve_aneg_linkmode(phydev);
> -	} else {
> -		int speed;

[ ... ]

> +	switch (speed & VEND1_SPEED_MASK) {

[ ... ]

> +	case VEND1_SPEED_1000:
> +		phydev->speed = SPEED_1000;
> +		if (bmcr & BMCR_FULLDPLX)
>  			phydev->duplex = DUPLEX_FULL;

[ ... ]

> +		else
> +			phydev->duplex = DUPLEX_HALF;
> +		break;

[Severity: Medium]
With autoneg enabled, does BMCR_FULLDPLX report the negotiated duplex for
the 1000/100/10 cases?

Before this patch, the autoneg duplex came from
phy_resolve_aneg_linkmode(), which intersects advertising with
lp_advertising. BMCR bit 8 is a forced mode control bit (IEEE 802.3
22.2.4.1.8). genphy_read_status_fixed() reads it only for
AUTONEG_DISABLE.

Nothing on the autoneg path writes this bit. genphy_restart_aneg() only
sets BMCR_ANENABLE | BMCR_ANRESTART, while genphy_setup_forced() rewrites
the whole BMCR.

For example, if the user forces 100/half and then turns autoneg back on,
BMCR_FULLDPLX stays clear. Every later autonegotiated 10/100/1000 link
would then be reported as half duplex. A 1000/half result also looks
impossible here, because 1000baseT_Half doesn't seem to be advertised.

The reverse also applies: if the bit is left at 1, a link that negotiated
half duplex would be reported as full.

A wrong duplex also disables pause, because phy_resolve_aneg_pause() only
resolves it for full duplex:

    if (phydev->duplex == DUPLEX_FULL) {
        phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, ...

Unless the vendor mapped C22 BMCR mirrors the resolved duplex, should the
autoneg case get the duplex from the advertisement intersection, as it did
before? The later patches in the series don't seem to change this either.

[ ... ]

> +	if (phydev->autoneg == AUTONEG_ENABLE)
> +		phy_resolve_aneg_pause(phydev);
> +
>  	return 0;
>  }

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