[PATCH net 0/3] net: stmmac: fix failed MTU reopening and hardware resume

[PATCH net 0/3] net: stmmac: fix failed MTU reopening and hardware resume

[James Hilliard]

A failed stmmac MTU-change reopen leaves the netdev administratively up
after its replacement DMA resources have been freed. A later close can
repeat NAPI shutdown and IRQ or DMA-resource teardown. A failed hardware
resume similarly leaves an up interface with NAPI already disabled, so
closing it can hang in a second napi_disable().

Fix these error paths by tracking whether the data path is open and
closing the netdev through the network core after failed reinitialization.
For failed resume, release the suspended data path without disabling NAPI
again. Allow phylink to finish shutdown directly from its suspended state
without restarting a MAC whose hardware setup has failed.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
These fixes were previously posted as patches 1-3 of the H616 EMAC1 v4
series. Split them into a separate net series as requested by Maxime
Chevallier. The sun8i MAC-reset change, EMAC1 binding and variant remain
in the net-next series.

Changes since that posting:
- Adapt the failed-resume fix to the net tree's older stmmac_resume()
  implementation, leaving timestamping behavior unchanged.
- Add a Fixes tag for the phylink suspend/stop handling.

Previous posting:
https://patch.msgid.link/20260920-submit-h616-emac1-v1-v4-0-8347dfe2eb7d@gmail.com

To: Russell King <linux@armlinux.org.uk>
To: Andrew Lunn <andrew@lunn.ch>
To: Heiner Kallweit <hkallweit1@gmail.com>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Joakim Zhang <qiangqing.zhang@nxp.com>
To: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
To: Maxime Chevallier <maxime.chevallier@bootlin.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: Maxime Coquelin <mcoquelin.stm32@gmail.com>
To: Alexandre Torgue <alexandre.torgue@foss.st.com>
To: Christian Marangi <ansuelsmth@gmail.com>
To: Tiezhu Yang <yangtiezhu@loongson.cn>
To: Huacai Chen <chenhuacai@kernel.org>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org

---
James Hilliard (3):
      net: phylink: allow stopping a suspended instance
      net: stmmac: close the interface after a failed MTU reopen
      net: stmmac: close the interface after failed hardware resume

 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 39 ++++++++++++++++++-----
 drivers/net/phy/phylink.c                         | 16 ++++++++++
 3 files changed, 49 insertions(+), 8 deletions(-)
---
base-commit: 1e24c4f2ee44be0eee94092b5d13cbdb4bdf0d60
change-id: 20260921-submit-stmmac-reset-fixes-v1-7c98b92d29a9

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

[PATCH net 1/3] net: phylink: allow stopping a suspended instance

[James Hilliard]

A MAC which cannot restore its hardware after system sleep may need to
close the network device instead of calling phylink_resume(). Without MAC
Wake-on-LAN, phylink_suspend() has already stopped the PHY and PCS, so
another phylink_stop() repeats their shutdown. With MAC Wake-on-LAN, the
saved link-up state still needs a matching mac_link_down(), and leaving
PHYLINK_DISABLE_MAC_WOL set prevents a subsequent start from resolving
the link.

Allow phylink_stop() to finish shutdown directly from either suspended
state. Leave an already stopped instance alone. For MAC Wake-on-LAN,
stop resolution, drain pending resolve work, complete the deferred
link-down and clear the WoL disable bit while retaining the stopped bit.
This does not restart the PHY, reconfigure the MAC or bring its link up.

Fixes: f97493657c63 ("net: phylink: add suspend/resume support")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/phy/phylink.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a1458da8111b..8f9278112623 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2495,11 +2495,17 @@ EXPORT_SYMBOL_GPL(phylink_start);
  *
  * This will synchronously bring down the link if the link is not already
  * down (in other words, it will trigger a mac_link_down() method call.)
+ * It may also be called after phylink_suspend() if the MAC cannot resume
+ * and the network device must be closed instead.
  */
 void phylink_stop(struct phylink *pl)
 {
 	ASSERT_RTNL();
 
+	/* phylink_suspend() already stops the link without MAC WoL. */
+	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
+		return;
+
 	if (pl->sfp_bus)
 		sfp_upstream_stop(pl->sfp_bus);
 	if (pl->phydev)
@@ -2512,6 +2518,16 @@ void phylink_stop(struct phylink *pl)
 
 	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
 
+	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
+		/* Finish the link-down deferred by MAC WoL, without restarting. */
+		flush_work(&pl->resolve);
+		mutex_lock(&pl->state_mutex);
+		if (pl->suspend_link_up)
+			phylink_link_down(pl);
+		__clear_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
+		mutex_unlock(&pl->state_mutex);
+	}
+
 	pl->pcs_state = PCS_STATE_DOWN;
 
 	phylink_pcs_disable(pl->pcs);

