Re: [PATCH net-next v2] net: phy: aquantia: add get_features() for AQR113C

Re: [PATCH net-next v2] net: phy: aquantia: add get_features() for AQR113C

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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] The new aqr113c_get_features() stops the kernel from
  discovering EEE on AQR113C.
- [Low] If the premise is true, meaning AQR113C has never had its
  10G/5G/2.5G modes set correctly under the generic PMA path since…

--

net: phy: aquantia: add get_features() for AQR113C

This commit adds a get_features() callback for the AQR113C PHY. The PHY is
said to misreport its PMA capabilities, so the callback sets the
10G/5G/2.5G/1G link modes directly instead of reading the PMA registers.

> AQR113c does not correctly report its supported PMA
> capabilities which includes speeds like 10G/5G/2.5G.

[Severity: Low]
If AQR113C has never had its 10G/5G/2.5G modes reported correctly through
the generic PMA path, the bug goes back to the commit that added the
driver entry:

12cf1b89a668 ("net: phy: Add support for AQR113C EPHY")

Should this carry a Fixes: tag so stable picks it up? The v2 changelog
shows the patch was moved to net-next on purpose, so this is only a
process question.

> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e95d391a6dc23..604f5492515dd 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -1094,6 +1094,18 @@ static int aqr111_get_features(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int aqr113c_get_features(struct phy_device *phydev)
> +{
> +	/* Generic C45 PMA abilities do not fully describe the PHY's copper
> +	 * autonegotiation modes, so add the 10G mode explicitly.
> +	 */
> +	aqr111_get_features(phydev);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> +			 phydev->supported);
> +
> +	return 0;
> +}

[Severity: Medium]
Does this stop EEE discovery on AQR113C?

Before this patch, AQR113C had no .get_features. So phy_probe() took the
is_c45 branch and called genphy_c45_pma_read_abilities(), which ends with
genphy_c45_read_eee_abilities() and fills in phydev->supported_eee.

With the new callback, phy_probe() takes the get_features branch instead.
The core reads EEE abilities itself only in the static features branch:

drivers/net/phy/phy_device.c:phy_probe() {
    ...
	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);
	else if (phydev->is_c45)
		err = genphy_c45_pma_read_abilities(phydev);
    ...
}

The new call chain is aqr113c_get_features()->aqr111_get_features()->
aqr115c_get_features(). It only sets speed bits in phydev->supported and
never touches supported_eee.

Later in phy_probe(), the empty supported_eee then clears advertising_eee
and eee_cfg.eee_enabled ends up false:

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

genphy_c45_write_eee_adv() also writes the 7.60/7.62 advertisement
registers only when supported_eee intersects PHY_EEE_CAP1_FEATURES or
PHY_EEE_CAP2_FEATURES. That means the kernel can no longer change or clear
the EEE advertisement the PHY comes up with. Also, ethtool --show-eee
would report no supported modes.

aqr105_get_features() in the same file keeps EEE discovery by calling the
generic helper first and then adding the missing modes:

	ret = genphy_c45_pma_read_abilities(phydev);
	if (ret)
		return ret;

Would it make sense to follow that pattern here? Another option is to call
genphy_c45_read_eee_abilities(phydev) after setting the link modes.

AQR111, AQR114C and AQR115C already have the same gap. On AQR113C it is
new with this patch.

[ ... ]

> @@ -1444,6 +1456,7 @@ static struct phy_driver aqr_driver[] = {
>  	.get_sset_count = aqr107_get_sset_count,
>  	.get_strings    = aqr107_get_strings,
>  	.get_stats      = aqr107_get_stats,
> +	.get_features   = aqr113c_get_features,
>  	.link_change_notify = aqr107_link_change_notify,
>  	.led_brightness_set = aqr_phy_led_brightness_set,
>  	.led_hw_is_supported = aqr_phy_led_hw_is_supported,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-net-phy-aqr113c-fix-pma-capability-v2-1-29b06283b0f3%40oss.qualcomm.com

Re: [PATCH net-next v2] net: phy: aquantia: add get_features() for AQR113C

[Andrew Lunn]

On Thu, Sep 24, 2026 at 11:51:06AM +0800, Hongmei Xu wrote:
> AQR113c does not correctly report its supported PMA
> capabilities which includes speeds like 10G/5G/2.5G.
> Add get_features() to explicitly declare 10G/5G/2.5G/1G
> support instead of relying on PMA register reads.

Please slow down. You need to let discussions about previous versions
come to a conclusion.

    Andrew

---
pw-bot: cr

[PATCH net-next v2] net: phy: aquantia: add get_features() for AQR113C

[Hongmei Xu]

AQR113c does not correctly report its supported PMA
capabilities which includes speeds like 10G/5G/2.5G.
Add get_features() to explicitly declare 10G/5G/2.5G/1G
support instead of relying on PMA register reads.

Signed-off-by: Hongmei Xu <hongmei.xu@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
---
 drivers/net/phy/aquantia/aquantia_main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index e95d391a6dc2..604f5492515d 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -1094,6 +1094,18 @@ static int aqr111_get_features(struct phy_device *phydev)
 	return 0;
 }
 
+static int aqr113c_get_features(struct phy_device *phydev)
+{
+	/* Generic C45 PMA abilities do not fully describe the PHY's copper
+	 * autonegotiation modes, so add the 10G mode explicitly.
+	 */
+	aqr111_get_features(phydev);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+			 phydev->supported);
+
+	return 0;
+}
+
 static int aqr_gen4_config_init(struct phy_device *phydev)
 {
 	struct aqr107_priv *priv = phydev->priv;
@@ -1444,6 +1456,7 @@ static struct phy_driver aqr_driver[] = {
 	.get_sset_count = aqr107_get_sset_count,
 	.get_strings    = aqr107_get_strings,
 	.get_stats      = aqr107_get_stats,
+	.get_features   = aqr113c_get_features,
 	.link_change_notify = aqr107_link_change_notify,
 	.led_brightness_set = aqr_phy_led_brightness_set,
 	.led_hw_is_supported = aqr_phy_led_hw_is_supported,

---
base-commit: 063510768490d85454a99b27be303aad5efdf624
change-id: 20260917-net-phy-aqr113c-fix-pma-capability-d7d08cbc9879

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