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

Hi Vladimir,

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com>
Sent: 2021年9月1日 17:22
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; linux@armlinux.org.uk;
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.

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.

@@ -5374,7 +5374,6 @@ int stmmac_suspend(struct device *dev)
                rtnl_lock();
                if (device_may_wakeup(priv->device))
                        phylink_speed_down(priv->phylink, false);
-               phylink_stop(priv->phylink);
                rtnl_unlock();
                mutex_lock(&priv->lock);

@@ -5385,6 +5384,10 @@ int stmmac_suspend(struct device *dev)
        }
        mutex_unlock(&priv->lock);

+       rtnl_lock();
+       phylink_stop(priv->phylink);
+       rtnl_unlock();
+
        priv->speed = SPEED_UNKNOWN;
        return 0;
 }

@@ -5448,6 +5451,12 @@ int stmmac_resume(struct device *dev)
                pinctrl_pm_select_default_state(priv->device);
                if (priv->plat->clk_ptp_ref)
                        clk_prepare_enable(priv->plat->clk_ptp_ref);
+
+               rtnl_lock();
+               /* We may have called phylink_speed_down before */
+               phylink_speed_up(priv->phylink);
+               rtnl_unlock();
+
                /* reset the phy so that it's ready */
                if (priv->mii && priv->mdio_rst_after_resume)
                        stmmac_mdio_reset(priv->mii);

@@ -5461,13 +5470,9 @@ int stmmac_resume(struct device *dev)
                        return ret;
        }

-       if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
-               rtnl_lock();
-               phylink_start(priv->phylink);
-               /* We may have called phylink_speed_down before */
-               phylink_speed_up(priv->phylink);
-               rtnl_unlock();
-       }
+       rtnl_lock();
+       phylink_start(priv->phylink);
+       rtnl_unlock();

        rtnl_lock();
        mutex_lock(&priv->lock);

quoted

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.

Fixes: 46f69ded988d ("net: stmmac: Use resolved link config in
mac_link_up()")
Signed-off-by: Joakim Zhang <redacted>
---

Broken patch cannot be reverted directly, so manually modified it.

I also tried to fix in other ways, but failed to find a better
solution, any suggestions would be appreciated. Thanks.

Joakim

Do you know exactly why it used to work prior to this patch?

Yes, since it configures the MAC_CTRL_REG register in .mac_config callback,
it will be called when system resume back with WoL enabled. 
https://elixir.bootlin.com/linux/v5.4.143/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L852 

If configure the MAC_CTRL_REG register in .mac_link_up callback, when system resume back with WoL active,
.mac_link_up would not be called, so MAC can't work any longer.
https://elixir.bootlin.com/linux/v5.14-rc7/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L1044

Thanks.
 
Best Regards,
Joakim Zhang