[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <99ac96ff-df39-4446-9ee7-752f4bf18a3e@gmail.com>
Date: Fri, 28 Jun 2024 10:37:00 +0100
From: Florian Fainelli <f.fainelli@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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
Powered by blists - more mailing lists