Message-ID: <ZHZcc/E/Hx1bnjcx@shell.armlinux.org.uk>
Date: Tue, 30 May 2023 21:28:35 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Florian Fainelli <f.fainelli@...il.com>,
	netdev <netdev@...r.kernel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Oleksij Rempel <linux@...pel-privat.de>
Subject: Re: [RFC/RFTv3 00/24] net: ethernet: Rework EEE

On Tue, May 30, 2023 at 09:48:00PM +0200, Andrew Lunn wrote:
> On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:
> > Hi Andrew, Russell,
> > 
> > On 3/30/23 17:54, Andrew Lunn wrote:
> > > Most MAC drivers get EEE wrong. The API to the PHY is not very
> > > obvious, which is probably why. Rework the API, pushing most of the
> > > EEE handling into phylib core, leaving the MAC drivers to just
> > > enable/disable support for EEE in there change_link call back, or
> > > phylink mac_link_up callback.
> > > 
> > > MAC drivers are now expect to indicate to phylib/phylink if they
> > > support EEE. If not, no EEE link modes are advertised. If the MAC does
> > > support EEE, on phy_start()/phylink_start() EEE advertisement is
> > > configured.
> > 
> > Thanks for doing this work, because it really is a happy mess out there. A
> > few questions as I have been using mvneta as the reference for fixing GENET
> > and its shortcomings.
> > 
> > In your new patches the decision to enable EEE is purely based upon the
> > eee_active boolean and not eee_enabled && tx_lpi_enabled unlike what mvneta
> > useed to do.
> 
> I don't really care much what we decide means 'enabled'. I just want
> it moved out of MAC drivers and into the core so it is consistent.
> 
> Russel, if you want to propose something which works for both Copper
> and Fibre, i'm happy to implement it. But as you pointed out, we need
> to decide where. Maybe phylib handles copper, and phylink is layered
> on top and handles fibre?

Phylib also handles fibre too with dual-media PHYs (such as 88E151x
and 88X3310), and as I've just pointed out, the recent attempts at
"fixing" phylib's handling particularly with eee_enabled have made it
rather odd.

That said, the 88E151x resolution of 1000BASE-X negotiation is also
rather odd, particularly with pause modes. So I don't trust one bit
that anyone is even using 88E151x in fibre setups - or if they are
they don't care about this odd behaviour.

Before we go any further, I think we need to hammer out eactly how the
ethtool EEE interface is supposed to work, because right now I can't
say that I fully understand it - and as I've said in my replies to
Florian recently, phylib's EEE implementation becomes utterly silly
when it comes to fibre.

In particular, we need to hammer out what the difference exactly is
between "eee_enabled" and "tx_lpi_enabled", and what they control,
and I suggest we look at it from the point of view of both copper
(where EEE is negotiated) and fibre (were EEE is optional, no
capability bits, no negotiation, so no advertisement.)

It seems fairly obvious to me that tx_lpi* are about the MAC
configuration, since that's the entity which is responsible for
signalling LPI towards the PHY(PCS) over GMII.

eee_active... what does "active" actually mean? From the API doc, it
means the "Result of the eee negotiation" which is fine for copper
links where EEE is negotiated, but in the case of fibre, there isn't
EEE negotiation, and EEE is optionally implemented in the PCS.

eee_enabled... doesn't seem to have a meaning as far as IEEE 802.3
goes, it's a Linux invention. Documentation says "EEE configured mode"
which is just as useful as a chocolate teapot for making tea, that
comment might as well be deleted for what use it is. To this day, I
have no idea what this setting is actually supposed to be doing.
It seemed sane to me that if eee_enabled is false, then we should
not report eee_active as true, nor should we allow the MAC to
generate LPI. Whether the advertisement gets programmed into the PHY
or not is something I never thought about, and I can't remember
phylib's old behaviour. Modern phylib treats eee_enabled = false to
program a zero advertisement, which means when reading back via
get_eee(), you get a zero advertisement back. Effectively, eee_active
in modern phylib means "allow the advertisement to be programmed
if enabled, otherwise clear the advertisement".

If it's simply there to zero the advertisement, then what if the
media type has no capability for EEE advertisement, but intrinsically
supports EEE. That's where phylib's interpretation falls down IMHO.

Maybe this ethtool interface doesn't work very well for cases where
there is EEE ability but no EEE advertisement? Not sure.

Until we get that settled, we can't begin to fathom how phylib (or
phylink) should make a decision as to whether the MAC should signal
LPI towards the media or not.

-- 
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