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

[Christophe ROULLIER]

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

BR

Christophe.

> This is in addition to a more general concern that almost every driver
> in the kernel is likely broken if we need to switch pinmux modes on
> resume to ensure that the "default" pinmux state is restored upon
> resume, which seems to be what you're saying by giving a r-b for this
> patch.