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(-)
--
2.17.1
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")
Reported-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Cc: <[email protected]>
Signed-off-by: Chen Yu <[email protected]>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 177c6da80c57..f6f730388705 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -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.
*/
--
2.17.1
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")
Reported-by: Len Brown <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Cc: <[email protected]>
Signed-off-by: Chen Yu <[email protected]>
---
drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 1d47e2503072..0cccd823ff24 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -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 |
--
2.17.1
On Fri, May 22, 2020 at 01:59:13AM +0800, Chen Yu wrote:
> 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
> Fixes: 6ff68026f475 ("e1000e: Use device_set_wakeup_enable")
> Reported-by: Len Brown <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Chen Yu <[email protected]>
> ---
> drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index 1d47e2503072..0cccd823ff24 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -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 |
> --
> 2.17.1
>
Hi Michal,
Thanks for reviewing,
and sorry for late reply.
On Thu, May 21, 2020 at 09:23:42PM +0200, Michal Kubecek wrote:
> On Fri, May 22, 2020 at 01:59:13AM +0800, Chen Yu wrote:
> > 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).
>
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.
> 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
>
> Michal
>
> > Fixes: 6ff68026f475 ("e1000e: Use device_set_wakeup_enable")
> > Reported-by: Len Brown <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Chen Yu <[email protected]>
> > ---
> > drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > index 1d47e2503072..0cccd823ff24 100644
> > --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > @@ -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 |
> > --
> > 2.17.1
> >
On Sat, May 23, 2020 at 05:09:50PM +0800, Chen Yu wrote:
> Hi Michal,
> Thanks for reviewing,
> and sorry for late reply.
> On Thu, May 21, 2020 at 09:23:42PM +0200, Michal Kubecek wrote:
> > On Fri, May 22, 2020 at 01:59:13AM +0800, Chen Yu wrote:
> > > 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).
> >
> 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.
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.
> > 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
On Sun, May 24, 2020 at 11:06:53PM +0200, Michal Kubecek wrote:
> On Sat, May 23, 2020 at 05:09:50PM +0800, Chen Yu wrote:
> > Hi Michal,
> > Thanks for reviewing,
> > and sorry for late reply.
> > On Thu, May 21, 2020 at 09:23:42PM +0200, Michal Kubecek wrote:
> > > On Fri, May 22, 2020 at 01:59:13AM +0800, Chen Yu wrote:
> > > > 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).
> > >
> > 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.
>
> 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.
>
> 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.
> > > 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.
>
This is a good point.
> 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
> Michal
> 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.
>
> Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver")
> Reported-by: Rafael J. Wysocki <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Chen Yu <[email protected]>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
Tested-by: Aaron Brown <[email protected]>
On Tue, Jun 16, 2020 at 02:51:27AM +0800, Brown, Aaron F wrote:
> > 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.
> >
> > Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver")
> > Reported-by: Rafael J. Wysocki <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Chen Yu <[email protected]>
> > ---
> > drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> Tested-by: Aaron Brown <[email protected]>
>
Thanks for testing, Aaron.
Best,
Chenyu