netdev - Re: [PATCH v1] net: phy: dp83867: Disable EEE support as not implemented

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <df3aac25-e8e9-46cb-bd92-637822665080@lunn.ch>
Date: Mon, 27 Oct 2025 14:25:12 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Emanuele Ghidoli <ghidoliemanuele@...il.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Emanuele Ghidoli <emanuele.ghidoli@...adex.com>,
	Russell King <linux@...linux.org.uk>,
	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
> - Enabling EEE manually using ethtool, triggers the problem too (and ethtool
> -show-eee report eee status enabled):
> ethtool --set-eee end0 eee on tx-lpi on
> 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
> 
> I understand Russell point of view but from my point of view EEE is now
> enabled by default, and before it wasn't, at least on my setup.

We like to try to understand what is going on, and give accurate
descriptions. You have given us important information here, which at
minimum should go into the commit message, but more likely, it will
help lead us to the correct fix.

So, two things here. You say:

> I think that the phylink commit is just enabling by default the EEE support,

That needs confirming, because you are blaming the conversion to
phylink, not that phylink now enabled EEE by default. Russell also
tries to avoid behaviour change, which this clearly is. We want a
better understanding what caused this behaviour change.

Also:

> - Enabling EEE manually using ethtool, triggers the problem too (and ethtool
> -show-eee report eee status enabled):

This indicates EEE has always been broken. This brokenness has been
somewhat hidden in the past, and it is the change in behaviour in
phylink which exposed this brokenness. A commit message using these
words would be much more factually correct, and it would also fit with
the Fixes: tag you used.

So, please work with Russell. I see two things which would be good to
understand before a new version of the patch is submitted:

What cause the behaviour change such that EEE is now enabled? Was it
deliberate? Should something be change to revert that behaviour
change?

Given that EEE has always been broken, do we understand it
sufficiently to say it is not fixable? Is there an errata? Are we sure
it is the PHY and not the MAC which is broken?

	Andrew