On 3/12/2024 5:22 PM, Ronnie.Kunin@microchip.com wrote:

quoted

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Tuesday, March 12, 2024 5:41 PM
To: Ronnie Kunin - C21729 <redacted>
Cc: Raju Lakkaraju - I30499 <redacted>; 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

quoted

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.

Agreed, this is definitively the case for the hardware that I maintain.

quoted

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

This seems reasonable to me, and as you say it matches the header 
comment. Question is whether the enforcement of WAKE_MAGICSECURE implies 
WAKE_MAGIC at the core ethtool level, or if this is up to individual 
drivers. Also, how many drivers need to be fixed?

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.

I suspect this might be breaking user-space in surprising ways and we 
would eventually get a report about that requesting the behavior to be 
changed.

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.

I agree about the not clear behavior though for bcm-phy-lib.c, 
specifying both will eventually have WAKE_MAGICSECURE "win" given how 
the code is structured (assuming I can remember my own code properly).

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

That would be my inclination for new drivers, or drivers that we are 
fixing, like lan743x. For existing drivers unfortunately we might have 
to preserve the incorrect behavior.
-- 
Florian