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 <hkallwe...@gmail.com>
> ---
> 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)
>
