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

[Paolo Abeni]

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