RE: [PATCH net 3/3] net: lan743x: Address problems with wake option flags configuration sequences
From: Raju.Lakkaraju
Date: Mon Mar 04 2024 - 06:20:30 EST
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@xxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Bryan Whitehead - C21958
> <Bryan.Whitehead@xxxxxxxxxxxxx>; richardcochran@xxxxxxxxx;
> UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>
> Subject: RE: [PATCH net 3/3] net: lan743x: Address problems with wake option
> flags configuration sequences
>
> Hi Andrew,
>
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: Thursday, February 29, 2024 8:36 PM
> > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
> > Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> > linux- kernel@xxxxxxxxxxxxxxx; Bryan Whitehead - C21958
> > <Bryan.Whitehead@xxxxxxxxxxxxx>; richardcochran@xxxxxxxxx;
> > UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx
> > wrote:
> > > Hi Andrew,
> > >
> > > Thank you for review comments.
> > >
> > > > -----Original Message-----
> > > > From: Andrew Lunn <andrew@xxxxxxx>
> > > > Sent: Tuesday, February 27, 2024 7:28 AM
> > > > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
> > > > Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> > > > linux- kernel@xxxxxxxxxxxxxxx; Bryan Whitehead - C21958
> > > > <Bryan.Whitehead@xxxxxxxxxxxxx>; richardcochran@xxxxxxxxx;
> > > > UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>
> > > > 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:
> > > > > Wake options handling has been reworked as follows:
> > > > > a. We only enable secure on magic packet when both secure and
> > > > > magic
> > wol
> > > > > options are requested together.
> > > > > b. If secure-on magic packet had been previously enabled, and a
> > > > subsequent
> > > > > command does not include it, we add it. This was done to
> > > > > workaround
> > a
> > > > > problem with the 'pm-suspend' application which is unaware of
> > secure-on
> > > > > magic packet being enabled and can unintentionally disable it prior
> to
> > > > > 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.
> >
>
> Yes. But, it's "Ethtool" issue.
> Even though ethtool get the WOL configuration before to set, over written
> with wanted wol request configuration.
>
> > > Please find the attached log information
> > > > So:
> > > >
> > > > > a. We only enable secure on magic packet when both secure and
> > > > > magic
> > wol
> > > > > 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.
> > > i.e.
> > > sudo ethtool -s enp9s0 wol ga
> >
> > which i think is wrong. A driver should allow incremental adding WoL
> options.
> >
>
> Ok.
>
> > >
> > > > And:
> > > >
> > > > > b. If secure-on magic packet had been previously enabled, and a
> > > > subsequent
> > > > > 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.
> > 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
Attachment:
0001-ethtool-netlink-adding-options-is-incremental.patch
Description: 0001-ethtool-netlink-adding-options-is-incremental.patch