On 6/1/23 9:22 AM, Justin Chen wrote:

On 6/1/2023 8:55 AM, Simon Horman wrote:

quoted

+ Daniil Tatianin [off-list ref], Andrew Lunn 
[off-list ref]
   as per ./scripts/get_maintainer.pl --git-min-percent 25 
net/ethtool/ioctl.c

On Wed, May 31, 2023 at 01:53:49PM -0700, Justin Chen wrote:

quoted

The netlink version of set_wol checks for not supported wolopts and 
avoids
setting wol when the correct wolopt is already set. If we do the same 
with
the ioctl version then we can remove these checks from the driver layer.

Hi Justin,

Are you planning follow-up patches for the driver layer?

I was planning to for the Broadcom drivers since those I can test. But I 
could do it across the board if that is preferred.

quoted

quoted

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
  net/ethtool/ioctl.c | 14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 6bb778e10461..80f456f83db0 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c

@@ -1436,15 +1436,25 @@ 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;
+    struct ethtool_wolinfo wol, cur_wol;
      int ret;
-    if (!dev->ethtool_ops->set_wol)
+    if (!dev->ethtool_ops->get_wol || !dev->ethtool_ops->set_wol)
          return -EOPNOTSUPP;

Are there cases where (in-tree) drivers provide set_wol byt not get_wol?
If so, does this break their set_wol support?

My original thought was to match netlink set wol behavior. So drivers 
that do that won't work with netlink set_wol right now. I'll skim around 
to see if any drivers do this. But I would reckon this should be a 
driver fix.

Thanks,
Justin

I see a driver at drivers/net/phy/microchip.c. But this is a phy driver 
set_wol hook.

Justin

quoted

quoted

+    memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
+    cur_wol.cmd = ETHTOOL_GWOL;
+    dev->ethtool_ops->get_wol(dev, &cur_wol);
+
      if (copy_from_user(&wol, useraddr, sizeof(wol)))
          return -EFAULT;
+    if (wol.wolopts & ~cur_wol.supported)
+        return -EOPNOTSUPP;
+
+    if (wol.wolopts == cur_wol.wolopts)
+        return 0;
+
      ret = dev->ethtool_ops->set_wol(dev, &wol);
      if (ret)
          return ret;