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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7fe419b2-fc73-4584-ae12-e9e313d229c3@lunn.ch>
Date: Thu, 2 May 2024 16:51:42 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org, lxu@...linear.com, hkallweit1@...il.com,
	linux@...linux.org.uk, 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, 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@...rochip.com>
> ---
>  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.

phy_ethtool_set_wol() can set phydev->wolopts after calling
phydev->drv->set_wol(). If it returns an error, including -ENOTSUPP,
set phydev->wolopts to 0, otherwise set it to wolopts.

> @@ -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);
> +	}

Why on suspend? I would expect it to be on resume, after the PHY has
been reset.

I also think you need to save sopass[] in phydev, since some PHYs
support WAKE_MAGICSECURE. Just because mlx-gpy does not need the
password does not mean we should ignore it in general. I also think it
is safe to store in memory. Its is not a highly confidential
password. I would not be too surprised if some PHYs have the registers
read/write rather than write only.

	Andrew