All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joakim Zhang <qiangqing.zhang@nxp.com>
To: Russell King <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>, Vladimir Oltean <olteanv@gmail.com>,
	"peppe.cavallaro@st.com" <peppe.cavallaro@st.com>,
	"alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
	"joabreu@synopsys.com" <joabreu@synopsys.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"hkallweit1@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
Date: Mon, 6 Sep 2021 02:29:30 +0000	[thread overview]
Message-ID: <DB8PR04MB6795FC58C1D0E2481E2BC35EE6D29@DB8PR04MB6795.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20210903201210.GF1350@shell.armlinux.org.uk>


Hi Russell,

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: 2021年9月4日 4:12
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> 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 <linux-imx@nxp.com>
> 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.

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

> +	} 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

  reply	other threads:[~2021-09-06  2:29 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-01  9:02 Joakim Zhang
2021-09-01  9:13 ` Russell King (Oracle)
2021-09-01 10:21   ` Joakim Zhang
2021-09-01 12:56     ` Russell King (Oracle)
2021-09-02  7:01       ` Joakim Zhang
2021-09-01 15:40     ` Heiner Kallweit
2021-09-02  7:35       ` Joakim Zhang
2021-09-01  9:21 ` Vladimir Oltean
2021-09-01 10:25   ` Joakim Zhang
2021-09-01 10:56     ` Vladimir Oltean
2021-09-01 11:42       ` Joakim Zhang
2021-09-01 13:25         ` Russell King (Oracle)
2021-09-02  7:28           ` Joakim Zhang
2021-09-02  8:32             ` Russell King (Oracle)
2021-09-02 10:26               ` Joakim Zhang
2021-09-02 10:49                 ` Russell King (Oracle)
2021-09-02 11:15                   ` Joakim Zhang
2021-09-02 12:24                     ` Andrew Lunn
2021-09-03  6:51                       ` Joakim Zhang
2021-09-03  8:01                         ` Russell King (Oracle)
2021-09-03  8:39                           ` Joakim Zhang
2021-09-03  9:32                             ` Russell King (Oracle)
2021-09-03 11:04                               ` Joakim Zhang
2021-09-03 12:01                                 ` Russell King (Oracle)
2021-09-03 20:12                                   ` Russell King - ARM Linux admin
2021-09-06  2:29                                     ` Joakim Zhang [this message]
2021-09-06  9:34                                       ` Russell King (Oracle)
2021-09-06 10:41                                         ` Joakim Zhang
2021-09-06 11:21                                           ` Russell King (Oracle)
2021-09-06 13:23                                             ` Andrew Lunn
2021-09-07  8:52                                             ` Russell King (Oracle)
2021-09-06  2:21                                   ` Joakim Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DB8PR04MB6795FC58C1D0E2481E2BC35EE6D29@DB8PR04MB6795.eurprd04.prod.outlook.com \
    --to=qiangqing.zhang@nxp.com \
    --cc=alexandre.torgue@foss.st.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=joabreu@synopsys.com \
    --cc=kuba@kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux@armlinux.org.uk \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=peppe.cavallaro@st.com \
    --subject='RE: [PATCH] net: stmmac: fix MAC not working when system resume back with WoL enabled' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.