-- 
2.53.0

[PATCH net 2/3] net: stmmac: close the interface after a failed MTU reopen

[James Hilliard]

An MTU change stops the data path and then reopens it using a newly
allocated DMA configuration. If reopening fails, the new allocation is
freed while the netdev remains up and priv->dma_conf retains copies of
the freed pointers. A subsequent close repeats NAPI disable and IRQ and
DMA-resource teardown. It can hang in napi_disable() or access resources
which have already been freed.

Track successful opens of the data path separately from the netdev's
administrative state. Clear that state when releasing the data path and
skip a second release after a failed reopen. Close the netdev through the
network core on the MTU error path, so the PHY attachment and runtime-PM
reference are released and the interface is marked down. A subsequent
open can then allocate fresh resources and retry normally.

Keep successful MTU changes on the existing path, retaining the PHY
attachment and runtime-PM reference. Allocation failures before stopping
the original data path still leave the running interface unchanged.

Fixes: 3470079687448 ("net: ethernet: stmicro: stmmac: permit MTU change with interface up")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..873331dbaf77 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -281,6 +281,8 @@ struct stmmac_priv {
 	struct mutex lock;
 
 	struct stmmac_dma_conf dma_conf;
+	/* Paired __stmmac_open()/__stmmac_release(), serialized by RTNL. */
+	bool opened;
 
 	/* Generic channel for NAPI */
 	struct stmmac_channel channel[STMMAC_CH_MAX];
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1fb5f804ea23..44b59d0f56b5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4179,6 +4179,7 @@ static int __stmmac_open(struct net_device *dev,
 	stmmac_enable_all_queues(priv);
 	netif_tx_start_all_queues(priv->dev);
 	stmmac_enable_all_dma_irq(priv);
+	priv->opened = true;
 
 	return 0;
 
@@ -4249,6 +4250,11 @@ static void __stmmac_release(struct net_device *dev)
 	struct stmmac_priv *priv = netdev_priv(dev);
 	u8 chan;
 
+	/* A failed MTU reopen has already released the data path. */
+	if (!priv->opened)
+		return;
+	priv->opened = false;
+
 	/* Stop and disconnect the PHY */
 	phylink_stop(priv->phylink);
 
@@ -6174,6 +6180,10 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
 		if (ret) {
 			free_dma_desc_resources(priv, dma_conf);
 			kfree(dma_conf);
+			/* Finish closing the PHY and PM state, but do not repeat
+			 * the data-path teardown after the failed reopen.
+			 */
+			netif_close(dev);
 			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
 			return ret;
 		}

-- 
2.53.0

[PATCH net 3/3] net: stmmac: close the interface after failed hardware resume

[James Hilliard]

System suspend disables NAPI and suspends phylink but retains the IRQs
and DMA resources. If hardware setup fails during resume, the interface
remains administratively up with NAPI still disabled. Closing it later
calls napi_disable() a second time and can hang indefinitely.

On hardware-setup failure, stop DMA and disable the MAC, then release the
suspended data path without repeating NAPI disable. Stop phylink directly
from its suspended state rather than restarting the link on hardware
which failed to resume. Drop the driver mutex before teardown, retaining
RTNL across cleanup and network-core close.

Close the netdev to detach the PHY, release its runtime-PM reference and
clear its administrative state. Reattach the now-down netdev so a later
open can allocate new resources and retry. Preserve the original resume
error and leave successful resume unchanged.

Fixes: 6896c2449a18 ("net: stmmac: Check stmmac_hw_setup() in stmmac_resume()")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 29 ++++++++++++++++-------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 44b59d0f56b5..87cb0978ed6a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4245,7 +4245,7 @@ static int stmmac_open(struct net_device *dev)
 	return ret;
 }
 
-static void __stmmac_release(struct net_device *dev)
+static void __stmmac_release(struct net_device *dev, bool napi_disabled)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 	u8 chan;
@@ -4258,7 +4258,9 @@ static void __stmmac_release(struct net_device *dev)
 	/* Stop and disconnect the PHY */
 	phylink_stop(priv->phylink);
 
-	stmmac_disable_all_queues(priv);
+	/* Suspend has already disabled NAPI when hardware resume fails. */
+	if (!napi_disabled)
+		stmmac_disable_all_queues(priv);
 
 	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
 		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
@@ -4297,7 +4299,7 @@ static int stmmac_release(struct net_device *dev)
 	if (device_may_wakeup(priv->device))
 		phylink_speed_down(priv->phylink, false);
 
-	__stmmac_release(dev);
+	__stmmac_release(dev, false);
 
 	stmmac_legacy_serdes_power_down(priv);
 	phylink_disconnect_phy(priv->phylink);
@@ -6174,7 +6176,7 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
 			return PTR_ERR(dma_conf);
 		}
 
