Thread (32 messages) flat view 32 messages, 5 authors, 2021-09-07

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 index
f0c769027145..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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help