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

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 :

quoted

On Sat, Mar 14, 2026 at 12:37:19AM +0000, Russell King (Oracle) wrote:

quoted

On Sat, Mar 14, 2026 at 12:44:56AM +0100, Linus Walleij wrote:

quoted

On Fri, Mar 13, 2026 at 12:08 PM Russell King (Oracle)
[off-list ref] wrote:

quoted

On Fri, Mar 13, 2026 at 11:57:16AM +0100, Christophe Roullier wrote:

quoted

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!