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

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

[Heiner Kallweit]

phy_suspend doesn't always recognize that WoL is enabled and therefore
suspends the PHY when it should not. First idea to address the issue
was to reuse checks used in mdio_bus_phy_may_suspend which check
whether relevant devices are wakeup-enabled.
Florian raised some concerns because drivers may enable wakeup even if
WoL isn't enabled (e.g. certain USB network drivers).

The new approach focuses on reducing the risk to break existing stuff.
We add a flag wol_enabled to struct net_device which is set in
ethtool_set_wol(). Then this flag is checked in phy_suspend().
This doesn't cover 100% of the cases yet (e.g. if WoL is enabled w/o
explicit configuration), but it covers the most relevant cases with
very little risk of regressions.

Heiner Kallweit (2):
  net: core: add member wol_enabled to struct net_device
  net: phy: fix WoL handling when suspending the PHY

 drivers/net/phy/phy_device.c | 12 +++++++++---
 include/linux/netdevice.h    |  3 +++
 net/core/ethtool.c           |  9 ++++++++-
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.19.0

[PATCH net 1/2] net: core: add member wol_enabled to struct net_device

[Heiner Kallweit]

Add flag wol_enabled to struct net_device indicating whether
Wake-on-LAN is enabled. As first user phy_suspend() will use it to
decide whether PHY can be suspended or not.

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>
---
 include/linux/netdevice.h | 3 +++
 net/core/ethtool.c        | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1cbbf77a6..f5f1f1450 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1756,6 +1756,8 @@ enum netdev_priv_flags {
  *			switch driver and used to set the phys state of the
  *			switch port.
  *
+ *	@wol_enabled:	Wake-on-LAN is enabled
+ *
  *	FIXME: cleanup struct net_device such that network protocol info
  *	moves out.
  */
@@ -2039,6 +2041,7 @@ struct net_device {
 	struct lock_class_key	*qdisc_tx_busylock;
 	struct lock_class_key	*qdisc_running_key;
 	bool			proto_down;
+	unsigned		wol_enabled:1;
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 9d4e56d97..86f765d42 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1395,6 +1395,7 @@ static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
 static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 {
 	struct ethtool_wolinfo wol;
+	int ret;
 
 	if (!dev->ethtool_ops->set_wol)
 		return -EOPNOTSUPP;
@@ -1402,7 +1403,13 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 	if (copy_from_user(&wol, useraddr, sizeof(wol)))
 		return -EFAULT;
 
-	return dev->ethtool_ops->set_wol(dev, &wol);
+	ret = dev->ethtool_ops->set_wol(dev, &wol);
+	if (ret)
+		return ret;
+
+	dev->wol_enabled = !!wol.wolopts;
+
+	return 0;
 }
 
 static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)
-- 
2.19.0

Re: [PATCH net 1/2] net: core: add member wol_enabled to struct net_device

[Michal Kubecek]

On Mon, Sep 24, 2018 at 08:09:48PM +0200, Heiner Kallweit wrote:
> Add flag wol_enabled to struct net_device indicating whether
> Wake-on-LAN is enabled. As first user phy_suspend() will use it to
> decide whether PHY can be suspended or not.
> 
> 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>
> ---
>  include/linux/netdevice.h | 3 +++
>  net/core/ethtool.c        | 9 ++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 1cbbf77a6..f5f1f1450 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1756,6 +1756,8 @@ enum netdev_priv_flags {
>   *			switch driver and used to set the phys state of the
>   *			switch port.
>   *
> + *	@wol_enabled:	Wake-on-LAN is enabled
> + *
>   *	FIXME: cleanup struct net_device such that network protocol info
>   *	moves out.
>   */
> @@ -2039,6 +2041,7 @@ struct net_device {
>  	struct lock_class_key	*qdisc_tx_busylock;
>  	struct lock_class_key	*qdisc_running_key;
>  	bool			proto_down;
> +	unsigned		wol_enabled:1;
>  };
>  #define to_net_dev(d) container_of(d, struct net_device, dev)
>  

As there is no bitfield yet, this would add 4 bytes to struct net_device.
How about using a bit in net_device::priv_flags like IFF_RXFH_CONFIGURED
in ethtool_set_rxfh_indir() and ethtool_set_rxfh()?

Michal Kubecek

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

[Heiner Kallweit]

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.

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 | 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.
+	 * 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))
 		return -EBUSY;
 
 	if (phydev->drv && phydrv->suspend)
-- 
2.19.0

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

[Florian Fainelli]

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.fainelli@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 <hkallweit1@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

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

[Heiner Kallweit]

On 24.09.2018 20:21, Florian Fainelli wrote:
> 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.fainelli@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 <hkallweit1@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.
> 
There's a call to phy_suspend() in the phylib state machine and I'm
not sure we can guarantee that a netdevice is attached.
Because phy_suspend() is exported anybody can use it, correct or
incorrect. Therefore I'd say core functions better should play safe.

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

[Florian Fainelli]

On 09/24/2018 12:56 PM, Heiner Kallweit wrote:
> On 24.09.2018 20:21, Florian Fainelli wrote:
>> 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.fainelli@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 <hkallweit1@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.
>>
> There's a call to phy_suspend() in the phylib state machine and I'm
> not sure we can guarantee that a netdevice is attached.
> Because phy_suspend() is exported anybody can use it, correct or
> incorrect. Therefore I'd say core functions better should play safe.

Sounds good to me, better safe than sorry.
-- 
Florian

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

[Heiner Kallweit]

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.

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>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v2:
- fix typo in comment
---
 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 long as not all affected network drivers support the
+	 * wol_enabled flag, let's check for hints that WoL is enabled.
+	 * Don't suspend PHY if the attached netdev parent may wake up.
 	 * 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))
 		return -EBUSY;
 
 	if (phydev->drv && phydrv->suspend)
-- 
2.19.0