Hi Andrew, 

For Ethtool netlink, we need to change the "ethnl_set_wol( )" to add option is incremental.
Can you please check those changes OK or not.

Thanks,
Raju.

-----Original Message-----
From: Raju Lakkaraju - I30499
Sent: Friday, March 1, 2024 3:52 PM
To: Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; linux-
kernel@vger.kernel.org; Bryan Whitehead - C21958
[off-list ref]; richardcochran@gmail.com;
UNGLinuxDriver [off-list ref]
Subject: RE: [PATCH net 3/3] net: lan743x: Address problems with wake option
flags configuration sequences

Hi Andrew,

quoted

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Thursday, February 29, 2024 8:36 PM
To: Raju Lakkaraju - I30499 <redacted>
Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
linux- kernel@vger.kernel.org; Bryan Whitehead - C21958
[off-list ref]; richardcochran@gmail.com;
UNGLinuxDriver [off-list ref]
Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake
option flags configuration sequences

EXTERNAL EMAIL: Do not click links or open attachments unless you know
the content is safe

On Thu, Feb 29, 2024 at 08:59:20AM +0000, Raju.Lakkaraju@microchip.com
wrote:

quoted

Hi Andrew,

Thank you for review comments.

quoted

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Tuesday, February 27, 2024 7:28 AM
To: Raju Lakkaraju - I30499 <redacted>
Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
linux- kernel@vger.kernel.org; Bryan Whitehead - C21958
[off-list ref]; richardcochran@gmail.com;
UNGLinuxDriver [off-list ref]
Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with
wake option flags configuration sequences

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

On Mon, Feb 26, 2024 at 01:39:34PM +0530, Raju Lakkaraju wrote:

quoted

Wake options handling has been reworked as follows:
a. We only enable secure on magic packet when both secure and
magic

wol

quoted

quoted

quoted

   options are requested together.
b. If secure-on magic packet had been previously enabled, and a

subsequent

quoted

   command does not include it, we add it. This was done to
workaround

a

quoted

quoted

quoted

   problem with the 'pm-suspend' application which is unaware of

secure-on

quoted

quoted

quoted

   magic packet being enabled and can unintentionally disable it prior

to

quoted

quoted

quoted

quoted

   putting the system into suspend.

This seems to be in a bit of a grey area. But ethtool says:

           wol p|u|m|b|a|g|s|f|d...
                  Sets  Wake-on-LAN  options.   Not  all devices support this.
                  The argument to this option is a string of characters speci‐
                  fying which options to enable.
                  p   Wake on PHY activity
                  u   Wake on unicast messages
                  m   Wake on multicast messages
                  b   Wake on broadcast messages
                  a   Wake on ARP
                  g   Wake on MagicPacket™
                  s   Enable SecureOn™ password for MagicPacket™
                  f   Wake on filter(s)
                  d   Disable (wake on  nothing).   This  option
                      clears all previous options.

d clears everything. All other things enable one option. There
does not appear to be a way to disable a single option, and i
would say, adding options is incremental.

Yes. "d" clears everything.
But, if we enable "g" then enable "a", "g" option overwritten by "a"

This is where i say it is a bit of a grey area. For me, they are
incremental. You can enable a and then later enable g, and you should have

both enabled.

quoted

Yes. But, it's "Ethtool" issue.
Even though ethtool get the WOL configuration before to set, over written
with wanted wol request configuration.

quoted

quoted

Please find the attached log information

quoted

So:

quoted

a. We only enable secure on magic packet when both secure and
magic

wol

quoted

quoted

quoted

   options are requested together.

I don't think they need to be requested together. I think you can
first enable Wake on MagicPacket and then in a second call to
ethtool Enable SecureOn password for MagicPacket. I also don't
think it would unreasonable to accept Enable SecureOn password for
MagicPacket and have that imply Wake on MagicPacket.

If we need to enable any 2 options, we have to provide both options

together.

quoted

i.e.
sudo ethtool -s enp9s0 wol ga

which i think is wrong. A driver should allow incremental adding WoL

options.

quoted

Ok.

quoted

quoted

quoted

And:

quoted

b. If secure-on magic packet had been previously enabled, and a

subsequent

quoted

   command does not include it, we add it.

If there has not been a d, secure-on magic packet should remain
enabled until there is a d.

This is not happened with existing "Ethtool".
Please find the log information in an attachment file.

That could just be a driver bug.

