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

[Raju.Lakkaraju]

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

Yes. I will do.

> 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