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

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

[netdev-bot+sashiko]

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

[PATCH net-next v3 0/2] net: phy: aquantia: fix PMA capability reporting and EEE discovery

[Hongmei Xu]

This series fixes two related issues in the Aquantia PHY driver:
- Patch 1 restores EEE discovery for AQR111/114C/115C by replacing
  the manual linkmode_or() with genphy_c45_pma_read_abilities().
- Patch 2 adds get_features() for AQR113C to correctly report
  10G/5G/2.5G/1G PMA capabilities.

---
Changes in v3:
- Add patch 1 to fix EEE discovery regression in aqr115c_get_features()
  affecting AQR111, AQR114C and AQR115C
- Extend commit message to note AQR113 may have the same PMA issue
- Link to v2: https://patch.msgid.link/20260924-net-phy-aqr113c-fix-pma-capability-v2-1-29b06283b0f3@oss.qualcomm.com

Changes in v2:
- Fix subject prefix to [PATCH net-next]
- Limit scope to AQR113C only as AQR113 not tested
- Link to v1: https://patch.msgid.link/20260923-net-phy-aqr113c-fix-pma-capability-v1-1-497359310f06@oss.qualcomm.com

To: Andrew Lunn <andrew@lunn.ch>
To: Heiner Kallweit <hkallweit1@gmail.com>
To: Russell King <linux@armlinux.org.uk>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Hongmei Xu (2):
      net: phy: aquantia: restore EEE discovery in get_features callbacks
      net: phy: aquantia: add get_features() for AQR113C

 drivers/net/phy/aquantia/aquantia_main.c | 40 ++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 10 deletions(-)
---
base-commit: 944ae66642b726bd6b25ae71b1e9ff88a0e0bdb0
change-id: 20260917-net-phy-aqr113c-fix-pma-capability-d7d08cbc9879

Best regards,
--  
Hongmei Xu <hongmei.xu@oss.qualcomm.com>

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

[Hongmei Xu]

aqr115c_get_features() populates phydev->supported by ORing in
phy_gbit_features and setting the 2.5G bit directly. This skips
genphy_c45_pma_read_abilities(), which is responsible for reading
EEE capabilities and filling phydev->supported_eee.

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

  linkmode_and(phydev->advertising_eee, phydev->supported_eee,
               phydev->advertising_eee);

This means EEE advertisement registers cannot be changed via ethtool
and ethtool --show-eee reports no supported modes.

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.

Also fix aqr111_get_features() to check and propagate the return value
of aqr115c_get_features(), which can now fail since it calls
genphy_c45_pma_read_abilities().

Signed-off-by: Hongmei Xu <hongmei.xu@oss.qualcomm.com>
---
 drivers/net/phy/aquantia/aquantia_main.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index e95d391a6dc2..ed5631f2956e 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;
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported);
 
 	return 0;
 }
 
 static int aqr111_get_features(struct phy_device *phydev)
 {
-	/* PHY supports speeds up to 5G with autoneg. PMA capabilities
-	 * are not useful.
-	 */
-	aqr115c_get_features(phydev);
+	int ret;
+
+	ret = aqr115c_get_features(phydev);
+	if (ret)
+		return ret;
+
 	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
 			 phydev->supported);
 

-- 
2.34.1

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

[Andrew Lunn]

On Fri, Oct 16, 2026 at 11:23:18AM +0800, Hongmei Xu wrote:
> aqr115c_get_features() populates phydev->supported by ORing in
> phy_gbit_features and setting the 2.5G bit directly. This skips
> genphy_c45_pma_read_abilities(), which is responsible for reading
> EEE capabilities and filling phydev->supported_eee.
> 
> As a result, phy_probe() never populates supported_eee for AQR111,
> AQR114C and AQR115C. The kernel then clears advertising_eee:
> 
>   linkmode_and(phydev->advertising_eee, phydev->supported_eee,
>                phydev->advertising_eee);
> 
> This means EEE advertisement registers cannot be changed via ethtool
> and ethtool --show-eee reports no supported modes.
> 
> 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.
> 
> Also fix aqr111_get_features() to check and propagate the return value
> of aqr115c_get_features(), which can now fail since it calls
> genphy_c45_pma_read_abilities().
> 
> Signed-off-by: Hongmei Xu <hongmei.xu@oss.qualcomm.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew