Received-SPF: pass (google.com: domain of linux-kernel+bounces-94753-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 7 Mar 2024 00:53:21 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
Cc: netdev@vger.kernel.org, davem@davemloft.net, kuba@kernel.org,
	linux-kernel@vger.kernel.org, bryan.whitehead@microchip.com,
	richardcochran@gmail.com, UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net 3/3] net: lan743x: Address problems with wake option
 flags configuration sequences
Message-ID: <78d7e538-9fa0-490e-bcfb-0a5943ad80c9@lunn.ch>
References: <20240226080934.46003-1-Raju.Lakkaraju@microchip.com>
 <20240226080934.46003-4-Raju.Lakkaraju@microchip.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240226080934.46003-4-Raju.Lakkaraju@microchip.com>

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

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