net: stmmac: stm32: Do not suspend downed interface

[net-next,PATCH] net: stmmac: stm32: Do not suspend downed interface

Posted by Marek Vasut 3 weeks, 3 days ago

If an interface is down, the ETHnSTP clock are not running. Suspending
such an interface will attempt to stop already stopped ETHnSTP clock,
and produce a warning in the kernel log about this.

STM32MP25xx that is booted from NFS root via its first ethernet MAC
(also the consumer of ck_ker_eth1stp) and with its second ethernet
MAC downed produces the following warnings during suspend resume
cycle. This can be provoked even using pm_test:

"
$ echo devices > /sys/power/pm_test
$ echo mem > /sys/power/state
...
ck_ker_eth2stp already disabled
...
ck_ker_eth2stp already unprepared
...
"

Fix this by not manipulating with the clock during suspend resume
of interfaces which are downed.

Signed-off-by: Marek Vasut <marex@nabladev.com>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: Christophe Roullier <christophe.roullier@st.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Cc: kernel@dh-electronics.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index e1b260ed4790b..5b0d111afcac3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -618,11 +618,21 @@ static void stm32_dwmac_remove(struct platform_device *pdev)
 
 static int stm32mp1_suspend(struct stm32_dwmac *dwmac)
 {
+	struct net_device *ndev = dev_get_drvdata(dwmac->dev);
+
+	if (!ndev || !netif_running(ndev))
+		return 0;
+
 	return clk_prepare_enable(dwmac->clk_ethstp);
 }
 
 static void stm32mp1_resume(struct stm32_dwmac *dwmac)
 {
+	struct net_device *ndev = dev_get_drvdata(dwmac->dev);
+
+	if (!ndev || !netif_running(ndev))
+		return;
+
 	clk_disable_unprepare(dwmac->clk_ethstp);
 }
 
-- 
2.51.0

Re: [net-next,PATCH] net: stmmac: stm32: Do not suspend downed interface

Posted by Russell King (Oracle) 3 weeks, 3 days ago

On Wed, Jan 14, 2026 at 09:17:54AM +0100, Marek Vasut wrote:
> If an interface is down, the ETHnSTP clock are not running. Suspending
> such an interface will attempt to stop already stopped ETHnSTP clock,
> and produce a warning in the kernel log about this.
> 
> STM32MP25xx that is booted from NFS root via its first ethernet MAC
> (also the consumer of ck_ker_eth1stp) and with its second ethernet
> MAC downed produces the following warnings during suspend resume
> cycle. This can be provoked even using pm_test:
> 
> "
> $ echo devices > /sys/power/pm_test
> $ echo mem > /sys/power/state
> ...
> ck_ker_eth2stp already disabled
> ...
> ck_ker_eth2stp already unprepared
> ...
> "
> 
> Fix this by not manipulating with the clock during suspend resume
> of interfaces which are downed.

I don't think this is the correct fix. Looking back at my commits:
b51f34bc85e3 net: stmmac: platform: legacy hooks for suspend()/resume() methods
07bbbfe7addf net: stmmac: add suspend()/resume() platform ops

I think I changed the behaviour of the suspend/resume callbacks
unintentionally. Sorry, I don't have time to complete this email
(meeting.)

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

Re: [net-next,PATCH] net: stmmac: stm32: Do not suspend downed interface

Posted by Russell King (Oracle) 3 weeks, 3 days ago

On Wed, Jan 14, 2026 at 04:29:17PM +0000, Russell King (Oracle) wrote:
> On Wed, Jan 14, 2026 at 09:17:54AM +0100, Marek Vasut wrote:
> > If an interface is down, the ETHnSTP clock are not running. Suspending
> > such an interface will attempt to stop already stopped ETHnSTP clock,
> > and produce a warning in the kernel log about this.
> > 
> > STM32MP25xx that is booted from NFS root via its first ethernet MAC
> > (also the consumer of ck_ker_eth1stp) and with its second ethernet
> > MAC downed produces the following warnings during suspend resume
> > cycle. This can be provoked even using pm_test:
> > 
> > "
> > $ echo devices > /sys/power/pm_test
> > $ echo mem > /sys/power/state
> > ...
> > ck_ker_eth2stp already disabled
> > ...
> > ck_ker_eth2stp already unprepared
> > ...
> > "
> > 
> > Fix this by not manipulating with the clock during suspend resume
> > of interfaces which are downed.
> 
> I don't think this is the correct fix. Looking back at my commits:
> b51f34bc85e3 net: stmmac: platform: legacy hooks for suspend()/resume() methods
> 07bbbfe7addf net: stmmac: add suspend()/resume() platform ops
> 
> I think I changed the behaviour of the suspend/resume callbacks
> unintentionally. Sorry, I don't have time to complete this email
> (meeting.)

I think I'm going to start over, trying to figure out what happened.

c7308b2f3d0d net: stmmac: stm32: convert to suspend()/resume() methods

Did the conversion, and it always called stm32_dwmac_clk_disable() and
where it exists, dwmac->ops->suspend() on suspend, provided
stmmac_suspend() returns zero (which it will do, even if the interface
is down. On resume, it always calls dwmac->ops->resume() and
stm32_dwmac_init() before calling stmmac_resume().

The conversion added hooks into ny new ->suspend() and ->resume()
methods to handle the stm32_dwmac_clk_disable(), dwmac->ops->suspend(),
dwmac->ops->resume() and stm32_dwmac_init() steps.

However, in 07bbbfe7addf I failed to realise that, in order to keep
things compatible with how stuff works, we need to call
priv->plat->suspend() even if the interface is down. This is where
the bug is, not in your glue driver.

Please try this:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a8a78fe7d01f..2acbb0107cd3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -8066,7 +8066,7 @@ int stmmac_suspend(struct device *dev)
 	u32 chan;
 
 	if (!ndev || !netif_running(ndev))
-		return 0;
+		goto suspend_bsp;
 
 	mutex_lock(&priv->lock);
 
@@ -8106,6 +8106,7 @@ int stmmac_suspend(struct device *dev)
 	if (stmmac_fpe_supported(priv))
 		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
 
+suspend_bsp:
 	if (priv->plat->suspend)
 		return priv->plat->suspend(dev, priv->plat->bsp_priv);
 

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

Re: [net-next,PATCH] net: stmmac: stm32: Do not suspend downed interface

Posted by Marek Vasut 3 weeks, 2 days ago

On 1/14/26 6:12 PM, Russell King (Oracle) wrote:
> On Wed, Jan 14, 2026 at 04:29:17PM +0000, Russell King (Oracle) wrote:
>> On Wed, Jan 14, 2026 at 09:17:54AM +0100, Marek Vasut wrote:
>>> If an interface is down, the ETHnSTP clock are not running. Suspending
>>> such an interface will attempt to stop already stopped ETHnSTP clock,
>>> and produce a warning in the kernel log about this.
>>>
>>> STM32MP25xx that is booted from NFS root via its first ethernet MAC
>>> (also the consumer of ck_ker_eth1stp) and with its second ethernet
>>> MAC downed produces the following warnings during suspend resume
>>> cycle. This can be provoked even using pm_test:
>>>
>>> "
>>> $ echo devices > /sys/power/pm_test
>>> $ echo mem > /sys/power/state
>>> ...
>>> ck_ker_eth2stp already disabled
>>> ...
>>> ck_ker_eth2stp already unprepared
>>> ...
>>> "
>>>
>>> Fix this by not manipulating with the clock during suspend resume
>>> of interfaces which are downed.
>>
>> I don't think this is the correct fix. Looking back at my commits:
>> b51f34bc85e3 net: stmmac: platform: legacy hooks for suspend()/resume() methods
>> 07bbbfe7addf net: stmmac: add suspend()/resume() platform ops
>>
>> I think I changed the behaviour of the suspend/resume callbacks
>> unintentionally. Sorry, I don't have time to complete this email
>> (meeting.)
> 
> I think I'm going to start over, trying to figure out what happened.
> 
> c7308b2f3d0d net: stmmac: stm32: convert to suspend()/resume() methods
> 
> Did the conversion, and it always called stm32_dwmac_clk_disable() and
> where it exists, dwmac->ops->suspend() on suspend, provided
> stmmac_suspend() returns zero (which it will do, even if the interface
> is down. On resume, it always calls dwmac->ops->resume() and
> stm32_dwmac_init() before calling stmmac_resume().
> 
> The conversion added hooks into ny new ->suspend() and ->resume()
> methods to handle the stm32_dwmac_clk_disable(), dwmac->ops->suspend(),
> dwmac->ops->resume() and stm32_dwmac_init() steps.
> 
> However, in 07bbbfe7addf I failed to realise that, in order to keep
> things compatible with how stuff works, we need to call
> priv->plat->suspend() even if the interface is down. This is where
> the bug is, not in your glue driver.
> 
> Please try this:
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a8a78fe7d01f..2acbb0107cd3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8066,7 +8066,7 @@ int stmmac_suspend(struct device *dev)
>   	u32 chan;
>   
>   	if (!ndev || !netif_running(ndev))
> -		return 0;
> +		goto suspend_bsp;
>   
>   	mutex_lock(&priv->lock);
>   
> @@ -8106,6 +8106,7 @@ int stmmac_suspend(struct device *dev)
>   	if (stmmac_fpe_supported(priv))
>   		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
>   
> +suspend_bsp:
>   	if (priv->plat->suspend)
>   		return priv->plat->suspend(dev, priv->plat->bsp_priv);
>   
This works too, thank you.

Will you send this fix ?

Re: [net-next,PATCH] net: stmmac: stm32: Do not suspend downed interface

Posted by Russell King (Oracle) 1 week, 1 day ago

On Thu, Jan 15, 2026 at 12:27:05AM +0100, Marek Vasut wrote:
> On 1/14/26 6:12 PM, Russell King (Oracle) wrote:
> > I think I'm going to start over, trying to figure out what happened.
> > 
> > c7308b2f3d0d net: stmmac: stm32: convert to suspend()/resume() methods
> > 
> > Did the conversion, and it always called stm32_dwmac_clk_disable() and
> > where it exists, dwmac->ops->suspend() on suspend, provided
> > stmmac_suspend() returns zero (which it will do, even if the interface
> > is down. On resume, it always calls dwmac->ops->resume() and
> > stm32_dwmac_init() before calling stmmac_resume().
> > 
> > The conversion added hooks into ny new ->suspend() and ->resume()
> > methods to handle the stm32_dwmac_clk_disable(), dwmac->ops->suspend(),
> > dwmac->ops->resume() and stm32_dwmac_init() steps.
> > 
> > However, in 07bbbfe7addf I failed to realise that, in order to keep
> > things compatible with how stuff works, we need to call
> > priv->plat->suspend() even if the interface is down. This is where
> > the bug is, not in your glue driver.
> > 
> > Please try this:
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index a8a78fe7d01f..2acbb0107cd3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -8066,7 +8066,7 @@ int stmmac_suspend(struct device *dev)
> >   	u32 chan;
> >   	if (!ndev || !netif_running(ndev))
> > -		return 0;
> > +		goto suspend_bsp;
> >   	mutex_lock(&priv->lock);
> > @@ -8106,6 +8106,7 @@ int stmmac_suspend(struct device *dev)
> >   	if (stmmac_fpe_supported(priv))
> >   		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
> > +suspend_bsp:
> >   	if (priv->plat->suspend)
> >   		return priv->plat->suspend(dev, priv->plat->bsp_priv);
> This works too, thank you.
> 
> Will you send this fix ?

Sorry, I appear to have dropped this patch on the floor, and just
tripped over it. I'm just build testing it and will send it later
today.

This problem affects every user of the platform ->suspend/resume()
stuff, so is not just a stm32 issue.

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

Re: [net-next,PATCH] net: stmmac: stm32: Do not suspend downed interface

Posted by Marek Vasut 6 days, 4 hours ago

On 1/30/26 4:52 PM, Russell King (Oracle) wrote:
> On Thu, Jan 15, 2026 at 12:27:05AM +0100, Marek Vasut wrote:
>> On 1/14/26 6:12 PM, Russell King (Oracle) wrote:
>>> I think I'm going to start over, trying to figure out what happened.
>>>
>>> c7308b2f3d0d net: stmmac: stm32: convert to suspend()/resume() methods
>>>
>>> Did the conversion, and it always called stm32_dwmac_clk_disable() and
>>> where it exists, dwmac->ops->suspend() on suspend, provided
>>> stmmac_suspend() returns zero (which it will do, even if the interface
>>> is down. On resume, it always calls dwmac->ops->resume() and
>>> stm32_dwmac_init() before calling stmmac_resume().
>>>
>>> The conversion added hooks into ny new ->suspend() and ->resume()
>>> methods to handle the stm32_dwmac_clk_disable(), dwmac->ops->suspend(),
>>> dwmac->ops->resume() and stm32_dwmac_init() steps.
>>>
>>> However, in 07bbbfe7addf I failed to realise that, in order to keep
>>> things compatible with how stuff works, we need to call
>>> priv->plat->suspend() even if the interface is down. This is where
>>> the bug is, not in your glue driver.
>>>
>>> Please try this:
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> index a8a78fe7d01f..2acbb0107cd3 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> @@ -8066,7 +8066,7 @@ int stmmac_suspend(struct device *dev)
>>>    	u32 chan;
>>>    	if (!ndev || !netif_running(ndev))
>>> -		return 0;
>>> +		goto suspend_bsp;
>>>    	mutex_lock(&priv->lock);
>>> @@ -8106,6 +8106,7 @@ int stmmac_suspend(struct device *dev)
>>>    	if (stmmac_fpe_supported(priv))
>>>    		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
>>> +suspend_bsp:
>>>    	if (priv->plat->suspend)
>>>    		return priv->plat->suspend(dev, priv->plat->bsp_priv);
>> This works too, thank you.
>>
>> Will you send this fix ?
> 
> Sorry, I appear to have dropped this patch on the floor, and just
> tripped over it. I'm just build testing it and will send it later
> today.
> 
> This problem affects every user of the platform ->suspend/resume()
> stuff, so is not just a stm32 issue.
ACK, thank you.

Re: [net-next,PATCH] net: stmmac: stm32: Do not suspend downed interface

Posted by Andrew Lunn 3 weeks, 3 days ago

On Wed, Jan 14, 2026 at 09:17:54AM +0100, Marek Vasut wrote:
> If an interface is down, the ETHnSTP clock are not running. Suspending
> such an interface will attempt to stop already stopped ETHnSTP clock,
> and produce a warning in the kernel log about this.

>  static int stm32mp1_suspend(struct stm32_dwmac *dwmac)
>  {
> +	struct net_device *ndev = dev_get_drvdata(dwmac->dev);
> +
> +	if (!ndev || !netif_running(ndev))
> +		return 0;
> +
>  	return clk_prepare_enable(dwmac->clk_ethstp);

The commit message might be missing some explanation. The
suspend method enables the clock, not disables it.

	Andrew