Re: [PATCH net-next] net: phy: add wol config options in phy device

From: Russell King (Oracle)
Date: Tue May 07 2024 - 13:57:30 EST


On Tue, May 07, 2024 at 04:18:27PM +0000, Ronnie.Kunin@xxxxxxxxxxxxx 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!