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

From: Paolo Abeni
Date: Thu May 02 2024 - 09:49:21 EST


On Tue, 2024-04-30 at 10:36 +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
>
> 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;
> if (wol->wolopts & WAKE_MAGIC) {
> /* MAC address - Byte0:Byte1:Byte2:Byte3:Byte4:Byte5
> * VPSPEC2_WOL_AD45 = Byte0:Byte1
> @@ -725,6 +726,8 @@ static int gpy_set_wol(struct phy_device *phydev,
> ret = phy_read(phydev, PHY_ISTAT);
> if (ret < 0)
> return ret;
> +
> + phydev->wolopts |= WAKE_MAGIC;

I'm wondering if 'phydev->wolopts' could/should be handled in the
common code.

> } else {
> /* Disable magic packet matching */
> ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
> @@ -748,6 +751,8 @@ static int gpy_set_wol(struct phy_device *phydev,
> if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC))
> phy_trigger_machine(phydev);
>
> + phydev->wolopts |= WAKE_PHY;
> +
> return 0;
> }
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 616bd7ba46cb..9740f08ad98e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2038,6 +2038,11 @@ int phy_suspend(struct phy_device *phydev)
> if (phydev->suspended)
> return 0;
>
> + if (phydev->wolopts) {
> + wol.wolopts = phydev->wolopts;
> + phy_ethtool_set_wol(phydev, &wol);

The above will fail when the phy does not provide wol operations -
should never happen when 'wolopts != 0', but possibly worth catching
the error?

Thanks,

Paolo