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

[Hongmei Xu <hongmei.xu@oss.qualcomm.com>]

The commit message is inaccurate in v3 — genphy_c45_pma_read_abilities()
is still called via the chain. The v4 commit message corrects this.
Fixes: tag added.

On 10/20/2026 2:26 AM, netdev-bot+sashiko@kernel.org 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] 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.
> 
> [ ... ]
>