linux-kernel - 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]

Date: Thu, 2 May 2024 09:17:51 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Andrew Lunn <andrew@...n.ch>
Cc: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>, netdev@...r.kernel.org,
 lxu@...linear.com, hkallweit1@...il.com, 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 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@...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.
> 
> 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