From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY Date: Sun, 23 Sep 2018 15:39:39 +0200 Message-ID: <4dc9ecd3-4b3e-ef6e-1bb7-432ce051d2c9@gmail.com> References: <4dbcdd2c-96c2-2921-5016-affc8fce1d19@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "netdev@vger.kernel.org" To: Andrew Lunn , Florian Fainelli , David Miller , Realtek linux nic maintainers Return-path: Received: from mail-wr1-f67.google.com ([209.85.221.67]:38362 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726036AbeIWTh3 (ORCPT ); Sun, 23 Sep 2018 15:37:29 -0400 Received: by mail-wr1-f67.google.com with SMTP id j8-v6so13348157wrw.5 for ; Sun, 23 Sep 2018 06:39:56 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: Sorry, this patch was accidentally sent as reply. On 23.09.2018 15:33, Heiner Kallweit wrote: > Actually there's nothing wrong with the two changes marked as "Fixes", > they just revealed a problem which has been existing before. > After having switched r8169 to phylib it was reported that WoL from > shutdown doesn't work any longer (WoL from suspend isn't affected). > Reason is that during shutdown phy_disconnect()->phy_detach()-> > phy_suspend() is called. > A similar issue occurs when the phylib state machine calls > phy_suspend() when handling state PHY_HALTED. > > Core of the problem is that phy_suspend() suspends the PHY when it > should not due to WoL. phy_suspend() checks for WoL already, but this > works only if the PHY driver handles WoL (what is rarely the case). > Typically WoL is handled by the MAC driver. > > phylib knows about this and handles it in mdio_bus_phy_may_suspend(), > but that's used only when suspending the system, not in other cases > like shutdown. > > Therefore factor out the relevant check from > mdio_bus_phy_may_suspend() to a new function phy_may_suspend() and > use it in phy_suspend(). > > Last but not least change phy_detach() to call phy_suspend() before > attached_dev is set to NULL. phy_suspend() accesses attached_dev > when checking whether the MAC driver activated WoL. > > Fixes: f1e911d5d0df ("r8169: add basic phylib support") > Fixes: e8cfd9d6c772 ("net: phy: call state machine synchronously in phy_stop") > Signed-off-by: Heiner Kallweit > --- > v2: > - improved commit message > - reduced scope of patch, don't touch functionality of > mdio_bus_phy_suspend and mdio_bus_phy_resume > --- > drivers/net/phy/phy_device.c | 42 ++++++++++++++++++++++-------------- > 1 file changed, 26 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index af64a9320..4cab94bae 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -75,6 +75,26 @@ extern struct phy_driver genphy_10g_driver; > static LIST_HEAD(phy_fixup_list); > static DEFINE_MUTEX(phy_fixup_lock); > > +static bool phy_may_suspend(struct phy_device *phydev) > +{ > + struct net_device *netdev = phydev->attached_dev; > + > + if (!netdev) > + return true; > + > + /* Don't suspend PHY if the attached netdev parent may wakeup. > + * The parent may point to a PCI device, as in tg3 driver. > + */ > + if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent)) > + return false; > + > + /* Also don't suspend PHY if the netdev itself may wakeup. This > + * is the case for devices w/o underlaying pwr. mgmt. aware bus, > + * e.g. SoC devices. > + */ > + return !device_may_wakeup(&netdev->dev); > +} > + > #ifdef CONFIG_PM > static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) > { > @@ -93,20 +113,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) > if (!netdev) > return !phydev->suspended; > > - /* Don't suspend PHY if the attached netdev parent may wakeup. > - * The parent may point to a PCI device, as in tg3 driver. > - */ > - if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent)) > - return false; > - > - /* Also don't suspend PHY if the netdev itself may wakeup. This > - * is the case for devices w/o underlaying pwr. mgmt. aware bus, > - * e.g. SoC devices. > - */ > - if (device_may_wakeup(&netdev->dev)) > - return false; > - > - return true; > + return phy_may_suspend(phydev); > } > > static int mdio_bus_phy_suspend(struct device *dev) > @@ -1132,9 +1139,9 @@ void phy_detach(struct phy_device *phydev) > sysfs_remove_link(&dev->dev.kobj, "phydev"); > sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev"); > } > + phy_suspend(phydev); > phydev->attached_dev->phydev = NULL; > phydev->attached_dev = NULL; > - phy_suspend(phydev); > phydev->phylink = NULL; > > phy_led_triggers_unregister(phydev); > @@ -1171,9 +1178,12 @@ int phy_suspend(struct phy_device *phydev) > struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; > int ret = 0; > > + if (phydev->suspended) > + return 0; > + > /* If the device has WOL enabled, we cannot suspend the PHY */ > phy_ethtool_get_wol(phydev, &wol); > - if (wol.wolopts) > + if (wol.wolopts || !phy_may_suspend(phydev)) > return -EBUSY; > > if (phydev->drv && phydrv->suspend) >