Received-SPF: pass (google.com: domain of linux-kernel+bounces-154945-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: <Raju.Lakkaraju@microchip.com>
To: <andrew@lunn.ch>
CC: <netdev@vger.kernel.org>, <davem@davemloft.net>, <kuba@kernel.org>,
	<pabeni@redhat.com>, <edumazet@google.com>, <linux-kernel@vger.kernel.org>,
	<Bryan.Whitehead@microchip.com>, <UNGLinuxDriver@microchip.com>
Subject: RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY
 does not
Thread-Topic: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when
 PHY does not
Thread-Index: AQHaen5pA5HIVZmbdE+YwqVuzvYGF7FBPcYAgBgZppCAAKxcAIAbyhww
Date: Tue, 23 Apr 2024 11:05:44 +0000
Message-ID: <LV8PR11MB8700A34520EA2521BC5F59EF9F112@LV8PR11MB8700.namprd11.prod.outlook.com>
References: <20240320042107.903051-1-Raju.Lakkaraju@microchip.com>
 <20240320042107.903051-3-Raju.Lakkaraju@microchip.com>
 <22089299-a3e2-4cbd-942a-65ea070657b8@lunn.ch>
 <LV8PR11MB87003ABBCA98F00F3EEA9AB09F032@LV8PR11MB8700.namprd11.prod.outlook.com>
 <fb5a6f19-ae4c-443e-babb-cbbcf6ba5fc6@lunn.ch>
In-Reply-To: <fb5a6f19-ae4c-443e-babb-cbbcf6ba5fc6@lunn.ch>
Accept-Language: en-GB, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?kyjwMIfbEBkvtU4nHVvHy8nk318YLIJQP0mPbiY9gZ/QG8aqI74Zz7BoGdn4?=
 =?us-ascii?Q?C7ojswpSCcD2p1phEoKfvXt+wrMwcnqFu40rxlnhRISEoSSWsufzGPqvYKM3?=
 =?us-ascii?Q?OHjS8S0u5npCvdbfDECrnQm4g2BYFqbuIsXpG4mLPF4xm0dHpkD3HqW3iZtg?=
 =?us-ascii?Q?HFgz+plEQIwAwjbtx3uVGY4X2wsw/hyGXUQekP37eWWDR0U4tlUwWCWqIR46?=
 =?us-ascii?Q?3SkLmWgtYXkvLWlx9r2ASck4HrSZAEq1IIuYScvxWmNB4SUTi+JoEzlMOLOV?=
 =?us-ascii?Q?R5BALTxA1b1zIXFjj3cyIeBhRIqX7BPB/QadgpQU/SwOjMGnCxzw3MWlZ/L1?=
 =?us-ascii?Q?nGOJWUu5vKaYslW2gEdzTwaRFduUfIn0+TtQ6JdhOtzPskievdosG+23nY5B?=
 =?us-ascii?Q?8jlljUjikvBN+yHpyMVXNhfDSqdzMWw2UNHH3Hy18gtCl6l3r++wBgjNINgM?=
 =?us-ascii?Q?NN0HbowiunPICooKzIjYLmdL8wdEH95ifdIIVId55HK7vg/JOUlTHuc/iOqE?=
 =?us-ascii?Q?4dcrgJ94jiBtsY359mxX8CAw5Xqu/5RWHmgZ4T4he8gnuZ9Q4/tgJJvMx46q?=
 =?us-ascii?Q?EcbcNyfS2KqUibeh5JAlEMyGyaJQueyYcg9ZKjn9YyRWd9zVZ7B3zw+Caklv?=
 =?us-ascii?Q?UhP7v4FSN8YXQri6APGGSP3i7m3bARlce4GMJNUnMmiKIcrW5wINUeP7eGMT?=
 =?us-ascii?Q?Mw1HnM6thtQdNzI1RSunZf0BbKyQ+0E/8vDbZ4qmi9x/gcOwGNOXX6x0wqre?=
 =?us-ascii?Q?FPSkynyl2w5H5GDpP/0sZSoNm4HH/Cc2FSMxqqmta9YuZfkyzDEkUBC/n+uM?=
 =?us-ascii?Q?ZL+mlMPgXTvSFtDeMnSUojXaG6DDSm96msuiopNRGo4aCcBbD3NbszlHc97G?=
 =?us-ascii?Q?/+olYKWqMZ9z3pEfsyengN9aGWS6RyFbibjD5oGXzYOUZA+UjoZvBczK2Xdj?=
 =?us-ascii?Q?tvDhcYHr7udair/Q/+gpaEDoxZxeAhqujreJfdhjpFA27jcYYZRRoHWjvFE9?=
 =?us-ascii?Q?6AWQzchKYhIiibM+623GnVPMtBOqWWw5jmsrTj6x5WvQFLtJdFJtV+q52Sy4?=
 =?us-ascii?Q?gBTSgDjtSPoREHZK0eIlb8wYvXn8iMRdqe+vstqZr2YWT98Obg1S/Xvw4mLj?=
 =?us-ascii?Q?CXSdoDxIFl14KVR261zsCU6sq1oOHoMSxCsGB71yjWjUHXY4i2AAGN3rk4dM?=
 =?us-ascii?Q?mZPFvhH0FthsawrcrwJl0PYhcMrCTp6rvIpifZvbZZdjv5C5Iy3YhMChlxKq?=
 =?us-ascii?Q?/2hh92zUrzATBIccvOAPPSuptG5uYfp4YtIixNqB032e8r4p7bE0VJXGeSag?=
 =?us-ascii?Q?WtvdDfm/cVv3vFenszS8r5jiX+uqSJkk0ajxAJttkBSKqbYhSTB8dU59mj6i?=
 =?us-ascii?Q?8Zm0YqB0xj+gcBV01tR2jBfrCRyP9b0+NjQayg/LPsBvdo6q9ghFHtmciADF?=
 =?us-ascii?Q?ceCwXSFB8/4yIb/3KOmZv+7nz9uh1Qhw862kA8LNifWECfzu9lKX0J0LM/D6?=
 =?us-ascii?Q?qT/BbhDnFZkEahOPwVxf9c9CRCehORLenF+eSLsLw6sksRw5ZblmVzVHz777?=
 =?us-ascii?Q?In6IGsbnptfV12+F5LSnH2vXZGKSYJNPSAgsB2Qvfk6Gra8RKafJQRvqAZW6?=
 =?us-ascii?Q?sQ=3D=3D?=
Content-Type: multipart/mixed;
	boundary="_002_LV8PR11MB8700A34520EA2521BC5F59EF9F112LV8PR11MB8700namp_"
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: LV8PR11MB8700.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ae167f58-b55e-459d-17da-08dc638552bc
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Apr 2024 11:05:44.9001
 (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: SK25Dzjjq72yqN1/GaWxD1XkgOx7ep3kM4uNpEdHq2xAGuM+j0xYRKMafak4KEUQeePgnZgO3EGXlM+Y0+0Np5UQOC4Jlb7Ow7tETB7kmSs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8666

--_002_LV8PR11MB8700A34520EA2521BC5F59EF9F112LV8PR11MB8700namp_
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, April 5, 2024 10:42 PM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; edumazet@google.com; linux-kernel@vger.kernel.org;
> Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even whe=
n
> PHY does not
>=20
> EXTERNAL EMAIL: Do not click links or open attachments unless you know th=
e
> content is safe
>=20
> On Fri, Apr 05, 2024 at 08:17:22AM +0000, Raju.Lakkaraju@microchip.com
> wrote:
> > Hi Andrew,
> >
> > Sorry for delayed response.
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Thursday, March 21, 2024 4:23 AM
> > > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> > > Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> > > pabeni@redhat.com; edumazet@google.com;
> > > linux-kernel@vger.kernel.org; Bryan Whitehead - C21958
> > > <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> > > <UNGLinuxDriver@microchip.com>
> > > Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC
> > > even when PHY does not
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > > +     if (netdev->phydev) {
> > > > +             ret =3D phy_ethtool_set_wol(netdev->phydev, wol);
> > > > +             if (ret !=3D -EOPNOTSUPP && ret !=3D 0)
> > > > +                     return ret;
> > >
> > > I'm not sure this condition is correct.
> > >
> > > If there is an error, and the error is not EOPNOTSUPP, you want to
> > > report that error. However, if the PHY can support the WoL
> > > configuration, it will return 0, and this function should exit, WoL
> > > in the MAC is not needed. And doing WoL in the PHY consumes less
> power since you can suspend the MAC.
> > >
> > > So i think it should simply be:
> > >
> > > > +             if (ret !=3D -EOPNOTSUPP)
> > > > +                     return ret;
> > >
> > > Do you have a board with this MAC with a PHY which does have some
> > > WoL support. Could you test PHY WoL is used when appropriate?
> >
> > Yes.
> > We have external PHY (Max Linear GPY211C) attach to MAC of PCI11x1x
> > (PCIe Ethernet chip) If I don't register the Ethernet module in wakeup
> source, WOL is not working. Ethernet device power state shows as disable.
>=20
> So i'm talking about the case where the PHY is doing the wakeup, without
> help from the MAC. In that case, "Ethernet device power state shows as
> disable." is sensible.
>=20

If we don't enable/register the PCI11x1x's ethernet device in wake list by =
calling " device_set_wakeup_enable( )" function, device power state shows a=
s disable.
When I refer the other vendor pcie's ethernet drivers, " device_set_wakeup_=
enable( )" function called in ethernet driver Power Management's suspend an=
d resume functions.
But LAN743x driver call this function when Ethtool configure the WOL.

> > i.e.
> >
> /sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00=
.
> 0/power/wakeup   <--  disabled

When I call the device_set_wakeup_enable( ) with enable =3D 1, wakeup (/sys=
/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.0/pow=
er/wakeup) shows enable.
Also observe that device (PCI11x1x's ethernet device : 0000:09:00.0) add in=
 wake list.
i.e.
device: 'wakeup34': device_add

> >
> > PCI11x1x is PCIe bridge device between PCIe and Ethernet along with
> > other peripherals (i.e. UART, SPI, I2C, USB and PCIe devices)
> > 0000:09:00.0 - Ethernet device
> > 0000:05:00.0 - PCIe Bridge Up link
>=20
> How does the PHY indicate wake up? It could be you can power off the MAC,
> but you need to keep parts of the PCIe bridge up, in order the wake up ge=
ts
> delivered?
>=20

PHY (GPY211C)'s interrupt pin (MDINT) connect to PCI11x1x's ethernet device=
.
When I configure the WAKE_PHY or WAKE_MAGIC on GPY211C PHY, Interrupt gener=
ation observed when magic packet or link activity (link down or link up).
If wakeup enable in PCI11x1x's ethernet device, System resumes from sleep.

> > When I test the WOL_PHY option on setup (PCI11x1x MAC + GPY211C PHY),
> > observe the following:
>=20
> > 1. When enable WOL_PHY by using ethtool (i.e. ethtool -s enp9s0 wol
> > p), GPY211 PHY configure the WOL. After resume from sleep, GPY211 WOL
> > configuration vanish. Observed that gpy_config_init( ) function reset.
> > Is it expected behaviour ? In other mail thread, we discussed that
> > Ethtool configuration should retain after resume from sleep.
>=20
> The whole point of suspend/resume is that the configuration is retained. =
So i
> would expect from the users perspective WoL is still enabled.
>=20
> As you point out, we might have a logic issue here, that on resume we hit=
 the
> PHY with a hardware reset. That could be clearing out WoL?  We might need
> to cache the PHY WoL configuration in phydev, and on resume re-apply it.
> WoL is complex so we need to be careful who is actually managing it. But =
this
> seems like something which could be done in the phylib core.
>=20

Yes. You are correct.  On resume we hit the PHY with a hardware reset. That=
 could be clearing out WoL.

On resume, phy_init_hw( ) calls the PHY's configuration and interrupt funct=
ions to resets.
As you suggested, I add the wolopts flag in phydev and update this when set=
_wol function execute. This change fix the issue.
i.e.
Re-apply WOL configuration on PHY in phy_suspend( ) function.

Please find the attached prototype code change (Temporary patch)for referen=
ce.=20
I will submit this patch separately.

> > 2. when WOL configure with ethtool, Either Link-down and Link-up on
> > CLI, WOL configuration vanish. Is it expected behaviour ? Due to this,
> > every time we have to configure WOL through Ethtool.
>=20
> This is unexpected. I would expect WoL to remain configured until the
> configuration is changed.
>=20

Observe that magic packet configuration(ethtool's wol g option) did not van=
ish.
But when Link flap occur for WAKE_PHY case, this issue observes. This might=
 be recent "netlink" changes issue.=20
I will debug and confirm.

> > Based on above information, We need to check for return < 0 condition
> > and return the error. Else enable the wakeup by calling
> > "device_set_wakeup_enable( )" function.
>=20
> Once it is clear how the PHY does the wakeup, we can look at this. Howeve=
r, if
> the PHY can support the WoL, you should not be configuring the MAC as wel=
l
> to do the same WoL, you should be putting the MAC into a low power mode.
>=20

When PHY support's the WOL (WAKE_PHY, WAKE_MAGIC), it's generated only MDIN=
T interrupt which can handle by MAC.
But we must configure the wake phy interrupt bit in MAC's Power Management =
register.

PCI11x1x's ethernet device MAC supports "Secure-ON" features, but GPY211 do=
es not support "Secure-ON".
So, we have to use MAC WOL for secure-on feature.

I tested this part.=20
When enable the MAC's WAKE_PHY_INT bit, when PHY generate the interrupt, Sy=
stem resumes from sleep.

>     Andrew

Thanks,
Raju

--_002_LV8PR11MB8700A34520EA2521BC5F59EF9F112LV8PR11MB8700namp_
Content-Type: application/octet-stream;
	name="0001-Wake-ON-LAN-PHY-reconfiguration.patch"
Content-Description: 0001-Wake-ON-LAN-PHY-reconfiguration.patch
Content-Disposition: attachment;
	filename="0001-Wake-ON-LAN-PHY-reconfiguration.patch"; size=2015;
	creation-date="Tue, 23 Apr 2024 10:19:51 GMT";
	modification-date="Tue, 23 Apr 2024 11:05:44 GMT"
Content-Transfer-Encoding: base64

RnJvbSA2YjVhYjUzMWEyYTJhMTNiZWY2YTU0YTBlMmE3OGZmZTQwMDhiZGI0IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBSYWp1IExha2thcmFqdSA8UmFqdS5MYWtrYXJhanVAbWljcm9j
aGlwLmNvbT4KRGF0ZTogVHVlLCAyMyBBcHIgMjAyNCAxNTo0NzoyOCArMDUzMApTdWJqZWN0OiBb
UEFUQ0hdIFdha2UtT04tTEFOIFBIWSByZWNvbmZpZ3VyYXRpb24KCi0tLQogZHJpdmVycy9uZXQv
cGh5L214bC1ncHkuYyAgICB8IDQgKysrKwogZHJpdmVycy9uZXQvcGh5L3BoeV9kZXZpY2UuYyB8
IDUgKysrKysKIGluY2x1ZGUvbGludXgvcGh5LmggICAgICAgICAgfCAxICsKIDMgZmlsZXMgY2hh
bmdlZCwgMTAgaW5zZXJ0aW9ucygrKQoKZGlmZiAtLWdpdCBhL2RyaXZlcnMvbmV0L3BoeS9teGwt
Z3B5LmMgYi9kcml2ZXJzL25ldC9waHkvbXhsLWdweS5jCmluZGV4IGIyZDM2YTNhOTZmMS4uOWI5
N2ZhY2M5YjAzIDEwMDY0NAotLS0gYS9kcml2ZXJzL25ldC9waHkvbXhsLWdweS5jCisrKyBiL2Ry
aXZlcnMvbmV0L3BoeS9teGwtZ3B5LmMKQEAgLTY4MCw2ICs2ODAsNyBAQCBzdGF0aWMgaW50IGdw
eV9zZXRfd29sKHN0cnVjdCBwaHlfZGV2aWNlICpwaHlkZXYsCiAJc3RydWN0IG5ldF9kZXZpY2Ug
KmF0dGFjaF9kZXYgPSBwaHlkZXYtPmF0dGFjaGVkX2RldjsKIAlpbnQgcmV0OwogCisJcGh5ZGV2
LT53b2xvcHRzID0gMDsKIAlpZiAod29sLT53b2xvcHRzICYgV0FLRV9NQUdJQykgewogCQkvKiBN
QUMgYWRkcmVzcyAtIEJ5dGUwOkJ5dGUxOkJ5dGUyOkJ5dGUzOkJ5dGU0OkJ5dGU1CiAJCSAqIFZQ
U1BFQzJfV09MX0FENDUgPSBCeXRlMDpCeXRlMQpAQCAtNzQ4LDkgKzc0OSwxMiBAQCBzdGF0aWMg
aW50IGdweV9zZXRfd29sKHN0cnVjdCBwaHlfZGV2aWNlICpwaHlkZXYsCiAJCWlmIChyZXQgJiAo
UEhZX0lNQVNLX01BU0sgJiB+UEhZX0lNQVNLX0xTVEMpKQogCQkJcGh5X3RyaWdnZXJfbWFjaGlu
ZShwaHlkZXYpOwogCisJCXBoeWRldi0+d29sb3B0cyA9IHdvbC0+d29sb3B0czsKIAkJcmV0dXJu
IDA7CiAJfQogCisJcGh5ZGV2LT53b2xvcHRzID0gd29sLT53b2xvcHRzOworCiAJLyogRGlzYWJs
ZSB0aGUgbGluayBzdGF0ZSBjaGFuZ2UgaW50ZXJydXB0ICovCiAJcmV0dXJuIHBoeV9jbGVhcl9i
aXRzKHBoeWRldiwgUEhZX0lNQVNLLCBQSFlfSU1BU0tfTFNUQyk7CiB9CmRpZmYgLS1naXQgYS9k
cml2ZXJzL25ldC9waHkvcGh5X2RldmljZS5jIGIvZHJpdmVycy9uZXQvcGh5L3BoeV9kZXZpY2Uu
YwppbmRleCA2MTZiZDdiYTQ2Y2IuLjk3NDBmMDhhZDk4ZSAxMDA2NDQKLS0tIGEvZHJpdmVycy9u
ZXQvcGh5L3BoeV9kZXZpY2UuYworKysgYi9kcml2ZXJzL25ldC9waHkvcGh5X2RldmljZS5jCkBA
IC0yMDM4LDYgKzIwMzgsMTEgQEAgaW50IHBoeV9zdXNwZW5kKHN0cnVjdCBwaHlfZGV2aWNlICpw
aHlkZXYpCiAJaWYgKHBoeWRldi0+c3VzcGVuZGVkKQogCQlyZXR1cm4gMDsKIAorCWlmIChwaHlk
ZXYtPndvbG9wdHMpIHsKKwkJd29sLndvbG9wdHMgPSBwaHlkZXYtPndvbG9wdHM7CisJCXBoeV9l
dGh0b29sX3NldF93b2wocGh5ZGV2LCAmd29sKTsKKwl9CisKIAlwaHlfZXRodG9vbF9nZXRfd29s
KHBoeWRldiwgJndvbCk7CiAJcGh5ZGV2LT53b2xfZW5hYmxlZCA9IHdvbC53b2xvcHRzIHx8IChu
ZXRkZXYgJiYgbmV0ZGV2LT53b2xfZW5hYmxlZCk7CiAJLyogSWYgdGhlIGRldmljZSBoYXMgV09M
IGVuYWJsZWQsIHdlIGNhbm5vdCBzdXNwZW5kIHRoZSBQSFkgKi8KZGlmZiAtLWdpdCBhL2luY2x1
ZGUvbGludXgvcGh5LmggYi9pbmNsdWRlL2xpbnV4L3BoeS5oCmluZGV4IDNkZGZlN2ZlNzgxYS4u
Yzg4ZmZhN2FjODliIDEwMDY0NAotLS0gYS9pbmNsdWRlL2xpbnV4L3BoeS5oCisrKyBiL2luY2x1
ZGUvbGludXgvcGh5LmgKQEAgLTc2Niw2ICs3NjYsNyBAQCBzdHJ1Y3QgcGh5X2RldmljZSB7CiAK
IAl1OCBtZGl4OwogCXU4IG1kaXhfY3RybDsKKwl1MzIgd29sb3B0czsKIAogCWludCBwbWFfZXh0
YWJsZTsKIAotLSAKMi4zNC4xCgo=

--_002_LV8PR11MB8700A34520EA2521BC5F59EF9F112LV8PR11MB8700namp_--