Re: [PATCH net v3] net: phy: mediatek-ge: disable EEE on the MT7530 PHY

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] mt7530_phy_probe() (drivers/net/phy/mediatek/mtk-ge.c:65-84)
  calls phy_disable_eee() from the driver's .probe callback, but phylib…
- [Medium] The hardware write that clears the EEE advertisement,
  phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0) at…

--

net: phy: mediatek-ge: disable EEE on the MT7530 PHY

This adds an mt7530_phy_probe() callback to the mediatek-ge driver which
clears MDIO_AN_EEE_ADV in hardware and then calls phy_disable_eee(), with
the stated goal that neither phylib nor userspace can re-enable EEE on the
MT7530 internal GE PHY.

> diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
> index 73d9b72f9d9e2..35600c70cab90 100644
> --- a/drivers/net/phy/mediatek/mtk-ge.c
> +++ b/drivers/net/phy/mediatek/mtk-ge.c
> @@ -62,6 +62,27 @@ static void mtk_gephy_config_init(struct phy_device *phydev)
>  		       FIELD_PREP(MTK_MCC_NEARECHO_OFFSET_MASK, 0x3));
>  }
>  
> +static int mt7530_phy_probe(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* The MT7530 internal GE PHY has broken EEE: with EEE advertised,
> +	 * some link partners fail to establish a stable link (on a 2-pair
> +	 * cable, 1000BASE-T training fails and the port loops instead of
> +	 * falling back). MediaTek recommends disabling EEE on this PHY.
> +	 * Clear the EEE advertisement early and mark EEE disabled, so
> +	 * that neither phylib nor userspace can re-enable it.
> +	 */
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Should this register write also live in mt7530_phy_config_init()?  This is
the only place where MDIO_AN_EEE_ADV is cleared in hardware, and ->probe is
the one callback phylib never replays.  phy_init_hw() deasserts the reset
line, optionally soft-resets, and then re-runs only ->config_init and
->config_intr:

drivers/net/phy/phy_device.c:phy_init_hw() {
	...
	if (phydev->drv->config_init) {
		ret = phydev->drv->config_init(phydev);
		if (ret < 0)
			return ret;
	}
	...
}

mt7530_phy_config_init() does not touch MDIO_AN_EEE_ADV, so after a PHY
register reset the register is back at its EEE-advertising hardware default
while phydev->advertising_eee stays empty and eee_cfg.eee_enabled stays
false, i.e. software and hardware state disagree.

Two paths that re-init without ->probe:

  - phy_detach() asserts the PHY reset signal and a later
    phy_attach_direct() -> phy_init_hw() only replays config_init.
  - the SIOCSMIIREG BMCR_RESET case in phy_mii_ioctl():

drivers/net/phy/phy.c:phy_mii_ioctl() {
	...
		if (prtad == phydev->mdio.addr &&
		    devad == MII_BMCR &&
		    val & BMCR_RESET)
			return phy_init_hw(phydev);
	...
}

which returns without any aneg reconfiguration, so nothing rewrites the
advertisement to zero.  __genphy_config_aneg() ->
genphy_c45_an_config_eee_aneg() covers the usual renegotiation case, but it
returns early without writing when phydev->autoneg == AUTONEG_DISABLE.  The
equivalent in-tree quirk issues the disable from the replayed callback,
dp83867_config_init() -> phy_disable_eee().

> +
> +	phy_disable_eee(phydev);

[Severity: High]
Does the phy_disable_eee() state survive the rest of phy_probe()?  The
linkmode_fill(phydev->eee_disabled_modes) done inside phy_disable_eee() is
what makes the disable permanent, but phylib clears that set later in the
same probe flow:

drivers/net/phy/phy_device.c:phy_probe() {
	...
	if (phydev->drv->probe) {
		err = phydev->drv->probe(phydev);
	...
	/* Get the EEE modes we want to prohibit. */
	of_set_phy_eee_broken(phydev);
	...
}

drivers/net/phy/phy-core.c:of_set_phy_eee_broken() {
	struct device_node *node = phydev->mdio.dev.of_node;
	unsigned long *modes = phydev->eee_disabled_modes;

	if (!IS_ENABLED(CONFIG_OF_MDIO) || !node)
		return;

	linkmode_zero(modes);
	...
}

So for any MT7530 PHY that has an of_node, eee_disabled_modes ends up empty
again unless the DT carries eee-broken-* properties.  An of_node is present
in a documented configuration: mt7530_setup_mdio() registers the internal
bus with the switch's "mdio" child node,

drivers/net/dsa/mt7530.c:mt7530_setup_mdio() {
	...
	mnp = of_get_child_by_name(np, "mdio");
	...
	ret = devm_of_mdiobus_register(dev, bus, mnp);
	...
}

and mediatek,mt7530.yaml documents that node, so ethernet-phy children
described there get an of_node.

With eee_disabled_modes empty, can userspace re-enable EEE again through
dsa_user_set_eee() -> phylink_ethtool_set_eee() -> phy_ethtool_set_eee() ->
genphy_c45_ethtool_set_eee()?

drivers/net/phy/phy-c45.c:genphy_c45_ethtool_set_eee() {
	...
		} else if (linkmode_empty(phydev->advertising_eee)) {
			phy_advertise_eee_all(phydev);
		}
	...
}

That repopulates advertising_eee with supported_eee, then
genphy_c45_an_config_eee_aneg() rewrites MDIO_AN_EEE_ADV and
phy_restart_aneg() renegotiates, which looks like it brings back exactly the
1000BASE-T training loop described in the commit message.

Would moving the disable into mt7530_phy_config_init(), which runs after
of_set_phy_eee_broken(), address both this and the reset case above?  That
is the placement dp83867_config_init() uses for the same kind of quirk.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904202800.3410838-1-vladislav.karmanov.dev%40gmail.com