RE: [PATCH] net: stmmac: fix MAC not working when system resume back with WoL enabled

Hi Heiner,

-----Original Message-----
From: Heiner Kallweit <hkallweit1@gmail.com>
Sent: 2021年9月1日 23:40
To: Joakim Zhang <redacted>; Russell King
[off-list ref]
Cc: peppe.cavallaro@st.com; alexandre.torgue@foss.st.com;
joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org;
mcoquelin.stm32@gmail.com; netdev@vger.kernel.org; andrew@lunn.ch;
f.fainelli@gmail.com; dl-linux-imx [off-list ref]
Subject: Re: [PATCH] net: stmmac: fix MAC not working when system resume
back with WoL enabled

On 01.09.2021 12:21, Joakim Zhang wrote:

quoted

Hi Russell,

quoted

-----Original Message-----
From: Russell King <linux@armlinux.org.uk>
Sent: 2021年9月1日 17:14
To: Joakim Zhang <redacted>
Cc: peppe.cavallaro@st.com; alexandre.torgue@foss.st.com;
joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org;
mcoquelin.stm32@gmail.com; netdev@vger.kernel.org; andrew@lunn.ch;
f.fainelli@gmail.com; hkallweit1@gmail.com; dl-linux-imx
[off-list ref]
Subject: Re: [PATCH] net: stmmac: fix MAC not working when system
resume back with WoL enabled

On Wed, Sep 01, 2021 at 05:02:28PM +0800, Joakim Zhang wrote:

quoted

We can reproduce this issue with below steps:
1) enable WoL on the host
2) host system suspended
3) remote client send out wakeup packets We can see that host system
resume back, but can't work, such as ping failed.

After a bit digging, this issue is introduced by the commit
46f69ded988d
("net: stmmac: Use resolved link config in mac_link_up()"), which
use the finalised link parameters in mac_link_up() rather than the
parameters in mac_config().

There are two scenarios for MAC suspend/resume:

1) MAC suspend with WoL disabled, stmmac_suspend() call
phylink_mac_change() to notify phylink machine that a change in MAC
state, then .mac_link_down callback would be invoked. Further, it
will call phylink_stop() to stop the phylink instance. When MAC
resume back, firstly phylink_start() is called to start the phylink
instance, then call phylink_mac_change() which will finally trigger
phylink machine to invoke .mac_config and .mac_link_up callback. All
is fine since configuration in these two callbacks will be initialized.

2) MAC suspend with WoL enabled, phylink_mac_change() will put link
down, but there is no phylink_stop() to stop the phylink instance,
so it will link up again, that means .mac_config and .mac_link_up
would be invoked before system suspended. After system resume back,
it will do DMA initialization and SW reset which let MAC lost the
hardware setting (i.e MAC_Configuration register(offset 0x0) is
reset). Since link is up before system suspended, so .mac_link_up
would not be invoked after system resume back, lead to there is no
chance to initialize the configuration in .mac_link_up callback, as
a result, MAC can't work any longer.

Above description is what I found when debug this issue, this patch
is just revert broken patch to workaround it, at least make MAC work
when system resume back with WoL enabled.

Said this is a workaround, since it has not resolve the issue completely.
I just move the speed/duplex/pause etc into .mac_config callback,
there are other configurations in .mac_link_up callback which also
need to be initialized to work for specific functions.

NAK. Please read the phylink documentation. speed/duplex/pause is
undefined in .mac_config.

Speed/duplex/pause also the field of " struct phylink_link_state", so
these can be refered in .mac_config, please see the link which stmmac did

before:

quoted

https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix

ir.bootlin.com%2Flinux%2Fv5.4.143%2Fsource%2Fdrivers%2Fnet%2Fethernet%

quoted

2Fstmicro%2Fstmmac%2Fstmmac_main.c%23L852&data=04%7C01%7Cq
iangqing

quoted

.zhang%40nxp.com%7C83fda179bf1d41fca42008d96d5ed3c3%7C686ea1d3b

c2b4c6f

quoted

a92cd99c5c301635%7C0%7C0%7C637661076316192906%7CUnknown%7CTW
FpbGZsb3d8

quoted

eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
%7C1

quoted

000&sdata=zM%2Fw1R%2BHkY067pg8wm2%2FS0zsLoBNQQ2TmdXWJat
Ptes%3D&amp

quoted

;reserved=0

quoted

I think the problem here is that you're not calling phylink_stop()
when WoL is enabled, which means phylink will continue to maintain
the state as per the hardware state, and phylib will continue to run
its state machine reporting the link state to phylink.

Yes, I also tried do below code change, but the host would not be
wakeup, phylink_stop() would call phy_stop(), phylib would call
phy_suspend() finally, it will not suspend phy if it detect WoL enabled, so now

I don't know why system can't be wakeup with this code change.

quoted

Follow-up question would be whether link breaks accidentally on suspend, or
whether something fails on resume.When suspending, does the link break and
link LEDs go off?

No, the LEDs is normal.

Depending on LED configuration you may also see whether link speed is
reduced on suspend.
struct net_device has a member wol_enabled, does it make a difference if set
it?

I have a try, there is no help. Have not clear why phylink_stop() will break WoL function,
lead it can't be waked up via magic packets.

Thanks.

Best Regards,
Joakim Zhang