* [PATCH 0/2] Make WOL of e1000e consistent with sysfs device wakeup @ 2020-05-21 17:58 Chen Yu 2020-05-21 17:59 ` [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled Chen Yu 2020-05-21 17:59 ` [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability Chen Yu 0 siblings, 2 replies; 9+ messages in thread From: Chen Yu @ 2020-05-21 17:58 UTC (permalink / raw) To: Jeff Kirsher, David S. Miller, Jakub Kicinski, Auke Kok, Jeff Garzik Cc: intel-wired-lan, netdev, linux-kernel, Len Brown, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Chen Yu 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled 2020-05-21 17:58 [PATCH 0/2] Make WOL of e1000e consistent with sysfs device wakeup Chen Yu @ 2020-05-21 17:59 ` Chen Yu 2020-06-15 18:51 ` Brown, Aaron F 2020-05-21 17:59 ` [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability Chen Yu 1 sibling, 1 reply; 9+ messages in thread From: Chen Yu @ 2020-05-21 17:59 UTC (permalink / raw) To: Jeff Kirsher, David S. Miller, Jakub Kicinski, Auke Kok, Jeff Garzik Cc: intel-wired-lan, netdev, linux-kernel, Len Brown, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Chen Yu, Stable 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 <rafael.j.wysocki@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: <Stable@vger.kernel.org> Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled 2020-05-21 17:59 ` [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled Chen Yu @ 2020-06-15 18:51 ` Brown, Aaron F 2020-06-16 7:01 ` Chen Yu 0 siblings, 1 reply; 9+ messages in thread From: Brown, Aaron F @ 2020-06-15 18:51 UTC (permalink / raw) To: Chen, Yu C, Kirsher, Jeffrey T, David S. Miller, Jakub Kicinski, Kok, Auke-jan H, Jeff Garzik Cc: intel-wired-lan, netdev, linux-kernel, Brown, Len, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Chen, Yu C, Stable > From: Chen Yu <yu.c.chen@intel.com> > Sent: Thursday, May 21, 2020 10:59 AM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; David S. Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Kok, Auke-jan H > <auke-jan.h.kok@intel.com>; Jeff Garzik <jeff@garzik.org> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Brown, Len <len.brown@intel.com>; Rafael J. Wysocki > <rjw@rjwysocki.net>; Shevchenko, Andriy <andriy.shevchenko@intel.com>; > Neftin, Sasha <sasha.neftin@intel.com>; Lifshits, Vitaly > <vitaly.lifshits@intel.com>; Chen, Yu C <yu.c.chen@intel.com>; > Stable@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. > > Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver") > Reported-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: <Stable@vger.kernel.org> > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > Tested-by: Aaron Brown <aaron.f.brown@intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled 2020-06-15 18:51 ` Brown, Aaron F @ 2020-06-16 7:01 ` Chen Yu 0 siblings, 0 replies; 9+ messages in thread From: Chen Yu @ 2020-06-16 7:01 UTC (permalink / raw) To: Brown, Aaron F Cc: Kirsher, Jeffrey T, David S. Miller, Jakub Kicinski, Kok, Auke-jan H, Jeff Garzik, intel-wired-lan, netdev, linux-kernel, Brown, Len, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Stable On Tue, Jun 16, 2020 at 02:51:27AM +0800, Brown, Aaron F wrote: > > From: Chen Yu <yu.c.chen@intel.com> > > Sent: Thursday, May 21, 2020 10:59 AM > > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; David S. Miller > > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Kok, Auke-jan H > > <auke-jan.h.kok@intel.com>; Jeff Garzik <jeff@garzik.org> > > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux- > > kernel@vger.kernel.org; Brown, Len <len.brown@intel.com>; Rafael J. Wysocki > > <rjw@rjwysocki.net>; Shevchenko, Andriy <andriy.shevchenko@intel.com>; > > Neftin, Sasha <sasha.neftin@intel.com>; Lifshits, Vitaly > > <vitaly.lifshits@intel.com>; Chen, Yu C <yu.c.chen@intel.com>; > > Stable@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. > > > > Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver") > > Reported-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: <Stable@vger.kernel.org> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > --- > > drivers/net/ethernet/intel/e1000e/netdev.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > Thanks for testing, Aaron. Best, Chenyu ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability 2020-05-21 17:58 [PATCH 0/2] Make WOL of e1000e consistent with sysfs device wakeup Chen Yu 2020-05-21 17:59 ` [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled Chen Yu @ 2020-05-21 17:59 ` Chen Yu 2020-05-21 19:23 ` Michal Kubecek 1 sibling, 1 reply; 9+ messages in thread From: Chen Yu @ 2020-05-21 17:59 UTC (permalink / raw) To: Jeff Kirsher, David S. Miller, Jakub Kicinski, Auke Kok, Jeff Garzik Cc: intel-wired-lan, netdev, linux-kernel, Len Brown, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Chen Yu, Stable 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 <len.brown@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: <Stable@vger.kernel.org> Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability 2020-05-21 17:59 ` [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability Chen Yu @ 2020-05-21 19:23 ` Michal Kubecek 2020-05-23 9:09 ` Chen Yu 0 siblings, 1 reply; 9+ messages in thread From: Michal Kubecek @ 2020-05-21 19:23 UTC (permalink / raw) To: netdev Cc: Chen Yu, Jeff Kirsher, David S. Miller, Jakub Kicinski, Auke Kok, Jeff Garzik, intel-wired-lan, linux-kernel, Len Brown, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Stable 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 <len.brown@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: <Stable@vger.kernel.org> > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability 2020-05-21 19:23 ` Michal Kubecek @ 2020-05-23 9:09 ` Chen Yu 2020-05-24 21:06 ` Michal Kubecek 0 siblings, 1 reply; 9+ messages in thread From: Chen Yu @ 2020-05-23 9:09 UTC (permalink / raw) To: Michal Kubecek Cc: netdev, Jeff Kirsher, David S. Miller, Jakub Kicinski, Auke Kok, Jeff Garzik, intel-wired-lan, linux-kernel, Len Brown, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Stable 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 <len.brown@intel.com> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: <Stable@vger.kernel.org> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability 2020-05-23 9:09 ` Chen Yu @ 2020-05-24 21:06 ` Michal Kubecek 2020-05-25 16:12 ` Chen Yu 0 siblings, 1 reply; 9+ messages in thread From: Michal Kubecek @ 2020-05-24 21:06 UTC (permalink / raw) To: Chen Yu Cc: netdev, Jeff Kirsher, David S. Miller, Jakub Kicinski, Auke Kok, Jeff Garzik, intel-wired-lan, linux-kernel, Len Brown, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Stable 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability 2020-05-24 21:06 ` Michal Kubecek @ 2020-05-25 16:12 ` Chen Yu 0 siblings, 0 replies; 9+ messages in thread From: Chen Yu @ 2020-05-25 16:12 UTC (permalink / raw) To: Michal Kubecek Cc: netdev, Jeff Kirsher, David S. Miller, Jakub Kicinski, Auke Kok, Jeff Garzik, intel-wired-lan, linux-kernel, Len Brown, Rafael J. Wysocki, Shevchenko, Andriy, Neftin, Sasha, Lifshits, Vitaly, Stable 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-16 6:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-21 17:58 [PATCH 0/2] Make WOL of e1000e consistent with sysfs device wakeup Chen Yu 2020-05-21 17:59 ` [PATCH 1/2] e1000e: Do not wake up the system via WOL if device wakeup is disabled Chen Yu 2020-06-15 18:51 ` Brown, Aaron F 2020-06-16 7:01 ` Chen Yu 2020-05-21 17:59 ` [PATCH 2/2] e1000e: Make WOL info in ethtool consistent with device wake up ability Chen Yu 2020-05-21 19:23 ` Michal Kubecek 2020-05-23 9:09 ` Chen Yu 2020-05-24 21:06 ` Michal Kubecek 2020-05-25 16:12 ` Chen Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).