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

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

[James Hilliard]

Keep the stmmac datapath coherent after failed MTU reopening or hardware
resume without changing the interface's administrative state. An ordinary
down/up must release the resources still owned and open a fresh datapath,
without disabling NAPI twice or releasing already-freed allocations.

The phylink prerequisite permits an ordinary close to terminate a
suspended instance directly. Resuming phylink solely to stop it would
reconfigure and restart a MAC which has not successfully resumed.

The stmmac fix tracks running, suspended-with-resources and released
datapaths separately from IFF_UP. A failed interface stays detached until
successful resume or administrative recovery. XDP, AF_XDP and debugfs
paths which are not excluded by detach also respect resource ownership.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes in v2:
- Drop forced netif_close() and retain the interface's administrative state,
  as requested by Maxime Chevallier.
- Separate queue quiescence from resource release and use explicit datapath
  state instead of a napi_disabled argument to the release helper.
- Handle repeated suspend/resume after failure, preserve PHY/PM ownership
  until ordinary close, and allow a fresh down/up recovery.
- Check XDP/AF_XDP cleanup, descriptor readback and asynchronous reset work
  while the netdev is administratively up but unavailable.
- Explain the generic phylink suspend-to-stop transition and why restarting
  phylink after a failed MAC resume is not a valid substitute, in response
  to Andrew Lunn.
- Combine the two stmmac error-path fixes so every user of the new state
  has consistent resource and NAPI lifetime handling in one patch.
- Link to v1: https://patch.msgid.link/20260921-submit-stmmac-reset-fixes-v1-v1-0-87a4e431ee00@gmail.com

---
James Hilliard (2):
      net: phylink: allow stopping a suspended instance
      net: stmmac: keep datapath state coherent after reinitialization failure

 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  11 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 116 ++++++++++++++++------
 drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c  |   8 +-
 drivers/net/phy/phylink.c                         |  17 ++++
 4 files changed, 120 insertions(+), 32 deletions(-)
---
base-commit: 17741334d00bf5ebd37f8c1c36bc9c146a351deb
change-id: 20260921-submit-stmmac-reset-fixes-v1-7c98b92d29a9

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

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

[James Hilliard]

If a network driver's system resume fails before phylink_resume(), the
network device can remain administratively up with phylink suspended.
Closing the interface then needs to terminate that suspended instance.
Calling phylink_resume() merely to make phylink_stop() work is not a safe
substitute: resume reconfigures the MAC and restarts link resolution,
although the driver has not successfully restored the MAC.

This is a missing suspend-to-stop transition, independent of the reason
hardware restoration failed. No MAC recovery policy belongs in phylink;
the driver still decides whether to retry resume or wait for an ordinary
administrative down/up cycle.

Without MAC Wake-on-LAN, phylink_suspend() has already called
phylink_stop(). Leave that stopped instance alone instead of repeating
PHY, SFP and PCS shutdown. With MAC Wake-on-LAN, suspend deliberately
defers mac_link_down() and sets PHYLINK_DISABLE_MAC_WOL. Finish that
deferred link-down, drain resolution work and clear the WoL disable bit
while retaining PHYLINK_DISABLE_STOPPED. Otherwise a subsequent start
cannot resolve the link.

Document that a suspended instance can be stopped directly. This neither
resumes the PHY nor reconfigures or brings up the MAC.

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

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a1458da8111b..df520d77fded 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2495,11 +2495,18 @@ 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.)
+ * A suspended instance may be stopped without first calling phylink_resume().
+ * In particular, closing a device after a failed resume must not restart the
+ * link or reconfigure the MAC just to finish shutting it down.
  */
 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 +2519,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 v2 2/2] net: stmmac: keep datapath state coherent after reinitialization failure

[James Hilliard]

An MTU change releases the running datapath before reopening it. If the
reopen fails, its replacement DMA resources are freed, but the netdev is
still administratively up. A later close repeats NAPI shutdown, IRQ
release and DMA cleanup. Hardware resume failure has a different partial
state: suspend disabled NAPI but retained the IRQs and DMA resources, so
ordinary close can hang in a second napi_disable().

Track the datapath independently of the administrative state, with three
states describing running queues, suspended queues with resources still
owned, and a released datapath. Separate quiescing the queues from
releasing their resources, so close can perform only the remaining work.
Serialize these transitions with RTNL, including suspend and resume.

On a failed MTU reopen, leave the PHY attachment and runtime-PM reference
owned until ndo_stop(), but detach the netdev so the released datapath
cannot be used. On failed hardware resume, stop any partially initialized
DMA and leave the retained datapath suspended and detached. Do not call
netif_close() or change the administrative state. A later successful
resume can retry the retained datapath; alternatively an ordinary down
releases it, reattaches the now-down netdev, and permits a fresh open.

Stop phylink directly from its suspended state during close. Do not
restart it on failed hardware just to balance its shutdown. Preserve
IRQ-before-final-DMA-stop ordering, and stop DMA on failed open before
the caller frees the replacement rings, including IRQ-request failure
after hardware setup has started DMA.

Account for callers which are not excluded by netif_device_detach():
guard descriptor readback with RTNL and resource ownership, reject TC
queue reconfiguration while detached, and prevent deferred reset work
from reopening the failed interface. Check availability under the TX
queue lock before XDP transmission. XDP configuration must use the actual
datapath state rather than IFF_UP. In particular, AF_XDP pool removal
cannot be rejected: release any suspended rings before a socket's pool
is unmapped and freed, leaving recovery to a subsequent down/up cycle.

Fixes: 3470079687448 ("net: ethernet: stmicro: stmmac: permit MTU change with interface up")
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.h      |  11 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 116 ++++++++++++++++------
 drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c  |   8 +-
 3 files changed, 103 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..5bb92339cbde 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -258,6 +258,15 @@ struct stmmac_msi {
 	char int_name_tx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 18];
 };
 
+enum stmmac_datapath_state {
+	/* No IRQs or DMA allocations owned by a successful open. */
+	STMMAC_DATAPATH_DOWN,
+	/* Resources allocated, NAPI enabled. */
+	STMMAC_DATAPATH_RUNNING,
+	/* Resources retained, NAPI and DMA stopped; also after failed resume. */
+	STMMAC_DATAPATH_SUSPENDED,
+};
+
 struct stmmac_priv {
 	/* Frequently used values are kept adjacent for cache effect */
 	u32 tx_coal_frames[MTL_MAX_TX_QUEUES];
@@ -281,6 +290,8 @@ struct stmmac_priv {
 	struct mutex lock;
 
 	struct stmmac_dma_conf dma_conf;
+	/* IRQ/DMA ownership and NAPI state, serialized by RTNL. */
+	enum stmmac_datapath_state datapath;
 
 	/* 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..7b423c87314c 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->datapath = STMMAC_DATAPATH_RUNNING;
 
 	return 0;
 
@@ -4190,6 +4191,8 @@ static int __stmmac_open(struct net_device *dev,
 
 	stmmac_release_ptp(priv);
 init_error:
+	stmmac_stop_all_dma(priv);
+	stmmac_mac_set(priv, priv->ioaddr, false);
 	return ret;
 }
 
@@ -4244,25 +4247,38 @@ static int stmmac_open(struct net_device *dev)
 	return ret;
 }
 
-static void __stmmac_release(struct net_device *dev)
+/* Quiesce NAPI and transmit queues without releasing their resources. */
+static void stmmac_quiesce(struct stmmac_priv *priv)
 {
-	struct stmmac_priv *priv = netdev_priv(dev);
 	u8 chan;
 
-	/* Stop and disconnect the PHY */
-	phylink_stop(priv->phylink);
-
 	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);
 
-	netif_tx_disable(dev);
+	netif_tx_disable(priv->dev);
+}
+
+static void __stmmac_release(struct net_device *dev)
+{
+	struct stmmac_priv *priv = netdev_priv(dev);
+
+	/* A failed MTU reopen has already released the data path. */
+	if (priv->datapath == STMMAC_DATAPATH_DOWN)
+		return;
+
+	phylink_stop(priv->phylink);
+
+	/* Suspend retains the resources, but has already stopped activity. */
+	if (priv->datapath == STMMAC_DATAPATH_RUNNING)
+		stmmac_quiesce(priv);
+	priv->datapath = STMMAC_DATAPATH_DOWN;
 
 	/* Free the IRQ lines */
 	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
 
-	/* Stop TX/RX DMA and clear the descriptors */
+	/* Stop TX/RX DMA after draining IRQ handlers which can restart it. */
 	stmmac_stop_all_dma(priv);
 
 	/* Release and free the Rx/Tx resources */
@@ -4296,6 +4312,8 @@ static int stmmac_release(struct net_device *dev)
 	stmmac_legacy_serdes_power_down(priv);
 	phylink_disconnect_phy(priv->phylink);
 	pm_runtime_put(priv->device);
+	/* Allow a fresh open after a failed MTU reopen or resume. */
+	netif_device_attach(dev);
 
 	return 0;
 }
@@ -6174,6 +6192,11 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
 		if (ret) {
 			free_dma_desc_resources(priv, dma_conf);
 			kfree(dma_conf);
+			/*
+			 * Keep the administrative state and PHY/PM ownership until
+			 * ndo_stop(), but prevent use of the released data path.
+			 */
+			netif_device_detach(dev);
 			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
 			return ret;
 		}
@@ -6422,6 +6445,8 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 
 	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
 		return ret;
+	if (!netif_device_present(priv->dev))
+		return -ENETDOWN;
 
 	__stmmac_disable_all_queues(priv);
 
@@ -6543,8 +6568,9 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
 	u8 tx_count = priv->plat->tx_queues_to_use;
 	u8 queue;
 
-	if ((dev->flags & IFF_UP) == 0)
-		return 0;
+	rtnl_lock();
+	if (priv->datapath == STMMAC_DATAPATH_DOWN)
+		goto out_unlock;
 
 	for (queue = 0; queue < rx_count; queue++) {
 		struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
@@ -6578,6 +6604,8 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
 		}
 	}
 
