[PATCH V2 0/2] fix mac not working after system resumed with WoL enabled

[PATCH V2 0/2] fix mac not working after system resumed with WoL enabled

[Clark Wang]

Hi,
The issue description is in the commit message.

This patchset is the second version following discussions with Russell.
But the name of each patch has changed, so V2 is not marked in each patch.

Thanks.

Clark Wang (2):
  net: phylink: add a function to resume phy alone to fix resume issue
    with WoL enabled
  net: stmmac: resume phy separately before calling stmmac_hw_setup()

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 16 +++++++-------
 drivers/net/phy/phylink.c                     | 21 ++++++++++++++++++-
 include/linux/phylink.h                       |  1 +
 3 files changed, 28 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled

[Clark Wang]

Issue we met:
On some platforms, mac cannot work after resumed from the suspend with WoL
enabled.

The cause of the issue:
1. phylink_resolve() is in a workqueue which will not be executed immediately.
   This is the call sequence:
       phylink_resolve()->phylink_link_up()->pl->mac_ops->mac_link_up()
   For stmmac driver, mac_link_up() will set the correct speed/duplex...
   values which are from link_state.
2. In stmmac_resume(), it will call stmmac_hw_setup() after called the
   phylink_resume(), because mac need phy rx_clk to do the reset.
   stmmac_core_init() is called in function stmmac_hw_setup(), which will
   reset the mac and set the speed/duplex... to default value.
Conclusion: Because phylink_resolve() cannot determine when it is called, it
            cannot be guaranteed to be called after stmmac_core_init().
	    Once stmmac_core_init() is called after phylink_resolve(),
	    the mac will be misconfigured and cannot be used.

In order to avoid this problem, add a function called phylink_phy_resume()
to resume phy separately. This eliminates the need to call phylink_resume()
before stmmac_hw_setup().

Add another judgement before called phy_start() in phylink_start(). This way
phy_start() will not be called multiple times when resumes. At the same time,
it may not affect other drivers that do not use phylink_phy_resume().

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/net/phy/phylink.c | 21 ++++++++++++++++++++-
 include/linux/phylink.h   |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 09cc65c0da93..5bab59142579 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1939,7 +1939,7 @@ void phylink_start(struct phylink *pl)
 	}
 	if (poll)
 		mod_timer(&pl->link_poll, jiffies + HZ);
-	if (pl->phydev)
+	if (pl->phydev && pl->phydev->state < PHY_UP)
 		phy_start(pl->phydev);
 	if (pl->sfp_bus)
 		sfp_upstream_start(pl->sfp_bus);
@@ -2020,6 +2020,25 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
 }
 EXPORT_SYMBOL_GPL(phylink_suspend);
 
