Re: [PATCH net-next v3 1/2] net: phy: aquantia: restore EEE discovery in get_features callbacks

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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).

- [High] This patch makes aqr115c_get_features()
  (drivers/net/phy/aquantia/aquantia_main.c:1072) take phydev->supported…
- [Medium] The commit message misdescribes the hardware and understates
  the impact.
- [Medium] genphy_c45_read_eee_abilities() runs at the end of
  genphy_c45_pma_read_abilities().
- [Low] The patch fixes a user-visible regression (EEE not reported or
  configurable through ethtool on…

--

net: phy: aquantia: restore EEE discovery in get_features callbacks

This patch changes aqr115c_get_features() to build phydev->supported with
genphy_c45_pma_read_abilities() and then set the 2.5G bit. Previously it
used phy_gbit_features plus 2.5G. The aim is to fill
phydev->supported_eee so ethtool can show and configure EEE.
aqr111_get_features() now also returns the error from
aqr115c_get_features().

> As a result, phy_probe() never populates supported_eee for AQR111,
> AQR114C and AQR115C. The kernel then clears advertising_eee:

[ ... ]

> Fix this by following the pattern used in aqr105_get_features(): call
> genphy_c45_pma_read_abilities() as the base, then set the 2.5G bit
> explicitly since the PHY does not report it correctly in PMA registers.

[Severity: Medium]
Is this description of the hardware accurate? aqr105_get_features() is
for a PHY that fails to report 2.5G and 5G:

    /* The AQR105 PHY misses to indicate the 2.5G and 5G modes, so add them
     * here
     */

Earlier commits for this PHY family describe the opposite problem. Commit
17cbfcdd85f6 ("net: phy: aquantia: AQR115c fix up PMA capabilities") says:

    AQR115c reports incorrect PMA capabilities which includes 10G/5G and
    also incorrectly disables capabilities like autoneg and 10Mbps support.

Commit 0974f1f03b07 says AQCS109 reports 10G and 5G but doesn't support
them. Commit 8f61d73306c6 removed an aqr111_config_init() comment saying
AQR111 reports speeds up to 10G but only supports up to 5G.

That history is the reason for the "PMA capabilities are not useful"
comments. The patch removes them without saying why the PMA registers can
now be trusted.

The list of affected parts also looks incomplete. In aqr_driver[],
AQCS109, AQR111B0 and AQR115 use these same callbacks.

The message describes an EEE-only change, but the supported and
advertised link modes change too. 10G/5G may be added, and Autoneg,
10baseT, 1000baseT_Half and TP may be lost. Should the commit message
say so?

> Signed-off-by: Hongmei Xu <hongmei.xu@oss.qualcomm.com>

[Severity: Low]
Should this patch have a Fixes: tag? The missing EEE discovery seems to
come from 17cbfcdd85f6 ("net: phy: aquantia: AQR115c fix up PMA
capabilities") and 8f61d73306c6 ("net: phy: aquantia: remove usage of
phy_set_max_speed"). Those commits added ->get_features callbacks that
skip genphy_c45_pma_read_abilities().

phy_probe() only reads EEE abilities on the static ->features path:

	if (phydrv->features) {
		linkmode_copy(phydev->supported, phydrv->features);
		genphy_c45_read_eee_abilities(phydev);
	}
	else if (phydrv->get_features)
		err = phydrv->get_features(phydev);

Before 8f61d73306c6, AQR111, AQR114C and AQCS109 had no get_features
callback. They used the default genphy_c45_pma_read_abilities() path,
which does read the EEE abilities.

> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e95d391a6dc23..ed5631f2956ee 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -1071,23 +1071,25 @@ static int aqr_gen1_resume(struct phy_device *phydev)
>  
>  static int aqr115c_get_features(struct phy_device *phydev)
>  {
> -	unsigned long *supported = phydev->supported;
> +	int ret;
>  
> -	/* PHY supports speeds up to 2.5G with autoneg. PMA capabilities
> -	 * are not useful.
> -	 */
> -	linkmode_or(supported, supported, phy_gbit_features);
> -	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
> +	ret = genphy_c45_pma_read_abilities(phydev);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this bring back the wrong speed capabilities that 17cbfcdd85f6,
0974f1f03b07 and 8f61d73306c6 fixed?

AQCS109, AQR115 and AQR115C use this callback. aqr111_get_features()
calls it too, so AQR111, AQR111B0 and AQR114C are also affected.

genphy_c45_pma_read_ext_abilities() sets 10000baseT_Full from
MDIO_PMA_EXTABLE_10GBT and 5000baseT_Full from MDIO_PMA_NG_EXTABLE_5GBT.
The new code only sets bits after that and never clears any. As a
result:

  - the 2.5G parts (AQCS109, AQR115, AQR115C) would report 5G and 10G
    again
  - the 5G parts (AQR111, AQR111B0, AQR114C) would report 10G again

The modes that phy_gbit_features used to supply (Autoneg, 10baseT,
1000baseT_Half, TP) are also no longer forced on.
genphy_c45_pma_read_abilities() starts with:

	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);

It sets the bit again only if AN STAT1 reports ABLE. 17cbfcdd85f6 says
AQR115C misreports this. If it does, phy_probe() runs:

	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
			       phydev->supported))
		phydev->autoneg = 0;

aqr_config_aneg() then calls genphy_c45_pma_setup_forced(), and
2.5GBASE-T can't link without autoneg.

If autoneg stays enabled, phy_advertise_supported() copies the false
10G/5G modes into advertising. aqr_config_aneg() then advertises them to
the link partner through genphy_c45_an_config_aneg().

The next patch in this series, "net: phy: aquantia: add get_features()
for AQR113C", builds on aqr111_get_features() and doesn't fix this.

Could the hard-coded speed mask stay, with a call to
genphy_c45_read_eee_abilities(phydev) after it is built? phy_probe()
already does this for drivers with static ->features.

> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);

[Severity: Medium]
Can supported_eee still be missing the 2.5G and 5G EEE modes?
genphy_c45_read_eee_abilities() runs at the end of
genphy_c45_pma_read_abilities(). That happens before this line sets
2500baseT_Full, and before aqr111_get_features() sets 5000baseT_Full.

It reads the EEE capability 2 register (2.5G/5G EEE) only in this case:

	if (linkmode_intersects(phydev->supported, PHY_EEE_CAP2_FEATURES)) {
		val = genphy_c45_read_eee_cap2(phydev);

PHY_EEE_CAP2_FEATURES contains only 2500baseT_Full and 5000baseT_Full.

The commit message says the PHY doesn't report 2.5G in the PMA
registers. If MDIO_PMA_NG_EXTABLE reports neither 2.5G nor 5G, cap2 is
never read. phy_probe() then drops these modes from advertising_eee, and
ethtool --set-eee can't enable EEE at 2.5G or 5G.

Would calling genphy_c45_read_eee_abilities() after the final supported
mask is built avoid this?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-net-phy-aqr113c-fix-pma-capability-v3-0-0bbb1b84035d%40oss.qualcomm.com