From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY Date: Wed, 23 May 2018 12:43:47 -0700 Message-ID: <5d823c98-e028-0edf-a48b-840e527384da@gmail.com> References: <1c4c6f1a-682c-5915-d239-58c7c4ec9f94@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "netdev@vger.kernel.org" To: Heiner Kallweit , Andrew Lunn , David Miller Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:35338 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934223AbeEWTn5 (ORCPT ); Wed, 23 May 2018 15:43:57 -0400 Received: by mail-wm0-f68.google.com with SMTP id o78-v6so12389906wmg.0 for ; Wed, 23 May 2018 12:43:56 -0700 (PDT) In-Reply-To: <1c4c6f1a-682c-5915-d239-58c7c4ec9f94@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 05/23/2018 12:31 PM, Heiner Kallweit wrote: > If the parent of the MDIO bus is runtime-suspended, we may not be able > to access the MDIO bus. Therefore add a check for this situation. > > So far phy_suspend() only checks for WoL being enabled, other checks > are in mdio_bus_phy_may_suspend(). Improve this and move all checks > to a new function phy_may_suspend() and call it from phy_suspend(). > > Signed-off-by: Heiner Kallweit > --- > drivers/net/phy/phy_device.c | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 1662781fb..e0a71e3e5 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -35,6 +35,7 @@ > #include > #include > #include > +#include > > #include > > @@ -75,14 +76,27 @@ extern struct phy_driver genphy_10g_driver; > static LIST_HEAD(phy_fixup_list); > static DEFINE_MUTEX(phy_fixup_lock); > > -#ifdef CONFIG_PM > -static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) > +static bool phy_may_suspend(struct phy_device *phydev) > { > struct device_driver *drv = phydev->mdio.dev.driver; > struct phy_driver *phydrv = to_phy_driver(drv); > struct net_device *netdev = phydev->attached_dev; > + struct device *mdio_bus_parent = phydev->mdio.bus->parent; > + struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; > + > + if (phydev->suspended || !drv || !phydrv->suspend) > + return false; > + > + /* If the device has WOL enabled, we cannot suspend the PHY */ > + phy_ethtool_get_wol(phydev, &wol); > + if (wol.wolopts) > + return false; phy_ethtool_get_wol() can created MDIO bus accesses so should not this be moved after the check for the MDIO bus being runtime suspended? > > - if (!drv || !phydrv->suspend) > + /* If the parent of the MDIO bus is runtime-suspended, the MDIO bus may > + * not be accessible and we expect the parent to suspend all devices > + * on the MDIO bus when it suspends. > + */ > + if (mdio_bus_parent && pm_runtime_suspended(mdio_bus_parent)) > return false; > > /* PHY not attached? May suspend if the PHY has not already been > @@ -91,7 +105,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) > * MDIO bus driver and clock gated at this point. > */ > if (!netdev) > - return !phydev->suspended; > + 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. > @@ -109,6 +123,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) > return true; > } > > +#ifdef CONFIG_PM > static int mdio_bus_phy_suspend(struct device *dev) > { > struct phy_device *phydev = to_phy_device(dev); > @@ -121,9 +136,6 @@ static int mdio_bus_phy_suspend(struct device *dev) > if (phydev->attached_dev && phydev->adjust_link) > phy_stop_machine(phydev); > > - if (!mdio_bus_phy_may_suspend(phydev)) > - return 0; Hummm why is it okay to drop that one? > - > return phy_suspend(phydev); > } > > @@ -1162,13 +1174,10 @@ EXPORT_SYMBOL(phy_detach); > int phy_suspend(struct phy_device *phydev) > { > struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver); > - struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; > int ret = 0; > > - /* If the device has WOL enabled, we cannot suspend the PHY */ > - phy_ethtool_get_wol(phydev, &wol); > - if (wol.wolopts) > - return -EBUSY; > + if (!phy_may_suspend(phydev)) > + return 0; > > if (phydev->drv && phydrv->suspend) > ret = phydrv->suspend(phydev); > -- Florian