Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Andrew Lunn]

On Fri, Apr 05, 2024 at 08:17:22AM +0000, Raju.Lakkaraju@xxxxxxxxxxxxx wrote:
> Hi Andrew,
> 
> Sorry for delayed response.
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: Thursday, March 21, 2024 4:23 AM
> > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
> > Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> > pabeni@xxxxxxxxxx; edumazet@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > Bryan Whitehead - C21958 <Bryan.Whitehead@xxxxxxxxxxxxx>;
> > UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>
> > Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when
> > PHY does not
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > 
> > > +     if (netdev->phydev) {
> > > +             ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > > +             if (ret != -EOPNOTSUPP && ret != 0)
> > > +                     return ret;
> > 
> > I'm not sure this condition is correct.
> > 
> > If there is an error, and the error is not EOPNOTSUPP, you want to report that
> > error. However, if the PHY can support the WoL configuration, it will return 0,
> > and this function should exit, WoL in the MAC is not needed. And doing WoL
> > in the PHY consumes less power since you can suspend the MAC.
> > 
> > So i think it should simply be:
> > 
> > > +             if (ret != -EOPNOTSUPP)
> > > +                     return ret;
> > 
> > Do you have a board with this MAC with a PHY which does have some WoL
> > support. Could you test PHY WoL is used when appropriate?
> 
> Yes. 
> We have external PHY (Max Linear GPY211C) attach to MAC of PCI11x1x (PCIe Ethernet chip) 
> If I don't register the Ethernet module in wakeup source, WOL is not working. Ethernet device power state shows as disable.

So i'm talking about the case where the PHY is doing the wakeup,
without help from the MAC. In that case, "Ethernet device power state
shows as disable." is sensible.

> i.e.
> /sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.0/power/wakeup   <--  disabled
> 
> PCI11x1x is PCIe bridge device between PCIe and Ethernet along with other peripherals (i.e. UART, SPI, I2C, USB and PCIe devices)
> 0000:09:00.0 - Ethernet device
> 0000:05:00.0 - PCIe Bridge Up link 

How does the PHY indicate wake up? It could be you can power off the
MAC, but you need to keep parts of the PCIe bridge up, in order the
wake up gets delivered?

> When I test the WOL_PHY option on setup (PCI11x1x MAC + GPY211C
> PHY), observe the following:

> 1. When enable WOL_PHY by using ethtool (i.e. ethtool -s enp9s0 wol
> p), GPY211 PHY configure the WOL. After resume from sleep, GPY211
> WOL configuration vanish. Observed that gpy_config_init( ) function
> reset. Is it expected behaviour ? In other mail thread, we discussed
> that Ethtool configuration should retain after resume from sleep.

The whole point of suspend/resume is that the configuration is
retained. So i would expect from the users perspective WoL is still
enabled.

As you point out, we might have a logic issue here, that on resume we
hit the PHY with a hardware reset. That could be clearing out WoL?  We
might need to cache the PHY WoL configuration in phydev, and on resume
re-apply it. WoL is complex so we need to be careful who is actually
managing it. But this seems like something which could be done in the
phylib core.

> 2. when WOL configure with ethtool, Either Link-down and Link-up on
> CLI, WOL configuration vanish. Is it expected behaviour ? Due to
> this, every time we have to configure WOL through Ethtool.

This is unexpected. I would expect WoL to remain configured until the
configuration is changed.

> Based on above information, We need to check for return < 0
> condition and return the error. Else enable the wakeup by calling
> "device_set_wakeup_enable( )" function.

Once it is clear how the PHY does the wakeup, we can look at
this. However, if the PHY can support the WoL, you should not be
configuring the MAC as well to do the same WoL, you should be putting
the MAC into a low power mode.

    Andrew