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

[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: 2 · Low: 0
>
> 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] aqr113c_get_features() adds 10000baseT_Full to
>    phydev->supported only after EEE discovery has already run, so…
> - [Medium] The commit message describes the fix's mechanism incorrectly.
>
> --
>
> net: phy: aquantia: add get_features() for AQR113C
>
> This adds an aqr113c_get_features() callback for the AQR113C. It builds on
> aqr111_get_features() and then sets 10000baseT_Full in the supported mask.
> Previously phy_probe() fell back to genphy_c45_pma_read_abilities(), which
> the commit says produces an incorrect supported mask for this PHY.
>
>> Add aqr113c_get_features() that calls aqr111_get_features() as a base
>> (which reads PMA abilities and forces 1G/5G/2.5G that the PHY misreports)
>> and sets 10000baseT_Full explicitly since the PHY also misreports 10G.
>
> [Severity: Medium]
> Is this description of aqr111_get_features() accurate? It calls
> aqr115c_get_features(), which does not read any PMA ability registers:
>
> drivers/net/phy/aquantia/aquantia_main.c:aqr115c_get_features() {
> /* 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);
>
> return genphy_c45_read_eee_abilities(phydev);
> }
>
> The only hardware read in this path is for the EEE abilities. MDIO_STAT2,
> MDIO_PMA_EXTABLE and MDIO_PMA_NG_EXTABLE are never read, so this patch
> moves AQR113C from a mask built from the registers to a fully static one.
>
> The message also leaves out a side effect. phy_gbit_features includes
> 1000baseT_Half and the 10/100 half and full duplex modes, so these are now
> always in supported. aqr_config_aneg() will then advertise 1000BASE-T half
> duplex:
>
> if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
>      phydev->advertising))
> reg |= MDIO_AN_VEND_PROV_1000BASET_HALF;
>
> The previous genphy_c45_pma_read_ext_abilities() path never set this bit.
> AQR111, AQR114C and AQR115C already behave this way, but could the change
> be mentioned in the message?
>
> The new comment in aqr113c_get_features() says only 10G is misreported in
> the PMA extended abilities. The commit message says 1G, 2.5G, 5G and 10G
> are all misreported. Could the message and the comment be brought in line
> with what the code does?
>
> The inherited "up to 2.5G" comment in aqr115c_get_features() is also stale
> now that a 10G PHY uses it.
>
>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
>> index 099ee919de94..153adfbda53f 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)
>> +{
>> + /* AQR113C does not correctly report 10G in PMA extended abilities.
>> + * Build on aqr111_get_features() for 5G/2.5G and add 10G explicitly.
>> + */
>> + int ret;
>> +
>> + ret = aqr111_get_features(phydev);
>> + if (ret)
>> + return ret;
>> +
>> + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
>> + phydev->supported);

The patch [net: phy: aquantia: restore EEE discovery in get_features
callbacks] will be reverted as it changed the existing logic for AQR111，
AQR111，AQR114C PHYs. I should not touch these unrealted PHYs.
And commit message should be more accurate.

> [Severity: Medium]
> 10000baseT_Full is set here only after EEE discovery has already run. Does
> that mean AQR113C always loses 10GBASE-T EEE?
>
> The call chain is:
>
> aqr113c_get_features()
>    aqr111_get_features()
>      aqr115c_get_features()
>        genphy_c45_read_eee_abilities()
>          genphy_c45_read_eee_cap1()
>
> At that point supported holds only the gbit modes and 2500baseT_Full.
> genphy_c45_read_eee_cap1() maps MDIO_EEE_10GT from register 3.20 into
> supported_eee and then masks it against supported:
>
> mii_eee_cap1_mod_linkmode_t(phydev->supported_eee, val);
> ...
> linkmode_and(phydev->supported_eee, phydev->supported_eee,
>     phydev->supported);
>
> 10000baseT_Full is not in supported yet, so the 10G EEE bit is cleared.
> Nothing reads the EEE abilities again afterwards: phy_probe() only calls
> genphy_c45_read_eee_abilities() in its drv->features branch.
>
> After that, phy_probe() ANDs advertising_eee with supported_eee. Then
> genphy_c45_an_config_eee_aneg() writes the reduced mask to MDIO_AN_EEE_ADV,
> which looks like it would clear any default 10GT EEE advertisement in the
> hardware.
>
> genphy_c45_ethtool_set_eee() would also return -EINVAL for a 10G EEE
> request:
>
> if (linkmode_andnot(tmp, adv, phydev->supported_eee)) {
>
> 5G is not affected, because cap2 is not masked against supported.
>
> Before this patch 10G was missing from supported entirely, so this is not
> a loss compared to the old state. However, supported and supported_eee now
> disagree on 10G.
>
> The earlier commit "net: phy: aquantia: restore EEE discovery in
> get_features callbacks" put EEE discovery at the end of
> aqr115c_get_features() so that supported would be final by then. With this
> nesting, that no longer holds for a cap1 mode.
>
> Would it work to set all the forced modes before calling
> genphy_c45_read_eee_abilities()? For example, the EEE read could move to
> the end of each top-level get_features callback.
>
>> +
>> + return 0;
>> +}
>
> [ ... ]
>

Agreed, will call genphy_c45_read_eee_abilities() at the the end of each
top-level get_features after all supported mask is built per driver.

pw-bot: cr


Regards,
Hongmei Xu