Re: [PATCH net] net: stmmac: Select sleep pin state after suspending phylink

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 7/30/26 00:45, Fabio Estevam wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
> 
> When MAC Wake-on-LAN is disabled, stmmac_suspend() selects the sleep
> pinctrl state before calling phylink_suspend().
> 
> phylink_suspend() may access PHY registers over MDIO while stopping or
> reconfiguring the PHY. If selecting the sleep state muxes the MDC and
> MDIO pins away from the MDIO bus function, these accesses no longer work.
> 
> Select the sleep pin state only after phylink_suspend() has completed.
> 
> Keep the transition conditional on MAC Wake-on-LAN being disabled so
> that the pins remain active when the MAC must receive wake-up packets.
I think we need that fix, but it does raise even more questions:

How about when the PHY handles WoL ? We may have to access it early at wakeup,
possibly even before the MAC has woken up and restored the pinmux for mdio ?

The mdio bus may be shared with other PHYs as well, they may race on suspend
as well if the stmmac instance that drives MDIO quiesces before the other MAC
that needs the other PHY ?

All in all the question is, should we instead deal with MDIO muxing independently
of MII muxing, especially for suspend/resume ?

Maxime

> 
> Fixes: 90702dcd19c0 ("net: stmmac: fix MAC not working when system resume back with WoL active")
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> [fabio: Reworded subject/commit log for clarity and made it priv->wolopts conditional]
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Based on the NXP kernel fix:
> 
> https://github.com/nxp-imx/linux-imx/commit/7905fa71ed07d8c2f1da88ad9fffd5ac5e89461f
> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 0de4bc949913..36ab66ad80fd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8183,13 +8183,15 @@ 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);
>  
>  	rtnl_lock();
>  	phylink_suspend(priv->phylink, !!priv->wolopts);
> +
> +	if (!priv->wolopts)
> +		pinctrl_pm_select_sleep_state(priv->device);
>  	rtnl_unlock();
>  
>  	if (stmmac_fpe_supported(priv))