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

[Florian Fainelli <f.fainelli@gmail.com>]

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@microchip.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