Re: [PATCH net] net: stmmac: fix system hang caused by eee_ctrl_timer during suspend/resume

[Vladimir Oltean <olteanv@gmail.com>]

Hi Joakim,

On Wed, Sep 08, 2021 at 03:43:35PM +0800, Joakim Zhang wrote:
> commit 5f58591323bf ("net: stmmac: delete the eee_ctrl_timer after
> napi disabled"), this patch tries to fix system hang caused by eee_ctrl_timer,
> unfortunately, it only can resolve it for system reboot stress test. System
> hang also can be reproduced easily during system suspend/resume stess test
> when mount NFS on i.MX8MP EVK board.
> 
> In stmmac driver, eee feature is combined to phylink framework. When do
> system suspend, phylink_stop() would queue delayed work, it invokes
> stmmac_mac_link_down(), where to deactivate eee_ctrl_timer synchronizly.
> In above commit, try to fix issue by deactivating eee_ctrl_timer obviously,
> but it is not enough. Looking into eee_ctrl_timer expire callback
> stmmac_eee_ctrl_timer(), it could enable hareware eee mode again. What is
> unexpected is that LPI interrupt (MAC_Interrupt_Enable.LPIEN bit) is always
> asserted. This interrupt has chance to be issued when LPI state entry/exit
> from the MAC, and at that time, clock could have been already disabled.
> The result is that system hang when driver try to touch register from
> interrupt handler.
> 
> The reason why above commit can fix system hang issue in stmmac_release()
> is that, deactivate eee_ctrl_timer not just after napi disabled, further
> after irq freed.
> 
> In conclusion, hardware would generate LPI interrupt when clock has been
> disabled during suspend or resume, since hardware is in eee mode and LPI
> interrupt enabled.
> 
> Interrupts from MAC, MTL and DMA level are enabled and never been disabled
> when system suspend, so postpone clocks management from suspend stage to
> noirq suspend stage should be more safe.
> 
> Fixes: 5f58591323bf ("net: stmmac: delete the eee_ctrl_timer after napi disabled")
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 14 ------
>  .../ethernet/stmicro/stmmac/stmmac_platform.c | 44 +++++++++++++++++++
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ece02b35a6ce..246f84fedbc8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7118,7 +7118,6 @@ int stmmac_suspend(struct device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  	u32 chan;
> -	int ret;
>  
>  	if (!ndev || !netif_running(ndev))
>  		return 0;
> @@ -7150,13 +7149,6 @@ int stmmac_suspend(struct device *dev)
>  	} else {
>  		stmmac_mac_set(priv, priv->ioaddr, false);
>  		pinctrl_pm_select_sleep_state(priv->device);
> -		/* Disable clock in case of PWM is off */
> -		clk_disable_unprepare(priv->plat->clk_ptp_ref);

This patch looks strange to me. You are basically saying that the LPI
timer for MAC-level EEE is clocked from the clk_ptp_ref clock?! Are you
sure this is correct? I thought this clock is only used for the PTP
timestamping counter. Maybe the clock definitions in imx8mp.dtsi are not
correct?

> -		ret = pm_runtime_force_suspend(dev);
> -		if (ret) {
> -			mutex_unlock(&priv->lock);
> -			return ret;
> -		}
>  	}
>  
>  	mutex_unlock(&priv->lock);
> @@ -7242,12 +7234,6 @@ int stmmac_resume(struct device *dev)
>  		priv->irq_wake = 0;
>  	} else {
>  		pinctrl_pm_select_default_state(priv->device);
> -		/* enable the clk previously disabled */
> -		ret = pm_runtime_force_resume(dev);
> -		if (ret)
> -			return ret;
> -		if (priv->plat->clk_ptp_ref)
> -			clk_prepare_enable(priv->plat->clk_ptp_ref);
>  		/* reset the phy so that it's ready */
>  		if (priv->mii)
>  			stmmac_mdio_reset(priv->mii);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 5ca710844cc1..4885f9ad1b1e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -9,6 +9,7 @@
>  *******************************************************************************/
>  
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/module.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> @@ -771,9 +772,52 @@ static int __maybe_unused stmmac_runtime_resume(struct device *dev)
>  	return stmmac_bus_clks_config(priv, true);
>  }
>  
> +static int stmmac_pltfr_noirq_suspend(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
> +		/* Disable clock in case of PWM is off */
> +		clk_disable_unprepare(priv->plat->clk_ptp_ref);
> +
> +		ret = pm_runtime_force_suspend(dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int stmmac_pltfr_noirq_resume(struct device *dev)
> +{
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int ret;
> +
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
> +		/* enable the clk previously disabled */
> +		ret = pm_runtime_force_resume(dev);
> +		if (ret)
> +			return ret;
> +
> +		clk_prepare_enable(priv->plat->clk_ptp_ref);
> +	}
> +
> +	return 0;
> +}
> +
>  const struct dev_pm_ops stmmac_pltfr_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(stmmac_pltfr_suspend, stmmac_pltfr_resume)
>  	SET_RUNTIME_PM_OPS(stmmac_runtime_suspend, stmmac_runtime_resume, NULL)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(stmmac_pltfr_noirq_suspend, stmmac_pltfr_noirq_resume)
>  };
>  EXPORT_SYMBOL_GPL(stmmac_pltfr_pm_ops);
>  
> -- 
> 2.17.1
>