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

[Florian Fainelli]

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@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;
> >
> > 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