-		__stmmac_release(dev);
+		__stmmac_release(dev, false);
 
 		ret = __stmmac_open(dev, dma_conf);
 		if (ret) {
@@ -8356,10 +8358,7 @@ int stmmac_resume(struct device *dev)
 	ret = stmmac_hw_setup(ndev);
 	if (ret < 0) {
 		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
-		stmmac_legacy_serdes_power_down(priv);
-		mutex_unlock(&priv->lock);
-		rtnl_unlock();
-		return ret;
+		goto error_stop_dma;
 	}
 
 	stmmac_init_timestamping(priv);
@@ -8386,6 +8385,20 @@ int stmmac_resume(struct device *dev)
 	netif_device_attach(ndev);
 
 	return 0;
+
+error_stop_dma:
+	stmmac_stop_all_dma(priv);
+	stmmac_mac_set(priv, priv->ioaddr, false);
+	mutex_unlock(&priv->lock);
+	/* Release the suspended data path before ndo_stop(), which must not
+	 * disable NAPI or free these resources a second time.
+	 */
+	__stmmac_release(ndev, true);
+	netif_close(ndev);
+	netif_device_attach(ndev);
+	rtnl_unlock();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(stmmac_resume);
 

-- 
2.53.0

Re: [PATCH net 1/3] net: phylink: allow stopping a suspended instance

[Andrew Lunn]

On Mon, Sep 21, 2026 at 10:54:02AM -0600, James Hilliard wrote:
> A MAC which cannot restore its hardware after system sleep may need to
> close the network device instead of calling phylink_resume().

You need to explain this in more detail. It sounds like you are
pushing a workaround for a broken MAC into the core of phylink. This
is generally wrong.

	Andrew

Re: [PATCH net 3/3] net: stmmac: close the interface after failed hardware resume

[Maxime Chevallier]

Hi,

On 9/21/26 18:54, James Hilliard wrote:
> System suspend disables NAPI and suspends phylink but retains the IRQs
> and DMA resources. If hardware setup fails during resume, the interface
> remains administratively up with NAPI still disabled. Closing it later
> calls napi_disable() a second time and can hang indefinitely.
> 
> On hardware-setup failure, stop DMA and disable the MAC, then release the
> suspended data path without repeating NAPI disable. Stop phylink directly
> from its suspended state rather than restarting the link on hardware
> which failed to resume. Drop the driver mutex before teardown, retaining
> RTNL across cleanup and network-core close.
> 
> Close the netdev to detach the PHY, release its runtime-PM reference and
> clear its administrative state. Reattach the now-down netdev so a later
> open can allocate new resources and retry. Preserve the original resume
> error and leave successful resume unchanged.

So this changes the admin state of the interface upon resume failure ?

using netif_close() in drivers is not common at all, only 3 drivers do that,
and among that, fbnic does that for the selftests...

Let's instead focus on making the state coherent.

Before entering suspend, interface is up and running. suspend happens, then
resume, and something goes wrong. As you mention, the next logical step as
the interface is now dead is to bring it down, and freshly set it back up.

I'd say, either the __stmmac_open / __stmmac_release are not at the right 
granularity, or there's an ordering issue, but we should focus on making
sure down'ing the interface after such a failure puts it back in a clean
state. I'm not really convinced with the boolean to the release indicating
if something's already cleaned or not.

Maxime> 
> Fixes: 6896c2449a18 ("net: stmmac: Check stmmac_hw_setup() in stmmac_resume()")
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 29 ++++++++++++++++-------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 44b59d0f56b5..87cb0978ed6a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4245,7 +4245,7 @@ static int stmmac_open(struct net_device *dev)
>  	return ret;
>  }
>  
> -static void __stmmac_release(struct net_device *dev)
> +static void __stmmac_release(struct net_device *dev, bool napi_disabled)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
> @@ -4258,7 +4258,9 @@ static void __stmmac_release(struct net_device *dev)
>  	/* Stop and disconnect the PHY */
>  	phylink_stop(priv->phylink);
>  
> -	stmmac_disable_all_queues(priv);
> +	/* Suspend has already disabled NAPI when hardware resume fails. */
> +	if (!napi_disabled)
> +		stmmac_disable_all_queues(priv);
>  
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> @@ -4297,7 +4299,7 @@ static int stmmac_release(struct net_device *dev)
>  	if (device_may_wakeup(priv->device))
>  		phylink_speed_down(priv->phylink, false);
>  
> -	__stmmac_release(dev);
> +	__stmmac_release(dev, false);
>  
>  	stmmac_legacy_serdes_power_down(priv);
>  	phylink_disconnect_phy(priv->phylink);
> @@ -6174,7 +6176,7 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  			return PTR_ERR(dma_conf);
>  		}
>  
> -		__stmmac_release(dev);
> +		__stmmac_release(dev, false);
>  
>  		ret = __stmmac_open(dev, dma_conf);
>  		if (ret) {
> @@ -8356,10 +8358,7 @@ int stmmac_resume(struct device *dev)
>  	ret = stmmac_hw_setup(ndev);
>  	if (ret < 0) {
>  		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
> -		stmmac_legacy_serdes_power_down(priv);
> -		mutex_unlock(&priv->lock);
> -		rtnl_unlock();
> -		return ret;
> +		goto error_stop_dma;
>  	}
>  
>  	stmmac_init_timestamping(priv);
> @@ -8386,6 +8385,20 @@ int stmmac_resume(struct device *dev)
>  	netif_device_attach(ndev);
>  
>  	return 0;
> +
> +error_stop_dma:
> +	stmmac_stop_all_dma(priv);
> +	stmmac_mac_set(priv, priv->ioaddr, false);
> +	mutex_unlock(&priv->lock);
> +	/* Release the suspended data path before ndo_stop(), which must not
> +	 * disable NAPI or free these resources a second time.
> +	 */
> +	__stmmac_release(ndev, true);
> +	netif_close(ndev);
> +	netif_device_attach(ndev);
> +	rtnl_unlock();
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(stmmac_resume);
>  
> 

Re: [PATCH net 1/3] net: phylink: allow stopping a suspended instance

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] phylink_stop() is newly documented and implemented as a legal
  replacement for phylink_resume() after phylink_suspend(), but it does…

--

net: phylink: allow stopping a suspended instance

This makes phylink_stop() usable directly on a suspended phylink instance,
so a MAC that cannot restore its hardware after system sleep can close the
netdev instead of calling phylink_resume(). An already stopped instance
returns early, and the MAC Wake-on-LAN case drains the resolve work,
completes the deferred link-down and clears PHYLINK_DISABLE_MAC_WOL.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a1458da8111b6..8f9278112623b 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2495,11 +2495,17 @@ EXPORT_SYMBOL_GPL(phylink_start);
>   *
>   * This will synchronously bring down the link if the link is not already
>   * down (in other words, it will trigger a mac_link_down() method call.)
> + * It may also be called after phylink_suspend() if the MAC cannot resume
> + * and the network device must be closed instead.
>   */
>  void phylink_stop(struct phylink *pl)
>  {
>  	ASSERT_RTNL();
>  
> +	/* phylink_suspend() already stops the link without MAC WoL. */
> +	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
> +		return;
> +

[Severity: Medium]
Does this new exit path leave the PHY advertisement clamped to the lowest
speed?

phylink_suspend() performs two things when PHY based power management speed
control is in use:

drivers/net/phy/phylink.c:phylink_suspend() {
	...
	} else {
		phylink_stop(pl);
	}

	if (phylink_phy_pm_speed_ctrl(pl))
		phylink_speed_down(pl, false);
}

The only place in phylink that reverses the second part is
phylink_resume():

drivers/net/phy/phylink.c:phylink_resume() {
	ASSERT_RTNL();

	if (phylink_phy_pm_speed_ctrl(pl))
		phylink_speed_up(pl);
	...
}

Both new exits added here, this early return and the MAC WoL completion
block below, return without calling phylink_speed_up().  So a MAC that
follows the new kernel-doc ("It may also be called after phylink_suspend()
if the MAC cannot resume and the network device must be closed instead")
ends up with the PHY still advertising only the lowest common speed, and
with the original advertisement left in phydev->adv_old where phylink will
never consume it.

Should the new documentation state that the MAC has to call
phylink_speed_up() itself in this case, or should phylink_stop() reverse
the speed-down on these paths?

The only in-tree user of wol_phy_speed_ctrl is stmmac, and stmmac_open()
calls phylink_speed_up() unconditionally, so the next open repairs the
residue in the common case.  The residue looks like it can become permanent
if another speed-down runs first, since stmmac_release() also calls
phylink_speed_down() when device_may_wakeup(priv->device) is true:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_release() {
	...
	if (device_may_wakeup(priv->device))
		phylink_speed_down(priv->phylink, false);
	...
}

and phy_speed_down() overwrites the saved advertisement each time:

drivers/net/phy/phy.c:phy_speed_down() {
	...
	linkmode_copy(phydev->adv_old, adv_tmp);
	...
}

With no intervening phylink_speed_up(), would the second phy_speed_down()
store the already reduced set into phydev->adv_old, so the later
phylink_speed_up() restores the reduced advertisement rather than the
configured one?  Note device_may_wakeup(priv->device) can be true while
pl->wolopts_mac is zero, since stmmac calls
device_set_wakeup_capable(priv->device, 1) when plat->pmt is set.

>  	if (pl->sfp_bus)
>  		sfp_upstream_stop(pl->sfp_bus);
>  	if (pl->phydev)
> @@ -2512,6 +2518,16 @@ void phylink_stop(struct phylink *pl)
>  
>  	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
>  
> +	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
> +		/* Finish the link-down deferred by MAC WoL, without restarting. */
> +		flush_work(&pl->resolve);
> +		mutex_lock(&pl->state_mutex);
> +		if (pl->suspend_link_up)
> +			phylink_link_down(pl);
> +		__clear_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
> +		mutex_unlock(&pl->state_mutex);
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-submit-stmmac-reset-fixes-v1-v1-0-87a4e431ee00%40gmail.com

Re: [PATCH net 2/3] net: stmmac: close the interface after a failed MTU reopen

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 1 · High: 2 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] The failed-MTU-reopen path leaves the MAC Rx/Tx enabled and
  all RX/TX DMA channels in RUN state while the descriptor rings and RX…
- [High] The new priv->opened state is only maintained by
  __stmmac_open()/__stmmac_release(), but the driver contains a second,…
- [High] The fix is incomplete for the sibling live-reconfiguration paths
  and makes a new inconsistent state reachable there.

--

net: stmmac: close the interface after a failed MTU reopen

The patch adds a new priv->opened flag that tracks whether the data path
was successfully opened, makes __stmmac_release() bail out when it is
clear, and calls netif_close() on the MTU reopen error path so the PHY
and runtime-PM reference are dropped. A few questions below about paths
that are not covered by the new flag.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 7582fca637419..873331dbaf772 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -281,6 +281,8 @@ struct stmmac_priv {
>  	struct mutex lock;
>  
>  	struct stmmac_dma_conf dma_conf;
> +	/* Paired __stmmac_open()/__stmmac_release(), serialized by RTNL. */
> +	bool opened;

[Severity: High]
Does the comment's pairing invariant hold for the XDP release/open pair?

stmmac_xdp_release() performs the same teardown as __stmmac_release()
(stmmac_disable_all_queues(), hrtimer_cancel() per TX queue,
stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0), stmmac_stop_all_dma(),
free_dma_desc_resources(priv, &priv->dma_conf), stmmac_mac_set(false))
but never clears priv->opened, and stmmac_xdp_open() never sets it.

stmmac_xdp_set_prog() also ignores the reopen result:

	if (if_running && need_update)
		stmmac_xdp_open(dev);

so if stmmac_xdp_open() fails in alloc_dma_desc_resources(),
init_dma_desc_rings() or stmmac_request_irq(), the netdev stays up with
NAPI disabled, IRQs freed and priv->dma_conf holding freed pointers,
while priv->opened is still true.

A later "ip link set down" then passes the new guard and runs the whole
__stmmac_release() a second time. Isn't that the same repeated teardown
the commit message describes, i.e. napi_disable() on already disabled
NAPI while holding RTNL, stmmac_free_irq() on freed IRQs and a second
dma_free_coherent()/page_pool_destroy() over the same rings?

