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

[Vladislav Karmanov]

Both findings confirmed against current sources -- thank you, the
analysis is spot on.

v4 keeps the probe()-time advertisement clear, which also feeds an
empty advertisement back into genphy_c45_read_eee_adv() during
phy_probe(), and moves the durable disable to config_init(), the
dp83867 placement: config_init() runs after of_set_phy_eee_broken() in
phy_probe(), so the eee_disabled_modes fill is not undone, and it is
the callback phy_init_hw() replays after a PHY reset, so the register
clear is restored as well. The register is also written again there
for the reset path.

v4 follows as a fresh thread.
https://lore.kernel.org/netdev/20260908145213.3976508-1-vladislav.karmanov.dev@xxxxxxxxx/

Tested on the affected hardware (ASUS RT-AX53U, MT7621): single clean
100 Mbps WAN link + DHCP lease over a 2-pair cable, no flapping. The
ethtool re-enable path is not reachable on this board (phylink rejects
EEE ops at the MAC layer), so that part is verified at the code level
for standalone use of the PHY.

вт, 8 сент. 2026 г. в 11:30, <netdev-bot+sashiko@xxxxxxxxxx>:
>
> 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