On 09/24/2018 11:11 AM, Heiner Kallweit wrote:
> Core of the problem is that phy_suspend() suspends the PHY when it
> should not because of 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.
>
> This patch uses new member wol_enabled of struct net_device as
> additional criteria in the check when not to suspend the PHY because
> of WoL.
>
> 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.
Looks fine to me, just a few nits/questions down below:
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
>
> Fixes: f1e911d5d0df ("r8169: add basic phylib support")
> Fixes: e8cfd9d6c772 ("net: phy: call state machine synchronously in phy_stop")
> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
> ---
> drivers/net/phy/phy_device.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index af64a9320..6c0195e53 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -93,7 +93,12 @@ 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.
> + if (netdev->wol_enabled)
> + return false;
> +
> + /* As lang as not all affected network drivers support the
> + * wol_enabled flag, let's check for hints that WoL is enabled.
Typo: as long (sorry for being that nitpicky).
> + * 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))
> @@ -1132,9 +1137,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);
> @@ -1168,12 +1173,13 @@ EXPORT_SYMBOL(phy_detach);
> int phy_suspend(struct phy_device *phydev)
> {
> struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
> + struct net_device *netdev = phydev->attached_dev;
> 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)
> + if (wol.wolopts || (netdev && netdev->wol_enabled))
Since you moved the phydev->attached_dev assignment to be after
phy_suspend(), do you really need to check for netdev here? Is there
another code path you found that might be running phy_suspend() with a
disconnected PHY? Not a problem per-se, just wondering.
--
Florian
