Message-ID: <aP-Hgo5mf7BQyee_@shell.armlinux.org.uk>
Date: Mon, 27 Oct 2025 14:53:54 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Emanuele Ghidoli <ghidoliemanuele@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Emanuele Ghidoli <emanuele.ghidoli@...adex.com>,
	Oleksij Rempel <o.rempel@...gutronix.de>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH v1] net: phy: dp83867: Disable EEE support as not
 implemented

On Mon, Oct 27, 2025 at 01:57:48PM +0100, Emanuele Ghidoli wrote:
> 
> 
> On 27/10/2025 00:45, Andrew Lunn wrote:
> >> Since the introduction of phylink-managed EEE support in the stmmac driver,
> >> EEE is now enabled by default, leading to issues on systems using the
> >> DP83867 PHY.
> > 
> > Did you do a bisect to prove this?
> Yes, I have done a bisect and the commit that introduced the behavior on our
> board is 4218647d4556 ("net: stmmac: convert to phylink managed EEE support").
> 
> > 
> >> Fixes: 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy")
> > 
> > What has this Fixes: tag got to do with phylink?
> I think that the phylink commit is just enabling by default the EEE support,
> and my commit is not really fixing that. It is why I didn't put a Fixes: tag
> pointing to that.
> 
> I’ve tried to trace the behavior, but it’s quite complex. From my testing, I
> can summarize the situation as follows:
> 
> - ethtool, after that patch, returns:
> ethtool --show-eee end0
> EEE settings for end0:
>         EEE status: enabled - active
>         Tx LPI: 1000000 (us)
>         Supported EEE link modes:  100baseT/Full
>                                    1000baseT/Full
>         Advertised EEE link modes:  100baseT/Full
>                                     1000baseT/Full
>         Link partner advertised EEE link modes:  100baseT/Full
>                                                  1000baseT/Full
> - before that patch returns, after boot:
> EEE settings for end0:
>         EEE status: disabled
>         Tx LPI: disabled
>         Supported EEE link modes:  100baseT/Full
>                                    1000baseT/Full
>         Advertised EEE link modes:  Not reported
>         Link partner advertised EEE link modes:  100baseT/Full
>                                                  1000baseT/Full

Oh damn. I see why now:

        /* Some DT bindings do not set-up the PHY handle. Let's try to
         * manually parse it
         */
        if (!phy_fwnode || IS_ERR(phy_fwnode)) {
                int addr = priv->plat->phy_addr;
		...
                if (priv->dma_cap.eee)
                        phy_support_eee(phydev);

                ret = phylink_connect_phy(priv->phylink, phydev);
        } else {
                fwnode_handle_put(phy_fwnode);
                ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);
        }

The driver only considers calling phy_support_eee() when DT fails to
describe the PHY (because in the other path, it doesn't have access to
the struct phy_device to make this call.)

My commit makes it apply even to DT described PHYs, so now (as has been
shown when you enable EEE manually) it's uncovering latent problems.

So now we understand why the change has occurred - this is important.
Now the question becomes, what to do about it.

For your issue, given that we have statements from TI that indicate
none of their gigabit PHYs support EEE, we really should not be
reporting to userspace that there is any EEE support. Therefore,
"Supported EEE link modes" should be completely empty.

I think I understand why we're getting EEE modes supported. In the
DP83867 manual, it states for the DEVAD field of the C45 indirect
access registers:

"Device Address: In general, these bits [4:0] are the device address
DEVAD that directs any accesses of ADDAR register (0x000E) to the
appropriate MMD. Specifically, the DP83867 uses the vendor specific
DEVAD [4:0] = 11111 for accesses. All accesses through registers
REGCR and ADDAR can use this DEVAD. Transactions with other
DEVAD are ignored."

Specifically, that last sentence, and the use of "ignored". If this
means the PHY does not drive the MDIO data line when registers are
read, they will return 0xffff, which is actually against the IEEE
requirements for C45 registers (unimplemented C45 registers are
supposed to be zero.)

So, this needs to be tested - please modify phylib's
genphy_c45_read_eee_cap1() to print the value read from the register.

If it is 0xffff, that confirms that theory.

The correct solution here, to stop other MAC drivers running into this
is for TI PHY drivers to implement the .get_features() phylib method,
call genphy_read_abilities() or genphy_c45_pma_read_abilities() as
appropriate, and then clear phydev->supported_eee so the PHY driver
reports (correctly according to TI's statements) that EEE modes are not
supported.

So, while my commit does have an unintended change of behaviour (thanks
for helping to locate that), it would appear that it has revealed a
latent bug in likely all TI PHY gigabit drivers that needs fixing
independently of what we do about this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists