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

[Hongmei Xu]

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@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] 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.
>
> [ ... ]