Date: Tue, 7 May 2024 18:56:54 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Ronnie.Kunin@...rochip.com
Cc: Raju.Lakkaraju@...rochip.com, andrew@...n.ch, netdev@...r.kernel.org,
	lxu@...linear.com, hkallweit1@...il.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	linux-kernel@...r.kernel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next] net: phy: add wol config options in phy device

On Tue, May 07, 2024 at 04:18:27PM +0000, Ronnie.Kunin@...rochip.com wrote:
> > So you want the MAC driver to access your new phydev->wolopts. What if there isn't a PHY, or the PHY
> > is on a pluggable module (e.g. SFP.) No, you don't want to have phylib tracking this for the MAC. The
> > MAC needs to track this itself if required.
> 
> There is definite value to having the phy be able to effectively
> communicate which specific wol events it currently has enabled so the
> mac driver can make better decisions on what to enable or not in the
> mac hardware (which of course will lead to more efficient power
> management). While not really needed for the purpose of fixing this
> driver's bugs, Raju's proposed addition of a wolopts tracking variable
> to phydev was also providing a direct way to access that information.
> In the current patch Raju is working on, the first call the lan743x
> mac driver does in its set_wol() function is to call the phy's
> set_wol() so that it gives the phy priority in wol handling. I guess
> when you say that phylib does not need to track this by adding a
> wolops member to the phydev structure, if we need that information
> the alternative way is to just immediately call the phy's get_wol()
> after set_wol() returns, correct ?

Depends what the driver wants to do.

>From the subset of drivers that implement WoL and use phylib:

drivers/net/ethernet/socionext/sni_ave.c:
	ave_init()
	ave_suspend() - to save the wolopts from the PHY
	ave_resume() - to restore them to the PHY - so presumably
		working around a buggy PHY driver, but no idea which
		PHY driver because although it uses DT, there's no way
		to know from DT which PHYs get used on this platform.

drivers/net/ethernet/ti/cpsw_ethtool.c:
	does nothing more than forwarding the set_wol()/get_wol()
	ethtool calls to phylib.

drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
	enetc_set_wol() merely sets the device for wakeup as appropriate
	based on the wolopts after passing it to phylib.

drivers/net/ethernet/microchip/lan743x_ethtool.c:
	lan743x_ethtool_set_wol() tracks the wolopts *before* passing
	to phylib, and enables the device for wakeup as appropriate.
	The whole:
	"return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
                        : -ENETDOWN;"
	thing at the end is buggy. You're updating the adapters state
	and the device wakeup enable, _then_ checking whether we have a
	phydev. If we don't have a phydev, you're then telling userspace
	that the request to change the WoL settings failed but you've
	already changed your configuration!

	Moreover, looking at lan743x_ethtool_get_wol(), you set
	WAKE_MAGIC | WAKE_PHY irrespective of what the PHY actually
	supports. This makes a total nonsense of the purpose of the
	supported flags here.

	I guess the _reason_ you do this is because the PHY may not be
	present (because you look it up in the .ndo_open() method) and
	thus you're trying to work around that... but then set_wol()
	fails in that instance. This all seems crazy.

	Broadcom bcmgenet also connects to the PHY in .ndo_open() and
	has sane semantics for .get_wol/.set_wol. I'd suggest having
	a look at that driver...

drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c:
	bcmgenet_set_wol() saves the value of wolopts in its
	private data structure and bcmgenet_wol_power_down_cfg()/
	bcmgenet_wol_power_up_cfg() uses this to decide what to
	power down/up.

Now, let's look at what ethtool already does for us when attempting
a set_wol() peration.

1. It retrieves the current WoL configuration from the netdev into
   'wol'.
2. It modifies wol.wolopts according to the users request.
3. It validates that the options set in wol.wolopts do _not_ include
   anything that is not reported as supported (in other words, no
   bits are set that aren't in wol.supported.)
4. It deals with the WoL SecureOn™ password.
5. It calls the netdev to set the new configuration.
6. If successful, it updates the netdev's flag which indicates whether
   WoL is currently enabled _in some form_ for this netdev.

Ergo, network device drivers are *not* supposed to report modes in
wol.supported that they do not support, and thus a network driver
can be assured that wol.wolopts will not contain any modes that are
not supported.

Therefore, if a network device wishes to track which WoL modes are
currently enabled, it can do this simply by:

1. calling phy_ethtool_set_wol(), and if that call is successful, then
2. save the value of wol.wolopts to its own private data structure to
   determine what it needs to do at suspend/resume.

This should be independent of which modes the PHY supports, because the
etwork driver should be using phy_ethtool_get_wol() to retrieve the
modes which the PHY supports, and then augmenting the wol.supported
mask with whatever additional modes the network driver supports
beyond that.

So, there is no real need for a network driver to query phylib for the
current configuration except possibly during probe or when connecting
to its PHY.

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