+out_unlock:
+	rtnl_unlock();
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(stmmac_rings_status);
@@ -6959,6 +6987,18 @@ static int stmmac_bpf(struct net_device *dev, struct netdev_bpf *bpf)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 
+	if (bpf->command != XDP_SETUP_PROG &&
+	    bpf->command != XDP_SETUP_XSK_POOL)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Pool removal must succeed even after a failed resume. Release the
+	 * suspended rings before their pool or XDP buffer layout can change.
+	 * Leave the interface detached until it is closed and reopened.
+	 */
+	if (priv->datapath == STMMAC_DATAPATH_SUSPENDED)
+		__stmmac_release(dev);
+
 	switch (bpf->command) {
 	case XDP_SETUP_PROG:
 		return stmmac_xdp_set_prog(priv, bpf->prog, bpf->extack);
@@ -6989,6 +7029,10 @@ static int stmmac_xdp_xmit(struct net_device *dev, int num_frames,
 	nq = netdev_get_tx_queue(priv->dev, queue);
 
 	__netif_tx_lock(nq, cpu);
+	if (unlikely(!netif_device_present(dev) || netif_tx_queue_stopped(nq))) {
+		__netif_tx_unlock(nq);
+		return -ENETDOWN;
+	}
 	/* Avoids TX time-out as we are sharing with slow path */
 	txq_trans_cond_update(nq);
 
@@ -7361,6 +7405,9 @@ static void stmmac_reset_subtask(struct stmmac_priv *priv)
 	netdev_err(priv->dev, "Reset adapter.\n");
 
 	rtnl_lock();
+	if (!netif_device_present(priv->dev))
+		goto out_unlock;
+
 	netif_trans_update(priv->dev);
 	while (test_and_set_bit(STMMAC_RESETING, &priv->state))
 		usleep_range(1000, 2000);
@@ -7370,6 +7417,7 @@ static void stmmac_reset_subtask(struct stmmac_priv *priv)
 	dev_open(priv->dev, NULL);
 	clear_bit(STMMAC_DOWN, &priv->state);
 	clear_bit(STMMAC_RESETING, &priv->state);
+out_unlock:
 	rtnl_unlock();
 }
 
@@ -8198,26 +8246,24 @@ int stmmac_suspend(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct stmmac_priv *priv = netdev_priv(ndev);
-	u8 chan;
 
-	if (!ndev || !netif_running(ndev))
+	rtnl_lock();
+	if (priv->datapath != STMMAC_DATAPATH_RUNNING) {
+		rtnl_unlock();
 		goto suspend_bsp;
+	}
 
 	mutex_lock(&priv->lock);
 
 	netif_device_detach(ndev);
 
-	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);
+	stmmac_quiesce(priv);
 
 	if (priv->eee_sw_timer_en) {
 		priv->tx_path_in_lpi_mode = false;
 		timer_delete_sync(&priv->eee_ctrl_timer);
 	}
 
-	/* Stop TX/RX DMA */
 	stmmac_stop_all_dma(priv);
 
 	stmmac_legacy_serdes_power_down(priv);
@@ -8233,12 +8279,12 @@ int stmmac_suspend(struct device *dev)
 
 	mutex_unlock(&priv->lock);
 
-	rtnl_lock();
 	phylink_suspend(priv->phylink, !!priv->wolopts);
-	rtnl_unlock();
+	priv->datapath = STMMAC_DATAPATH_SUSPENDED;
 
 	if (stmmac_fpe_supported(priv))
 		ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
+	rtnl_unlock();
 
 suspend_bsp:
 	if (priv->plat->suspend)
@@ -8302,8 +8348,11 @@ int stmmac_resume(struct device *dev)
 			return ret;
 	}
 
