[PATCH net v2] net: phy: fix WoL handling when suspending the PHY

[PATCH net v2] net: phy: fix WoL handling when suspending the PHY

[Heiner Kallweit]

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 <hkallweit1@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)
-- 
2.19.0

Re: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY

[Florian Fainelli]

On September 23, 2018 6:38:21 AM PDT, Heiner Kallweit <hkallweit1@gmail.com> 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.

The rationale for the change makes sense but I am worried about a couple of things:

- we have seen drivers fail to call SET_NETDEV_DEV() correctly, or sometimes it is made possible because they are Device Tree only objects without a backing parent struct device (CONFIG_OF makes it possible), so we could be missing some cases here but this is not a big deal

- most Ethernet controller implementations typically set the device as a wake-up enabled device when an user issues the appropriate ethtool -s iface wol <parameters> and not at driver probe or ndo_open() time. There are exceptions like ASIX USB Ethernet adapters that AFAIR are wake-up enabled all the time. The main concern here is that at the time of suspend device_may_wakeup() evaluates to true and we keep the PHY on even though the driver/user was not requesting for WoL per-se

What you encode here is definitely the behavior that Ethernet controllers should have, but we should also audit drivers that use PHYLIB and implement WoL that this is not going to regresses their power management or ability to wake up from PHY, and finally, that there is not redundant code already in place.

>
>Fixes: f1e911d5d0df ("r8169: add basic phylib support")
>Fixes: e8cfd9d6c772 ("net: phy: call state machine synchronously in
>phy_stop")
>Signed-off-by: Heiner Kallweit <hkallweit1@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)

-- 
Florian

Re: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY

[Heiner Kallweit]

On 23.09.2018 18:49, Florian Fainelli wrote:
> 
> 
> On September 23, 2018 6:38:21 AM PDT, Heiner Kallweit <hkallweit1@gmail.com> 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.
> 
> The rationale for the change makes sense but I am worried about a couple of things:
> 
> - we have seen drivers fail to call SET_NETDEV_DEV() correctly, or sometimes it is made possible because they are Device Tree only objects without a backing parent struct device (CONFIG_OF makes it possible), so we could be missing some cases here but this is not a big deal
> 
> - most Ethernet controller implementations typically set the device as a wake-up enabled device when an user issues the appropriate ethtool -s iface wol <parameters> and not at driver probe or ndo_open() time. There are exceptions like ASIX USB Ethernet adapters that AFAIR are wake-up enabled all the time. The main concern here is that at the time of suspend device_may_wakeup() evaluates to true and we keep the PHY on even though the driver/user was not requesting for WoL per-se
> 
> What you encode here is definitely the behavior that Ethernet controllers should have, but we should also audit drivers that use PHYLIB and implement WoL that this is not going to regresses their power management or ability to wake up from PHY, and finally, that there is not redundant code already in place.
> 
Good points. It's somehow unfortunate that we don't have a clear info
whether WoL is enabled for a netdevice or not. Instead we have to
evaluate different hints and make an educated guess (which can be
wrong like in the potential cases you described).

One idea that comes to my mind: Add a flag wol_enabled to struct net_device
and set it in ethtool_set_wol. Then we could use this new flag in phy_suspend().
This would transparently fix the issue for the most common cases.
Still we would have to think about cases like WoL being enabled w/o explicitly
configuring it (e.g. BIOS enables WoL).
What do you think?

>>
>> Fixes: f1e911d5d0df ("r8169: add basic phylib support")
>> Fixes: e8cfd9d6c772 ("net: phy: call state machine synchronously in
>> phy_stop")
>> Signed-off-by: Heiner Kallweit <hkallweit1@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)
> 

[PATCH net] net: phy: fix WoL handling when suspending the PHY

[Heiner Kallweit]

Actually there's nothing wrong with the two changes, they just
revealed a problem which has been existing before.

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.

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.

This patch basically moves the check for WoL having been activated
by the MAC driver into phy_suspend(). mdio_bus_phy_resume() now
resumes the PHY only if it's actually suspended. Also don't do
anything in phy_suspend() if the PHY is suspended already.

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 <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 42 +++++++++++++-----------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index db1172db1..597747ee7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -75,23 +75,12 @@ 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;
 
-	if (!drv || !phydrv->suspend)
-		return false;
-
-	/* PHY not attached? May suspend if the PHY has not already been
-	 * suspended as part of a prior call to phy_disconnect() ->
-	 * phy_detach() -> phy_suspend() because the parent netdev might be the
-	 * 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.
@@ -103,15 +92,14 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	 * 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 !device_may_wakeup(&netdev->dev);
 }
 
+#ifdef CONFIG_PM
 static int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
+	int ret;
 
 	/* We must stop the state machine manually, otherwise it stops out of
 	 * control, possibly with the phydev->lock held. Upon resume, netdev
@@ -121,25 +109,22 @@ 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;
+	ret = phy_suspend(phydev);
 
-	return phy_suspend(phydev);
+	return ret == -EBUSY ? 0 : ret;
 }
 
 static int mdio_bus_phy_resume(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
-	int ret;
+	int ret = 0;
 
-	if (!mdio_bus_phy_may_suspend(phydev))
-		goto no_resume;
+	if (phydev->suspended)
+		ret = phy_resume(phydev);
 
-	ret = phy_resume(phydev);
 	if (ret < 0)
 		return ret;
 
-no_resume:
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_start_machine(phydev);
 
@@ -1132,9 +1117,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 +1156,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)
-- 
2.19.0

[PATCH net v2] net: phy: fix WoL handling when suspending the PHY

[Heiner Kallweit]

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 <hkallweit1@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)
-- 
2.19.0

Re: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY

[Heiner Kallweit]

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 <hkallweit1@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)
>