On 6/1/23 11:27, Justin Chen wrote:

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

quoted

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.

That part of the driver appears to be dead code. It attempts to pretend 
to support Wake-on-LAN, but it does not do any specific programming of 
wake-up filters, nor does it implement get_wol. It also does not make 
use of the recently introduced PHY_ALWAYS_CALL_SUSPEND flag.

When it is time to determine whether to suspend the PHY or not, 
eventually phy_suspend() will call phy_ethtool_get_wol(). Since no 
get_wol is implemented, the wol.wolopts will remain zero, therefore we 
will just suspend the PHY.

I suspect this was added to work around MAC drivers that may forcefully 
try to suspend the PHY, but that should not even be possible these days.

I would just remove that logic from microchip.c entirely.
-- 
Florian