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

[Ronnie.Kunin]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Tuesday, March 12, 2024 5:41 PM
> To: Ronnie Kunin - C21729 <Ronnie.Kunin@xxxxxxxxxxxxx>
> Cc: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>; 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
> 
> > I understand that the TI devices give the *impression* of supporting
> > both, but based on what I explained above, even if you accept
> > WAKE_MAGIC and WAKE_MAGICSEGURE on a set and report them both back as
> > enabled on a get; whatever behavior your hardware does will not be
> > fully compliant to both specs simultaneously anyway. I discussed this
> > with Raju and what we decided to do for our driver/device is that if
> > you pass both WAKE_MAGIC and WAKE_MAGICSEGURE flags to us we will
> > report them back as both being enabled in a subsequent get as you
> > suggested, but the behavior of our driver/hardware will be as if you
> > had only enabled WAKE_MAGIC.
> 
> So i agree having WAKE_MAGIC and WAKE_MAGICSECURE at the same time seems very odd. So i see no

To me it is not just a little odd, *strictly speaking* as mentioned before it is an impossibility, since no 
hardware can do both at the same time because they have mutually exclusive requirements 
for some frames.

> real problem limiting the driver to only one or the other. However, if the user does ask for both, i would
> say silently ignoring one is incorrect. You should return -EOPNOTUPP to make it clear you don't support
> both at the same time.
> 
> I would also say that silently ignore the Secure version is probably the worst choice. Things should be
> secure by default...
> 
>      Andrew

We were just trying to accommodate your previous request to accept both "if the hardware supports it". 
And even though I didn't like it, this was an attempt to answer my previous question: "what does it 
mean to support both magic and secure magic at the same time ?" in some way that might make sense. 
It is not that the purpose was to "silently ignore" the secure flag (that's why we would still return it as being 
set on a subsequent get), we just took the interpretation that both flags together meant the user wanted 
to do an "OR" of both matching conditions (secure and non secure). I see your preference would be to do 
an "AND" of the two matching conditions citing security concerns. Honestly, I don't think there is a best or 
worse way, in my opinion the user does not really understand what he is doing if he Is asking to enable both 
secure and non-secure behaviors simultaneously, so security is probably down the drain already anyway. 

In that sense I would have agreed with your recommendation that the best course of action would have 
been to only accept one flag individually and fail with -EOPNOTUPP if both come simultaneously.  And 
being mutually exclusive at the definition level that really should have applied to all drivers and hardware 
(not just Microchip's). 

But then I looked at the actual definition of the flags themselves in the header file and I see this:
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1993

#define WAKE_MAGIC		(1 << 5)
#define WAKE_MAGICSECURE	(1 << 6) /* only meaningful if WAKE_MAGIC */

And even the ethtool manual says this 

                  g   Wake on MagicPacket(tm)
                  s   Enable SecureOn(tm) password for MagicPacket(tm)

So the "only meaningful" comment seems to imply the original intention of these flags was that 
WAKE_MAGICSECURE is an optional  modifier for WAKE_MAGIC. Since as Raju showed the ethertool 
application always overwrites previous settings (does not preserve anything) then you can only use 
WAKE_MAGICSECURE *simultaneously* with WAKE_MAGIC and not in a standalone manner. 
The ethtool manual seems to me to reinforce this since if says "Enable SecureOn password FOR magic 
packet", rather that "Enable SecureON MagicPacket", so the 's' option is something that enables the 
addition of a password to the 'g' Option.

So back to the beginning it is unclear what should happen... 
I'd say we seem to have 3 different approaches. Which way should we go now?
1. Follow the definition of the flags in ethtool.h and ethtool manual: 
     - accept WAKE_MAGIC standalone and wake on regular magic packet matching 
     - accept WAKE_MAGIC and WAKE_MAGICSECURE simultaneously and only wake on secure magic 
        packet with valid password matching. 
     - reject WAKE_MAGICSECURE standalone
     Note that this is not how any of the current drivers work and does not follow the conclusions from your 
     last email either
2. Treat WAKE_MAGIC as a request for magic packet behavior; and WAKE_MAGICSECURE as a request for 
     secure-on magic packet behavior. Since they are mutually exclusive only accept them individually and 
     reject it if they come simultaneously. This does not match the flags definitions or documentation, and 
     it is not how any of the existing drivers work, but it has consistency to it and it is the way you were 
     leaning in the last email based on what we knew by them.
3. Follow some of the other existing drivers' code behavior (Broadcom,  TI or MSCC), which do not seem to 
    match the flag definitions (because they all accept WAKE_MAGICSECURE standalone) and we do not really 
    know what the hardware exactly does in some of the flag combinations / received frame stimuli. I'd rather 
    not do this since we (Microchip) will probably end up behaving in yet some different behavior from 
    everybody else for at least some frame stimuli and not match any documentation either.

My opinion with this latest info from headers / man is that we should follow #1. What do you think Andrew?

Ronnie