Should stmmac_xdp_release() clear priv->opened and stmmac_xdp_open() set
it on success as well?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..44b59d0f56b5b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4179,6 +4179,7 @@ static int __stmmac_open(struct net_device *dev,
>  	stmmac_enable_all_queues(priv);
>  	netif_tx_start_all_queues(priv->dev);
>  	stmmac_enable_all_dma_irq(priv);
> +	priv->opened = true;
>  
>  	return 0;
>  
> @@ -4249,6 +4250,11 @@ static void __stmmac_release(struct net_device *dev)
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
>  
> +	/* A failed MTU reopen has already released the data path. */
> +	if (!priv->opened)
> +		return;
> +	priv->opened = false;
> +

[Severity: High]
Can this early return also be reached from the ethtool reconfiguration
paths, where the outer resources have already been dropped?

stmmac_reinit_ringparam() and stmmac_reinit_queues() call the ndo
handlers directly while the netdev stays administratively up:

	if (netif_running(dev))
		stmmac_release(dev);

	priv->dma_conf.dma_rx_size = rx_size;
	priv->dma_conf.dma_tx_size = tx_size;

	if (netif_running(dev))
		ret = stmmac_open(dev);

stmmac_release() now clears priv->opened and its tail also runs
stmmac_legacy_serdes_power_down(), phylink_disconnect_phy() and
pm_runtime_put(). If the following stmmac_open() fails (for example in
stmmac_setup_dma_desc(), pm_runtime_resume_and_get() or
stmmac_init_phy()), the interface is left IFF_UP with priv->opened
false, no PHY attached and no runtime-PM reference, and unlike the MTU
path no netif_close() is done here.

>From that state, a later "ip link set down" reaches stmmac_release(),
__stmmac_release() returns at the new guard, and the unconditional tail
still runs:

	__stmmac_release(dev);

	stmmac_legacy_serdes_power_down(priv);
	phylink_disconnect_phy(priv->phylink);
	pm_runtime_put(priv->device);

Does the second pm_runtime_put() here produce a "Runtime PM usage count
underflow!" report?

And in the same state, an MTU change still passes netif_running(),
__stmmac_release() becomes a no-op and __stmmac_open() proceeds into
phylink_prepare_resume() and stmmac_hw_setup() register programming with
no runtime-PM reference held and no PHY connected. Since stmmac's
runtime suspend gates the bus clocks via stmmac_bus_clks_config(priv,
false), can those register accesses hit a suspended device?

>  	/* Stop and disconnect the PHY */
>  	phylink_stop(priv->phylink);
>  

[ ... ]

> @@ -6174,6 +6180,10 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  		if (ret) {
>  			free_dma_desc_resources(priv, dma_conf);
>  			kfree(dma_conf);
> +			/* Finish closing the PHY and PM state, but do not repeat
> +			 * the data-path teardown after the failed reopen.
> +			 */
> +			netif_close(dev);
>  			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
>  			return ret;
>  		}

[Severity: Critical]
Is the premise of the comment, that the data path has already been
released, true for the hardware state when __stmmac_open() fails in
stmmac_request_irq()?

__stmmac_open() first copies the new configuration into priv->dma_conf
and then calls stmmac_hw_setup(), whose last steps are:

	/* Enable the MAC Rx/Tx */
	stmmac_mac_set(priv, priv->ioaddr, true);
	...
	/* Start the ball rolling... */
	stmmac_start_all_dma(priv);

If stmmac_request_irq() then fails, the unwind is only:

	irq_error:
		phylink_stop(priv->phylink);

		for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
			hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);

		stmmac_release_ptp(priv);
	init_error:
		return ret;

so the MAC stays enabled, all RX/TX DMA channels stay in RUN state, and
priv->opened stays false because it is only set after IRQ setup
succeeds.

stmmac_change_mtu() then calls free_dma_desc_resources(priv, dma_conf).
Since priv->dma_conf aliases dma_conf at this point, doesn't that free
the descriptor rings and RX page-pool pages whose addresses are still
live in the DMA channel base and tail registers?

The following netif_close() reaches __stmmac_release(), which now
returns at the new !priv->opened guard before stmmac_stop_all_dma(),
which was the only remaining caller that clears the RUN bits (and calls
stmmac_deinit_chan()). Can the device keep fetching descriptors and
writing received frames into the freed coherent memory and the returned
page-pool pages?

The commit message says:

	"A subsequent open can then allocate fresh resources and retry
	normally."