-	if (!netif_running(ndev))
-		return 0;
+	rtnl_lock();
+	if (priv->datapath != STMMAC_DATAPATH_SUSPENDED) {
+		ret = 0;
+		goto out_unlock;
+	}
 
 	/* Power Down bit, into the PM register, is cleared
 	 * automatically as soon as a magic packet or a Wake-up frame
@@ -8326,11 +8375,9 @@ int stmmac_resume(struct device *dev)
 	if (!(priv->plat->flags & STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP)) {
 		ret = stmmac_legacy_serdes_power_up(priv);
 		if (ret < 0)
-			return ret;
+			goto out_unlock;
 	}
 
-	rtnl_lock();
-
 	/* Prepare the PHY to resume, ensuring that its clocks which are
 	 * necessary for the MAC DMA reset to complete are running
 	 */
@@ -8346,10 +8393,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);
@@ -8371,11 +8415,25 @@ int stmmac_resume(struct device *dev)
 	 * workqueue thread, which will race with initialisation.
 	 */
 	phylink_resume(priv->phylink);
-	rtnl_unlock();
-
+	priv->datapath = STMMAC_DATAPATH_RUNNING;
 	netif_device_attach(ndev);
+	rtnl_unlock();
 
 	return 0;
+
+error_stop_dma:
+	stmmac_stop_all_dma(priv);
+	stmmac_mac_set(priv, priv->ioaddr, false);
+	stmmac_legacy_serdes_power_down(priv);
+	mutex_unlock(&priv->lock);
+	/*
+	 * Keep the suspended data path detached. A later resume may retry, or
+	 * ndo_stop() can release its resources without disabling NAPI again.
+	 */
+out_unlock:
+	rtnl_unlock();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(stmmac_resume);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
index d7e4db7224b0..909219775507 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
@@ -31,7 +31,8 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
 		return err;
 	}
 
