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

[netdev-bot+sashiko@kernel.org]

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