With the channels still armed, does the next stmmac_open() reliably
recover, given that the SWR poll in stmmac_init_dma_engine() is bounded
and can time out while the channels still have outstanding bus
transactions?

Would it be better to unwind the hardware where it was started, that is
stmmac_stop_all_dma() plus stmmac_mac_set(priv, priv->ioaddr, false) in
the irq_error path of __stmmac_open(), or before the rings are freed in
stmmac_change_mtu()? The follow-up patch in this series for the resume
path does exactly that (stmmac_stop_all_dma() and stmmac_mac_set(priv,
priv->ioaddr, false) before __stmmac_release()), but the MTU path is
left unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-submit-stmmac-reset-fixes-v1-v1-0-87a4e431ee00%40gmail.com

Re: [PATCH net 3/3] net: stmmac: close the interface after failed hardware resume

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the double-napi_disable() hang that the commit
  claims to eliminate is still reachable from three other PM paths.
- [High] A failed MTU reopen can leave the MAC enabled and TX/RX DMA
  running over descriptor memory that has already been freed, because…

Pre-existing issues:
- [High] The XDP program attach/detach path performs the same data-path
  teardown as __stmmac_release() but does not participate in either half…

--

net: stmmac: close the interface after failed hardware resume

On stmmac_hw_setup() failure in stmmac_resume(), the patch stops DMA,
disables the MAC, releases the suspended data path via a new napi_disabled
argument to __stmmac_release(), then closes and reattaches the netdev while
keeping RTNL held. The intent is to avoid a second napi_disable() on a
still-up interface whose NAPI was already disabled by suspend.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 44b59d0f56b5b..87cb0978ed6ad 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4245,7 +4245,7 @@ static int stmmac_open(struct net_device *dev)
>  	return ret;
>  }
>  
> -static void __stmmac_release(struct net_device *dev)
> +static void __stmmac_release(struct net_device *dev, bool napi_disabled)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
> @@ -4258,7 +4258,9 @@ static void __stmmac_release(struct net_device *dev)
>  	/* Stop and disconnect the PHY */
>  	phylink_stop(priv->phylink);
>  
> -	stmmac_disable_all_queues(priv);
> +	/* Suspend has already disabled NAPI when hardware resume fails. */
> +	if (!napi_disabled)
> +		stmmac_disable_all_queues(priv);

[Severity: High]
This isn't a bug introduced by this patch, but does the XDP attach/detach
path have the same problem that the new napi_disabled argument and the
priv->opened bookkeeping are meant to prevent?

stmmac_xdp_release() performs the same teardown as __stmmac_release() -
netif_tx_disable(), stmmac_disable_all_queues(), stmmac_free_irq(dev,
REQ_IRQ_ERR_ALL, 0), stmmac_stop_all_dma() and
free_dma_desc_resources(priv, &priv->dma_conf) - but never clears
priv->opened.

stmmac_xdp_open() re-enables NAPI only at its end, and its
dma_desc_error/init_error/irq_error labels return with NAPI still disabled
and priv->dma_conf's rings already freed. stmmac_xdp_set_prog() discards
that return value:

	if (if_running && need_update)
		stmmac_xdp_open(dev);

A later link-down then enters __stmmac_release(dev, false) with
priv->opened still true, so it skips the early return and calls
stmmac_disable_all_queues() on already-disabled NAPI, which spins in
napi_disable_locked():

net/core/dev.c:napi_disable_locked() {
	...
	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}
	...
}

Would that same pass also re-run free_dma_desc_resources() over rings
already freed? __free_dma_rx_desc_resources() and
__free_dma_tx_desc_resources() do not clear dma_rx/dma_tx/buf_pool/
tx_skbuff after freeing them.

Would a driver-tracked "data path already released / NAPI already disabled"
state, consulted by __stmmac_release() itself, cover this call site too
instead of a per-caller boolean?

[ ... ]

> @@ -6174,7 +6176,7 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  			return PTR_ERR(dma_conf);
>  		}
>  
> -		__stmmac_release(dev);
> +		__stmmac_release(dev, false);
>  
>  		ret = __stmmac_open(dev, dma_conf);
>  		if (ret) {

[Severity: High]
Can this leave the MAC enabled and DMA running over freed descriptor
memory when __stmmac_open() fails late?

__stmmac_open() copies dma_conf into priv->dma_conf and calls
stmmac_hw_setup(), which enables the MAC and starts every channel:

	/* Enable the MAC Rx/Tx */
	stmmac_mac_set(priv, priv->ioaddr, true);
	...
	/* Start the ball rolling... */
	stmmac_start_all_dma(priv);

If stmmac_request_irq() then fails, the irq_error label only stops phylink,
cancels the txtimers and releases PTP, and returns before priv->opened is
set back to true:

irq_error:
	phylink_stop(priv->phylink);

	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);

	stmmac_release_ptp(priv);
