On 5/30/23 13:28, Russell King (Oracle) wrote:

On Tue, May 30, 2023 at 09:48:00PM +0200, Andrew Lunn wrote:

quoted

On Tue, May 30, 2023 at 11:31:04AM -0700, Florian Fainelli wrote:

quoted

Hi Andrew, Russell,

On 3/30/23 17:54, Andrew Lunn wrote:

quoted

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