Take a step back. Is there any clear documentation about how ethtool
-s wol should really work? Any comments in the code? Any man page
documentation.

I'm not able to find  any more "ethtool" documentations other than ethtool
man page.

I have gone through the ethtool application code.
(git://git.kernel.org/pub/scm/network/ethtool/ethtool.git) and check the WOL
options.

In ioctl method (Not netlink), do_sset( ) function, before set the parameters,
first get the WOL configuration.
(i.e. in ethtool.c file, Line no. 3294)

Then, assign/overwrite WOL wanted config to wolopts parameter.

Existing Code:  From Line 3290 -  3312
################################################################
#######
3290         if (gwol_changed) {
3291                 struct ethtool_wolinfo wol;
3292
3293                 wol.cmd = ETHTOOL_GWOL;
3294                 err = send_ioctl(ctx, &wol);
3295                 if (err < 0) {
3296                         perror("Cannot get current wake-on-lan settings");
3297                 } else {
3298                         /* Change everything the user specified. */
3299                         if (wol_change)
3300                                 wol.wolopts = wol_wanted;
3301                         if (sopass_change) {
3302                                 int i;
3303                                 for (i = 0; i < SOPASS_MAX; i++)
3304                                         wol.sopass[i] = sopass_wanted[i];
3305                         }
3306
3307                         /* Try to perform the update. */
3308                         wol.cmd = ETHTOOL_SWOL;
3309                         err = send_ioctl(ctx, &wol);
3310                         if (err < 0)
3311                                 perror("Cannot set new wake-on-lan settings");
3312                 }
################################################################
#######
Current issue is overwriting wolopts with wol_wanted i.e.
3300                                 wol.wolopts = wol_wanted;

Instead of over write, Need to "OR" the wolopts and wol_wanted i.e.
3300                                 wol.wolopts |= wol_wanted

Final code changes should be:
################################################################
#######
3290         if (gwol_changed) {
3291                 struct ethtool_wolinfo wol;
3292
3293                 wol.cmd = ETHTOOL_GWOL;
3294                 err = send_ioctl(ctx, &wol);
3295                 if (err < 0) {
3296                         perror("Cannot get current wake-on-lan settings");
3297                 } else {
3298                         /* Change everything the user specified. */
3299                         if (wol_change  && wol_wanted)
3300                                 wol.wolopts |= wol_wanted;
		     else if (wol_change  && !wol_wanted)
			wol.wolopts = 0
3301                         if (sopass_change) {
3302                                 int i;
3303                                 for (i = 0; i < SOPASS_MAX; i++)
3304                                         wol.sopass[i] = sopass_wanted[i];
3305                         }
3306
3307                         /* Try to perform the update. */
3308                         wol.cmd = ETHTOOL_SWOL;
3309                         err = send_ioctl(ctx, &wol);
3310                         if (err < 0)
3311                                 perror("Cannot set new wake-on-lan settings");
3312                 }
################################################################
#######

Similar changes require in netlink functions also.

For Netlink:

--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c

@@ -107,16 +107,26 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
        struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
        struct net_device *dev = req_info->dev;
        struct nlattr **tb = info->attrs;
+       __u32 wolopts = 0;
        bool mod = false;
        int ret;
 
        dev->ethtool_ops->get_wol(dev, &wol);
-       ret = ethnl_update_bitset32(&wol.wolopts, WOL_MODE_COUNT,
+       ret = ethnl_update_bitset32(&wolopts, WOL_MODE_COUNT,
                                    tb[ETHTOOL_A_WOL_MODES], wol_mode_names,
                                    info->extack, &mod);
        if (ret < 0)
                return ret;
-       if (wol.wolopts & ~wol.supported) {
+
+       if (wolopts) {
+               wol.wolopts |= wolopts;
+       } else {
+               wol.wolopts = 0;
+               memset(&wol.sopass, 0, sizeof(wol.sopass));
+               mod = true;
+       }
+
+       if (wolopts & ~wol.supported) {
                NL_SET_ERR_MSG_ATTR(info->extack, tb[ETHTOOL_A_WOL_MODES],
                                    "cannot enable unsupported WoL mode");
                return -EINVAL;

###############################################################

Please find the an attachment file.

quoted

Lets first understand how it is expected to work. Then we can decided
if the driver is implementing it correctly.

   Andrew

Please find the "ethtool" application code main file as attachment file.

Thanks,
Raju