Re: [PATCH] net: phy: phy_device: fix PHY WOL enabled, PM failed to suspend

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

On 6/28/2024 10:24 AM, Russell King (Oracle) wrote:
> On Fri, Jun 28, 2024 at 09:25:54AM +0100, Florian Fainelli wrote:
>> Would not the situation described here be solved by having the Motorcomm PHY
>> driver set PHY_ALWAYS_CALL_SUSPEND since it deals with checking whether WoL
>> is enabled or not and will just return then.
> 
> Let's also look at PHY_ALWAYS_CALL_SUSPEND. There are currently two
> drivers that make use of it - realtek and broadcom.
> 
> Looking at realtek, it is used with driver instances that call
> 	rtl821x_suspend
> 	rtl821x_resume
> 
> rtl821x_suspend() does nothing if phydev->wol_enabled is true.
> rtl821x_resume() only re-enabled the clock if phydev->wol_enabled
> was false (in other words, rtl821x has disabled the clock.) However,
> it always calls genphy_resume() - presumably this is the reason for
> the flag.
> 
> The realtek driver instances do not populate .set_wol nor .get_wol,
> so the PHY itself does not support WoL configuration. This means
> that the phy_ethtool_get_wol() call in phy_suspend() will also fail,
> and since wol.wolopts is initialised to zero, phydev->wol_enabled
> will only be true if netdev->wol_enabled is true.
> 
> Thus, for phydev->wol_enabled to be true, netdev->wol_enabled must
> be true, and we won't get here via mdio_bus_phy_suspend() as
> mdio_bus_phy_may_suspend() will return false in this case.
> 
> 
> Looking at broadcom, it's used with only one driver instance for
> BCM54210E which calls:
> 	bcm54xx_suspend
> 	bcm54xx_resume
> 
> Other driver instances also call these two functions but do not
> set this flag - BCM54612E, BCM54810, BCM54811, BCM50610, and
> BCM50610M. Moreover, none of these implement the .get_wol and
> .set_wol methods which means the behaviour is as I describe for
> Realtek above that also doesn't implement these methods.
> 
> The only case where this is different is BCM54210E which does
> populate these methods.
> 
> bcm54xx_suspend() stops ptp, and if WoL is enabled, configures the
> wakeup IRQ. bcm54xx_resume() deconfigures the wakeup IRQ.
> 
> This could lead us into a case where the PHY has WoL enabled, the
> phy_ethtool_get_wol() call returns that, causing phydev->wol_enabled
> to be set, and netdev->wol_enabled is not set.
> 
> However, in this case, it would not be a problem because the driver
> has set PHY_ALWAYS_CALL_SUSPEND, so we won't return -EBUSY.
> 
> 
> Now, looking again at motorcomm, yt8521_resume() disables auto-sleep
> before checking whether WoL is enabled. So, the driver is probably
> missing PHY_ALWAYS_CALL_SUSPEND if auto-sleep needs to be disabled
> each and every resume whether WoL is enabled or not.
> 
> However, if we look at yt8521_config_init(), this will also disable
> auto sleep. This will be called from phy_init_hw(), and in the
> mdio_bus_phy_resume() path, immediately before phy_resume(). Likewise
> when we attach the PHY.
> 
> Then we have some net drivers that call phy_resume() directly...
> 
> drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> 	(we already have a workaround merged for
> 	PHY-not-providing-clock)
> 
> drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> 	A suspend/resume cycle of the PHY is done when entering loopback mode.
> 
> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> 	No idea on this one - it resumes the PHY before enabling
> 	loopback mode, and enters suspend when disabling loopback
> 	mode!
> 
> drivers/net/ethernet/broadcom/genet/bcmgenet.c
> 	bcmgenet_resume() calls phy_init_hw() before phy_resume().

Yes, there was a reason for that, that had to do with a finicky PHY 
IIRC, should be documented properly in the commit message since this 
came from my colleague Doug.

> 
> drivers/net/ethernet/broadcom/bcmsysport.c
> 	bcm_sysport_resume() *doesn't* appear to call phy_init_hw()
> 	before phy_resume(), so I wonder whether the config is
> 	properly restored on resume?

This driver is using the fixed PHY emulation so it does not really 
matter, it also sets mac_managed_pm to true.

> 
> drivers/net/ethernet/realtek/r8169_main.c
> 	rtl8169_up() calls phy_init_hw() before phy_resume().
> 
> drivers/net/usb/ax88172a.c
> 	This doesn't actually call phy_resume(), but calls
> 	genphy_resume() directly from ax88172a_reset() immediately
> 	after phy_connect(). However, connecting to a PHY will
> 	call phy_resume()... confused here.
> 
> So I'm left wondering whether yt8521_resume() should be fiddling with
> this auto-sleep mode, especially as yt8521_config_init() will do that
> if the appropriate DT property is set... and whether this should be
> done unconditionally.
> 

-- 
Florian