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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 5 · 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_DISABLE, as21xxx_read_status() no longer takes
  speed/duplex from the PHY.
- [Medium] In autoneg mode, as21xxx_read_status() now reads
  VEND1_SPEED_STATUS on every link-up, which is the PHY's own report of…
- [Medium] The new early `if (!phydev->link) return 0;` in
  as21xxx_read_status() (drivers/net/phy/as21xxx.c:732-733) makes the…
- [Medium] In forced mode (AUTONEG_DISABLE), phydev->speed/duplex hold
  the user's configuration.
- [Medium] net: phy: as21xxx: missed pause and <=1G speed in
  as21xxx_read_status()
- [Low] VEND1_SPEED_STATUS is now read on every autoneg link-up as well
  as in forced mode.

--

net: phy: as21xxx: fix read_status speed handling

This patch reworks as21xxx_read_status() to use the VEND1_SPEED_STATUS
vendor register for speeds above 1G. Speeds of 1G and below come from
phy_resolve_aneg_linkmode() in autoneg mode and from
genphy_read_status_fixed() in forced mode. It also changes the switch mask
to VEND1_SPEED_MASK, reads MII_LPA from the mapped C22 space, and clears
autoneg_complete when AN is restarted.

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

[ ... ]

> @@ -719,57 +729,63 @@ static int as21xxx_read_status(struct phy_device *phydev)
>  	phydev->pause = 0;
>  	phydev->asym_pause = 0;
>  
> +	if (!phydev->link)
> +		return 0;
> +

[Severity: Medium]
With AUTONEG_DISABLE, phydev->speed and phydev->duplex hold the forced
configuration. Just above this, as21xxx_read_status() does:

	phydev->speed = SPEED_UNKNOWN;
	phydev->duplex = DUPLEX_UNKNOWN;

The "why bother the PHY" shortcut only applies to AUTONEG_ENABLE.

In forced mode with the link down, does this new early return mean nothing
puts those values back? genphy_read_status() re-reads the fixed settings
whatever the link state is.

On the next _phy_start_aneg(), for example after ip link set down/up or
resume, phy_sanitize_settings() calls phy_caps_lookup() with
SPEED_UNKNOWN/DUPLEX_UNKNOWN. Nothing matches, so it falls back to:

	if (!match && !exact)
		match = last;

That is the slowest supported mode, which can be 10/half.

Would the PHY then be reprogrammed at that rate instead of the speed the
user forced?

The later patch in this series, "net: phy: as21xxx: force C45 OPs for
AUTONEG", makes as21xxx_config_aneg() call genphy_c45_pma_setup_forced().
That function returns -EINVAL for anything other than DUPLEX_FULL. With
that patch, could the PHY end up in PHY_ERROR on every restart until it is
reconfigured with ethtool?

>  	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]
Is this block reachable? as21xxx_read_link() sets link to 0 on
BMCR_ANRESTART. It also does:

	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
		phydev->link = 0;

After the new !phydev->link early return, autoneg_complete is therefore
always 1 at this point. For the same reason, the autoneg_complete test in
the phy_resolve_aneg_linkmode() condition below is redundant.

Does this mean lp_advertising is no longer cleared when the link drops or
AN restarts? Before this patch, genphy_c45_read_lpa() cleared the partner
bits when AN was not complete, and the forced branch always ran
linkmode_zero().

phylib zeroes lp_advertising only in PHY_HALTED, and
phy_ethtool_ksettings_get() copies it out unconditionally. Would ethtool
then keep showing the old link partner's advertised modes after the cable
is unplugged?

[ ... ]

> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> +		phy_resolve_aneg_linkmode(phydev);
> +	} else if (phydev->autoneg == AUTONEG_DISABLE) {
> +		ret = genphy_read_status_fixed(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Medium]
Before this patch, genphy_c45_read_lpa() set phydev->pause and
phydev->asym_pause directly from LPA_PAUSE_CAP and LPA_PAUSE_ASYM.
With that call gone, pause is only resolved by phy_resolve_aneg_pause()
inside phy_resolve_aneg_linkmode(). That helper only updates pause
when phydev->duplex is already DUPLEX_FULL at the time it runs.

phy_resolve_aneg_linkmode() only sets duplex when
phy_caps_lookup_by_linkmode() finds a match in lp_advertising &
advertising. The commit message suggests the >1G partner bits are not
usable on this PHY. If that is so, and there is no common <=1G
full-duplex mode (for example when only 2.5G/5G/10G is advertised),
duplex is still DUPLEX_UNKNOWN here.

The VEND1_SPEED_STATUS switch below then sets DUPLEX_FULL for the
2500/5000/10000 codes. Nothing resolves pause again after that.

Wouldn't that leave pause and asym_pause at 0 on a full-duplex >1G
link even when the partner advertised pause? The MAC would then come
up without flow control.

The same ordering causes the downshift problem with the 1000/100/10
cases below. If the mode resolved here is above the rate VEND1
reports, phydev->speed keeps the value taken from the advertisement
rather than the actual link rate.

Could speed and duplex be taken from VEND1 for every code first, with
phy_resolve_aneg_pause() called after the switch?

[Severity: Medium]
Can genphy_read_status_fixed() be trusted on this PHY? It does a plain C22
read:

	int bmcr = phy_read(phydev, MII_BMCR);

It then takes speed from BMCR_SPEED1000/BMCR_SPEED100 and duplex from
BMCR_FULLDPLX. as21xxx_read_link() deliberately avoids that register:

	/* Normal C22 BMCR report inconsistent data, use
	 * the mapped C22 in C45 to have more consistent link info.
	 */

The mapped bmcr value it returns is no longer used anywhere in
as21xxx_read_status(). The VEND1_SPEED_1000/100/10 cases below only
break, so for forced 10/100/1000 the reported values come from the native
BMCR.

With the later patch in this series, "net: phy: as21xxx: force C45 OPs for
AUTONEG", forced mode is set up only through genphy_c45_pma_setup_forced()
(PMA/PMD CTRL1/CTRL2, full duplex only). The native BMCR is then never
written.

Could this report DUPLEX_HALF, or a speed that differs from what VEND1
reports?

There is also the case of an MDIO bus with no C22 read callback. There,
__mdiobus_read() returns -EOPNOTSUPP, which would now leave read_status
and put the PHY in PHY_ERROR. The old forced path made no C22 access.

[ ... ]

> +	speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> +			     VEND1_SPEED_STATUS);
> +	if (speed < 0)
> +		return speed;
> +
> +	switch (speed & VEND1_SPEED_MASK) {
> +	case VEND1_SPEED_10000:
> +		phydev->speed = SPEED_10000;
> +		phydev->duplex = DUPLEX_FULL;
> +		break;

[ ... ]

> +	case VEND1_SPEED_1000:
> +	case VEND1_SPEED_100:
> +	case VEND1_SPEED_10:
> +		break;

[Severity: Medium]
In autoneg mode the hardware-reported rate is now read on every link-up.
For 1000/100/10, though, it is discarded, and phydev->speed/duplex keep
whatever phy_resolve_aneg_linkmode() set: the highest mode both sides
advertise.

The commit message says phy_resolve_aneg_linkmode() is "not enough" for
this PHY, but it stays the only source at 1G and below. Two cases look
like problems.

Suppose lp_advertising contains a mode above 1G (MDIO_AN_10GBT_STAT is
now read), but the link trains at 1G or 100M, for example after a firmware
downshift. Would read_status report 2.5G while VEND1 says 1G?

If the common advertisement has no match, would the link be reported up
with SPEED_UNKNOWN/DUPLEX_UNKNOWN even though VEND1 holds a valid code?

Would it make sense to set speed and duplex from VEND1 for all six codes?

> +	default:
> +		return -EINVAL;
>  	}

[Severity: Low]
Before this patch, VEND1_SPEED_STATUS was read only in forced mode. It is
now read on every autoneg link-up as well. Any value outside the six known
codes lands here. _phy_state_machine() turns the -EINVAL into
phy_error_precise(), which leaves the PHY in PHY_ERROR.

Can this firmware-maintained register hold a transient or unlisted value
at link-up? If so, would it be safer to keep the resolved speed than to
fail read_status?

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