* [PATCH net-next 0/2] net: phy: improve PHY suspend/resume @ 2018-05-23 19:28 Heiner Kallweit 2018-05-23 19:30 ` [PATCH net-next 1/2] net: phy: improve check for when to call phy_resume in mdio_bus_phy_resume Heiner Kallweit 2018-05-23 19:31 ` [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY Heiner Kallweit 0 siblings, 2 replies; 5+ messages in thread From: Heiner Kallweit @ 2018-05-23 19:28 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org I have the issue that suspending the MAC-integrated PHY gives an error during system suspend. The sequence is: 1. unconnected PHY/MAC are runtime-suspended already 2. system suspend commences 3. mdio_bus_phy_suspend is called 4. suspend callback of the network driver is called (implicitly MAC/PHY are runtime-resumed before) 5. suspend callback suspends MAC/PHY The problem occurs in step 3. phy_suspend() fails because the MDIO bus isn't accessible due to the chip being runtime-suspended. This series mainly adds a check to not suspend the PHY if the MDIO bus parent is runtime-suspended. Heiner Kallweit (2): net: phy: improve check for when to call phy_resume in mdio_bus_phy_resume net: phy: improve checks when to suspend the PHY drivers/net/phy/phy_device.c | 45 +++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 19 deletions(-) -- 2.17.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net-next 1/2] net: phy: improve check for when to call phy_resume in mdio_bus_phy_resume 2018-05-23 19:28 [PATCH net-next 0/2] net: phy: improve PHY suspend/resume Heiner Kallweit @ 2018-05-23 19:30 ` Heiner Kallweit 2018-05-23 19:31 ` [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY Heiner Kallweit 1 sibling, 0 replies; 5+ messages in thread From: Heiner Kallweit @ 2018-05-23 19:30 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org We don't have to do all the checks again which we did in mdio_bus_phy_suspend already. Instead we can simply check whether the PHY is actually suspended and needs to be resumed. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy_device.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 9e4ba8e80..1662781fb 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -132,14 +132,12 @@ static int mdio_bus_phy_resume(struct device *dev) struct phy_device *phydev = to_phy_device(dev); int ret; - if (!mdio_bus_phy_may_suspend(phydev)) - goto no_resume; - - ret = phy_resume(phydev); - if (ret < 0) - return ret; + if (phydev->suspended) { + ret = phy_resume(phydev); + if (ret < 0) + return ret; + } -no_resume: if (phydev->attached_dev && phydev->adjust_link) phy_start_machine(phydev); -- 2.17.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY 2018-05-23 19:28 [PATCH net-next 0/2] net: phy: improve PHY suspend/resume Heiner Kallweit 2018-05-23 19:30 ` [PATCH net-next 1/2] net: phy: improve check for when to call phy_resume in mdio_bus_phy_resume Heiner Kallweit @ 2018-05-23 19:31 ` Heiner Kallweit 2018-05-23 19:43 ` Florian Fainelli 1 sibling, 1 reply; 5+ messages in thread From: Heiner Kallweit @ 2018-05-23 19:31 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org 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 <hkallweit1@gmail.com> --- 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 <linux/io.h> #include <linux/uaccess.h> #include <linux/of.h> +#include <linux/pm_runtime.h> #include <asm/irq.h> @@ -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; - 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; - 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); -- 2.17.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY 2018-05-23 19:31 ` [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY Heiner Kallweit @ 2018-05-23 19:43 ` Florian Fainelli 2018-05-23 20:05 ` Heiner Kallweit 0 siblings, 1 reply; 5+ messages in thread From: Florian Fainelli @ 2018-05-23 19:43 UTC (permalink / raw) To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org 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 <hkallweit1@gmail.com> > --- > 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 <linux/io.h> > #include <linux/uaccess.h> > #include <linux/of.h> > +#include <linux/pm_runtime.h> > > #include <asm/irq.h> > > @@ -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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY 2018-05-23 19:43 ` Florian Fainelli @ 2018-05-23 20:05 ` Heiner Kallweit 0 siblings, 0 replies; 5+ messages in thread From: Heiner Kallweit @ 2018-05-23 20:05 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org Am 23.05.2018 um 21:43 schrieb Florian Fainelli: > 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 <hkallweit1@gmail.com> >> --- >> 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 <linux/io.h> >> #include <linux/uaccess.h> >> #include <linux/of.h> >> +#include <linux/pm_runtime.h> >> >> #include <asm/irq.h> >> >> @@ -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? > Good point. Yes, the WoL check needs to be moved. >> >> - 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? > All checks in mdio_bus_phy_may_suspend() have been moved to phy_may_suspend() which is called from phy_suspend() directly now. >> - >> 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); >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-23 20:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-23 19:28 [PATCH net-next 0/2] net: phy: improve PHY suspend/resume Heiner Kallweit 2018-05-23 19:30 ` [PATCH net-next 1/2] net: phy: improve check for when to call phy_resume in mdio_bus_phy_resume Heiner Kallweit 2018-05-23 19:31 ` [PATCH net-next 2/2] net: phy: improve checks for when to suspend the PHY Heiner Kallweit 2018-05-23 19:43 ` Florian Fainelli 2018-05-23 20:05 ` Heiner Kallweit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).