init_error:
	return ret;

stmmac_change_mtu() then frees the very rings the live DMA is programmed
with and calls netif_close(dev), but the close path returns immediately:

__stmmac_release() {
	/* A failed MTU reopen has already released the data path. */
	if (!priv->opened)
		return;
	...
}

so stmmac_stop_all_dma() is never reached. If phylink_stop() ran before the
link ever resolved up, no mac_link_down() occurs either, so would inbound
frames keep being written into memory already returned by
dma_free_coherent()?

Should the partial-open unwind in __stmmac_open() stop DMA and disable the
MAC, rather than having the close path treat every opened == false state as
fully released?

> @@ -8356,10 +8358,7 @@ int stmmac_resume(struct device *dev)
>  	ret = stmmac_hw_setup(ndev);
>  	if (ret < 0) {
>  		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
> -		stmmac_legacy_serdes_power_down(priv);
> -		mutex_unlock(&priv->lock);
> -		rtnl_unlock();
> -		return ret;
> +		goto error_stop_dma;
>  	}
>  
>  	stmmac_init_timestamping(priv);
> @@ -8386,6 +8385,20 @@ int stmmac_resume(struct device *dev)
>  	netif_device_attach(ndev);
>  
>  	return 0;
> +
> +error_stop_dma:
> +	stmmac_stop_all_dma(priv);
> +	stmmac_mac_set(priv, priv->ioaddr, false);
> +	mutex_unlock(&priv->lock);
> +	/* Release the suspended data path before ndo_stop(), which must not
> +	 * disable NAPI or free these resources a second time.
> +	 */
> +	__stmmac_release(ndev, true);
> +	netif_close(ndev);
> +	netif_device_attach(ndev);
> +	rtnl_unlock();
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(stmmac_resume);

[Severity: High]
Is the hang described in the commit message still reachable from the
sibling PM error exits that were not converted?

stmmac_suspend() disables NAPI on every channel whenever
netif_running(ndev) is true, and leaves IFF_UP set:

	netif_device_detach(ndev);

	stmmac_disable_all_queues(priv);

stmmac_resume() still has two plain returns taken after that state exists.
The platform resume hook:

	if (priv->plat->resume) {
		ret = priv->plat->resume(dev, priv->plat->bsp_priv);
		if (ret)
			return ret;
	}

which stmmac_pci_plat_resume() can reach by propagating a
pci_enable_device() failure, and the serdes power-up:

	if (!(priv->plat->flags & STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP)) {
		ret = stmmac_legacy_serdes_power_up(priv);
		if (ret < 0)
			return ret;
	}

where intel_serdes_powerup() returns -ETIMEDOUT from serdes_status_poll().
Only dwmac-tegra sets STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP, so the Intel
path does execute here, and this point is only reached when
netif_running() was true, i.e. NAPI is already disabled.

There is a third case on the suspend side:

suspend_bsp:
	if (priv->plat->suspend)
		return priv->plat->suspend(dev, priv->plat->bsp_priv);

stmmac_pci_plat_suspend() can fail on pci_save_state() after NAPI was
already disabled. device_resume() in drivers/base/power/main.c then bails
out early:

	if (!dev->power.is_suspended)
		goto Complete;

since is_suspended is only set when the suspend callback returned 0, so
stmmac_resume() - and therefore the new error_stop_dma recovery - never
runs for that case at all.

In all three cases priv->opened stays true, so a later
"ip link set dev X down" reaches __stmmac_release(dev, false) ->
stmmac_disable_all_queues() -> napi_disable(), which spins indefinitely in
napi_disable_locked() because nothing clears NAPIF_STATE_SCHED/NPSVC, with
RTNL held:

net/core/dev.c:napi_disable_locked() {
	...
	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}
	...
}

The new napi_disabled argument is passed true from exactly one call site,
so it does not generalize to these paths. Would a driver-level state flag
recording that the data path is suspended with NAPI disabled, checked
inside __stmmac_release(), cover all of them?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-submit-stmmac-reset-fixes-v1-v1-0-87a4e431ee00%40gmail.com