Received-SPF: pass (google.com: domain of linux-kernel+bounces-97487-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: <Ronnie.Kunin@microchip.com>
To: <Raju.Lakkaraju@microchip.com>, <andrew@lunn.ch>
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
Thread-Topic: [PATCH net 3/3] net: lan743x: Address problems with wake option
 flags configuration sequences
Thread-Index: AQHaaIt9/9VvJ3PDJ0OgsYtGoeCEQLErcdiAgAIgIYCAAJu7MA==
Disposition-Notification-To: <Ronnie.Kunin@microchip.com>
Date: Fri, 8 Mar 2024 18:56:39 +0000
Message-ID: <PH8PR11MB79656DCF7806D7390C7100DE95272@PH8PR11MB7965.namprd11.prod.outlook.com>
References: <20240226080934.46003-1-Raju.Lakkaraju@microchip.com>
 <20240226080934.46003-4-Raju.Lakkaraju@microchip.com>
 <78d7e538-9fa0-490e-bcfb-0a5943ad80c9@lunn.ch>
 <LV8PR11MB87008454A629EE15B9CE14099F272@LV8PR11MB8700.namprd11.prod.outlook.com>
In-Reply-To: <LV8PR11MB87008454A629EE15B9CE14099F272@LV8PR11MB8700.namprd11.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?wesaNpZlIHMygpH/3zEl4d/h3J4whXYHlVtw6qeOA/kh9FDQiUBvT6p1A9kS?=
 =?us-ascii?Q?dPg8MCgKlkhBnGfhfFRUzAzVPwzcP5oG3CC9HqAjU/fZH4UOGLcD/sxtURXJ?=
 =?us-ascii?Q?O16H4Iy/fsKYeNNjb9aV9jMTE2drrqKHshjR3QKcrJmKYdOiea7LFsfDxrfI?=
 =?us-ascii?Q?bLyi0uqHk8XGURJjC3haEquU5ngiuP2pMrq6GxwLh+NqHx7sl6avxQ6zb4pi?=
 =?us-ascii?Q?XHtd5eRLE4+11cubEExQ3MVmthgS1aw4+l+d3dMNu04hXXeZYW91ZbyN8Ooq?=
 =?us-ascii?Q?K4MX+8TbBp7fVGcsWp1cdrH2zibI8olUIP3D0z/UTZVr8TMEC9NewDeEfKKQ?=
 =?us-ascii?Q?fH3FcWNm4HjZlkRffYsb/yGhYcanG/eIqyB5Ht3PMfllAss2ZEbNt2qbMMME?=
 =?us-ascii?Q?lk2ptWUfIT4LBuiRme7bd+kuhqV/EcgimkVXHdUNixuOCowkmWFdFU9K2Y4A?=
 =?us-ascii?Q?3twFt8pEkLwI8lcwT9+AfJgVrj4UrB+AeasoyhhvvjaKyr4ELSfXHJTJ7F52?=
 =?us-ascii?Q?XUPlvGwqv7eodUG/YEpQC2LxXlllUc3m/fU/jiOvGxBYOJnya5svb3tvQjBp?=
 =?us-ascii?Q?zokS+BVhc+ge1zBXEew6BY6DZ+UlDDBMMT6iV5rWxVuIKxJ+QzxyTtTl8cR7?=
 =?us-ascii?Q?NcsiLQftToaimqU1zajKJJV8MBTyk8AEanww9Be6/Of/IY1iSsX9WB9QECqe?=
 =?us-ascii?Q?uecpPrpYlwyEFmsRr71IdpdYGX6AV/f63kK2unMj+VJjeZ1EdRyqpgAkgYux?=
 =?us-ascii?Q?nefMqxFQwxXicWsCVg50s9oKi0qCdUh4AW69EepT+lAAWY1A4dgc50RYOmT8?=
 =?us-ascii?Q?aDRmB9+K5pP1FtbLDqvDa27qZIXewkxy90E9LtgpQl0fdNlzLMrGV/PICZD8?=
 =?us-ascii?Q?7zfU2xhCdxRWYS9yYnBpYLCDoDWIFXju9ETMBJ8qbJTWu3goeF0AMKGrKDxe?=
 =?us-ascii?Q?7A1qe7Y9p5oxBH2fQndL7qCV/X6yg7+yNycTjDC0CvdODAhkA1sLaCclbwgO?=
 =?us-ascii?Q?jg8GiNHOcbj1NxqrJyxUpvxTcybaLI6pZ4655itpzAv9yaBLdCozGpqlwxhF?=
 =?us-ascii?Q?PgXG74aQ+8DMPH7J7/jdCmmx6VCxRMx0+2ZQmGpMAGv/2TjWc6wZpzxU+vnA?=
 =?us-ascii?Q?TSYKJjmEfK5cCa1lWnl8eU3zpTKnk76rft3trww++zUyQRgoyg3TbIaeppnV?=
 =?us-ascii?Q?PYgW/I+aeWaKjpTrK3mLRLpBYtwC+5ASyKwdkQOli9JtKoZOqztKAaIyZGOk?=
 =?us-ascii?Q?gKwXz6zWF1EasJJB9xzl1hPGJrLIAyEKzcqwVayRsZsvllROA1483w7hE76F?=
 =?us-ascii?Q?o7Ztx6MSERXu/Dj0Qvd0Pz/tPL5Mnf/S9lgjTjuHOAVrb2/78kQPimNu0jkB?=
 =?us-ascii?Q?cX6wneOuSW2+RQcWeQ1oak1ig6iSnq8CXAgGOnZEQ33cIXk1h0oIrvkQ68Ha?=
 =?us-ascii?Q?p8e3L8q6kANZnDZKcrD1OP2GSfPRTMipHogMyS80cJ5oUUuBuPxBdMWliaZJ?=
 =?us-ascii?Q?cInX7jKzj3cbNzUWD8lE+g3T7XY9oAeYdbZh3mAu7YoZgvIJFA+BtJqqfyVK?=
 =?us-ascii?Q?3h0EVxU5sZWVzVUeKE0oWK5p+t60vj9P+0zv6R03?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB7965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 57a8af67-8e4c-44e0-7845-08dc3fa17ce1
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Mar 2024 18:56:39.6941
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: VebfRYD5qhegGmDJgK/JXm1wwczf0c0jv9Johai7DMfXwWzpZLtQSZ/ru95iimQz+ofFFG9pIQ3e0Jgd83vV5YXTlt9J8gz1y3XFLraWDFQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR11MB8392

Hi Andrew

> -----Original Message-----
> From: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Sent: Friday, March 8, 2024 3:21 AM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; linux-k=
ernel@vger.kernel.org;
> Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; richardcochran@=
gmail.com;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: RE: [PATCH net 3/3] net: lan743x: Address problems with wake opt=
ion flags configuration
> sequences
>=20
> Hi Andrew,
>=20
> Thank you for valuable information.
>=20
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Thursday, March 7, 2024 5:23 AM
> > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> > Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> > linux- kernel@vger.kernel.org; Bryan Whitehead - C21958
> > <Bryan.Whitehead@microchip.com>; richardcochran@gmail.com;
> > UNGLinuxDriver <UNGLinuxDriver@microchip.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
> >
> > 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 w=
ol
> > >    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 t=
he same time" means ...

The original (i.e. not secure) Magic packet specification says it is a fram=
e 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 pat=
tern) 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 f=
rame includes a "magic packet pattern", whatever is before (MISC_PRE) and a=
fter (MISC_POST) the "magic packet pattern" does not matter, the device wil=
l 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 inco=
ming 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).

>=20
> Yes. I will do.
>=20

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; whatev=
er behavior your hardware does will not be fully compliant to both specs si=
multaneously anyway. I discussed this with Raju and what we decided to do f=
or our driver/device is that if you pass both WAKE_MAGIC and WAKE_MAGICSEGU=
RE flags to us we will report them back as both being enabled in a subseque=
nt 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 workaroun=
d a
> > >    problem with the 'pm-suspend' application which is unaware of secu=
re-on
> > >    magic packet being enabled and can unintentionally disable it prio=
r to
> > >    putting the system into suspend.
> >
>=20
> Ok. I will try to fix in 'pm-suspend' application
>=20
> > 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 s=
omething else.
> >
> >           Andrew
>=20
> Thanks,
> Raju