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

Hi Russell,

-----Original Message-----
From: Russell King <linux@armlinux.org.uk>
Sent: 2021年9月1日 20:57
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 10:21:59AM +0000, 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

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%7C431c0f2b1b904ad6f1ec08d96d47f399%7C686ea1d3b

c2b4c6f

quoted

a92cd99c5c301635%7C0%7C0%7C637660978077417864%7CUnknown%7CTW
FpbGZsb3d8

quoted

eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
%7C1

quoted

000&sdata=GOmisORcQtC4MvLbOD6s7wlfdMahhoVv6z79xsjeTrI%3D&a
mp;reser

quoted

ved=0

The phylink documentation says:

/**
 * mac_config() - configure the MAC for the selected mode and state
 * @config: a pointer to a &struct phylink_config.
 * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
 * @state: a pointer to a &struct phylink_link_state.
 *
 * Note - not all members of @state are valid.  In particular,
 * @state->lp_advertising, @state->link, @state->an_complete are never
 * guaranteed to be correct, and so any mac_config() implementation must
 * never reference these fields.
...
 * %MLO_AN_FIXED, %MLO_AN_PHY:
...
 *   Older drivers (prior to the mac_link_up() change) may use
@state->speed,
 *   @state->duplex and @state->pause to configure the MAC, but this is
 *   deprecated; such drivers should be converted to use mac_link_up().
 *   Valid state members: interface, advertising.
 *   Deprecated state members: speed, duplex, pause.
...
 * %MLO_AN_INBAND:
...
 *   Valid state members: interface, an_enabled, pause, advertising.

The reason for this is there have _always_ been code paths through phylink
where particularly speed and duplex are _not_ _set_ according to the current
link settings. For example, a call to ksettings_set.
This is why I revised the interface so that mac_link_up() receives the link
settings and depreciated these members in mac_config().

In any case, as can be seen from the documentation, speed and duplex have
_never_ been valid when operating in inband mode in mac_config.

Ok, thanks for your detailed explanation.

quoted

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

@@ -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);

You also need to remove the calls to phylink_mac_change() from the
suspend/resume functions. Without knowing how WoL is configured to work in
your setup, I couldn't comment why it isn't working. Can you give some hints
please?

The wakeup pattern is WAKE_MAGIC.

For WoL active case,
I tried remove phylink_mac_change()=false, add phylink_stop() from suspend patch;
And remove phylink_mac_change()=true, add phylink_start() from resume patch;

System still can't be waked up, I will follow up to debug why system can't be waked up
by magic packets if we call phylink_stop() when suspend. If you have any experience, happy you 
can share with me. The only difference is that whether we call phylink_stop() or not.

Also, what configuration of WoL are you using? I see stmmac supports several
different configurations, but I assume priv->plat->pmt is NULL here?

WoL pattern is WAKE_MAGIC, and priv->plat->pmt is set since we use MAC-based WoL, not PHY-based WoL.

quoted

quoted

phylink_stop() (and therefore phy_stop()) should be called even if
WoL is active to shut down this state reporting, as other network drivers

do.

quoted

Ok, you mean that phylink_stop() also should be called even if WoL is
active, I would look in this direction since you are a professional.

Yes. If the system is suspending for whatever reason, you want to bring the
MAC down so that when it resumes, the MAC will see a link up event
afterwards.

Ok, thanks.

Best Regards,
Joakim Zhang