Message-ID: <Zn6BZ4b4h8YJ3Z0u@shell.armlinux.org.uk>
Date: Fri, 28 Jun 2024 10:24:55 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Florian Fainelli <f.fainelli@...il.com>
Cc: Youwan Wang <youwan@...china.com>, andrew@...n.ch, hkallweit1@...il.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: phy_device: fix PHY WOL enabled, PM failed to
 suspend

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

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?

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists