Skip to contentSkip to conversation

Make WOL of e1000e consistent with sysfs device wakeup

PATCH In review Chen Yu 4 participants 11 messages May 21, 2020 – Jun 16, 2020

Subsystems (4)
lore
Share & tools
lore

Chen Yu

author
#0 lore raw

cover Make WOL of e1000e consistent with sysfs device wakeup

Currently the WOL(Wake On Lan) bahavior of e1000e is not consistent with its corresponding device wake up ability.
Fix this by:
1. Do not wake up the system via WOL if device wakeup is disabled
2. Make WOL display info from ethtool consistent with device wake up settings in sysfs

Chen Yu (2):
e1000e: Do not wake up the system via WOL if device wakeup is disabled
e1000e: Make WOL info in ethtool consistent with device wake up
ability

 drivers/net/ethernet/intel/e1000e/ethtool.c |  2 +-
 drivers/net/ethernet/intel/e1000e/netdev.c  | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 1/2 e1000e: Do not wake up the system via WOL if device wakeup is disabled

Currently the system will be woken up via WOL(Wake On Lan) even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup disabled

The system should not be woken up if the user has explicitly disabled the wake up ability for this device.

This patch clears the WOL ability of this network device if the user has disabled the wake up ability in sysfs.

Fixes bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver")
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/net/ethernet/intel/e1000e/netdev.c +10 -4
index 177c6da80c57..f6f730388705 100644
@@ -6516,11 +6516,17 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
-	u32 ctrl, ctrl_ext, rctl, status;
-	/* Runtime suspend should only enable wakeup for link changes */
-	u32 wufc = runtime ? E1000_WUFC_LNKC : adapter->wol;
+	u32 ctrl, ctrl_ext, rctl, status, wufc;
 	int retval = 0;
 
+	/* Runtime suspend should only enable wakeup for link changes */
+	if (runtime)
+		wufc = E1000_WUFC_LNKC;
+	else if (device_may_wakeup(&pdev->dev))
+		wufc = adapter->wol;
+	else
+		wufc = 0;
+
 	status = er32(STATUS);
 	if (status & E1000_STATUS_LU)
 		wufc &= ~E1000_WUFC_LNKC;
@@ -6577,7 +6583,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 	if (adapter->hw.phy.type == e1000_phy_igp_3) {
 		e1000e_igp3_phy_powerdown_workaround_ich8lan(&adapter->hw);
 	} else if (hw->mac.type >= e1000_pch_lpt) {
-		if (!(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | E1000_WUFC_BC)))
+		if (wufc && !(wufc & (E1000_WUFC_EX | E1000_WUFC_MC | E1000_WUFC_BC)))
 			/* ULP does not support wake from unicast, multicast
 			 * or broadcast.
 			 */
Signature · 1 line
2.17.1
author
#6 lore raw

PATCH 2/2 e1000e: Make WOL info in ethtool consistent with device wake up ability

In reply to Chen Yu

Currently the ethtool shows that WOL(Wake On Lan) is enabled even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup
disabled

ethtool eno1
...
Wake-on: g

Fix this in ethtool to check if the user has explicitly disabled the wake up ability for this device.

Fixes 6ff68026f475 ("e1000e: Use device_set_wakeup_enable")
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/intel/e1000e/ethtool.c +1 -1
index 1d47e2503072..0cccd823ff24 100644
@@ -1891,7 +1891,7 @@ static void e1000_get_wol(struct net_device *netdev,
 	wol->wolopts = 0;
 
 	if (!(adapter->flags & FLAG_HAS_WOL) ||
-	    !device_can_wakeup(&adapter->pdev->dev))
+	    !device_may_wakeup(&adapter->pdev->dev))
 		return;
 
 	wol->supported = WAKE_UCAST | WAKE_MCAST |
Signature · 1 line
2.17.1
#7 lore raw

Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability

Quoted 11 lines · Chen Yu

Currently the ethtool shows that WOL(Wake On Lan) is enabled even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup
disabled

ethtool eno1
...
Wake-on: g

Fix this in ethtool to check if the user has explicitly disabled the wake up ability for this device.

Wouldn't this lead to rather unexpected and inconsistent behaviour when the wakeup is disabled? As you don't touch the set_wol handler, I assume it will still allow setting enabled modes as usual so that you get e.g.

ethtool -s eth0 wol g # no error or warning, returns 0
ethtool eth0 # reports no modes enabled

The first command would set the enabled wol modes but that would be hidden from user and even the netlink notification would claim something different. Another exampe (with kernel and ethtool >= 5.6):

ethtool -s eth0 wol g
ethtool -s eth0 wol +m

resulting in "mg" if device wakeup is enabled but "m" when it's disabled (but the latter would be hidden from user and only revealed when you enable the device wakeup).

This is a general problem discussed recently for EEE and pause
autonegotiation: if setting A can be effectively used only when B is enabled, should we hide actual setting of A from userspace when B is disabled or even reset the value of A whenever B gets toggled or rather allow setting A and B independently? AFAICS the consensus seemed to be that A should be allowed to be set and queried independently of the value of B.

Michal

Quoted 25 lines
Fixes 6ff68026f475 ("e1000e: Use device_set_wakeup_enable")
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/intel/e1000e/ethtool.c +1 -1
index 1d47e2503072..0cccd823ff24 100644
@@ -1891,7 +1891,7 @@ static void e1000_get_wol(struct net_device *netdev,
 	wol->wolopts = 0;
 
 	if (!(adapter->flags & FLAG_HAS_WOL) ||
-	    !device_can_wakeup(&adapter->pdev->dev))
+	    !device_may_wakeup(&adapter->pdev->dev))
 		return;
 
 	wol->supported = WAKE_UCAST | WAKE_MCAST |
Signature · 1 line
2.17.1
author
#8 lore raw

Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability

Hi Michal,
Thanks for reviewing,
and sorry for late reply.

Quoted 24 lines · Michal Kubecek
Quoted 11 lines · Chen Yu

Currently the ethtool shows that WOL(Wake On Lan) is enabled even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup
disabled

ethtool eno1
...
Wake-on: g

Fix this in ethtool to check if the user has explicitly disabled the wake up ability for this device.

Wouldn't this lead to rather unexpected and inconsistent behaviour when the wakeup is disabled? As you don't touch the set_wol handler, I assume it will still allow setting enabled modes as usual so that you get e.g.

ethtool -s eth0 wol g # no error or warning, returns 0
ethtool eth0 # reports no modes enabled

The first command would set the enabled wol modes but that would be hidden from user and even the netlink notification would claim something different. Another exampe (with kernel and ethtool >= 5.6):

Quoted 7 lines

ethtool -s eth0 wol g
ethtool -s eth0 wol +m

resulting in "mg" if device wakeup is enabled but "m" when it's disabled (but the latter would be hidden from user and only revealed when you enable the device wakeup).

I've tested ethtool v5.6 on top of kernel v5.7-rc6, it looks like the scenario you described will not happen as it will not allow the user to enable the wol options with device wakeup disabled, not sure if I missed something:

/sys/devices/pci0000:00/0000:00:1f.6/power# echo disabled > wakeup

ethtool -s eno1 wol g
netlink error: cannot enable unsupported WoL mode (offset 36) netlink error: Invalid argument

I've not digged into the code too much, but according to ethhl_set_wol(), it will first get the current wol options via dev->ethtool_ops->get_wol(), and both the wolopts and wol.supported are 0 when device wake up are disabled. Then ethnl_update_bitset32 might manipulate on wolopts and make it non-zero each is controdict with the precondition that no opts should be enabled due to 0 wol.supported.

Quoted 7 lines

This is a general problem discussed recently for EEE and pause
autonegotiation: if setting A can be effectively used only when B is enabled, should we hide actual setting of A from userspace when B is disabled or even reset the value of A whenever B gets toggled or rather allow setting A and B independently? AFAICS the consensus seemed to be that A should be allowed to be set and queried independently of the value of B.

But then there would be an inconsistence between A and B. I was thinking
if there's a way to align them in kernel space and maintain the difference in user space?

Thanks,
Chenyu

Quoted 28 lines

Michal

Quoted 25 lines
Fixes 6ff68026f475 ("e1000e: Use device_set_wakeup_enable")
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/intel/e1000e/ethtool.c +1 -1
index 1d47e2503072..0cccd823ff24 100644
@@ -1891,7 +1891,7 @@ static void e1000_get_wol(struct net_device *netdev,
 	wol->wolopts = 0;
 
 	if (!(adapter->flags & FLAG_HAS_WOL) ||
-	    !device_can_wakeup(&adapter->pdev->dev))
+	    !device_may_wakeup(&adapter->pdev->dev))
 		return;
 
 	wol->supported = WAKE_UCAST | WAKE_MCAST |
Signature · 1 line
2.17.1
#9 lore raw

Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability

Quoted 46 lines · Chen Yu

Hi Michal,
Thanks for reviewing,
and sorry for late reply.

Quoted 24 lines · Michal Kubecek
Quoted 11 lines · Chen Yu

Currently the ethtool shows that WOL(Wake On Lan) is enabled even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup
disabled

ethtool eno1
...
Wake-on: g

Fix this in ethtool to check if the user has explicitly disabled the wake up ability for this device.

Wouldn't this lead to rather unexpected and inconsistent behaviour when the wakeup is disabled? As you don't touch the set_wol handler, I assume it will still allow setting enabled modes as usual so that you get e.g.

ethtool -s eth0 wol g # no error or warning, returns 0
ethtool eth0 # reports no modes enabled

The first command would set the enabled wol modes but that would be hidden from user and even the netlink notification would claim something different. Another exampe (with kernel and ethtool >= 5.6):

Quoted 7 lines

ethtool -s eth0 wol g
ethtool -s eth0 wol +m

resulting in "mg" if device wakeup is enabled but "m" when it's disabled (but the latter would be hidden from user and only revealed when you enable the device wakeup).

I've tested ethtool v5.6 on top of kernel v5.7-rc6, it looks like the scenario you described will not happen as it will not allow the user to enable the wol options with device wakeup disabled, not sure if I missed something:

/sys/devices/pci0000:00/0000:00:1f.6/power# echo disabled > wakeup

ethtool -s eno1 wol g
netlink error: cannot enable unsupported WoL mode (offset 36) netlink error: Invalid argument

Quoted 7 lines

I've not digged into the code too much, but according to ethhl_set_wol(), it will first get the current wol options via dev->ethtool_ops->get_wol(), and both the wolopts and wol.supported are 0 when device wake up are disabled. Then ethnl_update_bitset32 might manipulate on wolopts and make it non-zero each is controdict with the precondition that no opts should be enabled due to 0 wol.supported.

You are right, I didn't realize that you report 0 even for supported WoL modes. However, this feels even more wrong from my point of view as then even the list of supported WoL modes would be hidden from user when the sysfs switch is off.

Also, AFAICS "ethtool -s <dev> wol d" would be still allowed but the behaviour would differ between ioctl and netlink code path: netlink would identify the operation as no-op and do nothing. But ioctl does not check new value against old one so that it would call your set_wol() handler which would set the (hidden) set of enabled WoL modes to empty which would mean WoL would stay disabled even after enabling the wakeup via sysctl. In other words, you would allow disabling all WoL modes (via ioctl) but not setting them to anything else.

Quoted 11 lines
Quoted 7 lines

This is a general problem discussed recently for EEE and pause
autonegotiation: if setting A can be effectively used only when B is enabled, should we hide actual setting of A from userspace when B is disabled or even reset the value of A whenever B gets toggled or rather allow setting A and B independently? AFAICS the consensus seemed to be that A should be allowed to be set and queried independently of the value of B.

But then there would be an inconsistence between A and B. I was thinking if there's a way to align them in kernel space and maintain the difference in user space?

I'm not sure what exactly you mean by maintaining the difference in userspace but there are many situations like this and we usually do not block the ability to query or set A when the "main switch" is off. For example, you can add IPv4/6 addresses to an interface when it is down, even if you cannot receive or transmit packets with these addresses. Or you can set up netlilter rules in FORWARDING chain independently of the global ip_forward sysctl which can block all IPv4 forwarding.

Moreover, if we really wanted to report no supported and enabled WoL modes when device wakeup is disabled, it should be done for all network devices, not only in one driver.

Michal

author
#10 lore raw

Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability

Quoted 55 lines · Michal Kubecek
Quoted 46 lines · Chen Yu

Hi Michal,
Thanks for reviewing,
and sorry for late reply.

Quoted 24 lines · Michal Kubecek
Quoted 11 lines · Chen Yu

Currently the ethtool shows that WOL(Wake On Lan) is enabled even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup
disabled

ethtool eno1
...
Wake-on: g

Fix this in ethtool to check if the user has explicitly disabled the wake up ability for this device.

Wouldn't this lead to rather unexpected and inconsistent behaviour when the wakeup is disabled? As you don't touch the set_wol handler, I assume it will still allow setting enabled modes as usual so that you get e.g.

ethtool -s eth0 wol g # no error or warning, returns 0
ethtool eth0 # reports no modes enabled

The first command would set the enabled wol modes but that would be hidden from user and even the netlink notification would claim something different. Another exampe (with kernel and ethtool >= 5.6):

Quoted 7 lines

ethtool -s eth0 wol g
ethtool -s eth0 wol +m

resulting in "mg" if device wakeup is enabled but "m" when it's disabled (but the latter would be hidden from user and only revealed when you enable the device wakeup).

I've tested ethtool v5.6 on top of kernel v5.7-rc6, it looks like the scenario you described will not happen as it will not allow the user to enable the wol options with device wakeup disabled, not sure if I missed something:

/sys/devices/pci0000:00/0000:00:1f.6/power# echo disabled > wakeup

ethtool -s eno1 wol g
netlink error: cannot enable unsupported WoL mode (offset 36) netlink error: Invalid argument

Quoted 7 lines

I've not digged into the code too much, but according to ethhl_set_wol(), it will first get the current wol options via dev->ethtool_ops->get_wol(), and both the wolopts and wol.supported are 0 when device wake up are disabled. Then ethnl_update_bitset32 might manipulate on wolopts and make it non-zero each is controdict with the precondition that no opts should be enabled due to 0 wol.supported.

Quoted 4 lines

You are right, I didn't realize that you report 0 even for supported WoL modes. However, this feels even more wrong from my point of view as then even the list of supported WoL modes would be hidden from user when the sysfs switch is off.

I see, the WOL modes should be exposed anyway no matter what wake up setting it is, as it is read-only.

Quoted 10 lines

Also, AFAICS "ethtool -s <dev> wol d" would be still allowed but the behaviour would differ between ioctl and netlink code path: netlink would identify the operation as no-op and do nothing. But ioctl does not check new value against old one so that it would call your set_wol() handler which would set the (hidden) set of enabled WoL modes to empty which would mean WoL would stay disabled even after enabling the wakeup via sysctl. In other words, you would allow disabling all WoL modes (via ioctl) but not setting them to anything else.

I see, then there would be inconsistence between netlink and ioctl mode as a sequence.

Quoted 12 lines
Quoted 11 lines
Quoted 7 lines

This is a general problem discussed recently for EEE and pause
autonegotiation: if setting A can be effectively used only when B is enabled, should we hide actual setting of A from userspace when B is disabled or even reset the value of A whenever B gets toggled or rather allow setting A and B independently? AFAICS the consensus seemed to be that A should be allowed to be set and queried independently of the value of B.

But then there would be an inconsistence between A and B. I was thinking if there's a way to align them in kernel space and maintain the difference in user space?

Quoted 9 lines

I'm not sure what exactly you mean by maintaining the difference in userspace but there are many situations like this and we usually do not block the ability to query or set A when the "main switch" is off. For example, you can add IPv4/6 addresses to an interface when it is down, even if you cannot receive or transmit packets with these addresses. Or you can set up netlilter rules in FORWARDING chain independently of the global ip_forward sysctl which can block all IPv4 forwarding.

This is a good point.

Quoted 4 lines

Moreover, if we really wanted to report no supported and enabled WoL modes when device wakeup is disabled, it should be done for all network devices, not only in one driver.

I think the examples have persuaded me that we should leave the ethtool code as it is now that, it is okay to let ethtool be unaware of device wake up ability. Besides, it would be overkilled if we try to 'fix' it for all device drivers.

Thanks,
Chenyu

Quoted 1 line

Michal

maintainer
#2 lore raw

Re: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled

In reply to Chen Yu

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)").

The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181, v4.9.224, v4.4.224.

v5.6.14: Build OK!
v5.4.42: Build OK!
v4.19.124: Build OK!
v4.14.181: Build OK!
v4.9.224: Failed to apply! Possible dependencies:

    c8744f44aeae ("e1000e: Add Support for CannonLake")

v4.4.224: Failed to apply! Possible dependencies:

    16ecba59bc33 ("e1000e: Do not read ICR in Other interrupt")
    18dd23920703 ("e1000e: use BIT() macro for bit defines")
    74f31299a41e ("e1000e: Increase PHY PLL clock gate timing")
    c8744f44aeae ("e1000e: Add Support for CannonLake")
    f3ed935de059 ("e1000e: initial support for i219-LM (3)")

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

Signature · 2 lines
Thanks
Sasha
author
#3 lore raw

Re: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled

Hi Sasha,

Quoted 24 lines · Sasha Levin

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently for ICH9 devices only)").

The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181, v4.9.224, v4.4.224.

v5.6.14: Build OK!
v5.4.42: Build OK!
v4.19.124: Build OK!
v4.14.181: Build OK!
v4.9.224: Failed to apply! Possible dependencies:

    c8744f44aeae ("e1000e: Add Support for CannonLake")

v4.4.224: Failed to apply! Possible dependencies:

    16ecba59bc33 ("e1000e: Do not read ICR in Other interrupt")
    18dd23920703 ("e1000e: use BIT() macro for bit defines")
    74f31299a41e ("e1000e: Increase PHY PLL clock gate timing")
    c8744f44aeae ("e1000e: Add Support for CannonLake")
    f3ed935de059 ("e1000e: initial support for i219-LM (3)")
Quoted 4 lines

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

We could discard the change for v4.9 and v4.4 IMO, as their impact should be minor.

Thanks,
Chenyu

Quoted 3 lines
Signature · 2 lines
Thanks
Sasha
Tested-by on v1 1/2
#4 lore raw

RE: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled

In reply to Chen Yu
Quoted 25 lines

From: Chen Yu <[email protected]>
Sent: Thursday, May 21, 2020 10:59 AM
To: Kirsher, Jeffrey T <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kok, Auke-jan H <[email protected]>; Jeff Garzik <[email protected]>
Cc: intel-wired-lan at lists.osuosl.org; netdev at vger.kernel.org; linux- kernel at vger.kernel.org; Brown, Len <[email protected]>; Rafael J. Wysocki <[email protected]>; Shevchenko, Andriy <[email protected]>; Neftin, Sasha <[email protected]>; Lifshits, Vitaly <[email protected]>; Chen, Yu C <[email protected]>; Stable at vger.kernel.org
Subject: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled

Currently the system will be woken up via WOL(Wake On Lan) even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup disabled

The system should not be woken up if the user has explicitly disabled the wake up ability for this device.

This patch clears the WOL ability of this network device if the user has disabled the wake up ability in sysfs.

Quoted 9 lines
Fixes bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver")
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
author
#5 lore raw

Re: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled

Quoted 34 lines · Aaron F
Quoted 25 lines

From: Chen Yu <[email protected]>
Sent: Thursday, May 21, 2020 10:59 AM
To: Kirsher, Jeffrey T <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kok, Auke-jan H <[email protected]>; Jeff Garzik <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; Brown, Len <[email protected]>; Rafael J. Wysocki <[email protected]>; Shevchenko, Andriy <[email protected]>; Neftin, Sasha <[email protected]>; Lifshits, Vitaly <[email protected]>; Chen, Yu C <[email protected]>; [email protected]
Subject: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled

Currently the system will be woken up via WOL(Wake On Lan) even if the device wakeup ability has been disabled via sysfs:
cat /sys/devices/pci0000:00/0000:00:1f.6/power/wakeup disabled

The system should not be woken up if the user has explicitly disabled the wake up ability for this device.

This patch clears the WOL ability of this network device if the user has disabled the wake up ability in sysfs.

Quoted 9 lines
Fixes bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver")
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
Quoted 2 lines

Thanks for testing, Aaron.

Best,
Chenyu