Message-ID: <ZHdcrjANi/Uo0nLf@shell.armlinux.org.uk>
Date: Wed, 31 May 2023 15:41:50 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Florian Fainelli <f.fainelli@...il.com>, Andrew Lunn <andrew@...n.ch>,
	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 Wed, May 31, 2023 at 09:14:19AM +0200, Oleksij Rempel wrote:
> Hi Russell,
> 
> On Tue, May 30, 2023 at 08:49:50PM +0100, Russell King (Oracle) wrote:
> > On Tue, May 30, 2023 at 08:35:55PM +0100, Russell King (Oracle) wrote:
> > > Going back to phylib, given this, things get even more "fun" if you have
> > > a dual-media PHY. As there's no EEE capability bits for 1000base-X, but
> > > a 1000base-X PCS optionally supports EEE. So, even with a zero EEE
> > > advertisement with a dual-media PHY that would only affect the copper
> > > side, and EEE may still be possible in the fibre side... which makes
> > > phylib's new interpretation of "eee_enabled" rather odd.
> > > 
> > > In any case, "eee_enabled" I don't think has much meaning for the fibre
> > > case because there's definitely no control beyond what "tx_lpi_enabled"
> > > already offers.
> > > 
> > > I think this is a classic case where the EEE interface has been designed
> > > solely around copper without checking what the situation is for fibre!
> > 
> > Let me be a bit more explicit on this. If one does (e.g.) this:
> > 
> > # ethtool --set-eee eth0 advertise 0 tx-lpi on tx-timer 100
> > 
> > with a dual-media PHY, if the MAC is programmed to enable LPI, the
> > dual-media PHY is linked via fibre, and the remote end supports fibre
> > EEE, phylib will force "eee" to "off" purely on the grounds that the
> > advertisement was empty.
> > 
> > If one looks at the man page for ethtool, it says:
> > 
> >            eee on|off
> >                   Enables/disables the device support of EEE.
> > 
> > What does that mean, exactly, and how is it different from:
> > 
> >            tx-lpi on|off
> >                   Determines whether the device should assert its Tx LPI.
> > 
> > since the only control at the MAC is whether LPI can be asserted or
> > not and what the timer is.
> > 
> > The only control at the PHY end of things is what the advertisement
> > is, if an advertisement even exists for the media type in use.
> > 
> > So, honestly, I don't get what this ethtool interface actually intends
> > the "eee_enabled" control to do.
> 
> Thank you for your insightful observations on the EEE interface and its
> related complexities, particularly in the case of fiber interfaces.
> 
> Your comments regarding the functionality of eee_enabled and
> tx_lpi_enabled commands have sparked a good amount of thought on the
> topic. Based on my understanding and observations, I've put together a
> table that outlines the interactions between these commands, and their
> influence on the MAC LPI status, PHY EEE advertisement, and the overall
> EEE status on the link level.
> 
> For Copper assuming link partner advertise EEE as well:
> +------+--------+------------+----------------+--------------------------------+---------------------------------+
> | eee  | tx-lpi | advertise  | MAC LPI Status | PHY EEE Advertisement Status  | EEE Status on Link Level        |
> +------+--------+------------+----------------+--------------------------------+---------------------------------+
> | on   | on     |   !=0      | Enabled        | Advertise EEE for supported   | EEE enabled for supported       |
> |      |        |            |                | speeds                        | speeds (Full EEE operation)     |
> | on   | off    |   !=0      | Disabled       | Advertise EEE for supported   | EEE enabled for RX, disabled    |
> |      |        |            |                | speeds                        | for TX (Partial EEE operation)  |
> | off  | on     |   !=0      | Disabled       | No EEE advertisement          | EEE disabled                    |
> | off  | off    |   !=0      | Disabled       | No EEE advertisement          | EEE disabled                    |
> | on   | on     |    0       | Enabled        | No EEE advertisement          | EEE TX enabled, RX depends on   |
> |      |        |            |                |                               | link partner                    |
> | on   | off    |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
> | off  | on     |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
> | off  | off    |    0       | Disabled       | No EEE advertisement          | EEE disabled                    |
> +------+--------+------------+----------------+--------------------------------+---------------------------------+
> 
> For Fiber:
> +-----------+-----------+-----------------+---------------------+-------------------------+
> |     eee   |   tx-lpi  | PHY EEE Adv.    | MAC LPI Status      | EEE Status on Link Level|
> +-----------+-----------+-----------------+---------------------+-------------------------+
> |     on    |     on    |         NA      | Enabled             | EEE supported           |
> |     on    |     off   |         NA      | Disabled            | EEE not supported       |
> |     off   |     on    |         NA      | Disabled            | EEE not supported       |
> |     off   |     off   |         NA      | Disabled            | EEE not supported       |
> +-----------+-----------+-----------------+---------------------+-------------------------+
> 
> In my perspective, eee_enabled serves as a global administrative control for
> all EEE properties, including PHY EEE advertisement and MAC LPI status. When
> EEE is turned off (eee_enabled = off), both PHY EEE advertisement and MAC LPI
> status should be disabled, regardless of the tx_lpi_enabled setting.
> 
> On the other hand, advertise retains the EEE advertisement configuration, even
> when EEE is turned off. This way, users can temporarily disable EEE without
> losing their specific advertisement settings, which can then be reinstated when
> EEE is turned back on.

I agree. I've found phylib's interpretation to be weird, particularly
that the advertisement settings are lost if one sets eee_enabled to be
false. That alone makes eee_enabled entirely redundant.

To me, sane behaviour is exactly as you describe - if the user sets
eee_enabled false, but sets an advertisement, the kernel should store
the advertisement setting so it can be retrieved via get_eee(), ready
for use when eee_enabled is subsequently set true.

> In the context of fiber interfaces, where there is no concept of advertisement,
> the eee and tx-lpi commands may appear redundant. However, maintaining both
> commands could offer consistency across different media types in the ethtool
> interface.

Yes, because having a consistent behaviour is important for a user API.
Having a user API randomly change behaviour depending on what's behind
it leads to user confusion.

Consider the case of a dual-media PHY - should the behaviour of the
EEE setter/getter change depending on what media is in use? Clearly
not, since one normally configures the EEE _policy_ when configuring
the network device, rather than each time the link comes up.

Thanks for your input. I now have some patches that add:

1. generic EEE configuration helpers, which I hope phylib and phylink
   can both use for consistent behaviour.
2. phylink gaining infrastructure for EEE management both of existing
   phylib and also fibre setups.
3. mvneta converted to use phylink's implementation. I may also do
   mvpp2 as well as I have patches for EEE support there as well.

I hope to post the patches later today.

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