Re: [PATCH v2 1/2] net: stmmac: fix pinctrl management during suspend/resume

[Russell King (Oracle)]

On Mon, Mar 16, 2026 at 11:01:02AM +0100, Christophe ROULLIER wrote:
> Hi Russell, Linus, All,
> 
> Le 16/03/2026 à 10:03, Russell King (Oracle) a écrit :
> > On Sat, Mar 14, 2026 at 12:37:19AM +0000, Russell King (Oracle) wrote:
> > > On Sat, Mar 14, 2026 at 12:44:56AM +0100, Linus Walleij wrote:
> > > > On Fri, Mar 13, 2026 at 12:08 PM Russell King (Oracle)
> > > > <linux@xxxxxxxxxxxxxxx> wrote:
> > > > > On Fri, Mar 13, 2026 at 11:57:16AM +0100, Christophe Roullier wrote:
> > > > > > In the deepest low-power modes, the pinctrl configuration is lost
> > > > > > and is never restored if the interface is down.
> > > > > > This commit ensures that the pinctrl state is set in all cases.
> > > > > Shouldn't the pin state be restored by the pinctrl layer?
> > > > What we have in the device core only applies "init" and "default"
> > > > states, and provides these handles for transitioning to "sleep"
> > > > and "default" again (like a state machine).
> > > What I was meaning is that - for a driver using the "default" state,
> > > if the hardware loses the pinctrl state during sleep, isn't it the
> > > responsibility of the pinctrl driver to restore the state rather
> > > than leaving it in whatever states it happens to be when the SoC
> > > comes back from suspend?
> > > 
> > > If that is not the case, then don't we have a major issue where
> > > drivers using pinctrl but do not issue any pinctrl calls in the
> > > resume function are buggy?
> > I would like an answer on this before this patch is merged, because
> > even with your reviewed-by, I don't think this patch is correct.
> > 
> > For example, if pinctrl loses the pinmux state across suspend/resume,
> > then this patch only solves the case where the NIC is down when
> > suspending.
> > 
> > It does not address the case where the NIC is up but WoL is disabled.
> > Also, what happens when WoL is enabled at the MAC, when we expect the
> > NIC to still be functional - which means that the pinmux state must
> > remain active over suspend.
> 
> For me this case (when NIC is up) is already managed by the driver and
> function suspend/resume:
> 
> On stmmac_suspend :
> 
> ==>      /* Enable Power down mode by programming the PMT regs */
>      if (priv->wolopts) {
>          stmmac_pmt(priv, priv->hw, priv->wolopts);
>          priv->irq_wake = 1;
>      } else {
>          stmmac_mac_set(priv, priv->ioaddr, false);
> *         pinctrl_pm_select_sleep_state(priv->device);*
>      }
> 
> On stmmac_resume :
> 
> ==>     if (priv->wolopts) {
>          mutex_lock(&priv->lock);
>          stmmac_pmt(priv, priv->hw, 0);
>          mutex_unlock(&priv->lock);
>          priv->irq_wake = 0;
>      } else {
> *         pinctrl_pm_select_default_state(priv->device);*
>          /* reset the phy so that it's ready */
>          if (priv->mii)
>              stmmac_mdio_reset(priv->mii);
>      }
> 

I don't know if there's a difference between your two seperate replies
that appear to be saying the same thing.

I also didn't realise that we're already changing the pinctrl state
on suspend/resume when the interface is up - thanks for pointing that
out.

I think it may be better to move the pinctrl changes after the
priv->plat->suspend() call:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 428b2e5bb4c4..596f9e2349cb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -8174,6 +8174,7 @@ int stmmac_suspend(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	u8 chan;
+	int ret;
 
 	if (!ndev || !netif_running(ndev))
 		goto suspend_bsp;
@@ -8203,7 +8204,6 @@ int stmmac_suspend(struct device *dev)
 		priv->irq_wake = 1;
 	} else {
 		stmmac_mac_set(priv, priv->ioaddr, false);
-		pinctrl_pm_select_sleep_state(priv->device);
 	}
 
 	mutex_unlock(&priv->lock);
@@ -8225,8 +8225,14 @@ int stmmac_suspend(struct device *dev)
 		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
 
 suspend_bsp:
-	if (priv->plat->suspend)
-		return priv->plat->suspend(dev, priv->plat->bsp_priv);
+	if (priv->plat->suspend) {
+		ret = priv->plat->suspend(dev, priv->plat->bsp_priv);
+		if (ret)
+			return ret;
+	}
+
+	if (!priv->wolopts)
+		pinctrl_pm_select_sleep_state(priv->device);
 
 	return 0;
 }
@@ -8280,6 +8286,9 @@ int stmmac_resume(struct device *dev)
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	int ret;
 
+	if (!priv->wolopts)
+		pinctrl_pm_select_default_state(priv->device);
+
 	if (priv->plat->resume) {
 		ret = priv->plat->resume(dev, priv->plat->bsp_priv);
 		if (ret)
@@ -8301,7 +8310,6 @@ int stmmac_resume(struct device *dev)
 		mutex_unlock(&priv->lock);
 		priv->irq_wake = 0;
 	} else {
-		pinctrl_pm_select_default_state(priv->device);
 		/* reset the phy so that it's ready */
 		if (priv->mii)
 			stmmac_mdio_reset(priv->mii);

This means that we don't switch the pinctrl state until we've finished
suspending the device as necessary, which seems way more sane than
trying to do it part-way through suspending - for example, while
phylink and phylib is still active.

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