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

Re: [PATCH net-next v3 2/2] 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 commit message says the patch adds get_features() to
  "explicitly declare 10G/5G/2.5G/1G support instead of relying on PMA…
- [Low] The patch is written as a fix for a hardware-reporting defect
  ("AQR113c does not correctly report its supported PMA capabilities"),…

--

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

This adds an aqr113c_get_features() callback to the AQR113C entry in
aqr_driver[]. The callback builds on aqr111_get_features() and also sets
10000baseT_Full in phydev->supported. The commit message says the PHY
does not correctly report its PMA capabilities.

> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index ed5631f2956ee..8213d9a15b4fe 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -1096,6 +1096,23 @@ 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.
> +	 */
> +	int ret;
> +
> +	ret = aqr111_get_features(phydev);
> +	if (ret)
> +		return ret;
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> +			 phydev->supported);
> +
> +	return 0;
> +}

[Severity: Medium]
Does the commit message match what this code does? It says:

  Add get_features() to explicitly declare 10G/5G/2.5G/1G support
  instead of relying on PMA register reads.

The call chain still starts with the generic PMA reads:

aqr113c_get_features()
  aqr111_get_features()
    aqr115c_get_features()
      genphy_c45_pma_read_abilities()

So phydev->supported is still built from the PMA registers. On top of
that, the chain only adds 2500baseT_Full in aqr115c_get_features(),
5000baseT_Full in aqr111_get_features() and 10000baseT_Full here.

Nothing in this chain sets 1000baseT_Full explicitly. It still comes only
from the PMA extended abilities register:

drivers/net/phy/phy-c45.c:genphy_c45_pma_read_ext_abilities() {
    ...
	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
			 phydev->supported,
			 val & MDIO_PMA_EXTABLE_1000BT);
    ...
}

The comment in the function gives a third version ("add the 10G mode
explicitly"). It does not mention that 5G and 2.5G are also forced
through the callees.

Could the commit message and the comment say which abilities the AQR113C
PMA registers (MDIO_PMA_EXTABLE / MDIO_PMA_NG_EXTABLE) actually
misreport? For example, if the PHY already reports 10GBASE-T in
MDIO_PMA_EXTABLE, the extra linkmode_set_bit() here does nothing.

[ ... ]

> @@ -1446,6 +1463,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,

[Severity: Low]
The commit message describes this as a fix for a hardware reporting
problem. Should it have a Fixes: tag?

This commit added the AQR113C entry in aqr_driver[] without a
get_features callback:

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

Because of that, phy_probe() has always used
genphy_c45_pma_read_abilities() for this PHY:

drivers/net/phy/phy_device.c:phy_probe() {
    ...
	else if (phydrv->get_features)
		err = phydrv->get_features(phydev);
	else if (phydev->is_c45)
		err = genphy_c45_pma_read_abilities(phydev);
    ...
}

Earlier fixes for the same quirk on related Aquantia PHYs had Fixes:
tags. Two examples are 17cbfcdd85f6 ("net: phy: aquantia: AQR115c fix up
PMA capabilities") and 8f61d73306c6 ("net: phy: aquantia: remove usage
of phy_set_max_speed").

Would something like this be appropriate?

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

Without the tag, stable kernels that support AQR113C may not pick up this
change.

[ ... ]

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

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

[Andrew Lunn]

On Fri, Oct 16, 2026 at 11:23:19AM +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.
> 
> The AQR113 likely has the same issue since it is a closely related
> variant, but this fix is limited to AQR113C as that is the only
> hardware available for testing.
> 
> Signed-off-by: Hongmei Xu <hongmei.xu@oss.qualcomm.com>

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

    Andrew

[PATCH net-next v3 2/2] 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.

The AQR113 likely has the same issue since it is a closely related
variant, but this fix is limited to AQR113C as that is the only
hardware available for testing.

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

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index ed5631f2956e..8213d9a15b4f 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -1096,6 +1096,23 @@ 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.
+	 */
+	int ret;
+
+	ret = aqr111_get_features(phydev);
+	if (ret)
+		return ret;
+
+	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;
@@ -1446,6 +1463,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,

-- 
2.34.1