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

From: Florian Fainelli
Date: Thu May 02 2024 - 12:18:09 EST


On 5/2/24 08:59, Russell King (Oracle) wrote:
On Thu, May 02, 2024 at 04:51:42PM +0200, Andrew Lunn wrote:
On Tue, Apr 30, 2024 at 10:36:35AM +0530, Raju Lakkaraju wrote:
Introduce a new member named 'wolopts' to the 'phy_device' structure to
store the user-specified Wake-on-LAN (WOL) settings. Update this member
within the phy driver's 'set_wol()' function whenever the WOL configuration
is modified by the user.

Currently, when the system resumes from sleep, the 'phy_init_hw()' function
resets the PHY's configuration and interrupts, which leads to problems upon
subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
we can ensure that the PHY's WOL configuration is correctly reapplied
through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
the issue

Sorry it took a white to review this.


Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@xxxxxxxxxxxxx>
---
drivers/net/phy/mxl-gpy.c | 5 +++++
drivers/net/phy/phy_device.c | 5 +++++
include/linux/phy.h | 2 ++
3 files changed, 12 insertions(+)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index b2d36a3a96f1..6edb29a1d77e 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
struct net_device *attach_dev = phydev->attached_dev;
int ret;
+ phydev->wolopts = 0;

Is this specific to mlx-gpy?

You should be trying to solve the problem for all PHYs which support
WoL. So i expect the core to be doing most of the work. In fact, i
don't think there is any need for driver specific code.

It would be good to hear exactly why its necessary for phylib to track
this state, and why the PHY isn't retaining it.

Agreed. I contemplated doing something similar while adding support for Wake-on-LAN to the Broadcom PHY driver, but eventually convinced myself this was not necessary as the hardware was capable of retaining the wake-up event, and that the PHY driver *must* be able to charge the PHY device for wake-up purposes, even on a cold boot.


I suspect this may have something to do with resets - the PHY being
hardware reset when coming out of resume (resulting in all state
being lost.) What's resetting it would also be good to track down
(as in hardware, firmware, or the kernel.)


Since it is possible to override the soft_reset callback called by phy_init_hw(), I would be inclined to make this a driver specific solution by doing something like:

mxl_gphy_soft_reset(...)
/* Save WoL status */
priv->wol_enabled = ...
return genphy_soft_reset()
--
Florian