+/**
+ * phylink_phy_resume() - resume phy alone
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * In the MAC driver using phylink, if the MAC needs the clock of the phy
+ * when it resumes, can call this function to resume the phy separately.
+ * Then proceed to MAC resume operations.
+ */
+void phylink_phy_resume(struct phylink *pl)
+{
+	ASSERT_RTNL();
+
+	if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)
+	    && pl->phydev)
+		phy_start(pl->phydev);
+
+}
+EXPORT_SYMBOL_GPL(phylink_phy_resume);
+
 /**
  * phylink_resume() - handle a network device resume event
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..6edfab5f754c 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -589,6 +589,7 @@ void phylink_stop(struct phylink *);
 
 void phylink_suspend(struct phylink *pl, bool mac_wol);
 void phylink_resume(struct phylink *pl);
+void phylink_phy_resume(struct phylink *pl);
 
 void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *);
 int phylink_ethtool_set_wol(struct phylink *, struct ethtool_wolinfo *);
-- 
2.34.1

[PATCH 2/2] net: stmmac: resume phy separately before calling stmmac_hw_setup()

[Clark Wang]

On some platforms, mac cannot work after resumed from the suspend with WoL
enabled.

We found the stmmac_hw_setup() when system resumes will called after the
stmmac_mac_link_up(). So the correct values set in stmmac_mac_link_up()
are overwritten by stmmac_core_init() in phylink_resume().

So call the new added function in phylink to resume phy fristly.
Then can call the stmmac_hw_setup() before calling phy_resume().

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c    | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index c6951c976f5d..d0bdc9b6dbe8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7532,16 +7532,9 @@ int stmmac_resume(struct device *dev)
 	}
 
 	rtnl_lock();
-	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
-		phylink_resume(priv->phylink);
-	} else {
-		phylink_resume(priv->phylink);
-		if (device_may_wakeup(priv->device))
-			phylink_speed_up(priv->phylink);
-	}
-	rtnl_unlock();
 
-	rtnl_lock();
+	phylink_phy_resume(priv->phylink);
+
 	mutex_lock(&priv->lock);
 
 	stmmac_reset_queues_param(priv);
@@ -7559,6 +7552,11 @@ int stmmac_resume(struct device *dev)
 	stmmac_enable_all_dma_irq(priv);
 
 	mutex_unlock(&priv->lock);
+
+	phylink_resume(priv->phylink);
+	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
+		phylink_speed_up(priv->phylink);
+
 	rtnl_unlock();
 
 	netif_device_attach(ndev);
-- 
2.34.1

Re: [PATCH V2 0/2] fix mac not working after system resumed with WoL enabled

[Piotr Raczynski]

On Wed, Dec 21, 2022 at 04:01:42PM +0800, Clark Wang wrote:
> Hi,
> The issue description is in the commit message.
> 
> This patchset is the second version following discussions with Russell.
> But the name of each patch has changed, so V2 is not marked in each patch.
> 
> Thanks.
> 

Please include target tree name in the subject eg. [net 1/2]

> Clark Wang (2):
>   net: phylink: add a function to resume phy alone to fix resume issue
>     with WoL enabled
>   net: stmmac: resume phy separately before calling stmmac_hw_setup()
> 
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 16 +++++++-------
>  drivers/net/phy/phylink.c                     | 21 ++++++++++++++++++-
>  include/linux/phylink.h                       |  1 +
>  3 files changed, 28 insertions(+), 10 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] net: stmmac: resume phy separately before calling stmmac_hw_setup()

[Piotr Raczynski]

On Wed, Dec 21, 2022 at 04:01:44PM +0800, Clark Wang wrote:
> On some platforms, mac cannot work after resumed from the suspend with WoL
> enabled.
> 
> We found the stmmac_hw_setup() when system resumes will called after the
> stmmac_mac_link_up(). So the correct values set in stmmac_mac_link_up()
> are overwritten by stmmac_core_init() in phylink_resume().
> 
> So call the new added function in phylink to resume phy fristly.
							  first.
> Then can call the stmmac_hw_setup() before calling phy_resume().

It'd be nice to add Fixes tag with appropriate commit id.
Other than that looks fine.

Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>

> 
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c    | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index c6951c976f5d..d0bdc9b6dbe8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7532,16 +7532,9 @@ int stmmac_resume(struct device *dev)
>  	}
>  
>  	rtnl_lock();
> -	if (device_may_wakeup(priv->device) && priv->plat->pmt) {
> -		phylink_resume(priv->phylink);
> -	} else {
> -		phylink_resume(priv->phylink);
> -		if (device_may_wakeup(priv->device))
> -			phylink_speed_up(priv->phylink);
> -	}
> -	rtnl_unlock();
>  
> -	rtnl_lock();
> +	phylink_phy_resume(priv->phylink);
> +
>  	mutex_lock(&priv->lock);
>  
>  	stmmac_reset_queues_param(priv);
> @@ -7559,6 +7552,11 @@ int stmmac_resume(struct device *dev)
>  	stmmac_enable_all_dma_irq(priv);
>  
>  	mutex_unlock(&priv->lock);
> +
> +	phylink_resume(priv->phylink);
> +	if (device_may_wakeup(priv->device) && !priv->plat->pmt)
> +		phylink_speed_up(priv->phylink);
> +
>  	rtnl_unlock();
>  
>  	netif_device_attach(ndev);
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled

[Piotr Raczynski]

On Wed, Dec 21, 2022 at 04:01:43PM +0800, Clark Wang wrote:
> Issue we met:
> On some platforms, mac cannot work after resumed from the suspend with WoL
> enabled.
> 
> The cause of the issue:
> 1. phylink_resolve() is in a workqueue which will not be executed immediately.
>    This is the call sequence:
>        phylink_resolve()->phylink_link_up()->pl->mac_ops->mac_link_up()
>    For stmmac driver, mac_link_up() will set the correct speed/duplex...
>    values which are from link_state.
> 2. In stmmac_resume(), it will call stmmac_hw_setup() after called the
>    phylink_resume(), because mac need phy rx_clk to do the reset.
>    stmmac_core_init() is called in function stmmac_hw_setup(), which will
>    reset the mac and set the speed/duplex... to default value.
> Conclusion: Because phylink_resolve() cannot determine when it is called, it
>             cannot be guaranteed to be called after stmmac_core_init().
> 	    Once stmmac_core_init() is called after phylink_resolve(),
> 	    the mac will be misconfigured and cannot be used.
> 
> In order to avoid this problem, add a function called phylink_phy_resume()
> to resume phy separately. This eliminates the need to call phylink_resume()
> before stmmac_hw_setup().
> 
> Add another judgement before called phy_start() in phylink_start(). This way
> phy_start() will not be called multiple times when resumes. At the same time,
> it may not affect other drivers that do not use phylink_phy_resume().
> 
It'd be nice to see Fixes tag.

> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> ---
>  drivers/net/phy/phylink.c | 21 ++++++++++++++++++++-
>  include/linux/phylink.h   |  1 +
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 09cc65c0da93..5bab59142579 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1939,7 +1939,7 @@ void phylink_start(struct phylink *pl)
>  	}
>  	if (poll)
>  		mod_timer(&pl->link_poll, jiffies + HZ);
> -	if (pl->phydev)
> +	if (pl->phydev && pl->phydev->state < PHY_UP)
>  		phy_start(pl->phydev);
>  	if (pl->sfp_bus)
>  		sfp_upstream_start(pl->sfp_bus);
> @@ -2020,6 +2020,25 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
>  }
>  EXPORT_SYMBOL_GPL(phylink_suspend);
>  
> +/**
> + * phylink_phy_resume() - resume phy alone
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + *
> + * In the MAC driver using phylink, if the MAC needs the clock of the phy
> + * when it resumes, can call this function to resume the phy separately.
> + * Then proceed to MAC resume operations.
> + */
> +void phylink_phy_resume(struct phylink *pl)
> +{
> +	ASSERT_RTNL();
> +
> +	if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)
> +	    && pl->phydev)
you can fit && at the end of the previous line.
> +		phy_start(pl->phydev);
> +
this blank line is not necessary.
> +}
> +EXPORT_SYMBOL_GPL(phylink_phy_resume);
> +
>  /**
>   * phylink_resume() - handle a network device resume event
>   * @pl: a pointer to a &struct phylink returned from phylink_create()
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index c492c26202b5..6edfab5f754c 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -589,6 +589,7 @@ void phylink_stop(struct phylink *);
>  
>  void phylink_suspend(struct phylink *pl, bool mac_wol);
>  void phylink_resume(struct phylink *pl);
> +void phylink_phy_resume(struct phylink *pl);
>  
>  void phylink_ethtool_get_wol(struct phylink *, struct ethtool_wolinfo *);
>  int phylink_ethtool_set_wol(struct phylink *, struct ethtool_wolinfo *);
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled

[Russell King (Oracle)]

On Wed, Dec 21, 2022 at 04:01:43PM +0800, Clark Wang wrote:
> Issue we met:
> On some platforms, mac cannot work after resumed from the suspend with WoL
> enabled.
> 
> The cause of the issue:
> 1. phylink_resolve() is in a workqueue which will not be executed immediately.
>    This is the call sequence:
>        phylink_resolve()->phylink_link_up()->pl->mac_ops->mac_link_up()
>    For stmmac driver, mac_link_up() will set the correct speed/duplex...
>    values which are from link_state.
> 2. In stmmac_resume(), it will call stmmac_hw_setup() after called the
>    phylink_resume(), because mac need phy rx_clk to do the reset.
>    stmmac_core_init() is called in function stmmac_hw_setup(), which will
>    reset the mac and set the speed/duplex... to default value.
> Conclusion: Because phylink_resolve() cannot determine when it is called, it
>             cannot be guaranteed to be called after stmmac_core_init().
> 	    Once stmmac_core_init() is called after phylink_resolve(),
> 	    the mac will be misconfigured and cannot be used.
> 
> In order to avoid this problem, add a function called phylink_phy_resume()
> to resume phy separately. This eliminates the need to call phylink_resume()
> before stmmac_hw_setup().
> 
> Add another judgement before called phy_start() in phylink_start(). This way
> phy_start() will not be called multiple times when resumes. At the same time,
> it may not affect other drivers that do not use phylink_phy_resume().
> 
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> ---
>  drivers/net/phy/phylink.c | 21 ++++++++++++++++++++-
>  include/linux/phylink.h   |  1 +
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 09cc65c0da93..5bab59142579 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1939,7 +1939,7 @@ void phylink_start(struct phylink *pl)
>  	}
>  	if (poll)
>  		mod_timer(&pl->link_poll, jiffies + HZ);
> -	if (pl->phydev)
> +	if (pl->phydev && pl->phydev->state < PHY_UP)

I'm really not happy with this - not only does this subvert the checks in
phy_start(), it's a layering violation, and it delves into internals of
phylib in an unprotected way.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!