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

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

[netdev-bot+sashiko]

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);

[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;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-net-phy-aqr113c-fix-pma-capability-v4-0-95a17f75dd7a%40oss.qualcomm.com

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

[Hongmei Xu]

AQR113C does not correctly report its supported speeds in the PMA
extended abilities registers. Without a get_features callback,
phy_probe() uses genphy_c45_pma_read_abilities() directly, which
produces an incorrect supported mask.

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.

The AQR113 likely has the same issue since it is a closely related
variant, but this fix is limited to AQR113C as that is the only
hardware available for testing.

Fixes: 12cf1b89a668 ("net: phy: Add support for AQR113C EPHY")

Signed-off-by: Hongmei Xu <hongmei.xu@oss.qualcomm.com>
---
 drivers/net/phy/aquantia/aquantia_main.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

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);
+
+	return 0;
+}
+
 static int aqr_gen4_config_init(struct phy_device *phydev)
 {
 	struct aqr107_priv *priv = phydev->priv;
@@ -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,
 	.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,

-- 
2.34.1