Date: Tue, 30 May 2023 16:03:35 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Andrew Lunn <andrew@...n.ch>
Cc: 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 5/30/23 13:28, Russell King (Oracle) wrote:
> 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.

Yes that much we can agree on that tx_lpi* is from the perspective of 
the MAC.

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

Is there any way to feed back whether EEE is actually being used at the 
time on a fiber link? In which case eee_active would reflect that state.

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

At the time it was introduced, there certainly was not much care being 
given to fiber use cases.

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

Now that we have SmartEEE and AutogrEEEn as additional supported modes 
by certain PHY drivers for MACs that lack any LPI signaling capability 
towards the PHY, there may be a reason for re-purposing or clarifying 
the meaning of eee_enabled=true with tx_lpi_enabled=false. Although in 
those cases, I would just issue a warning that tx_lpi_enabled is ignored 
because the MAC is incapable of LPI signaling.

It seems that eee_enabled is intended to be a local administrative 
parameter that provides an additional gating level to the local & remote 
advertisement resolution. As a driver you can only enable EEE at the MAC 
and/or PHY level if local & remote & enabled = true.
-- 
Florian

Powered by blists - more mailing lists