Received-SPF: pass (google.com: domain of linux-kernel+bounces-81926-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 26 Feb 2024 17:44:31 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>,
	"Russell King (Oracle)" <linux@armlinux.org.uk>,
	Wei Fang <wei.fang@nxp.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Heiner Kallweit <hkallweit1@gmail.com>, kernel@pengutronix.de,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	Shenwei Wang <shenwei.wang@nxp.com>,
	Clark Wang <xiaoning.wang@nxp.com>,
	NXP Linux Team <linux-imx@nxp.com>
Subject: Re: [PATCH net-next v6 5/8] net: phy: Immediately call adjust_link
 if only tx_lpi_enabled changes
Message-ID: <c6b0716d-f093-4aba-8453-c89a562ab581@lunn.ch>
References: <20240223094425.691209-1-o.rempel@pengutronix.de>
 <20240223094425.691209-6-o.rempel@pengutronix.de>
 <Zdh1nMWZDynP/AMc@shell.armlinux.org.uk>
 <84e1368d-ec6a-48af-945b-509528c45dff@lunn.ch>
 <Zdic+ua5LnWxjLPn@shell.armlinux.org.uk>
 <6af3406a-7968-41e5-bf6e-71d020d8b28a@broadcom.com>
 <Zdot-Mqw1z4ZEo8v@pengutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zdot-Mqw1z4ZEo8v@pengutronix.de>

On Sat, Feb 24, 2024 at 06:57:12PM +0100, Oleksij Rempel wrote:
> On Fri, Feb 23, 2024 at 09:53:06AM -0800, Florian Fainelli wrote:
> > On 2/23/24 05:26, Russell King (Oracle) wrote:
> > > On Fri, Feb 23, 2024 at 02:17:59PM +0100, Andrew Lunn wrote:
> > > > On Fri, Feb 23, 2024 at 10:38:20AM +0000, Russell King (Oracle) wrote:
> > > > > On Fri, Feb 23, 2024 at 10:44:22AM +0100, Oleksij Rempel wrote:
> > > > > > +static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> > > > > > +				      struct ethtool_keee *data)
> > > > > > +{
> > > > > > +	if (phydev->eee_cfg.tx_lpi_enabled !=
> > > > > > +	    data->tx_lpi_enabled) {
> > > > > > +		eee_to_eeecfg(data, &phydev->eee_cfg);
> > > > > > +		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> > > > > > +		if (phydev->link)
> > > > > > +			phy_link_up(phydev);
> > > > > 
> > > > > I'm not convinced this is a good idea. Hasn't phylib previously had
> > > > > the guarantee that the link will go down between two link-up events?
> > > > > So calling phy_link_up() may result in either the MAC driver ignoring
> > > > > it, or modifying registers that are only supposed to be modified while
> > > > > the MAC side is down.
> > > > 
> > > > When auto-neg is used, we expect the link to go down and come back up
> > > > again.
> > > > 
> > > > Here we are dealing with the case that autoneg is not used. The MAC
> > > > needs informing somehow. If we want to preserve the down/up, we could
> > > > call phy_link_down() and then phy_link_up() back to back.
> > > 
> > > Would it be better to have a separate callback for EEE state (as I
> > > mentioned in another comment on this series?) That would be better
> > > for future SmartEEE support.
> > 
> > That sounds like a good approach to me. The additional callback also helps
> > figure out which drivers use the API and it should be simpler to audit for
> > changes in the future, too.
> 
> At this point I need help to understand how to proceed.
> What exactly do you have in mind? Some description like following would
> be helpful:
> Add callback with name phy_link_set_eee(), which should be executed in
> function bla/blup..

When i first did this patchset, SmartEEE was out of scope. One
question we should decide is, is it still out of scope, and we should
first get 'dumb' EEE fixed everywhere, and than come back and look at
SmartEEE? Or do we want to consider SmartEEE now?

The idea of this patchset was to push as much as possible down into
phylib. The MAC needs to say it supports EEE. I left handling of the
tx_lpi_timer to the MAC driver, because the PHY has nothing it can do
with that value. phylib then just needs to tell the MAC to enable or
disable EEE when autoneg has completed. That i made part of the adjust
link callback because that is the only callback we have, and most
developers seem to understand it, and the locking around it. However,
it does get messy when EEE can change without an auto-neg, as pointed
out here.

If we are leaving SmartEEE out of scope for the moment, i would say
just doing a down/up is sufficient, lets get this merged and all
'dumb' EEE fixed.

If we want feature creep and to think about SmartEEE then we need a
few changes in the overall design.

We need to make eee_get and eee_set transparent to the MAC driver,
since the PHY could be doing it all. So phylib needs to track
tx_lpi_timer. If the MAC driver indicates it can do 'dumb' EEE we
probably want to use that in preference to SmartEEE, since i guess the
MAC can also save a little power in LPI mode. So the adjust link
callback needs to say: Enable MAC EEE with this value of tx_lpi_timer,
or turn off MAC EEE. When using SmartEEE it will never actually do
either.

The current phylib model is that adjust_link is the only callback, and
the MAC driver peeks into phydev to find what it needs. I would
probably stick to that model, and not add MAC callbacks. phylink is
slightly different, mac_link_up() passes everything the MAC needs to
know as parameters, so one of my patches adds an extra parameter to
indicate if EEE should be enabled or disabled. That would need
extending with the tx_lpi_timer value.

	  Andrew