Re: [PATCH net-next] ethtool: ioctl: improve error checking for set_wol

From: Florian Fainelli
Date: Thu Jun 01 2023 - 14:38:00 EST

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


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


On 6/1/2023 8:55 AM, Simon Horman wrote:
+ Daniil Tatianin <d-tatianin@xxxxxxxxxxxxxx>, Andrew Lunn <andrew@xxxxxxx>
   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:
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.

Signed-off-by: Justin Chen <justin.chen@xxxxxxxxxxxx>
---
  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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature