Date: Tue, 12 Mar 2024 17:53:48 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Ronnie.Kunin@...rochip.com, andrew@...n.ch
Cc: Raju.Lakkaraju@...rochip.com, netdev@...r.kernel.org,
 davem@...emloft.net, kuba@...nel.org, linux-kernel@...r.kernel.org,
 Bryan.Whitehead@...rochip.com, richardcochran@...il.com,
 UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option
 flags configuration sequences



On 3/12/2024 5:22 PM, Ronnie.Kunin@...rochip.com wrote:
> 
>> -----Original Message-----
>> From: Andrew Lunn <andrew@...n.ch>
>> Sent: Tuesday, March 12, 2024 5:41 PM
>> To: Ronnie Kunin - C21729 <Ronnie.Kunin@...rochip.com>
>> Cc: Raju Lakkaraju - I30499 <Raju.Lakkaraju@...rochip.com>; netdev@...r.kernel.org;
>> davem@...emloft.net; kuba@...nel.org; linux-kernel@...r.kernel.org; Bryan Whitehead - C21958
>> <Bryan.Whitehead@...rochip.com>; richardcochran@...il.com; UNGLinuxDriver
>> <UNGLinuxDriver@...rochip.com>
>> 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.

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

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

Powered by blists - more mailing lists