Re: [PATCH net 3/3] net: lan743x: Address problems with wake option flags configuration sequences

[Andrew Lunn]

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.

> 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.

> 
> > 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.

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

   Andrew