-	need_update = netif_running(priv->dev) && stmmac_xdp_is_enabled(priv);
+	need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
+		      stmmac_xdp_is_enabled(priv);
 
 	if (need_update) {
 		napi_disable(&ch->rx_napi);
@@ -69,7 +70,8 @@ static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
 	if (!pool)
 		return -EINVAL;
 
-	need_update = netif_running(priv->dev) && stmmac_xdp_is_enabled(priv);
+	need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
+		      stmmac_xdp_is_enabled(priv);
 
 	if (need_update) {
 		napi_disable(&ch->rxtx_napi);
@@ -107,7 +109,7 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
 	bool need_update;
 	bool if_running;
 
-	if_running = netif_running(dev);
+	if_running = priv->datapath == STMMAC_DATAPATH_RUNNING;
 
 	if (prog && dev->mtu > ETH_DATA_LEN) {
 		/* For now, the driver doesn't support XDP functionality with

-- 
2.53.0

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

[Andrew Lunn]

On Tue, Sep 22, 2026 at 05:27:40PM -0600, James Hilliard wrote:
> If a network driver's system resume fails before phylink_resume(), the
> network device can remain administratively up with phylink suspended.
> Closing the interface then needs to terminate that suspended instance.
> Calling phylink_resume() merely to make phylink_stop() work is not a safe
> substitute: resume reconfigures the MAC and restarts link resolution,
> although the driver has not successfully restored the MAC.

The first thing phylink_resume() does is:

	if (phylink_phy_pm_speed_ctrl(pl))
		phylink_speed_up(pl);

Is it guaranteed that phylink_stop() followed by phylink_start() will
somehow do the equivalent?

	Andrew

Re: [PATCH net v2 2/2] net: stmmac: keep datapath state coherent after reinitialization failure

[Andrew Lunn]

On Tue, Sep 22, 2026 at 05:27:41PM -0600, James Hilliard wrote:
> An MTU change releases the running datapath before reopening it.

This is the real problem. Changing the MTU should not release the
datapath. 

> If the reopen fails

Why is reopen failing?

You want the driver to allocate all the resources it needs before
releasing its old resources. That way you can always undo part way
through and still have a working interface.

	Andrew

Re: [PATCH net v2 1/2] 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() (drivers/net/phy/phylink.c:2506-2508) now
  returns early when PHYLINK_DISABLE_STOPPED is already set.

--

net: phylink: allow stopping a suspended instance

This commit lets phylink_stop() handle an instance that is still
suspended by phylink_suspend(), for example after a driver's resume
failed. Without MAC WoL it returns early, since phylink_suspend() already
called phylink_stop(). With MAC WoL it finishes the deferred
mac_link_down() and clears PHYLINK_DISABLE_MAC_WOL.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a1458da8111b6..df520d77fdedd 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2495,11 +2495,18 @@ EXPORT_SYMBOL_GPL(phylink_start);

[ ... ]

>  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 early return leave the PHY speed-down from phylink_suspend() in
place?

phylink_suspend() does this after its own phylink_stop():

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

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

In phylink, only the phylink_speed_up() call in phylink_resume() undoes
this. With this patch, stopping a suspended instance leaves
phydev->advertising reduced and the saved adv_old still pending. Phylink
still treats the instance as cleanly stopped.

With stmmac, which sets wol_phy_speed_ctrl, this looks reachable with
PHY-only WoL (wolopts_mac == 0 and phy_may_wakeup() true):

1. stmmac_resume() fails before it reaches phylink_resume(). Examples are
   a failure in plat->resume, stmmac_legacy_serdes_power_up() or
   stmmac_hw_setup().

2. The interface is closed while device_may_wakeup(priv->device) is true.
   stmmac_release() then calls phylink_speed_down() a second time:

	if (device_may_wakeup(priv->device))
		phylink_speed_down(priv->phylink, false);

   phy_speed_down() always overwrites adv_old, before it checks whether
   anything changed:

	linkmode_copy(adv_tmp, phydev->advertising);
	ret = phy_speed_down_core(phydev);
	...
	linkmode_copy(phydev->adv_old, adv_tmp);

   This replaces the saved full advertisement with the reduced one.
   __stmmac_release() then calls phylink_stop(), which returns early here.
   phylink_disconnect_phy() runs next.

3. On reopen, phylink_bringup_phy() copies the reduced phy->advertising
   into pl->link_config.advertising. The phylink_speed_up() call in
   stmmac_open() can only restore the reduced adv_old.

Would the link then negotiate only the lowest common speed, for example
10 Mbps instead of 1 Gbps? This would last through later close/open and
suspend/resume cycles until the advertisement is reset with ethtool -s.

The commit message says:

  the driver still decides whether to retry resume or wait for an ordinary
  administrative down/up cycle.

In this case the down/up cycle does not appear to restore the speed. If
device_may_wakeup() is false, the phylink_speed_up() call in stmmac_open()
does restore the advertisement. That fix comes from the driver, though,
and not from the new suspend-to-stop transition in phylink.

Should the suspend-to-stop path in phylink_stop() also call
phylink_speed_up() when phylink_phy_pm_speed_ctrl(pl) is true?

>  	if (pl->sfp_bus)
>  		sfp_upstream_stop(pl->sfp_bus);
>  	if (pl->phydev)

[ ... ]

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

Re: [PATCH net v2 2/2] net: stmmac: keep datapath state coherent after reinitialization failure

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 6 · 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] Changing the XDP program on a running interface releases and
  reopens the datapath but never updates the new priv->datapath state,…
- [Medium] The ethtool reinit paths are also release-then-reopen paths,
  and the patch does not handle their failure.
- [Medium] stmmac_bpf() calls `if (priv->datapath ==
  STMMAC_DATAPATH_SUSPENDED) __stmmac_release(dev);` for every…
- [Medium] The patch adds a lasting state where the interface is IFF_UP
  but datapath is DOWN, and in that state clk_ptp_ref has already been…
- [Medium] STMMAC_DATAPATH_SUSPENDED covers two different situations:
  'the system is asleep right now' and 'hardware resume failed'.
- [Medium] The patch leaves a failed resume in SUSPENDED so that 'a later
  successful resume can retry the retained datapath'.
- [Medium] On Tegra MGBE, tegra_mgbe_resume() calls
  clk_bulk_disable_unprepare() on all MGBE clocks when stmmac_resume()…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  touches it and leaves it reachable.

--

net: stmmac: keep datapath state coherent after reinitialization failure

This commit adds a priv->datapath state (DOWN, RUNNING, SUSPENDED) to
stmmac, so that closing after a failed MTU reopen or a failed hardware
resume only does the teardown that is still needed. It moves suspend and
resume under RTNL and detaches the netdev when an MTU reopen fails. The XDP,
TC, debugfs and reset paths now check either the new state or whether the
device is present.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..7b423c87314ca 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4244,25 +4247,38 @@ static int stmmac_open(struct net_device *dev)

[ ... ]

> +static void __stmmac_release(struct net_device *dev)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +
> +	/* A failed MTU reopen has already released the data path. */
> +	if (priv->datapath == STMMAC_DATAPATH_DOWN)
> +		return;

[Severity: Medium]
Could this early return leak an enable of clk_ptp_ref?

With this patch the interface can stay IFF_UP while priv->datapath is
STMMAC_DATAPATH_DOWN and stmmac_release_ptp() has already released
clk_ptp_ref. That happens after a failed MTU reopen, a failed ethtool
reinit, or stmmac_bpf() releasing a suspended datapath.

stmmac_suspend() and stmmac_resume() now check priv->datapath. The
platform noirq callbacks in stmmac_platform.c still check
netif_running():

stmmac_pltfr_noirq_suspend():
	if (!netif_running(ndev))
		return 0;

	if (!priv->wolopts) {
		/* Disable clock in case of PWM is off */
		clk_disable_unprepare(priv->plat->clk_ptp_ref);

Here is what happens on a system suspend without WoL in this state:

- noirq suspend disables a clock that is already disabled, which
  triggers the clk core warning.
- stmmac_pltfr_noirq_resume() then calls clk_prepare_enable() on
  clk_ptp_ref.
- stmmac_resume() does nothing, because the datapath is not SUSPENDED.
- The later ndo_stop takes this early return and never calls
  stmmac_release_ptp().

Does the PTP reference clock then stay enabled for good, with each later
open/close cycle leaving one extra enable behind?

[ ... ]

> @@ -4296,6 +4312,8 @@ static int stmmac_release(struct net_device *dev)
>  	stmmac_legacy_serdes_power_down(priv);
>  	phylink_disconnect_phy(priv->phylink);
>  	pm_runtime_put(priv->device);
> +	/* Allow a fresh open after a failed MTU reopen or resume. */
> +	netif_device_attach(dev);
>  
>  	return 0;
>  }

[Severity: Medium]
Do the ethtool reinit paths still end up in an inconsistent state when
the reopen fails?

stmmac_reinit_ringparam() (set_ringparam) and stmmac_reinit_queues()
(set_channels) call stmmac_release() and then reopen with:

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

If stmmac_open() fails, its own unwind already calls
phylink_disconnect_phy() and pm_runtime_put(). The interface is left
IFF_UP and present, with priv->datapath at STMMAC_DATAPATH_DOWN.

On the next ndo_stop, __stmmac_release() returns early. stmmac_release()
still calls phylink_disconnect_phy() and pm_runtime_put() unconditionally
here.

Is this pm_runtime_put() unbalanced, so that it triggers the "Runtime PM
usage count underflow!" warning? The detach and PHY/PM ownership handling
added to stmmac_change_mtu() does not seem to cover these callers.

[ ... ]

> @@ -6422,6 +6445,8 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  
>  	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
>  		return ret;
> +	if (!netif_device_present(priv->dev))
> +		return -ENETDOWN;
>  
>  	__stmmac_disable_all_queues(priv);
>  

[Severity: High]
This isn't a bug introduced by this patch, but is netif_device_present()
the right check here?

__stmmac_disable_all_queues() calls napi_disable() on every rx/tx/rxtx
NAPI. In several states the device is present but its NAPIs are already
disabled, or were never enabled:

- the interface is administratively down, including after the failed
  MTU state is closed and stmmac_release() calls netif_device_attach();
- stmmac_reinit_ringparam() or stmmac_reinit_queues() failed in
  stmmac_open();
- an XDP reopen failed.

tc_cls_can_offload_and_chain0() does not check the admin state, so a
sequence such as:

  ip link set dev X down
  tc qdisc add dev X clsact
  tc filter add dev X ingress flower ... skip_sw

would reach napi_disable_locked(), which then spins forever on
NAPIF_STATE_SCHED with RTNL held.

The new state tracks whether NAPI is enabled and the resources are
owned. Would checking priv->datapath == STMMAC_DATAPATH_RUNNING here be
more accurate?

[ ... ]

> @@ -6959,6 +6987,18 @@ static int stmmac_bpf(struct net_device *dev, struct netdev_bpf *bpf)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  
> +	if (bpf->command != XDP_SETUP_PROG &&
> +	    bpf->command != XDP_SETUP_XSK_POOL)
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Pool removal must succeed even after a failed resume. Release the
> +	 * suspended rings before their pool or XDP buffer layout can change.
> +	 * Leave the interface detached until it is closed and reopened.
> +	 */
> +	if (priv->datapath == STMMAC_DATAPATH_SUSPENDED)
> +		__stmmac_release(dev);

[Severity: Medium]
Should this release only happen for requests that actually change the
pool or the program?

It runs for every XDP_SETUP_PROG and XDP_SETUP_XSK_POOL request, before
any validation. That includes requests that are later rejected or do
nothing:

- stmmac_xdp_set_prog() returns -EOPNOTSUPP for a prog when
  dev->mtu > ETH_DATA_LEN
- one program replaces another, so need_update is false
- stmmac_xdp_enable_pool() rejects a small frame_size, or
  xsk_pool_dma_map() fails

In each of these cases the retained rings are freed and the datapath
becomes DOWN. A later stmmac_resume() then does:

	if (priv->datapath != STMMAC_DATAPATH_SUSPENDED) {
		ret = 0;
		goto out_unlock;
	}

It never calls netif_device_attach(), so the interface stays detached
until someone runs a manual down/up.

The commit message only gives a reason for this in the AF_XDP pool
removal case. It also says "A later successful resume can retry the
retained datapath".

[Severity: Medium]
Can this also run during a normal system sleep, and not only after a
failed resume?

stmmac_suspend() sets STMMAC_DATAPATH_SUSPENDED before it drops RTNL. A
failed resume leaves the same state. AF_XDP pool teardown can reach
ndo_bpf from the non-freezable system_wq:

xp_put_pool()
  schedule_work(xp_release_deferred)
    rtnl_lock()
    xp_clear_dev()
      ndo_bpf(XDP_SETUP_XSK_POOL, NULL)
        stmmac_bpf()
          __stmmac_release()

If that work runs after stmmac_suspend() releases RTNL, __stmmac_release()
would:

- call phylink_stop();
- free the IRQs;
- write DMA registers that stmmac_pltfr_noirq_suspend() or
  pm_runtime_force_suspend() may already have clock-gated;
- disable clk_ptp_ref a second time via stmmac_release_ptp().

On wake, stmmac_resume() sees STMMAC_DATAPATH_DOWN and returns 0 without
calling netif_device_attach(). Before this patch, removing a pool with
no XDP program loaded did not touch the hardware.

Is there a way to tell the two SUSPENDED cases apart here?

[ ... ]

> @@ -8198,26 +8246,24 @@ int stmmac_suspend(struct device *dev)
>  {
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> -	u8 chan;
>  
> -	if (!ndev || !netif_running(ndev))
> +	rtnl_lock();
> +	if (priv->datapath != STMMAC_DATAPATH_RUNNING) {
> +		rtnl_unlock();
>  		goto suspend_bsp;
> +	}

[Severity: Medium]
After a failed resume, does the next suspend skip re-arming WoL?

The failed stmmac_resume() has already cleared PMT with
stmmac_pmt(priv, priv->hw, 0) and set irq_wake to 0, or has switched
pinctrl to the default state.

The datapath is left SUSPENDED so that a later resume can retry. On the
next system suspend, though, this check sends stmmac_suspend() straight
to suspend_bsp and skips:

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

With WoL configured, the noirq suspend keeps the clocks on, but the MAC
wake mode is never armed again, so the system will not wake on LAN.
Without WoL, the pins stay in the active pinctrl state through suspend.

Is that intended?

[ ... ]

> @@ -8371,11 +8415,25 @@ int stmmac_resume(struct device *dev)

[ ... ]

> +error_stop_dma:
> +	stmmac_stop_all_dma(priv);
> +	stmmac_mac_set(priv, priv->ioaddr, false);
> +	stmmac_legacy_serdes_power_down(priv);
> +	mutex_unlock(&priv->lock);
> +	/*
> +	 * Keep the suspended data path detached. A later resume may retry, or
> +	 * ndo_stop() can release its resources without disabling NAPI again.
> +	 */
> +out_unlock:
> +	rtnl_unlock();
> +
> +	return ret;
>  }

[Severity: Medium]
On Tegra MGBE, is it safe to recover from this failure with an ordinary
down?

When the common resume fails, tegra_mgbe_resume() in dwmac-tegra.c turns
the clocks off:

	err = stmmac_resume(dev);
	if (err < 0)
		clk_bulk_disable_unprepare(ARRAY_SIZE(mgbe_clks), mgbe->clks);

The datapath stays STMMAC_DATAPATH_SUSPENDED. A later ndo_stop, or the
release in stmmac_bpf(), then runs __stmmac_release() with the MGBE
clocks off:

- phylink_stop() takes the MAC link down and uses MDIO;
- stmmac_stop_all_dma() accesses the XGMAC channel-control and MAC TX
  configuration registers.

Nothing turns these clocks back on first, and the runtime-PM reference
does not cover them. Part of this predates the patch, since the old
ndo_stop also accessed registers through phylink_stop() before it hung.

stmmac_suspend() now returns early when the datapath is not RUNNING. The
next tegra_mgbe_suspend() would therefore call
clk_bulk_disable_unprepare() a second time.

Can this lead to register access on unclocked hardware and to
unbalanced clock disables?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index d7e4db7224b0c..909219775507b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c

[ ... ]

> @@ -107,7 +109,7 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
>  	bool need_update;
>  	bool if_running;
>  
> -	if_running = netif_running(dev);
> +	if_running = priv->datapath == STMMAC_DATAPATH_RUNNING;

[Severity: High]
Does attaching or detaching an XDP program leave priv->datapath stale?

When need_update is true on a running interface, stmmac_xdp_set_prog()
does:

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

stmmac_xdp_release() disables NAPI, frees the IRQs, stops DMA and frees
the rings.

stmmac_xdp_open() can fail in alloc_dma_desc_resources(),
init_dma_desc_rings() or stmmac_request_irq(). Its error labels free the
new rings and return with NAPI disabled and no IRQs requested.

The return value is ignored here, and neither function writes
priv->datapath. It therefore stays STMMAC_DATAPATH_RUNNING, and the
netdev is still present.

After that, would ndo_stop hang?

stmmac_release()
  __stmmac_release()
    if (priv->datapath == STMMAC_DATAPATH_RUNNING)
        stmmac_quiesce(priv);
          stmmac_disable_all_queues()
            napi_disable()   <- already disabled

napi_disable_locked() would spin forever on NAPIF_STATE_SCHED with RTNL
held. It would then be followed by a second stmmac_free_irq() and
free_dma_desc_resources().

Other paths would hit the same state:

- stmmac_suspend(), stmmac_xdp_enable_pool() and
  stmmac_xdp_disable_pool() would reach the same napi_disable();
- stmmac_rings_status_show() only skips the dump for
  STMMAC_DATAPATH_DOWN, so it would read the freed descriptor rings.

The failed-reopen problem here existed before this patch. However, this
commit targets this kind of failed-reopen state and moves this function
onto priv->datapath.

Should stmmac_xdp_release() and stmmac_xdp_open() update priv->datapath?
Should a failed stmmac_xdp_open() also be handled the same way as the
MTU failure path?

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