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

[Ronnie.Kunin]

Hi Andrew

> -----Original Message-----
> From: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
> Sent: Friday, March 8, 2024 3:21 AM
> 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,
> 
> Thank you for valuable information.
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: Thursday, March 7, 2024 5:23 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.
> >
> > So it appears unclear what should happen here.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/bcm-phy
> > -
> > lib.c#L909
> >
> > WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> > And even i you did request both WAKE_MAGIC and WAKE_MAGICSECURE, the
> > WAKE_MAGIC would be ignored.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/dp83822
> > .c#L153
> >
> > WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> > However, unlike the broadcom device, you can have both WAKE_MAGIC and
> > WAKE_MAGICSECURE at the same time. They are not mutually exclusive.
> >
> > This also looks to be true for other dp8**** devices.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/mscc/ms
> > cc_mai
> > n.c#L318
> >
> > WAKE_MAGICSECURE is a standalone option. You do not need WAKE_MAGIC.
> > Also, you can have both WAKE_MAGIC and WAKE_MAGICSECURE at the same
> > time. They are not mutually exclusive.
> >
> > So i think your point a. above is questionable. Can the hardware
> > support both magic and secure magic at the same time? If so, follow the TI way of doing it.

I guess the question here is what "support both magic and secure magic at the same time" means ...

The original (i.e. not secure) Magic packet specification says it is a frame like this:
DESTINATION, SOURCE, MISC_PRE, 6 x FF, 16 x MAC_ADDR, MISC_POST, FCS
So a "magic packet pattern" consists of six 'FF' bytes (synchronization pattern) immediately followed by 16 consecutive repetitions of the mac address of the device and this can be anywhere within the frame.
(a) Note that if magic packet wake is enabled in a device and an incoming frame includes a "magic packet pattern", whatever is before (MISC_PRE) and after (MISC_POST) the "magic packet pattern" does not matter, the device will generate a wake up event.

The "Secure-on" magic packet, adds a 6 byte password immediately after the 16 repetitions of the mac address of the device to the pattern that has to be matched, so the frames looks like this:
DESTINATION, SOURCE, MISC_PRE, 6 x FF, 16 x MAC_ADDR, 6-BYTE_PWD, MISC_POST, FCS
(b) Note that If secure-on magic packet wake is enabled in a device an incoming frame will only cause a wake event if the whole pattern *including the password* matches.

 (a) and (b) are mutually exclusive behaviors for frames that do not carry passwords (i.e. they have the FCS right after the 16th repletion of the Mac address) or have a non-matching password, so you cannot really enable both simultaneously (at least not in the sense that you would be able to comply with both standards for all possible frames simultaneously).

> 
> Yes. I will do.
> 

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.

> > If you cannot do both at the same time, and that is requested, you
> > should probably return -EOPNOTSUPP. That is probably better than what
> > the broadcom driver does, silently ignore WAKE_MAGIC.
> >
> > > 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.
> >
> 
> Ok. I will try to fix in 'pm-suspend' application
> 
> > The kernel should not be working around broken userspace. But i also
> > suspect this is to do with it being unclear if WOL options are
> > incremental or not. Since it seems that they are not incremental, it
> > does not matter if "If secure-on magic packet had been previously
> > enable". pm-suspend is setting Wol how it wants it, which you say is
> > plain magic. So magic is what the PHY driver should do. Feel free to
> > submit patches to pm-suspend to make it understand secure magic, or
> > not touch WoL at all with the assumption it has already been setup by something else.
> >
> >           Andrew
> 
> Thanks,
> Raju