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

[Raju Lakkaraju]

Hi Andrew,

Thank you for review comments.

The 05/02/2024 16:51, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
> 

Based on your review comments, I will update 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?
> 

Currently I have GPY211C PHY along with PCI11414 chip hardware. That's reason
I add these changes for GPY211C PHY.
I test the changes on my board.

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

Ok. I will change.

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

Ok.
One quick question.
some of the options (ex. WAKE_PHY, WAKE_MAGIC etc) support on PHY and other
options (ex. WAKE_UCAST, WAKE_MAGICSECURE etc) on MAC of Ethernet device.

Suppose, user configure the combination (i.e. wol gu) option,
Is PHY flag should hold combination option or only PHY supported option ?
Ex:
$ sudo ethtool -s enp5s0 wol gu

Output of phydev's wolopts flag values should be 0x00000022 or 0x00000020 ?
In this case, PHY support WAKE_MAGIC and MAC support WAKE_UCAST

Anyhow, even phy's wolopts holds the user configuration value, get_wol( )
function read from PHY register and display only "g" 

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

Ok. I will change.
May be in phy_init_hw( ) function is better place to re-config the WOL

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

Ok. I will add sopass[] also.

> 
>         Andrew

-- 
Thanks,                                                                         
Raju