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

[Hongmei Xu]

On 10/26/2026 5:48 PM, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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] net: phy: aquantia: premature and unmasked EEE read in
> aqr115c_get_features()
> - [Low] aqr115c_get_features()
> (drivers/net/phy/aquantia/aquantia_main.c:1082) now ends with `return…
>
> --
>
> net: phy: aquantia: restore EEE discovery in get_features callbacks
>
> This patch makes aqr115c_get_features() call
> genphy_c45_read_eee_abilities() after it builds the supported mask, so
> supported_eee gets filled in for AQR111, AQR111B0, AQR114C, AQR115,
> AQCS109 and AQR115C. It also makes aqr111_get_features() pass on the
> return value of aqr115c_get_features().
>
>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
>> index e95d391a6dc2..099ee919de94 100644
>> --- a/drivers/net/phy/aquantia/aquantia_main.c
>> +++ b/drivers/net/phy/aquantia/aquantia_main.c
>> @@ -1079,15 +1079,17 @@ static int aqr115c_get_features(struct phy_device *phydev)
>> linkmode_or(supported, supported, phy_gbit_features);
>> linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
>>
>> - return 0;
>> + return genphy_c45_read_eee_abilities(phydev);
>> }
>>
>> 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;
>
> [Severity: Medium]
> Two related problems here. The 2.5G-only parts can end up with
> supported_eee out of sync with supported. In aqr111_get_features(),
> the EEE read also runs before the final supported mask is built.
>
> genphy_c45_read_eee_abilities() reads 3.20 via
> genphy_c45_read_eee_cap1(). That function masks the result against
> supported:
>
> linkmode_and(phydev->supported_eee, phydev->supported_eee,
> phydev->supported);
>
> It then reads 3.21 via genphy_c45_read_eee_cap2(), which only does:
>
> mii_eee_cap2_mod_linkmode_sup_t(phydev->supported_eee, val);
>
> That helper sets 2500baseT_Full and 5000baseT_Full straight from
> MDIO_EEE_2_5GT and MDIO_EEE_5GT. Nothing masks that result against
> supported afterwards, either here or in phy_probe().
>
> For AQR115C, AQR115 and AQCS109, supported stops at 2500baseT_Full.
> The commit message itself says these PHYs misreport their
> capabilities. If 3.21 also reports 5GT on them, supported_eee ends
> up with 5000baseT_Full on a 2.5G PHY. phy_probe() only intersects
> advertising_eee with supported_eee, so a 5G EEE advertisement can
> survive into advertising_eee. ethtool --show-eee would then list
> 5000baseT/Full on a PHY that cannot link at 5G.
>
> The ordering in aqr111_get_features() does not match the commit
> message, which says the EEE read happens after the final supported
> mask is built. Here aqr115c_get_features(), and with it the EEE
> read, runs before 5000baseT_Full is added to supported. For 5G this
> only works because cap2 is unmasked.
>
> For cap1 modes it breaks. Patch 2/2 has the AQR113C get_features()
> chain onto aqr111_get_features() before it sets 10000baseT_Full. So
> the linkmode_and() in genphy_c45_read_eee_cap1() strips
> 10000baseT_Full from supported_eee while 10G is not yet in
> supported, and it never comes back.
>
> Could the EEE read be done once per driver, after that driver's
> supported mask is complete? For example, keep the mask building in a
> shared helper and call genphy_c45_read_eee_abilities() last in each
> get_features(). A linkmode_and() of supported_eee against supported
> after that would keep 3.21 from adding modes the PHY lacks. Or do
> you think genphy_c45_read_eee_cap2() should apply the same mask that
> cap1 already does?
>
Agreed. genphy_c45_read_eee_abilities() should only be called after the
supported parameter is built. So it should be called per driver. Adding
it to aqr115c_get_features() is wrong as aqr115c_get_features() is
called in aqr111_get_features() and coming aqr113c_get_features() before
the supported mask is been built in those PHYs.

> [Severity: Low]
> Should the result of genphy_c45_read_eee_abilities() be passed back as
> the ->get_features() result here?
>
> With this change, if reading the optional EEE ability registers (3.20
> MDIO_PCS_EEE_ABLE or 3.21 MDIO_PCS_EEE_ABLE2) fails, aqr115c_get_features()
> fails, and so does aqr111_get_features(). phy_probe() then aborts:
>
> drivers/net/phy/phy_device.c:phy_probe() {
> ...
> else if (phydrv->get_features)
> err = phydrv->get_features(phydev);
> ...
> }
>
> The phylib core ignores the return value of this helper on purpose at
> all three of its call sites. genphy_c45_pma_read_abilities() and
> genphy_read_abilities() both have:
>
> /* This is optional functionality. If not supported, we may get an error
> * which should be ignored.
> */
> genphy_c45_read_eee_abilities(phydev);
>
> The phydrv->features branch of phy_probe() also ignores it:
>
> if (phydrv->features) {
> linkmode_copy(phydev->supported, phydrv->features);
> genphy_c45_read_eee_abilities(phydev);
> }
>
Agreed. Should not take genphy_c45_pma_read_abilities() return value as
.get_features result.

> The commit message says it restores EEE discovery. The code path it
> restores, genphy_c45_pma_read_abilities() as used before 17cbfcdd85f6
> and 8f61d73306c6, never failed probe because of EEE discovery.
>
> In practice this is hard to hit. aqr107_probe() already reads VEND1
> registers in aqr_wait_reset_complete() and fails probe on read errors.
> That mostly leaves a transient MDIO bus error during this exact read as
> the newly fatal case.
>
> Would it match the core better to call genphy_c45_read_eee_abilities()
> without passing its result back, and keep return 0?
>
>> +
>> linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>> phydev->supported);
>>
>

Since my goal is just to add .get_features for aqr113c phy, I should not
touch the existing logic of aqr115c_get_features which will impact
AQR111，AQR111，AQR114C PHYs. I should revert this patch.

pw-bot: cr


Regards,
Hongmei Xu