RE: [PATCH] net: stmmac: fix MAC not working when system resume back with WoL enabled
From: Joakim Zhang <hidden>
Date: 2021-09-06 02:29:35
Hi Russell,
-----Original Message----- From: Russell King <linux@armlinux.org.uk> Sent: 2021年9月4日 4:12 To: Joakim Zhang <redacted> Cc: Andrew Lunn <andrew@lunn.ch>; Vladimir Oltean <olteanv@gmail.com>; 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; 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 Hi, Here's a patch to try - you'll need to integrate the new calls into stmmac's suspend and resume hooks. Obviously, given my previous comments, this isn't tested! I didn't need to repeat the mac_wol boolean to phylink_resume as we can record the state internally - mac_wol should not change between a call to phylink_suspend() and subsequent phylink_resume() anyway. mac_wol should only be true if the MAC is involved in processing packets for WoL, false otherwise. Please let me know if this resolves your stmmac WoL issue.
Thanks a lot for your work. 😊 There is a build issue in this patch, could you please have a check? I work on the latest net-next repo.
quoted hunk ↗ jump to hunk
Thanks.diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c indexf0c769027145..c4d0de04416a 100644--- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c@@ -33,6 +33,7 @@ enum { PHYLINK_DISABLE_STOPPED, PHYLINK_DISABLE_LINK, + PHYLINK_DISABLE_MAC_WOL, }; /**@@ -1313,6 +1314,9 @@ EXPORT_SYMBOL_GPL(phylink_start); * network device driver's &struct net_device_ops ndo_stop() method. The * network device's carrier state should not be changed prior to calling this * function. + * + * This will synchronously bring down the link if the link is not + already + * down (in other words, it will trigger a mac_link_down() method + call.) */ void phylink_stop(struct phylink *pl) {@@ -1338,6 +1342,81 @@ void phylink_stop(struct phylink *pl) }EXPORT_SYMBOL_GPL(phylink_stop); + +/** + * phylink_suspend() - handle a network device suspend event + * @pl: a pointer to a &struct phylink returned from phylink_create() + * @mac_wol: true if the MAC needs to receive packets for Wake-on-Lan + * + * Handle a network device suspend event. There are several cases: + * - If Wake-on-Lan is not active, we can bring down the link between + * the MAC and PHY by calling phylink_stop(). + * - If Wake-on-Lan is active, and being handled only by the PHY, we + * can also bring down the link between the MAC and PHY. + * - If Wake-on-Lan is active, but being handled by the MAC, the MAC + * still needs to receive packets, so we can not bring the link down. + */ +void phylink_suspend(struct phylink *pl, bool mac_wol) { + ASSERT_RTNL(); + + if (mac_wol && (!pl->netdev || pl->netdev->wol_enabled)) { + /* Wake-on-Lan enabled, MAC handling */ + mutex_lock(&pl->state_mutex); + + /* Stop the resolver bringing the link up */ + __set_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state); + + /* Disable the carrier, to prevent transmit timeouts, + * but one would hope all packets have been sent. + */ + netif_carrier_off(pl->netdev); + + /* We do not call mac_link_down() here as we want the + * link to remain up to receive the WoL packets. + */ + mutex_unlock(&pl->state_mutex); + } else { + phylink_stop(pl); + } +} +EXPORT_SYMBOL_GPL(phylink_suspend); + +/** + * phylink_resume() - handle a network device resume event + * @pl: a pointer to a &struct phylink returned from phylink_create() + * + * Undo the effects of phylink_suspend(), returning the link to an + * operational state. + */ +void phylink_resume(struct phylink *pl) { + ASSERT_RTNL(); + + if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) { + /* Wake-on-Lan enabled, MAC handling */ + + /* Call mac_link_down() so we keep the overall state balanced. + * Do this under the state_mutex lock for consistency. This + * will cause a "Link Down" message to be printed during + * resume, which is harmless - the true link state will be + * printed when we run a resolve. + */ + mutex_lock(&pl->state_mutex); + phylink_link_down(pl); + mutex_unlock(&pl->state_mutex); + + /* Re-apply the link parameters so that all the settings get + * restored to the MAC. + */ + phylink_mac_initial_config(pl, true); + phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_MAC_WOL);
There is no "phylink_enable_and_run_resolve " sysbol, I guess you want do below operations in this function: clear_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state); phylink_run_resolve(pl);
quoted hunk ↗ jump to hunk
+ } else { + phylink_start(pl); + } +} +EXPORT_SYMBOL_GPL(phylink_resume); + /** * phylink_ethtool_get_wol() - get the wake on lan parameters for the PHY * @pl: a pointer to a &struct phylink returned from phylink_create() diff --git a/include/linux/phylink.h b/include/linux/phylink.h index bdeec800da5c..ba0ab7126b96 100644--- a/include/linux/phylink.h +++ b/include/linux/phylink.h@@ -462,6 +462,9 @@ void phylink_mac_change(struct phylink *, bool up);void phylink_start(struct phylink *); void phylink_stop(struct phylink *); +void phylink_suspend(struct phylink *pl, bool mac_wol); void +phylink_resume(struct phylink *pl); + void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *); int phylink_ethtool_set_wol(struct phylink *, struct ethtool_wolinfo *);
Best Regards, Joakim Zhang