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

[Ronnie.Kunin]

Thanks for the detailed write-up Russell, much appreciated. 

Yes, the lan743x wol handling is currently severely broken. Raju was in the process of fixing that (the patch I mentioned in my previous email that will call the phy driver first to give it priority) when he ran into issues with the mxl-gpy driver that led to this thread. 

Understood that the mac driver should filter out any wol options it received through .set_wol that the mac supports but that the underlying phy does not support (as per what the phy driver reported earlier over its own .get_wol in the wol.supported field), prior to calling the Phy's set_wol  (that is yet another bug that needs to be fixed in the current lan743x driver). That way the mac driver can then properly track (and save if it needs to) the currently enabled phy options and it own enabled wol options.

Hopefully, with the clarity you have provided on how this whole thing should be done, Raju will be submitting patches to fix both drivers soon.

Thanks again and regards,
Ronnie


> -----Original Message-----
> From: Russell King <linux@xxxxxxxxxxxxxxx>
> Sent: Tuesday, May 7, 2024 1:57 PM
> To: Ronnie Kunin - C21729 <Ronnie.Kunin@xxxxxxxxxxxxx>
> Cc: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>; andrew@xxxxxxx;
> netdev@xxxxxxxxxxxxxxx; lxu@xxxxxxxxxxxxx; hkallweit1@xxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>
> Subject: Re: [PATCH net-next] net: phy: add wol config options in phy device
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, May 07, 2024 at 04:18:27PM +0000, Ronnie.Kunin@xxxxxxxxxxxxx wrote:
> > > So you want the MAC driver to access your new phydev->wolopts. What
> > > if there isn't a PHY, or the PHY is on a pluggable module (e.g.
> > > SFP.) No, you don't want to have phylib tracking this for the MAC. The MAC needs to track this itself
> if required.
> >
> > There is definite value to having the phy be able to effectively
> > communicate which specific wol events it currently has enabled so the
> > mac driver can make better decisions on what to enable or not in the
> > mac hardware (which of course will lead to more efficient power
> > management). While not really needed for the purpose of fixing this
> > driver's bugs, Raju's proposed addition of a wolopts tracking variable
> > to phydev was also providing a direct way to access that information.
> > In the current patch Raju is working on, the first call the lan743x
> > mac driver does in its set_wol() function is to call the phy's
> > set_wol() so that it gives the phy priority in wol handling. I guess
> > when you say that phylib does not need to track this by adding a
> > wolops member to the phydev structure, if we need that information the
> > alternative way is to just immediately call the phy's get_wol() after
> > set_wol() returns, correct ?
> 
> Depends what the driver wants to do.
> 
> From the subset of drivers that implement WoL and use phylib:
> 
> drivers/net/ethernet/socionext/sni_ave.c:
>         ave_init()
>         ave_suspend() - to save the wolopts from the PHY
>         ave_resume() - to restore them to the PHY - so presumably
>                 working around a buggy PHY driver, but no idea which
>                 PHY driver because although it uses DT, there's no way
>                 to know from DT which PHYs get used on this platform.
> 
> drivers/net/ethernet/ti/cpsw_ethtool.c:
>         does nothing more than forwarding the set_wol()/get_wol()
>         ethtool calls to phylib.
> 
> drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
>         enetc_set_wol() merely sets the device for wakeup as appropriate
>         based on the wolopts after passing it to phylib.
> 
> drivers/net/ethernet/microchip/lan743x_ethtool.c:
>         lan743x_ethtool_set_wol() tracks the wolopts *before* passing
>         to phylib, and enables the device for wakeup as appropriate.
>         The whole:
>         "return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
>                         : -ENETDOWN;"
>         thing at the end is buggy. You're updating the adapters state
>         and the device wakeup enable, _then_ checking whether we have a
>         phydev. If we don't have a phydev, you're then telling userspace
>         that the request to change the WoL settings failed but you've
>         already changed your configuration!
> 
>         Moreover, looking at lan743x_ethtool_get_wol(), you set
>         WAKE_MAGIC | WAKE_PHY irrespective of what the PHY actually
>         supports. This makes a total nonsense of the purpose of the
>         supported flags here.
> 
>         I guess the _reason_ you do this is because the PHY may not be
>         present (because you look it up in the .ndo_open() method) and
>         thus you're trying to work around that... but then set_wol()
>         fails in that instance. This all seems crazy.
> 
>         Broadcom bcmgenet also connects to the PHY in .ndo_open() and
>         has sane semantics for .get_wol/.set_wol. I'd suggest having
>         a look at that driver...
> 
> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c:
>         bcmgenet_set_wol() saves the value of wolopts in its
>         private data structure and bcmgenet_wol_power_down_cfg()/
>         bcmgenet_wol_power_up_cfg() uses this to decide what to
>         power down/up.
> 
> Now, let's look at what ethtool already does for us when attempting a set_wol() peration.
> 
> 1. It retrieves the current WoL configuration from the netdev into
>    'wol'.
> 2. It modifies wol.wolopts according to the users request.
> 3. It validates that the options set in wol.wolopts do _not_ include
>    anything that is not reported as supported (in other words, no
>    bits are set that aren't in wol.supported.) 4. It deals with the WoL SecureOn™ password.
> 5. It calls the netdev to set the new configuration.
> 6. If successful, it updates the netdev's flag which indicates whether
>    WoL is currently enabled _in some form_ for this netdev.
> 
> Ergo, network device drivers are *not* supposed to report modes in wol.supported that they do not
> support, and thus a network driver can be assured that wol.wolopts will not contain any modes that are
> not supported.
> 
> Therefore, if a network device wishes to track which WoL modes are currently enabled, it can do this
> simply by:
> 
> 1. calling phy_ethtool_set_wol(), and if that call is successful, then 2. save the value of wol.wolopts to
> its own private data structure to
>    determine what it needs to do at suspend/resume.
> 
> This should be independent of which modes the PHY supports, because the etwork driver should be
> using phy_ethtool_get_wol() to retrieve the modes which the PHY supports, and then augmenting the
> wol.supported mask with whatever additional modes the network driver supports beyond that.
> 
> So, there is no real need for a network driver to query phylib for the current configuration except
> possibly during probe or when connecting to its PHY.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!