diff mbox series

[net-next,v14,01/10] net: phy: aquantia: fix system interface type not updated in forced mode

Message ID 20260908-qcom-sa8255p-emac-v14-1-ec212b747a7d@oss.qualcomm.com (mailing list archive)
State New
Headers
Series net: stmmac: qcom-ethqos: add support for SCMI power domains |

Commit Message

Bartosz Golaszewski Sept. 8, 2026, 2:57 p.m. UTC 
aqr_gen1_read_status() decodes the MDIO_PHYXS_VEND_IF_STATUS register
to determine which SerDes interface the PHY is currently using on its
system side and stores the result in phydev->interface. phylink relies
on this value to configure the MAC.

The autoneg == AUTONEG_DISABLE check is not correct:
MDIO_PHYXS_VEND_IF_STATUS is set by the PHY firmware based on the
negotiated link speed, not based on whether autoneg was used to reach
it. When the link comes up at 1G in forced mode, the register correctly
reads SGMII, but the early return prevents phydev->interface from being
updated. It stays at whatever value it held before (typically 2500BASE-X
from the initial autoneg run), so phylink configures the MAC for the
wrong interface and the link cannot come up.

Remove the autoneg guard so that the system interface type is always
decoded when the link is up.

Cc: stable@vger.kernel.org
Fixes: 110a2432c520 ("net: phy: aquantia: add downshift support")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/net/phy/aquantia/aquantia_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Kicinski Sept. 14, 2026, 11:14 p.m. UTC | #1 
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: aquantia: fix system interface type not updated in forced mode

This drops the phydev->autoneg == AUTONEG_DISABLE test from the early
return in aqr_gen1_read_status(), so that MDIO_PHYXS_VEND_IF_STATUS is
decoded into phydev->interface whenever the link is up. The commit is
marked for stable and carries Fixes: 110a2432c520 ("net: phy: aquantia:
add downshift support").

> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e95d391a6dc23..b720e4e9028cc 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -544,7 +544,7 @@ static int aqr_gen1_read_status(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
> +	if (!phydev->link)
>  		return 0;
>  
>  	/* The status register is not immediately correct on line side link up.

[Severity: Medium]
This early return was also gating the rest of aqr_gen1_read_status(), not
just the MDIO_PHYXS_VEND_IF_STATUS decode. With the autoneg test gone, the
tail of the function now runs in forced mode too:

drivers/net/phy/aquantia/aquantia_main.c:aqr_gen1_read_status() {
	...
	phydev->interface = aqr_translate_interface(phydev, interface);

	/* Read rate from vendor register */
	return aqr_gen1_read_rate(phydev);
}

aqr_gen1_read_rate() overwrites both speed and duplex unconditionally from
the autoneg MMD vendor register:

drivers/net/phy/aquantia/aquantia_main.c:aqr_gen1_read_rate() {
	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1);
	if (val < 0)
		return val;

	if (val & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX)
		phydev->duplex = DUPLEX_FULL;
	else
		phydev->duplex = DUPLEX_HALF;

	switch (FIELD_GET(MDIO_AN_TX_VEND_STATUS1_RATE_MASK, val)) {
	case MDIO_AN_TX_VEND_STATUS1_10BASET:
		phydev->speed = SPEED_10;
	...
	default:
		phydev->speed = SPEED_UNKNOWN;
	}
}

In forced mode the speed and duplex were just established from the PMA by
aqr_read_status() -> genphy_c45_read_status() -> genphy_c45_read_pma(),
which reads the PMA MDIO_CTRL1 speed select and sets DUPLEX_FULL. Is
MDIO_AN_TX_VEND_STATUS1 still maintained by the firmware after
genphy_c45_an_disable_aneg() (called from genphy_c45_pma_setup_forced()
and aqr105_setup_forced())?

If it is not, a zeroed or stale rate field decodes as SPEED_10 with
DUPLEX_HALF, and an unrecognised code as SPEED_UNKNOWN, so a forced
1000BASE-T full duplex link would be reported to phylink at the wrong
rate and duplex. There is no validity qualifier and no "leave unchanged"
path in aqr_gen1_read_rate().

On gen2 parts a wrong or unknown speed also feeds the rate matching
lookup, which silently keeps the previous value when nothing matches:

drivers/net/phy/aquantia/aquantia_main.c:aqr_gen2_read_status() {
	for (i = 0; i < AQR_NUM_GLOBAL_CFG; i++) {
		struct aqr_global_syscfg *syscfg = &priv->global_cfg[i];

		if (syscfg->speed != phydev->speed)
			continue;
		...
			phydev->rate_matching = RATE_MATCH_PAUSE;
	...
}

The guard being removed came from the Fixes: commit 110a2432c520, where it
was added to keep the autoneg vendor rate handling out of the forced path.
Would it work to move the AUTONEG_DISABLE check onto the
aqr_gen1_read_rate() call instead of deleting it, so the interface decode
is always done but speed and duplex keep coming from the PMA in forced
mode? Alternatively, could the commit message state that
MDIO_AN_TX_VEND_STATUS1 is valid with autoneg disabled, since the message
currently only argues this for MDIO_PHYXS_VEND_IF_STATUS:

    MDIO_PHYXS_VEND_IF_STATUS is set by the PHY firmware based on the
    negotiated link speed, not based on whether autoneg was used to reach
    it.

This matters for the stable backport since the change is larger than the
one described.
diff mbox series

Patch

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index e95d391a6dc233879ae1fb2a97758b3df9600ce5..b720e4e9028cc20823297e8bbd239121aeafa09f 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -544,7 +544,7 @@  static int aqr_gen1_read_status(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	if (!phydev->link || phydev->autoneg == AUTONEG_DISABLE)
+	if (!phydev->link)
 		return 0;
 
 	/* The status register is not immediately correct on line side link up.