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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Sep 01, 2021 at 11:42:08AM +0000, Joakim Zhang wrote:
> Hi Vladimir,
> 
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Sent: 2021年9月1日 18:56
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: peppe.cavallaro@st.com; alexandre.torgue@foss.st.com;
> > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org;
> > mcoquelin.stm32@gmail.com; linux@armlinux.org.uk;
> > netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com;
> > hkallweit1@gmail.com; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH] net: stmmac: fix MAC not working when system resume
> > back with WoL enabled
> > 
> > On Wed, Sep 01, 2021 at 10:25:15AM +0000, Joakim Zhang wrote:
> > >
> > > Hi Vladimir,
> > >
> > > > -----Original Message-----
> > > > From: Vladimir Oltean <olteanv@gmail.com>
> > > > Sent: 2021年9月1日 17:22
> > > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > Cc: peppe.cavallaro@st.com; alexandre.torgue@foss.st.com;
> > > > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org;
> > > > mcoquelin.stm32@gmail.com; linux@armlinux.org.uk;
> > > > netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com;
> > > > hkallweit1@gmail.com; dl-linux-imx <linux-imx@nxp.com>
> > > > 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:
> > > > > 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.
> > > >
> > > > Have you tried putting phylink_stop in .suspend, and phylink_start
> > in .resume?
> > >
> > > Yes, I tried, but the system can't be wakeup with remote packets.
> > > Please see the code change.
> > 
> > That makes it a PHY driver issue then, I guess?
> > At least some PHY drivers avoid suspending when WoL is active, like
> > lan88xx_suspend.
> > Even the phy_suspend function takes wol.wolopts into consideration before
> > proceeding to call the driver. What PHY driver is it?
> 
> I think it's not the PHY issue, since both STMMAC and FEC controllers on i.MX8MP use the same
> PHY(Realtek RTL8211FD, drivers/net/phy/realtek.c), there is no issue with FEC.

Note that FEC calls phylink_stop() in fec_suspend() if the net device
was up. So that kind of rules out phylink and phylib too... and
points towards stmmac doing something it shouldn't.

> > Bad assumption in the stmmac driver, if the intention was for the link state
> > change to be induced to phylink after the resume?
> 
> Yes, I also think link state change should be captured after the resume, it's very strange that
> link up again before suspended. You would see below log if I add no_console_suspend in cmdline.

... because phylink_mac_change() is not supposed to be used to force
the link down. I can't say this loudly enough: Read the documentation.
I don't write it just for my pleasure, it's there to help others get
stuff correct. If people aren't going to read it, I might as well not
waste the time writing it.

 * phylink_mac_change() - notify phylink of a change in MAC state
 * @pl: a pointer to a &struct phylink returned from phylink_create()
 * @up: indicates whether the link is currently up.
 *
 * The MAC driver should call this driver when the state of its link
 * changes (eg, link failure, new negotiation results, etc.)

Realise that "up" is there merely to capture that the link has gone
down - but by the time phylink reacts to that (which may be some time
*after* this call has been made - it is *not* synchronous since it's
meant to be called from an *interrupt*) the link state may well have
changed again. So, phylink will always recheck the link state with
up = false, so you _will_ get the link going down and then up.

In any case "change in MAC state" is only applicable when in in-band
mode, not in PHY mode, so you should not be calling this if you have
a PHY attached which isn't in in-band mode.

> 
> root@imx8mpevk:~# ethtool -s eth1 wol g
> [   76.309460] stmmac: wakeup enable

So you've asked it to wake on MagicPacket, which is WAKE_MAGIC. As
you got the message "wakeup enable" which is emitted by
stmmac_set_wol(), this will only be emitted if priv->plat->pmt() is
set. It will _not_ call phylink_ethtool_set_wol().

So, you are not using the PHY-based wake-on-lan, but the MAC based
wake-on-lan. This means you need to have the phy <-> mac link up
during suspend, and in that case, yes, you do not want to call
phylink_stop() or phylink_start().

I'm not sure what stmmac_pmt() does - thanks to the macro stuff, I'd
need to trace it through the driver and find out where that goes,
and also which variant of stmmac you're using... so without more
information I can't follow what the driver is doing.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!