* [PATCH net v3 01/10] net: stmmac: unwind the WoL IRQ after a safety IRQ request failure
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-24 17:44 ` [PATCH net v3 02/10] net: phylink: allow stopping a suspended instance James Hilliard
` (8 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
The IRQ setup paths request the MAC IRQ, then the optional WoL IRQ, then
the common safety IRQ. If requesting the safety IRQ fails, cleanup must
release the WoL and MAC IRQs, but not the failed safety IRQ.
The REQ_IRQ_ERR_SFTY case instead frees the safety IRQ and skips the WoL
IRQ. Allocation fault injection during live XDP reopening reproduces a
"Trying to free already-free IRQ" warning and leaves the WoL handler
registered after the datapath resources have been released. A subsequent
open can then fail to request that still-owned IRQ.
Move safety IRQ cleanup before REQ_IRQ_ERR_SFTY and WoL IRQ cleanup after
it. This restores reverse acquisition order for both shared and MSI IRQ
setup, including unwind after failures in later per-queue IRQ requests.
Fixes: 5c2215167d12 ("net: stmmac: Add driver support for common safety IRQ")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3f34d491c959..4f3d452c3503 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3797,13 +3797,13 @@ static void stmmac_free_irq(struct net_device *dev,
free_irq(msi->sfty_ce_irq, dev);
fallthrough;
case REQ_IRQ_ERR_SFTY_CE:
- if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
- free_irq(priv->wol_irq, dev);
- fallthrough;
- case REQ_IRQ_ERR_SFTY:
if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq)
free_irq(priv->sfty_irq, dev);
fallthrough;
+ case REQ_IRQ_ERR_SFTY:
+ if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
+ free_irq(priv->wol_irq, dev);
+ fallthrough;
case REQ_IRQ_ERR_WOL:
free_irq(dev->irq, dev);
fallthrough;
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net v3 02/10] net: phylink: allow stopping a suspended instance
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
2026-09-24 17:44 ` [PATCH net v3 01/10] net: stmmac: unwind the WoL IRQ after a safety IRQ request failure James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-24 17:44 ` [PATCH net v3 03/10] net: stmmac: keep datapath state coherent after reinitialization failure James Hilliard
` (7 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, 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(). Do not repeat PHY, SFP and PCS shutdown. However,
phylink_prepare_resume() may have powered that stopped PHY back up to
provide the receive clock for MAC reset. Suspend it again if WoL permits,
without repeating phy_stop() on a PHY which is already halted.
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.
Also undo PHY speed control performed by phylink_suspend(). As Andrew
Lunn pointed out, phylink_start() does not restore the advertised speeds
that phylink_resume() normally restores. Track suspend-owned speed control
and restore the saved advertisement from either resume or suspended stop,
including when PHY shutdown has already completed.
An explicit driver speed-down request, such as stmmac's close-time power
saving, must remain in effect until its matching speed-up. Restore any
suspend-owned reduction before applying that request, so it cannot save
the reduced advertisement over the original one. The following stop must
not undo the driver's new reduction.
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 | 51 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 48 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1bbcf46c8356..b7d30ca34031 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -78,6 +78,7 @@ struct phylink {
bool link_failed;
bool suspend_link_up;
+ bool suspend_speed_down;
bool force_major_config;
bool major_config_failed;
bool mac_supports_eee_ops;
@@ -2498,6 +2499,14 @@ void phylink_start(struct phylink *pl)
}
EXPORT_SYMBOL_GPL(phylink_start);
+static void phylink_restore_suspend_speed(struct phylink *pl)
+{
+ if (pl->suspend_speed_down) {
+ phylink_speed_up(pl);
+ pl->suspend_speed_down = false;
+ }
+}
+
/**
* phylink_stop() - stop a phylink instance
* @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -2509,11 +2518,30 @@ 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.
+ * Any PHY advertisement reduced by phylink_suspend() is restored as part
+ * of this transition.
+ * If phylink_prepare_resume() powered up an already stopped PHY, suspend
+ * it again when Wake-on-LAN permits.
*/
void phylink_stop(struct phylink *pl)
{
ASSERT_RTNL();
+ /* Also undo PHY speed control when terminating a suspended instance. */
+ phylink_restore_suspend_speed(pl);
+
+ if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
+ /* A failed MAC resume may have called phylink_prepare_resume()
+ * and powered the stopped PHY back up to supply its RX clock.
+ */
+ if (pl->phydev)
+ phy_suspend(pl->phydev);
+ return;
+ }
+
if (pl->sfp_bus)
sfp_upstream_stop(pl->sfp_bus);
if (pl->phydev)
@@ -2526,6 +2554,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);
@@ -2657,8 +2695,10 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
phylink_stop(pl);
}
- if (phylink_phy_pm_speed_ctrl(pl))
+ if (phylink_phy_pm_speed_ctrl(pl)) {
phylink_speed_down(pl, false);
+ pl->suspend_speed_down = true;
+ }
}
EXPORT_SYMBOL_GPL(phylink_suspend);
@@ -2698,8 +2738,7 @@ void phylink_resume(struct phylink *pl)
{
ASSERT_RTNL();
- if (phylink_phy_pm_speed_ctrl(pl))
- phylink_speed_up(pl);
+ phylink_restore_suspend_speed(pl);
if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
/* Wake-on-Lan enabled, MAC handling */
@@ -3616,6 +3655,12 @@ int phylink_speed_down(struct phylink *pl, bool sync)
ASSERT_RTNL();
+ /* An explicit request takes over from suspend-time speed control.
+ * Restore the original advertisement before saving it again, so a
+ * repeated speed-down cannot replace it with the reduced advertisement.
+ */
+ phylink_restore_suspend_speed(pl);
+
if (!pl->sfp_bus && pl->phydev)
ret = phy_speed_down(pl->phydev, sync);
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net v3 03/10] net: stmmac: keep datapath state coherent after reinitialization failure
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
2026-09-24 17:44 ` [PATCH net v3 01/10] net: stmmac: unwind the WoL IRQ after a safety IRQ request failure James Hilliard
2026-09-24 17:44 ` [PATCH net v3 02/10] net: phylink: allow stopping a suspended instance James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-25 19:11 ` netdev-bot+sashiko
2026-09-24 17:44 ` [PATCH net v3 04/10] net: stmmac: leave the datapath running for normal-size MTU changes James Hilliard
` (6 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, 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 and its queue-count snapshot 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.
AF_XDP pool removal cannot be rejected: release any suspended rings before
a socket's pool is unmapped and freed, leaving recovery to down/up.
Track completion of the core sleep sequence separately from datapath
ownership. An administratively-up interface with a failed datapath still
needs SerDes shutdown, sleep pinctrl and PHY PM handling. Only quiesce
queues which are actually running, and preserve the resource state so
resume cannot restart queues whose resources were released.
A platform suspend or early resume failure can leave core sleep state
pending without a successful system resume before close. Share the
pinctrl/PMT restoration between resume and close, without restarting the
link or datapath. Use the installed wake state rather than a subsequently
changed WoL setting. Retain the pending marker on restoration errors and
retry on open after acquiring runtime PM; a successful datapath open must
not simply discard outstanding sleep cleanup.
A failed datapath cannot promise a working MAC WoL path and may have
released its wake IRQ. Reject suspend with -EBUSY in that case, before
changing hardware or invoking platform suspend, rather than silently
losing the configured wake source. Ordinary down/up can recover the
interface. PHY-only WoL does not require this MAC wake path.
The platform noirq callbacks must also distinguish ownership from IFF_UP:
XDP cleanup may already have released the PTP clock reference. Do not
disable that reference twice or acquire a new one on resume, while still
performing runtime-PM bus-clock transitions.
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 | 13 ++
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 215 ++++++++++++++++-----
.../net/ethernet/stmicro/stmmac/stmmac_platform.c | 19 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c | 8 +-
4 files changed, 193 insertions(+), 62 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..b35b554e4ab7 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,10 @@ 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;
+ /* Core sleep sequence completed, independently of datapath ownership. */
+ bool hw_suspended;
/* 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 4f3d452c3503..43a6154fcf6d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4127,6 +4127,37 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
return ERR_PTR(ret);
}
+/* Finish core sleep state even if the platform resume callback failed. */
+static int stmmac_resume_hw(struct stmmac_priv *priv)
+{
+ int ret;
+
+ if (!priv->hw_suspended)
+ return 0;
+
+ /* Use the state installed by suspend, not a subsequently changed WoL
+ * setting. Clear PMT even when a different device caused the wakeup.
+ */
+ if (priv->irq_wake) {
+ mutex_lock(&priv->lock);
+ stmmac_pmt(priv, priv->hw, 0);
+ mutex_unlock(&priv->lock);
+ priv->irq_wake = 0;
+ } else {
+ ret = pinctrl_pm_select_default_state(priv->device);
+ if (ret)
+ return ret;
+ if (priv->mii) {
+ ret = stmmac_mdio_reset(priv->mii);
+ if (ret)
+ return ret;
+ }
+ }
+ priv->hw_suspended = false;
+
+ return 0;
+}
+
/**
* __stmmac_open - open entry point of the driver
* @dev : pointer to the device structure.
@@ -4181,6 +4212,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;
@@ -4192,6 +4224,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;
}
@@ -4213,6 +4247,10 @@ static int stmmac_open(struct net_device *dev)
if (ret < 0)
goto err_dma_resources;
+ ret = stmmac_resume_hw(priv);
+ if (ret)
+ goto err_runtime_pm;
+
ret = stmmac_init_phy(dev);
if (ret)
goto err_runtime_pm;
@@ -4246,25 +4284,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 */
@@ -4285,6 +4336,15 @@ static void __stmmac_release(struct net_device *dev)
static int stmmac_release(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
+ int ret;
+
+ /* Resume may have failed before restoring pins or disabling MAC wake.
+ * Complete that cleanup without restarting the link or the datapath.
+ * If it fails, keep hw_suspended set so a fresh open can retry it.
+ */
+ ret = stmmac_resume_hw(priv);
+ if (ret)
+ netdev_err(dev, "failed to restore hardware sleep state: %d\n", ret);
/* If the PHY or MAC has WoL enabled, then the PHY will not be
* suspended when phylink_stop() is called below. Set the PHY
@@ -4298,6 +4358,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;
}
@@ -6177,6 +6239,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;
}
@@ -6425,6 +6492,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);
@@ -6542,12 +6611,14 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
{
struct net_device *dev = seq->private;
struct stmmac_priv *priv = netdev_priv(dev);
- u8 rx_count = priv->plat->rx_queues_to_use;
- u8 tx_count = priv->plat->tx_queues_to_use;
- u8 queue;
+ u8 rx_count, tx_count, queue;
- if ((dev->flags & IFF_UP) == 0)
- return 0;
+ rtnl_lock();
+ if (priv->datapath == STMMAC_DATAPATH_DOWN)
+ goto out_unlock;
+
+ rx_count = priv->plat->rx_queues_to_use;
+ tx_count = priv->plat->tx_queues_to_use;
for (queue = 0; queue < rx_count; queue++) {
struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
@@ -6581,6 +6652,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);
@@ -6962,6 +7035,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);
@@ -6992,6 +7077,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);
@@ -7364,6 +7453,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);
@@ -7373,6 +7465,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();
}
@@ -8193,34 +8286,41 @@ EXPORT_SYMBOL_GPL(stmmac_dvr_remove);
/**
* stmmac_suspend - suspend callback
* @dev: device pointer
- * Description: this is the function to suspend the device and it is called
- * by the platform driver to stop the network queue, release the resources,
- * program the PMT register (for WoL), clean and release driver resources.
+ * Description: stop network activity and program hardware for system sleep,
+ * preserving any datapath resources still owned for resume or close.
*/
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 (!netif_running(ndev) || priv->hw_suspended) {
+ rtnl_unlock();
goto suspend_bsp;
+ }
+
+ /* A failed datapath cannot provide a working MAC wake path. It may
+ * even have released its wake IRQ. Do not silently suspend without WoL.
+ */
+ if (priv->wolopts && priv->datapath != STMMAC_DATAPATH_RUNNING) {
+ netdev_err(ndev, "cannot suspend failed datapath with MAC WoL enabled\n");
+ rtnl_unlock();
+ return -EBUSY;
+ }
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);
+ if (priv->datapath == STMMAC_DATAPATH_RUNNING)
+ 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);
@@ -8236,12 +8336,14 @@ int stmmac_suspend(struct device *dev)
mutex_unlock(&priv->lock);
- rtnl_lock();
phylink_suspend(priv->phylink, !!priv->wolopts);
- rtnl_unlock();
+ if (priv->datapath == STMMAC_DATAPATH_RUNNING)
+ priv->datapath = STMMAC_DATAPATH_SUSPENDED;
+ priv->hw_suspended = true;
if (stmmac_fpe_supported(priv))
ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
+ rtnl_unlock();
suspend_bsp:
if (priv->plat->suspend)
@@ -8305,35 +8407,35 @@ int stmmac_resume(struct device *dev)
return ret;
}
- if (!netif_running(ndev))
- return 0;
+ rtnl_lock();
+ if (!netif_running(ndev)) {
+ 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
- * is received. Anyway, it's better to manually clear
- * this bit because it can generate problems while resuming
- * from another devices (e.g. serial console).
- */
- if (priv->wolopts) {
- mutex_lock(&priv->lock);
- stmmac_pmt(priv, priv->hw, 0);
- mutex_unlock(&priv->lock);
- priv->irq_wake = 0;
- } else {
- pinctrl_pm_select_default_state(priv->device);
- /* reset the phy so that it's ready */
- if (priv->mii)
- stmmac_mdio_reset(priv->mii);
+ if (priv->hw_suspended) {
+ ret = stmmac_resume_hw(priv);
+ if (ret)
+ goto out_unlock;
+
+ /* Terminate PM speed control without restarting a datapath
+ * whose IRQs or rings were released before system sleep.
+ */
+ if (priv->datapath != STMMAC_DATAPATH_SUSPENDED)
+ phylink_stop(priv->phylink);
+ }
+
+ if (priv->datapath != STMMAC_DATAPATH_SUSPENDED) {
+ ret = 0;
+ goto out_unlock;
}
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
*/
@@ -8349,10 +8451,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);
@@ -8374,11 +8473,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_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 6128ed1bd521..9bd622517389 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -958,8 +958,9 @@ static int __maybe_unused stmmac_pltfr_noirq_suspend(struct device *dev)
return 0;
if (!priv->wolopts) {
- /* Disable clock in case of PWM is off */
- clk_disable_unprepare(priv->plat->clk_ptp_ref);
+ /* A detached datapath may already have released its PTP clock. */
+ if (priv->datapath != STMMAC_DATAPATH_DOWN)
+ clk_disable_unprepare(priv->plat->clk_ptp_ref);
ret = pm_runtime_force_suspend(dev);
if (ret)
@@ -984,12 +985,14 @@ static int __maybe_unused stmmac_pltfr_noirq_resume(struct device *dev)
if (ret)
return ret;
- ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
- if (ret < 0) {
- netdev_warn(priv->dev,
- "failed to enable PTP reference clock: %pe\n",
- ERR_PTR(ret));
- return ret;
+ if (priv->datapath != STMMAC_DATAPATH_DOWN) {
+ ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
+ if (ret < 0) {
+ netdev_warn(priv->dev,
+ "failed to enable PTP reference clock: %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
}
}
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net v3 03/10] net: stmmac: keep datapath state coherent after reinitialization failure
2026-09-24 17:44 ` [PATCH net v3 03/10] net: stmmac: keep datapath state coherent after reinitialization failure James Hilliard
@ 2026-09-25 19:11 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 19:11 UTC (permalink / raw)
To: james.hilliard1
Cc: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni,
qiangqing.zhang, rmk+kernel, maxime.chevallier, andrew+netdev,
mcoquelin.stm32, alexandre.torgue, ansuelsmth, yangtiezhu,
chenhuacai, ast, daniel, hawk, john.fastabend, sdf,
fancer.lancer, quic_jsuraj, richardcochran, Joao.Pinto,
vladimir.oltean, boon.leong.ong, weifeng.voon, yoong.siang.song,
richard.genoud, alastair, mripard, netdev, linux-kernel,
linux-stm32, linux-arm-kernel, bpf, zhaojinming,
lorenzo.bianconi, dinghui1111
Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.
Critical: 0 · High: 3 (2) · Medium: 4 (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).
- [High] stmmac_resume_hw() now returns the error from
stmmac_mdio_reset().
- [High] (Baseline: fixed later in the series.) In this commit,
stmmac_xdp_set_prog() gates on priv->datapath == RUNNING, but…
- [High] (Baseline: fixed later in the series.) In this commit, the
irq_error path of stmmac_xdp_open() frees the descriptor rings after…
- [Medium] stmmac_bpf() calls __stmmac_release() whenever datapath ==
STMMAC_DATAPATH_SUSPENDED (and also HALTED at the end of the series).
- [Medium] In stmmac_suspend(), the early exit on `priv->hw_suspended`
(goto suspend_bsp) comes before the new check that rejects suspend…
- [Medium] After an early resume failure, the new close-time sleep
cleanup touches MAC registers without confirming that the hardware's…
- [Medium] (Baseline: fixed later in the series.) In this commit,
stmmac_reinit_queues() and stmmac_reinit_ringparam() call the full…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
a guard at this exact spot and the guard does not cover it.
- [High] (Baseline: preexisting and fixed later in the series.)
stmmac_quiesce() cancels the TX coalescing hrtimers before…
- [Medium] (Baseline: preexisting, not introduced.) If
stmmac_pltfr_noirq_resume() fails in pm_runtime_force_resume() or in…
--
net: stmmac: keep datapath state coherent after reinitialization failure
This adds a datapath ownership state and a hw_suspended flag to
stmmac_priv. Close, suspend, resume and XDP paths use them to tell which
IRQ, DMA and NAPI resources are still owned after a failed MTU reopen or
a failed resume. It splits queue quiescing out of __stmmac_release(),
serializes suspend and resume with RTNL, and adds guards to the TC,
debugfs, XDP xmit and reset task paths.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 4f3d452c35036..43a6154fcf6d7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4127,6 +4127,37 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
> return ERR_PTR(ret);
> }
>
> +/* Finish core sleep state even if the platform resume callback failed. */
> +static int stmmac_resume_hw(struct stmmac_priv *priv)
> +{
> + int ret;
> +
> + if (!priv->hw_suspended)
> + return 0;
> +
> + /* Use the state installed by suspend, not a subsequently changed WoL
> + * setting. Clear PMT even when a different device caused the wakeup.
> + */
> + if (priv->irq_wake) {
> + mutex_lock(&priv->lock);
> + stmmac_pmt(priv, priv->hw, 0);
> + mutex_unlock(&priv->lock);
> + priv->irq_wake = 0;
> + } else {
> + ret = pinctrl_pm_select_default_state(priv->device);
> + if (ret)
> + return ret;
> + if (priv->mii) {
> + ret = stmmac_mdio_reset(priv->mii);
> + if (ret)
> + return ret;
[Severity: High]
On boards that use the snps,reset GPIO, could this return -EBUSY on
every resume without WoL?
stmmac_mdio_reset() requests the GPIO each time it runs:
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c:stmmac_mdio_reset() {
...
reset_gpio = devm_gpiod_get_optional(priv->device,
"snps,reset",
GPIOD_OUT_LOW);
if (IS_ERR(reset_gpio))
return PTR_ERR(reset_gpio);
...
}
The first call happens at probe through __mdiobus_register()->bus->reset().
Devres then keeps the line requested until unbind. Later calls reach
gpiod_request_commit(), which returns -EBUSY for a line that is already
requested, because GPIOD_OUT_LOW is not a nonexclusive request.
Before this patch, stmmac_resume() ignored this return value. Now it does:
if (priv->hw_suspended) {
ret = stmmac_resume_hw(priv);
if (ret)
goto out_unlock;
On this error it skips stmmac_hw_setup(). The datapath stays
STMMAC_DATAPATH_SUSPENDED, the netdev stays detached and hw_suspended
stays set.
After that, stmmac_open() calls stmmac_resume_hw() and fails the same
way, while stmmac_release() only logs the error. Would the interface stay
unusable until the driver is rebound?
> + }
> + }
> + priv->hw_suspended = false;
> +
> + return 0;
> +}
[ ... ]
> @@ -4192,6 +4224,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;
> }
[Severity: High]
This stops DMA on the __stmmac_open() failure path. Does stmmac_xdp_open()
still have the same problem? In this commit its error path is:
irq_error:
for (chan = 0; ...)
hrtimer_cancel(...);
init_error:
free_dma_desc_resources(priv, &priv->dma_conf);
By this point stmmac_mac_set(priv, priv->ioaddr, true) and
stmmac_start_all_dma() have already run.
If the IRQ request fails during an XDP reopen, can the running DMA write
into the freed descriptor rings?
The later commit "net: stmmac: account for failed live XDP reopening" in
this series adds stmmac_stop_tx_queues(), stmmac_stop_all_dma() and
stmmac_mac_set(false) to that path.
[ ... ]
> @@ -4246,25 +4284,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);
> +}
[Severity: High]
This isn't a bug introduced by this patch, but stmmac_quiesce() keeps the
old ordering. It cancels the TX coalescing hrtimers before
netif_tx_disable(), and it does not drain NAPI poll tails. Can
stmmac_xmit() or stmmac_napi_poll_tx()->stmmac_tx_timer_arm() re-arm a
txtimer after its final hrtimer_cancel()?
On the MTU path, __stmmac_open() then copies the new dma_conf over the
embedded timer. That could overwrite a queued hrtimer.
Later in this series, stmmac_quiesce() uses stmmac_stop_tx_queues(),
which calls netif_tx_disable() and synchronize_net() before
hrtimer_cancel(). The commit "net: stmmac: keep DMA configurations at
stable addresses" also replaces the dma_conf copy with a stable pointer.
[ ... ]
> @@ -4285,6 +4336,15 @@ static void __stmmac_release(struct net_device *dev)
> static int stmmac_release(struct net_device *dev)
> {
> struct stmmac_priv *priv = netdev_priv(dev);
> + int ret;
> +
> + /* Resume may have failed before restoring pins or disabling MAC wake.
> + * Complete that cleanup without restarting the link or the datapath.
> + * If it fails, keep hw_suspended set so a fresh open can retry it.
> + */
> + ret = stmmac_resume_hw(priv);
> + if (ret)
> + netdev_err(dev, "failed to restore hardware sleep state: %d\n", ret);
[Severity: Medium]
Is it safe to touch MAC registers here after priv->plat->resume() has
failed? In that case stmmac_resume() returns early and leaves
hw_suspended set:
if (priv->plat->resume) {
ret = priv->plat->resume(dev, priv->plat->bsp_priv);
if (ret)
return ret;
}
Glue suspend callbacks, such as the one in dwmac-rk, turn off clocks and
regulators. Only the resume callback that just failed turns them back on.
A later close then runs stmmac_release()->stmmac_resume_hw(). That does
the PMT register write, or the dummy writel() in stmmac_mdio_reset().
Next, __stmmac_release() runs stmmac_stop_all_dma() and stmmac_mac_set()
on the retained datapath. Neither step takes pm_runtime_resume_and_get()
or checks the power state first.
Before this patch, the same close hung in napi_disable() before it got
here. Depending on how far the glue resume got, could this now cause a
bus abort or silently dropped writes?
[ ... ]
> @@ -4298,6 +4358,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]
How do the ethtool reinit paths behave in this commit?
stmmac_reinit_queues() and stmmac_reinit_ringparam() still do:
if (netif_running(dev))
stmmac_release(dev);
...
if (netif_running(dev))
ret = stmmac_open(dev);
If that stmmac_open() fails, stmmac_release() has already dropped the PHY
and runtime PM references. IFF_UP is still set and the device is still
attached.
Would the next ndo_stop call pm_runtime_put() a second time and underflow
the usage count? In that state the netif_device_present() guards in the
TC callback and the reset task would not fire either.
The later commit "net: stmmac: retain PHY and PM ownership during ethtool
reopening" changes both functions to use netif_device_detach(),
__stmmac_release() and stmmac_reopen().
[ ... ]
> @@ -6425,6 +6492,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 does this guard cover an
interface that is administratively down? In that state the device is
present and NAPI is disabled. That holds for a device that was never
opened, and stmmac_release() now calls netif_device_attach() explicitly.
Adding a u32 or flower filter in that state still reaches
__stmmac_disable_all_queues():
if (queue < rx_queues_cnt)
napi_disable(&ch->rx_napi);
napi_disable_locked() then loops forever with RTNL held:
while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
usleep_range(20, 200);
Would checking priv->datapath == STMMAC_DATAPATH_RUNNING here work better
than netif_device_present()?
[ ... ]
> @@ -6962,6 +7035,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]
Can this run in the middle of an ordinary system suspend? The check does
not look at hw_suspended, so it fires for any suspended datapath. Later in
the series the same applies to the halted state.
XSK pool teardown runs from xp_release_deferred(). That work is queued with
schedule_work() on system_wq, which is not frozen during suspend, and it
takes only RTNL:
rtnl_lock();
xp_clear_dev(pool);
rtnl_unlock();
So the work can run after stmmac_suspend() has dropped RTNL and called
plat->suspend, or after stmmac_pltfr_noirq_suspend(). The noirq callbacks
also read priv->datapath without RTNL, although the new stmmac_priv
comment says the field is serialized by RTNL.
Suppose noirq suspend has already done clk_disable_unprepare(clk_ptp_ref)
and pm_runtime_force_suspend(). __stmmac_release() would then call
stmmac_free_irq(), stmmac_stop_all_dma() and stmmac_mac_set() on a
clock-gated MAC. stmmac_release_ptp() would also underflow the clk_ptp_ref
enable and prepare counts.
[ ... ]
> @@ -8193,34 +8286,41 @@ EXPORT_SYMBOL_GPL(stmmac_dvr_remove);
[ ... ]
> 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 (!netif_running(ndev) || priv->hw_suspended) {
> + rtnl_unlock();
> goto suspend_bsp;
> + }
> +
> + /* A failed datapath cannot provide a working MAC wake path. It may
> + * even have released its wake IRQ. Do not silently suspend without WoL.
> + */
> + if (priv->wolopts && priv->datapath != STMMAC_DATAPATH_RUNNING) {
> + netdev_err(ndev, "cannot suspend failed datapath with MAC WoL enabled\n");
> + rtnl_unlock();
> + return -EBUSY;
> + }
[Severity: Medium]
Can the hw_suspended shortcut above skip this WoL rejection? Consider
this sequence:
1. Suspend with MAC WoL sets irq_wake = 1, sets hw_suspended = true and
moves the datapath to STMMAC_DATAPATH_SUSPENDED.
2. priv->plat->resume() fails, so stmmac_resume() returns before it
clears hw_suspended.
3. An XDP program change or AF_XDP pool teardown calls
stmmac_bpf()->__stmmac_release(). That frees all IRQs, including the
wake IRQ, and sets the datapath to STMMAC_DATAPATH_DOWN.
4. On the next system suspend, hw_suspended is still set, so this
function jumps straight to suspend_bsp.
The -EBUSY check never runs, and the system suspends with MAC WoL
configured but no wake IRQ. The commit message says this case is rejected
"rather than silently losing the configured wake source".
Should the WoL check come before the hw_suspended shortcut?
[ ... ]
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 6128ed1bd521a..9bd622517389d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
[ ... ]
> @@ -984,12 +985,14 @@ static int __maybe_unused stmmac_pltfr_noirq_resume(struct device *dev)
> if (ret)
> return ret;
>
> - ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> - if (ret < 0) {
> - netdev_warn(priv->dev,
> - "failed to enable PTP reference clock: %pe\n",
> - ERR_PTR(ret));
> - return ret;
> + if (priv->datapath != STMMAC_DATAPATH_DOWN) {
> + ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> + if (ret < 0) {
> + netdev_warn(priv->dev,
> + "failed to enable PTP reference clock: %pe\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> }
> }
[Severity: Medium]
This is a pre-existing issue, but consider a failure in
pm_runtime_force_resume() or clk_prepare_enable(clk_ptp_ref) here. The
function then returns without holding the PTP clock reference, while the
datapath is still not STMMAC_DATAPATH_DOWN. Would a later close, through
stmmac_release_ptp(), or the next noirq suspend then disable clk_ptp_ref
a second time?
The pre-patch code had the same sequence. The new datapath checks only
cover the XDP cleanup case.
> 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;
>
> if (prog && dev->mtu > ETH_DATA_LEN) {
> /* For now, the driver doesn't support XDP functionality with
[Severity: High]
In this commit, stmmac_xdp_release() and stmmac_xdp_open() never update
priv->datapath. Further down, the return value of stmmac_xdp_open() is
ignored:
if (if_running && need_update)
stmmac_xdp_open(dev);
If that reopen fails, NAPI is disabled and the IRQs and rings are freed,
but the datapath stays STMMAC_DATAPATH_RUNNING.
Would a later close then call stmmac_quiesce() from __stmmac_release() and
hang in napi_disable() on NAPI that is already disabled? It could also
free the IRQs and rings a second time. Suspend, AF_XDP pool changes and
the debugfs ring dump would all trust the stale running state too.
The later commit "net: stmmac: account for failed live XDP reopening" in
this series fixes this. It sets the datapath to STMMAC_DATAPATH_DOWN in
stmmac_xdp_release(), sets it to running only when stmmac_xdp_open()
succeeds, and handles the reopen error in stmmac_xdp_set_prog().
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net v3 04/10] net: stmmac: leave the datapath running for normal-size MTU changes
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
` (2 preceding siblings ...)
2026-09-24 17:44 ` [PATCH net v3 03/10] net: stmmac: keep datapath state coherent after reinitialization failure James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-24 17:44 ` [PATCH net v3 05/10] net: stmmac: unwind partially allocated DMA configurations James Hilliard
` (5 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
Changing an MTU at or below ETH_DATA_LEN does not change the receive
buffer size or the MAC receive limit when the previous MTU was also in
that range. Do not release and reopen the datapath for those changes.
Besides avoiding unnecessary hardware resets and their failure paths,
this keeps live AF_XDP pool bindings intact. Preparing replacement rings
before stopping the old rings otherwise binds the pool to a temporary
RXQ and consumes fill-ring entries while its current RXQ is still active.
XDP already rejects jumbo MTUs, so all supported live XDP MTU changes can
use this path without preparing replacement rings.
Keep jumbo transitions on the existing reinitialization path for now.
The later ownership and rollback changes address that path separately.
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_main.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 43a6154fcf6d..f7e91ae21448 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6223,7 +6223,12 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
if ((txfifosz < new_mtu) || (new_mtu > BUF_SIZE_16KiB))
return -EINVAL;
- if (netif_running(dev)) {
+ /* Normal-size frames use the same buffers and MAC receive limits.
+ * In particular, do not disturb a live AF_XDP pool: XDP does not
+ * support jumbo frames, so it never needs the ring replacement below.
+ */
+ if (netif_running(dev) &&
+ (dev->mtu > ETH_DATA_LEN || mtu > ETH_DATA_LEN)) {
netdev_dbg(priv->dev, "restarting interface to change its MTU\n");
/* Try to allocate the new DMA conf with the new mtu */
dma_conf = stmmac_setup_dma_desc(priv, mtu);
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net v3 05/10] net: stmmac: unwind partially allocated DMA configurations
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
` (3 preceding siblings ...)
2026-09-24 17:44 ` [PATCH net v3 04/10] net: stmmac: leave the datapath running for normal-size MTU changes James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-25 19:11 ` netdev-bot+sashiko
2026-09-24 17:44 ` [PATCH net v3 06/10] net: stmmac: keep DMA configurations at stable addresses James Hilliard
` (4 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
RX and TX allocation failure paths release all configured queues, even
when the failing queue has not allocated its buffer array or descriptors.
Only walk buffer arrays and free coherent memory when those allocations
exist. If TX allocation fails after RX allocation succeeds, release the
RX resources too.
Propagate RXQ memory-model registration errors instead of continuing
with an unusable RXQ. Clear XSK RXQ bindings before the RXQ goes away,
release any saved partial packet, and clear released resource pointers.
This also leaves configurations which are reused after close or XDP
reconfiguration with a consistent empty state.
The MTU transaction added later relies on preparation failures being
fully unwound without touching the currently active configuration.
Fixes: 71fedb0198cb ("net: stmmac: break some functions into RX and TX scopes")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 49 ++++++++++++++++++-----
1 file changed, 39 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f7e91ae21448..3b918cf89806 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1761,6 +1761,9 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
int i;
+ if (!rx_q->buf_pool)
+ return;
+
for (i = 0; i < dma_conf->dma_rx_size; i++)
stmmac_free_rx_buffer(priv, rx_q, i);
}
@@ -1884,17 +1887,19 @@ static int __init_dma_rx_desc_rings(struct stmmac_priv *priv,
rx_q->xsk_pool = stmmac_get_xsk_pool(priv, queue);
if (rx_q->xsk_pool) {
- WARN_ON(xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
- MEM_TYPE_XSK_BUFF_POOL,
- NULL));
+ ret = xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
+ MEM_TYPE_XSK_BUFF_POOL, NULL);
+ if (ret)
+ return ret;
netdev_info(priv->dev,
"Register MEM_TYPE_XSK_BUFF_POOL RxQ-%d\n",
queue);
xsk_pool_set_rxq_info(rx_q->xsk_pool, &rx_q->xdp_rxq);
} else {
- WARN_ON(xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
- MEM_TYPE_PAGE_POOL,
- rx_q->page_pool));
+ ret = xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
+ MEM_TYPE_PAGE_POOL, rx_q->page_pool);
+ if (ret)
+ return ret;
netdev_info(priv->dev,
"Register MEM_TYPE_PAGE_POOL RxQ-%d\n",
queue);
@@ -1956,6 +1961,8 @@ static int init_dma_rx_desc_rings(struct net_device *dev,
dma_free_rx_skbufs(priv, dma_conf, queue);
rx_q->buf_alloc_num = 0;
+ if (rx_q->xsk_pool)
+ xsk_pool_set_rxq_info(rx_q->xsk_pool, NULL);
rx_q->xsk_pool = NULL;
queue--;
@@ -2099,6 +2106,9 @@ static void dma_free_tx_skbufs(struct stmmac_priv *priv,
tx_q->xsk_frames_done = 0;
+ if (!tx_q->tx_skbuff_dma || !tx_q->tx_skbuff)
+ return;
+
for (i = 0; i < dma_conf->dma_tx_size; i++)
stmmac_free_tx_buffer(priv, dma_conf, queue, i);
@@ -2137,10 +2147,16 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
void *addr;
/* Release the DMA RX socket buffers */
- if (rx_q->xsk_pool)
+ if (rx_q->xsk_pool) {
dma_free_rx_xskbufs(priv, dma_conf, queue);
- else
+ xsk_pool_set_rxq_info(rx_q->xsk_pool, NULL);
+ } else {
dma_free_rx_skbufs(priv, dma_conf, queue);
+ }
+ if (rx_q->state_saved)
+ dev_kfree_skb_any(rx_q->state.skb);
+ rx_q->state.skb = NULL;
+ rx_q->state_saved = 0;
rx_q->buf_alloc_num = 0;
rx_q->xsk_pool = NULL;
@@ -2153,7 +2169,8 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
- dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
+ if (addr)
+ dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
if (xdp_rxq_info_is_reg(&rx_q->xdp_rxq))
xdp_rxq_info_unreg(&rx_q->xdp_rxq);
@@ -2161,6 +2178,10 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
kfree(rx_q->buf_pool);
if (rx_q->page_pool)
page_pool_destroy(rx_q->page_pool);
+ rx_q->buf_pool = NULL;
+ rx_q->page_pool = NULL;
+ rx_q->dma_erx = NULL;
+ rx_q->dma_rx = NULL;
}
static void free_dma_rx_desc_resources(struct stmmac_priv *priv,
@@ -2201,10 +2222,16 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
- dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);
+ if (addr)
+ dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);
kfree(tx_q->tx_skbuff_dma);
kfree(tx_q->tx_skbuff);
+ tx_q->tx_skbuff_dma = NULL;
+ tx_q->tx_skbuff = NULL;
+ tx_q->dma_etx = NULL;
+ tx_q->dma_entx = NULL;
+ tx_q->dma_tx = NULL;
}
static void free_dma_tx_desc_resources(struct stmmac_priv *priv,
@@ -2411,6 +2438,8 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv,
return ret;
ret = alloc_dma_tx_desc_resources(priv, dma_conf);
+ if (ret)
+ free_dma_rx_desc_resources(priv, dma_conf);
return ret;
}
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net v3 05/10] net: stmmac: unwind partially allocated DMA configurations
2026-09-24 17:44 ` [PATCH net v3 05/10] net: stmmac: unwind partially allocated DMA configurations James Hilliard
@ 2026-09-25 19:11 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 19:11 UTC (permalink / raw)
To: james.hilliard1
Cc: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni,
qiangqing.zhang, rmk+kernel, maxime.chevallier, andrew+netdev,
mcoquelin.stm32, alexandre.torgue, ansuelsmth, yangtiezhu,
chenhuacai, ast, daniel, hawk, john.fastabend, sdf,
fancer.lancer, quic_jsuraj, richardcochran, Joao.Pinto,
vladimir.oltean, boon.leong.ong, weifeng.voon, yoong.siang.song,
richard.genoud, alastair, mripard, netdev, linux-kernel,
linux-stm32, linux-arm-kernel, bpf, zhaojinming,
lorenzo.bianconi, dinghui1111
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 0 (2)
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] Possible double free of a saved RX skb.
- [Medium] Baseline decision: discard, because it is resolved later in
the series.
Pre-existing issues:
- [Low] Baseline decision: discard, because it is resolved later in the
series.
- [Low] Baseline decision: discard, because it predates this patch and is
resolved later in the series.
--
net: stmmac: unwind partially allocated DMA configurations
This commit lets the RX and TX DMA free paths handle partially allocated
queues, and releases RX resources when TX allocation fails. It propagates
xdp_rxq_info_reg_mem_model() errors and clears XSK RXQ bindings. It also
frees any saved partial RX skb and NULLs the released resource pointers.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f7e91ae214483..3b918cf898067 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -1884,17 +1887,19 @@ static int __init_dma_rx_desc_rings(struct stmmac_priv *priv,
> rx_q->xsk_pool = stmmac_get_xsk_pool(priv, queue);
>
> if (rx_q->xsk_pool) {
> - WARN_ON(xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> - MEM_TYPE_XSK_BUFF_POOL,
> - NULL));
> + ret = xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> + MEM_TYPE_XSK_BUFF_POOL, NULL);
> + if (ret)
> + return ret;
> netdev_info(priv->dev,
> "Register MEM_TYPE_XSK_BUFF_POOL RxQ-%d\n",
> queue);
> xsk_pool_set_rxq_info(rx_q->xsk_pool, &rx_q->xdp_rxq);
[Severity: Low]
This problem already existed before this patch, but is the XSK pool bound
here to an xdp_rxq that is about to be freed?
On the stmmac_open() path, __init_dma_rx_desc_rings() works on the
temporary dma_conf from stmmac_setup_dma_desc(). __stmmac_open() then
copies that conf:
memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
After that, stmmac_open() calls kfree(dma_conf). Nothing rebinds the pool
to &priv->dma_conf.rx_queue[queue].xdp_rxq, so the pool's rxq pointers
seem to point into freed memory.
The next patch in the series, "net: stmmac: keep DMA configurations at
stable addresses", appears to fix this. It keeps the allocated dma_conf
and removes the memcpy.
> } else {
> - WARN_ON(xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> - MEM_TYPE_PAGE_POOL,
> - rx_q->page_pool));
> + ret = xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> + MEM_TYPE_PAGE_POOL, rx_q->page_pool);
> + if (ret)
> + return ret;
[Severity: Medium]
Now that this error is returned, how do the per-queue callers handle it?
stmmac_enable_rx_queue() returns void. When this call fails, it releases
the queue and returns:
ret = __init_dma_rx_desc_rings(priv, &priv->dma_conf, queue, GFP_KERNEL);
if (ret) {
__free_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
netdev_err(priv->dev, "Failed to init RX desc.\n");
return;
}
With this patch, __free_dma_rx_desc_resources() also sets buf_pool,
dma_rx, dma_erx and page_pool to NULL. After that,
stmmac_xdp_enable_pool() and stmmac_xdp_disable_pool() in stmmac_xdp.c
carry on as usual:
stmmac_enable_rx_queue(priv, queue);
stmmac_enable_tx_queue(priv, queue);
napi_enable(&ch->rx_napi);
napi_enable(&ch->tx_napi);
Both return 0, so NAPI can run on a released RX queue whose ring pointers
are NULL. stmmac_xdp_set_prog() also ignores the return value of
stmmac_xdp_open().
These callers already behaved this way for the -ENOMEM return from
stmmac_alloc_rx_buffers(). This patch adds a second way to reach it.
Two later patches in the series appear to fix this: "net: stmmac: account
for failed live XDP reopening" and "net: stmmac: use the tracked datapath
restart for XSK pool changes". They check the stmmac_xdp_open() result
and remove the per-queue enable helpers.
[ ... ]
> @@ -2137,10 +2147,16 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
> void *addr;
>
> /* Release the DMA RX socket buffers */
> - if (rx_q->xsk_pool)
> + if (rx_q->xsk_pool) {
> dma_free_rx_xskbufs(priv, dma_conf, queue);
> - else
> + xsk_pool_set_rxq_info(rx_q->xsk_pool, NULL);
> + } else {
> dma_free_rx_skbufs(priv, dma_conf, queue);
> + }
> + if (rx_q->state_saved)
> + dev_kfree_skb_any(rx_q->state.skb);
> + rx_q->state.skb = NULL;
> + rx_q->state_saved = 0;
[Severity: High]
Can this free an skb that the driver no longer owns?
This free assumes that state_saved means the driver still owns
rx_q->state.skb. However, stmmac_rx() does not clear the saved state when
it takes it:
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx() {
...
if (!count && rx_q->state_saved) {
skb = rx_q->state.skb;
error = rx_q->state.error;
len = rx_q->state.len;
} else {
rx_q->state_saved = false;
...
}
state_saved is only cleared in the else branch, which runs on a later
iteration with count > 0. With limit == 1, the first iteration can finish
the restored skb. It can pass it to napi_gro_receive(), or free it on the
error path:
if (unlikely(error)) {
dev_kfree_skb(skb);
skb = NULL;
count++;
continue;
}
count is now 1, so the while (count < limit) loop exits. The save block
after the loop is skipped, because skb is NULL and the last descriptor
did not have rx_not_ls set:
if (status & rx_not_ls || skb) {
rx_q->state_saved = true;
rx_q->state.skb = skb;
...
}
state_saved stays set, and state.skb points to an skb that was freed or
handed to the stack. A later poll that hits a DMA-owned descriptor right
away breaks before using the skb and saves the same stale pointer again,
so the stale pointer can survive across polls.
No privileges are needed to get a budget of 1. The EPIOCSPARAMS ioctl in
fs/eventpoll.c only requires CAP_NET_ADMIN when busy_poll_budget >
NAPI_POLL_WEIGHT, and stmmac_napi_poll_rx() passes the budget straight to
stmmac_rx(). A frame that spans several descriptors needs a jumbo MTU.
Suppose the interface is then closed (ndo_stop -> free_dma_desc_resources)
or the XSK pool or XDP program is changed. __free_dma_rx_desc_resources()
then calls dev_kfree_skb_any() on the stale pointer. Before this patch,
teardown dropped the stale pointer without freeing it.
The stale state in stmmac_rx() is older than this patch, and so is its
reuse on the next poll. This patch adds a new path that frees it a second
time. A later patch in the series copies the same pattern into
stmmac_reinit_dma_desc(), and stmmac_rx() is still unchanged at the end
of the series.
Could stmmac_rx() clear rx_q->state_saved and rx_q->state.skb as soon as
it takes the saved state?
[ ... ]
> @@ -2411,6 +2438,8 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv,
> return ret;
>
> ret = alloc_dma_tx_desc_resources(priv, dma_conf);
> + if (ret)
> + free_dma_rx_desc_resources(priv, dma_conf);
>
> return ret;
> }
[Severity: Low]
This one also predates the patch: stmmac_enable_rx_queue() and
stmmac_enable_tx_queue() do not unwind in the same way. When
__alloc_dma_rx_desc_resources() fails:
ret = __alloc_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
if (ret) {
netdev_err(priv->dev, "Failed to alloc RX desc.\n");
return;
}
Does this leak the page_pool, buf_pool and descriptor memory that were
already allocated? stmmac_enable_tx_queue() has the same early return,
which would leak tx_skbuff_dma, tx_skbuff and the TX descriptor memory.
"net: stmmac: use the tracked datapath restart for XSK pool changes",
later in the series, removes both helpers. After that, pool changes go
through stmmac_xdp_open() and alloc_dma_desc_resources().
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net v3 06/10] net: stmmac: keep DMA configurations at stable addresses
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
` (4 preceding siblings ...)
2026-09-24 17:44 ` [PATCH net v3 05/10] net: stmmac: unwind partially allocated DMA configurations James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-25 19:12 ` netdev-bot+sashiko
2026-09-24 17:44 ` [PATCH net v3 07/10] net: stmmac: account for failed live XDP reopening James Hilliard
` (3 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
The allocated DMA configuration contains RXQ metadata registered with
XDP and referenced by AF_XDP pools. Copying the configuration into priv
and freeing its original allocation leaves those references pointing at
the old storage. Retain the allocated object instead and keep a pointer
in priv, preserving ring sizes and per-queue settings while down.
Use persistent channel objects for per-queue IRQ contexts instead of
deriving priv from an embedded DMA configuration. XSK wakeup must also
avoid accessing replaceable queue objects. Drain transmitters and NAPI
poll tails before cancelling TX timers so a late rearm cannot outlive
the configuration containing the timer.
Update both open callers with the ownership change. Successful open
keeps the new configuration and frees the empty old object. Failed open
restores the old pointer before freeing the failed replacement. Detach
around the existing MTU reopen so XDP transmit cannot enter while that
pointer is being replaced, and reattach after success. The later MTU
transaction replaces this reopen path with retained-resource rollback.
The empty configuration remains allocated while down because ethtool,
TC and the next open still use its ring sizes and per-queue settings.
The probe-managed action frees the current object after netdev teardown.
Fixes: ba39b344e924 ("net: ethernet: stmicro: stmmac: generate stmmac dma conf before open")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/chain_mode.c | 6 +-
drivers/net/ethernet/stmicro/stmmac/ring_mode.c | 4 +-
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +-
.../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 4 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 268 +++++++++++----------
.../net/ethernet/stmicro/stmmac/stmmac_selftests.c | 8 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 6 +-
7 files changed, 155 insertions(+), 143 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
index 66025e2509e9..65243c5e539e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
@@ -48,7 +48,7 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
while (len != 0) {
tx_q->tx_skbuff[entry] = NULL;
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_tx_size);
desc = tx_q->dma_tx + entry;
if (len > bmax) {
@@ -137,7 +137,7 @@ static void refill_desc3(struct stmmac_rx_queue *rx_q, struct dma_desc *p)
*/
p->des3 = cpu_to_le32((unsigned int)(rx_q->dma_rx_phy +
(((rx_q->dirty_rx) + 1) %
- priv->dma_conf.dma_rx_size) *
+ priv->dma_conf->dma_rx_size) *
sizeof(struct dma_desc)));
}
@@ -154,7 +154,7 @@ static void clean_desc3(struct stmmac_tx_queue *tx_q, struct dma_desc *p)
*/
p->des3 = cpu_to_le32((unsigned int)((tx_q->dma_tx_phy +
((tx_q->dirty_tx + 1) %
- priv->dma_conf.dma_tx_size))
+ priv->dma_conf->dma_tx_size))
* sizeof(struct dma_desc)));
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
index d2f0c321661d..0299d6a6c32b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
@@ -52,7 +52,7 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
stmmac_prepare_tx_desc(priv, desc, 1, bmax, csum,
STMMAC_RING_MODE, 0, false, skb->len);
tx_q->tx_skbuff[entry] = NULL;
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_tx_size);
if (priv->extend_desc)
desc = (struct dma_desc *)(tx_q->dma_etx + entry);
@@ -102,7 +102,7 @@ static void refill_desc3(struct stmmac_rx_queue *rx_q, struct dma_desc *p)
struct stmmac_priv *priv = rx_q->priv_data;
/* Fill DES3 in case of RING mode */
- if (priv->dma_conf.dma_buf_sz == BUF_SIZE_16KiB)
+ if (priv->dma_conf->dma_buf_sz == BUF_SIZE_16KiB)
p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index b35b554e4ab7..ab5157fccc46 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -289,7 +289,7 @@ struct stmmac_priv {
int (*hwif_quirks)(struct stmmac_priv *priv);
struct mutex lock;
- struct stmmac_dma_conf dma_conf;
+ struct stmmac_dma_conf *dma_conf;
/* IRQ/DMA ownership and NAPI state, serialized by RTNL. */
enum stmmac_datapath_state datapath;
/* Core sleep sequence completed, independently of datapath ownership. */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 1be5310ca766..994350fcdbfa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -396,8 +396,8 @@ static void stmmac_get_ringparam(struct net_device *netdev,
ring->rx_max_pending = DMA_MAX_RX_SIZE;
ring->tx_max_pending = DMA_MAX_TX_SIZE;
- ring->rx_pending = priv->dma_conf.dma_rx_size;
- ring->tx_pending = priv->dma_conf.dma_tx_size;
+ ring->rx_pending = priv->dma_conf->dma_rx_size;
+ ring->tx_pending = priv->dma_conf->dma_tx_size;
}
static int stmmac_set_ringparam(struct net_device *netdev,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3b918cf89806..ce598b575763 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -79,7 +79,7 @@ static int phyaddr = -1;
module_param(phyaddr, int, 0444);
MODULE_PARM_DESC(phyaddr, "Physical device address");
-#define STMMAC_TX_THRESH(x) ((x)->dma_conf.dma_tx_size / 4)
+#define STMMAC_TX_THRESH(x) ((x)->dma_conf->dma_tx_size / 4)
/* Limit to make sure XDP TX and slow path can coexist */
#define STMMAC_XSK_TX_BUDGET_MAX 256
@@ -297,7 +297,7 @@ static void stmmac_disable_all_queues(struct stmmac_priv *priv)
/* synchronize_rcu() needed for pending XDP buffers to drain */
for (queue = 0; queue < rx_queues_cnt; queue++) {
- rx_q = &priv->dma_conf.rx_queue[queue];
+ rx_q = &priv->dma_conf->rx_queue[queue];
if (rx_q->xsk_pool) {
synchronize_rcu();
break;
@@ -356,10 +356,10 @@ static void print_pkt(unsigned char *buf, int len)
static inline u32 stmmac_tx_avail(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
return CIRC_SPACE(tx_q->cur_tx, tx_q->dirty_tx,
- priv->dma_conf.dma_tx_size);
+ priv->dma_conf->dma_tx_size);
}
static size_t stmmac_get_tx_desc_size(struct stmmac_priv *priv,
@@ -438,7 +438,7 @@ static void stmmac_set_queue_rx_buf_size(struct stmmac_priv *priv,
if (rx_q->xsk_pool && rx_q->buf_alloc_num)
buf_size = xsk_pool_get_rx_frame_size(rx_q->xsk_pool);
else
- buf_size = priv->dma_conf.dma_buf_sz;
+ buf_size = priv->dma_conf->dma_buf_sz;
stmmac_set_dma_bfsize(priv, priv->ioaddr, buf_size, chan);
}
@@ -450,10 +450,10 @@ static void stmmac_set_queue_rx_buf_size(struct stmmac_priv *priv,
*/
static inline u32 stmmac_rx_dirty(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
return CIRC_CNT(rx_q->cur_rx, rx_q->dirty_rx,
- priv->dma_conf.dma_rx_size);
+ priv->dma_conf->dma_rx_size);
}
static bool stmmac_eee_tx_busy(struct stmmac_priv *priv)
@@ -463,7 +463,7 @@ static bool stmmac_eee_tx_busy(struct stmmac_priv *priv)
/* check if all TX queues have the work finished */
for (queue = 0; queue < tx_cnt; queue++) {
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
if (tx_q->dirty_tx != tx_q->cur_tx)
return true; /* still unfinished work */
@@ -2129,7 +2129,7 @@ static void stmmac_free_tx_skbufs(struct stmmac_priv *priv)
u8 queue;
for (queue = 0; queue < tx_queue_cnt; queue++)
- dma_free_tx_skbufs(priv, &priv->dma_conf, queue);
+ dma_free_tx_skbufs(priv, priv->dma_conf, queue);
}
/**
@@ -2638,7 +2638,7 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
/* configure all channels */
for (chan = 0; chan < rx_channels_count; chan++) {
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[chan];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[chan];
qmode = priv->plat->rx_queues_cfg[chan].mode_to_use;
@@ -2710,7 +2710,7 @@ static const struct xsk_tx_metadata_ops stmmac_xsk_tx_metadata_ops = {
static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
{
struct netdev_queue *nq = netdev_get_tx_queue(priv->dev, queue);
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[queue];
bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;
struct xsk_buff_pool *pool = tx_q->xsk_pool;
@@ -2799,7 +2799,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
xsk_tx_metadata_to_compl(meta,
&tx_q->tx_skbuff_dma[entry].xsk_meta);
- tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
+ tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf->dma_tx_size);
entry = tx_q->cur_tx;
}
u64_stats_update_begin(&txq_stats->napi_syncp);
@@ -2847,7 +2847,7 @@ static void stmmac_bump_dma_threshold(struct stmmac_priv *priv, u32 chan)
static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
bool *pending_packets)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[queue];
unsigned int bytes_compl = 0, pkts_compl = 0;
unsigned int entry, xmits = 0, count = 0;
@@ -2860,7 +2860,7 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
entry = tx_q->dirty_tx;
/* Try to clean all TX complete frame in 1 shot */
- while ((entry != tx_q->cur_tx) && count < priv->dma_conf.dma_tx_size) {
+ while ((entry != tx_q->cur_tx) && count < priv->dma_conf->dma_tx_size) {
struct xdp_frame *xdpf;
struct sk_buff *skb;
struct dma_desc *p;
@@ -2967,7 +2967,7 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
stmmac_release_tx_desc(priv, p, priv->descriptor_mode);
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_tx_size);
}
tx_q->dirty_tx = entry;
@@ -3035,13 +3035,13 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
*/
static void stmmac_tx_err(struct stmmac_priv *priv, u32 chan)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[chan];
netif_tx_stop_queue(netdev_get_tx_queue(priv->dev, chan));
stmmac_stop_tx_dma(priv, chan);
- dma_free_tx_skbufs(priv, &priv->dma_conf, chan);
- stmmac_clear_tx_descriptors(priv, &priv->dma_conf, chan);
+ dma_free_tx_skbufs(priv, priv->dma_conf, chan);
+ stmmac_clear_tx_descriptors(priv, priv->dma_conf, chan);
stmmac_reset_tx_queue(priv, chan);
stmmac_init_tx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
tx_q->dma_tx_phy, chan);
@@ -3102,8 +3102,8 @@ static int stmmac_napi_check(struct stmmac_priv *priv, u32 chan, u32 dir)
{
int status = stmmac_dma_interrupt_status(priv, priv->ioaddr,
&priv->xstats, chan, dir);
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[chan];
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[chan];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[chan];
struct stmmac_channel *ch = &priv->channel[chan];
struct napi_struct *rx_napi;
struct napi_struct *tx_napi;
@@ -3323,7 +3323,7 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
/* DMA RX Channel Configuration */
for (chan = 0; chan < rx_channels_count; chan++) {
- rx_q = &priv->dma_conf.rx_queue[chan];
+ rx_q = &priv->dma_conf->rx_queue[chan];
stmmac_init_rx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
rx_q->dma_rx_phy, chan);
@@ -3334,7 +3334,7 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
/* DMA TX Channel Configuration */
for (chan = 0; chan < tx_channels_count; chan++) {
- tx_q = &priv->dma_conf.tx_queue[chan];
+ tx_q = &priv->dma_conf->tx_queue[chan];
stmmac_init_tx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
tx_q->dma_tx_phy, chan);
@@ -3347,7 +3347,7 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
u32 tx_coal_timer = priv->tx_coal_timer[queue];
struct stmmac_channel *ch;
struct napi_struct *napi;
@@ -3415,7 +3415,7 @@ static void stmmac_init_coalesce(struct stmmac_priv *priv)
u8 chan;
for (chan = 0; chan < tx_channel_count; chan++) {
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[chan];
priv->tx_coal_frames[chan] = STMMAC_TX_FRAMES;
priv->tx_coal_timer[chan] = STMMAC_COAL_TX_TIMER;
@@ -3436,12 +3436,12 @@ static void stmmac_set_rings_length(struct stmmac_priv *priv)
/* set TX ring length */
for (chan = 0; chan < tx_channels_count; chan++)
stmmac_set_tx_ring_len(priv, priv->ioaddr,
- (priv->dma_conf.dma_tx_size - 1), chan);
+ (priv->dma_conf->dma_tx_size - 1), chan);
/* set RX ring length */
for (chan = 0; chan < rx_channels_count; chan++)
stmmac_set_rx_ring_len(priv, priv->ioaddr,
- (priv->dma_conf.dma_rx_size - 1), chan);
+ (priv->dma_conf->dma_rx_size - 1), chan);
}
/**
@@ -3650,7 +3650,7 @@ static bool stmmac_tso_channel_permitted(struct stmmac_priv *priv,
unsigned int chan)
{
/* TSO and TBS cannot co-exist */
- return !(priv->dma_conf.tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
+ return !(priv->dma_conf->tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
}
/**
@@ -3768,7 +3768,7 @@ static int stmmac_hw_setup(struct net_device *dev)
/* TBS */
for (chan = 0; chan < tx_cnt; chan++) {
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[chan];
int enable = tx_q->tbs & STMMAC_TBS_AVAIL;
stmmac_enable_tbs(priv, priv->ioaddr, enable, chan);
@@ -3804,7 +3804,7 @@ static void stmmac_free_irq(struct net_device *dev,
if (msi->tx_irq[j] > 0) {
irq_set_affinity_hint(msi->tx_irq[j], NULL);
free_irq(msi->tx_irq[j],
- &priv->dma_conf.tx_queue[j]);
+ &priv->channel[j]);
}
}
irq_idx = priv->plat->rx_queues_to_use;
@@ -3814,7 +3814,7 @@ static void stmmac_free_irq(struct net_device *dev,
if (msi->rx_irq[j] > 0) {
irq_set_affinity_hint(msi->rx_irq[j], NULL);
free_irq(msi->rx_irq[j],
- &priv->dma_conf.rx_queue[j]);
+ &priv->channel[j]);
}
}
@@ -3968,7 +3968,7 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
sprintf(int_name, "%s:%s-%d", dev->name, "rx", i);
ret = request_irq(msi->rx_irq[i],
stmmac_msi_intr_rx,
- 0, int_name, &priv->dma_conf.rx_queue[i]);
+ 0, int_name, &priv->channel[i]);
if (unlikely(ret < 0)) {
netdev_err(priv->dev,
"%s: alloc rx-%d MSI %d (error: %d)\n",
@@ -3992,7 +3992,7 @@ static int stmmac_request_irq_multi_msi(struct net_device *dev)
sprintf(int_name, "%s:%s-%d", dev->name, "tx", i);
ret = request_irq(msi->tx_irq[i],
stmmac_msi_intr_tx,
- 0, int_name, &priv->dma_conf.tx_queue[i]);
+ 0, int_name, &priv->channel[i]);
if (unlikely(ret < 0)) {
netdev_err(priv->dev,
"%s: alloc tx-%d MSI %d (error: %d)\n",
@@ -4116,8 +4116,8 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
/* Chose the tx/rx size from the already defined one in the
* priv struct. (if defined)
*/
- dma_conf->dma_tx_size = priv->dma_conf.dma_tx_size;
- dma_conf->dma_rx_size = priv->dma_conf.dma_rx_size;
+ dma_conf->dma_tx_size = priv->dma_conf->dma_tx_size;
+ dma_conf->dma_rx_size = priv->dma_conf->dma_rx_size;
if (!dma_conf->dma_tx_size)
dma_conf->dma_tx_size = DMA_DEFAULT_TX_SIZE;
@@ -4131,6 +4131,7 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
/* Setup per-TXQ tbs flag before TX descriptor alloc */
tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
+ tx_q->tbs |= priv->dma_conf->tx_queue[chan].tbs & STMMAC_TBS_EN;
}
ret = alloc_dma_desc_resources(priv, dma_conf);
@@ -4204,10 +4205,8 @@ static int __stmmac_open(struct net_device *dev,
u8 chan;
int ret;
- for (int i = 0; i < priv->plat->tx_queues_to_use; i++)
- if (priv->dma_conf.tx_queue[i].tbs & STMMAC_TBS_EN)
- dma_conf->tx_queue[i].tbs = priv->dma_conf.tx_queue[i].tbs;
- memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
+ /* Keep RXQ metadata and timers at their registered addresses. */
+ priv->dma_conf = dma_conf;
/* The PHY is suspended when the interface is reopened without
* disconnecting the PHY, e.g. on MTU change. IEEE 802.3 allows PHYs
@@ -4249,7 +4248,7 @@ static int __stmmac_open(struct net_device *dev,
phylink_stop(priv->phylink);
for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
- hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
+ hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
stmmac_release_ptp(priv);
init_error:
@@ -4261,6 +4260,7 @@ static int __stmmac_open(struct net_device *dev,
static int stmmac_open(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
+ struct stmmac_dma_conf *old_conf = priv->dma_conf;
struct stmmac_dma_conf *dma_conf;
int ret;
@@ -4294,7 +4294,7 @@ static int stmmac_open(struct net_device *dev)
if (ret)
goto err_serdes;
- kfree(dma_conf);
+ kfree(old_conf);
/* We may have called phylink_speed_down before */
phylink_speed_up(priv->phylink);
@@ -4308,6 +4308,7 @@ static int stmmac_open(struct net_device *dev)
err_runtime_pm:
pm_runtime_put(priv->device);
err_dma_resources:
+ priv->dma_conf = old_conf;
free_dma_desc_resources(priv, dma_conf);
kfree(dma_conf);
return ret;
@@ -4319,11 +4320,16 @@ static void stmmac_quiesce(struct stmmac_priv *priv)
u8 chan;
stmmac_disable_all_queues(priv);
+ netif_tx_disable(priv->dev);
- for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
- hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
+ /* A poll function can still arm a timer after napi_complete_done().
+ * Drain those poll tails and in-flight transmitters before cancelling
+ * the timers, so none can be rearmed after their final cancellation.
+ */
+ synchronize_net();
- netif_tx_disable(priv->dev);
+ for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
+ hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
}
static void __stmmac_release(struct net_device *dev)
@@ -4348,7 +4354,7 @@ static void __stmmac_release(struct net_device *dev)
stmmac_stop_all_dma(priv);
/* Release and free the Rx/Tx resources */
- free_dma_desc_resources(priv, &priv->dma_conf);
+ free_dma_desc_resources(priv, priv->dma_conf);
stmmac_release_ptp(priv);
@@ -4413,7 +4419,7 @@ static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
return false;
stmmac_set_tx_owner(priv, p);
- tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
+ tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf->dma_tx_size);
return true;
}
@@ -4433,7 +4439,7 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, u32 *entry,
dma_addr_t des, int total_len,
bool last_segment, u32 queue)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
struct dma_desc *desc;
u32 buff_size;
int tmp_len;
@@ -4443,7 +4449,7 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, u32 *entry,
while (tmp_len > 0) {
dma_addr_t curr_addr;
- *entry = STMMAC_NEXT_ENTRY(*entry, priv->dma_conf.dma_tx_size);
+ *entry = STMMAC_NEXT_ENTRY(*entry, priv->dma_conf->dma_tx_size);
WARN_ON(tx_q->tx_skbuff[*entry]);
if (tx_q->tbs & STMMAC_TBS_AVAIL)
@@ -4467,7 +4473,7 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, u32 *entry,
static void stmmac_flush_tx_descriptors(struct stmmac_priv *priv, int queue)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
/* The own bit must be the latest setting done when prepare the
* descriptor and then barrier is needed to make sure that
@@ -4620,7 +4626,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
nfrags = skb_shinfo(skb)->nr_frags;
queue = skb_get_queue_mapping(skb);
- tx_q = &priv->dma_conf.tx_queue[queue];
+ tx_q = &priv->dma_conf->tx_queue[queue];
txq_stats = &priv->xstats.txq_stats[queue];
first_tx = tx_q->cur_tx;
@@ -4658,7 +4664,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
stmmac_set_mss(priv, mss_desc, mss);
tx_q->mss = mss;
tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx,
- priv->dma_conf.dma_tx_size);
+ priv->dma_conf->dma_tx_size);
WARN_ON(tx_q->tx_skbuff[tx_q->cur_tx]);
}
@@ -4729,7 +4735,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
/* Manage tx mitigation */
tx_packets = CIRC_CNT(tx_q->cur_tx + 1, first_tx,
- priv->dma_conf.dma_tx_size);
+ priv->dma_conf->dma_tx_size);
tx_q->tx_count_frames += tx_packets;
if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && priv->hwts_tx_en)
@@ -4761,7 +4767,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
* ndo_start_xmit will fill this descriptor the next time it's
* called and stmmac_tx_clean may clean up to this descriptor.
*/
- tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
+ tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf->dma_tx_size);
if (unlikely(stmmac_tx_avail(priv, queue) <= (MAX_SKB_FRAGS + 1))) {
netif_dbg(priv, hw, priv->dev, "%s: stop transmitted packets\n",
@@ -4791,7 +4797,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
* segment.
*/
is_last_segment = CIRC_CNT(tx_q->cur_tx, first_entry,
- priv->dma_conf.dma_tx_size) == 1;
+ priv->dma_conf->dma_tx_size) == 1;
/* Complete the first descriptor before granting the DMA */
stmmac_prepare_tso_tx_desc(priv, first, 1, proto_hdr_len, 0, 1,
@@ -4829,13 +4835,13 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
for (;;) {
desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
stmmac_release_tx_desc(priv, desc, priv->descriptor_mode);
- stmmac_free_tx_buffer(priv, &priv->dma_conf, queue,
+ stmmac_free_tx_buffer(priv, priv->dma_conf, queue,
first_entry);
if (first_entry == entry)
break;
first_entry = STMMAC_NEXT_ENTRY(first_entry,
- priv->dma_conf.dma_tx_size);
+ priv->dma_conf->dma_tx_size);
}
error:
dev_err(priv->device, "Tx dma map failed\n");
@@ -4920,7 +4926,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_BUSY;
}
- tx_q = &priv->dma_conf.tx_queue[queue];
+ tx_q = &priv->dma_conf->tx_queue[queue];
first_tx = tx_q->cur_tx;
/* Check if VLAN can be inserted by HW */
@@ -4996,7 +5002,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
unsigned int frag_size = skb_frag_size(frag);
bool last_segment = (i == (nfrags - 1));
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_tx_size);
WARN_ON(tx_q->tx_skbuff[entry]);
desc = stmmac_get_tx_desc(priv, tx_q, entry);
@@ -5026,7 +5032,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
* This approach takes care about the fragments: desc is the first
* element in case of no SG.
*/
- tx_packets = CIRC_CNT(entry + 1, first_tx, priv->dma_conf.dma_tx_size);
+ tx_packets = CIRC_CNT(entry + 1, first_tx, priv->dma_conf->dma_tx_size);
tx_q->tx_count_frames += tx_packets;
if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && priv->hwts_tx_en)
@@ -5054,7 +5060,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
* ndo_start_xmit will fill this descriptor the next time it's
* called and stmmac_tx_clean may clean up to this descriptor.
*/
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_tx_size);
tx_q->cur_tx = entry;
if (netif_msg_pktdata(priv)) {
@@ -5169,7 +5175,7 @@ static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
*/
static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
int dirty = stmmac_rx_dirty(priv, queue);
unsigned int entry = rx_q->dirty_rx;
gfp_t gfp = (GFP_ATOMIC | __GFP_NOWARN);
@@ -5220,7 +5226,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
dma_wmb();
stmmac_set_rx_owner(priv, p, use_rx_wd);
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_rx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_rx_size);
}
rx_q->dirty_rx = entry;
stmmac_set_queue_rx_tail_ptr(priv, rx_q, queue, rx_q->dirty_rx);
@@ -5248,12 +5254,12 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
/* First descriptor, not last descriptor and not split header */
if (status & rx_not_ls)
- return priv->dma_conf.dma_buf_sz;
+ return priv->dma_conf->dma_buf_sz;
plen = stmmac_get_rx_frame_len(priv, p, coe);
/* First descriptor and last descriptor and not split header */
- return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen);
+ return min_t(unsigned int, priv->dma_conf->dma_buf_sz, plen);
}
static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
@@ -5283,7 +5289,7 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
/* Not GMAC4 and not last descriptor */
if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
- return priv->dma_conf.dma_buf_sz;
+ return priv->dma_conf->dma_buf_sz;
/* GMAC4 or last descriptor */
plen = stmmac_get_rx_frame_len(priv, p, coe);
@@ -5295,7 +5301,7 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
struct xdp_frame *xdpf, bool dma_map)
{
struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[queue];
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;
unsigned int entry = tx_q->cur_tx;
enum stmmac_txbuf_type buf_type;
@@ -5361,7 +5367,7 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_tx_size);
tx_q->cur_tx = entry;
return STMMAC_XDP_TX;
@@ -5552,7 +5558,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
static bool stmmac_rx_refill_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
{
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
unsigned int entry = rx_q->dirty_rx;
struct dma_desc *rx_desc = NULL;
bool ret = true;
@@ -5592,7 +5598,7 @@ static bool stmmac_rx_refill_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
dma_wmb();
stmmac_set_rx_owner(priv, rx_desc, use_rx_wd);
- entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_rx_size);
+ entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf->dma_rx_size);
}
if (rx_desc) {
@@ -5616,7 +5622,7 @@ static struct stmmac_xdp_buff *xsk_buff_to_stmmac_ctx(struct xdp_buff *xdp)
static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
{
struct stmmac_rxq_stats *rxq_stats = &priv->xstats.rxq_stats[queue];
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
unsigned int count = 0, error = 0, len = 0;
int dirty = stmmac_rx_dirty(priv, queue);
unsigned int next_entry = rx_q->cur_rx;
@@ -5633,7 +5639,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
netdev_dbg(priv->dev, "%s: descriptor ring:\n", __func__);
desc_size = stmmac_get_rx_desc_size(priv);
- stmmac_display_ring(priv, rx_head, priv->dma_conf.dma_rx_size, true,
+ stmmac_display_ring(priv, rx_head, priv->dma_conf->dma_rx_size, true,
rx_q->dma_rx_phy, desc_size);
}
while (count < limit) {
@@ -5677,7 +5683,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
/* Prefetch the next RX descriptor */
next_entry = STMMAC_NEXT_ENTRY(rx_q->cur_rx,
- priv->dma_conf.dma_rx_size);
+ priv->dma_conf->dma_rx_size);
if (unlikely(next_entry == rx_q->dirty_rx))
break;
@@ -5802,7 +5808,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
{
u32 rx_errors = 0, rx_dropped = 0, rx_bytes = 0, rx_packets = 0;
struct stmmac_rxq_stats *rxq_stats = &priv->xstats.rxq_stats[queue];
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
struct stmmac_channel *ch = &priv->channel[queue];
unsigned int count = 0, error = 0, len = 0;
int status = 0, coe = priv->hw->rx_csum;
@@ -5815,7 +5821,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
int bufsz;
dma_dir = page_pool_get_dma_dir(rx_q->page_pool);
- bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
+ bufsz = DIV_ROUND_UP(priv->dma_conf->dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
if (netif_msg_rx_status(priv)) {
void *rx_head = stmmac_get_rx_desc(priv, rx_q, 0);
@@ -5823,7 +5829,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
netdev_dbg(priv->dev, "%s: descriptor ring:\n", __func__);
desc_size = stmmac_get_rx_desc_size(priv);
- stmmac_display_ring(priv, rx_head, priv->dma_conf.dma_rx_size, true,
+ stmmac_display_ring(priv, rx_head, priv->dma_conf->dma_rx_size, true,
rx_q->dma_rx_phy, desc_size);
}
while (count < limit) {
@@ -5863,7 +5869,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
break;
next_entry = STMMAC_NEXT_ENTRY(rx_q->cur_rx,
- priv->dma_conf.dma_rx_size);
+ priv->dma_conf->dma_rx_size);
if (unlikely(next_entry == rx_q->dirty_rx))
break;
@@ -5997,7 +6003,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
buf1_len, dma_dir);
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
buf->page, buf->page_offset, buf1_len,
- priv->dma_conf.dma_buf_sz);
+ priv->dma_conf->dma_buf_sz);
buf->page = NULL;
}
@@ -6006,7 +6012,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
buf2_len, dma_dir);
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
buf->sec_page, 0, buf2_len,
- priv->dma_conf.dma_buf_sz);
+ priv->dma_conf->dma_buf_sz);
buf->sec_page = NULL;
}
@@ -6231,6 +6237,7 @@ static void stmmac_set_rx_mode(struct net_device *dev)
static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
{
struct stmmac_priv *priv = netdev_priv(dev);
+ struct stmmac_dma_conf *old_conf = priv->dma_conf;
int txfifosz = priv->plat->tx_fifo_size;
struct stmmac_dma_conf *dma_conf;
const int mtu = new_mtu;
@@ -6267,10 +6274,12 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
return PTR_ERR(dma_conf);
}
+ netif_device_detach(dev);
__stmmac_release(dev);
ret = __stmmac_open(dev, dma_conf);
if (ret) {
+ priv->dma_conf = old_conf;
free_dma_desc_resources(priv, dma_conf);
kfree(dma_conf);
/*
@@ -6282,9 +6291,10 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
return ret;
}
- kfree(dma_conf);
+ kfree(old_conf);
stmmac_set_rx_mode(dev);
+ netif_device_attach(dev);
}
WRITE_ONCE(dev->mtu, mtu);
@@ -6455,15 +6465,11 @@ static irqreturn_t stmmac_safety_interrupt(int irq, void *dev_id)
static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
{
- struct stmmac_tx_queue *tx_q = (struct stmmac_tx_queue *)data;
- struct stmmac_dma_conf *dma_conf;
- int chan = tx_q->queue_index;
- struct stmmac_priv *priv;
+ struct stmmac_channel *ch = data;
+ struct stmmac_priv *priv = ch->priv_data;
+ int chan = ch->index;
int status;
- dma_conf = container_of(tx_q, struct stmmac_dma_conf, tx_queue[chan]);
- priv = container_of(dma_conf, struct stmmac_priv, dma_conf);
-
/* Check if adapter is up */
if (test_bit(STMMAC_DOWN, &priv->state))
return IRQ_HANDLED;
@@ -6482,13 +6488,9 @@ static irqreturn_t stmmac_msi_intr_tx(int irq, void *data)
static irqreturn_t stmmac_msi_intr_rx(int irq, void *data)
{
- struct stmmac_rx_queue *rx_q = (struct stmmac_rx_queue *)data;
- struct stmmac_dma_conf *dma_conf;
- int chan = rx_q->queue_index;
- struct stmmac_priv *priv;
-
- dma_conf = container_of(rx_q, struct stmmac_dma_conf, rx_queue[chan]);
- priv = container_of(dma_conf, struct stmmac_priv, dma_conf);
+ struct stmmac_channel *ch = data;
+ struct stmmac_priv *priv = ch->priv_data;
+ int chan = ch->index;
/* Check if adapter is up */
if (test_bit(STMMAC_DOWN, &priv->state))
@@ -6655,34 +6657,34 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
tx_count = priv->plat->tx_queues_to_use;
for (queue = 0; queue < rx_count; queue++) {
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
seq_printf(seq, "RX Queue %d:\n", queue);
if (priv->extend_desc) {
seq_printf(seq, "Extended descriptor ring:\n");
sysfs_display_ring((void *)rx_q->dma_erx,
- priv->dma_conf.dma_rx_size, 1, seq, rx_q->dma_rx_phy);
+ priv->dma_conf->dma_rx_size, 1, seq, rx_q->dma_rx_phy);
} else {
seq_printf(seq, "Descriptor ring:\n");
sysfs_display_ring((void *)rx_q->dma_rx,
- priv->dma_conf.dma_rx_size, 0, seq, rx_q->dma_rx_phy);
+ priv->dma_conf->dma_rx_size, 0, seq, rx_q->dma_rx_phy);
}
}
for (queue = 0; queue < tx_count; queue++) {
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
seq_printf(seq, "TX Queue %d:\n", queue);
if (priv->extend_desc) {
seq_printf(seq, "Extended descriptor ring:\n");
sysfs_display_ring((void *)tx_q->dma_etx,
- priv->dma_conf.dma_tx_size, 1, seq, tx_q->dma_tx_phy);
+ priv->dma_conf->dma_tx_size, 1, seq, tx_q->dma_tx_phy);
} else if (!(tx_q->tbs & STMMAC_TBS_AVAIL)) {
seq_printf(seq, "Descriptor ring:\n");
sysfs_display_ring((void *)tx_q->dma_tx,
- priv->dma_conf.dma_tx_size, 0, seq, tx_q->dma_tx_phy);
+ priv->dma_conf->dma_tx_size, 0, seq, tx_q->dma_tx_phy);
}
}
@@ -7148,31 +7150,31 @@ void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue)
spin_unlock_irqrestore(&ch->lock, flags);
stmmac_stop_rx_dma(priv, queue);
- __free_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
+ __free_dma_rx_desc_resources(priv, priv->dma_conf, queue);
}
void stmmac_enable_rx_queue(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
struct stmmac_channel *ch = &priv->channel[queue];
unsigned long flags;
int ret;
- ret = __alloc_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
+ ret = __alloc_dma_rx_desc_resources(priv, priv->dma_conf, queue);
if (ret) {
netdev_err(priv->dev, "Failed to alloc RX desc.\n");
return;
}
- ret = __init_dma_rx_desc_rings(priv, &priv->dma_conf, queue, GFP_KERNEL);
+ ret = __init_dma_rx_desc_rings(priv, priv->dma_conf, queue, GFP_KERNEL);
if (ret) {
- __free_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
+ __free_dma_rx_desc_resources(priv, priv->dma_conf, queue);
netdev_err(priv->dev, "Failed to init RX desc.\n");
return;
}
stmmac_reset_rx_queue(priv, queue);
- stmmac_clear_rx_descriptors(priv, &priv->dma_conf, queue);
+ stmmac_clear_rx_descriptors(priv, priv->dma_conf, queue);
stmmac_init_rx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
rx_q->dma_rx_phy, queue);
@@ -7198,31 +7200,31 @@ void stmmac_disable_tx_queue(struct stmmac_priv *priv, u32 queue)
spin_unlock_irqrestore(&ch->lock, flags);
stmmac_stop_tx_dma(priv, queue);
- __free_dma_tx_desc_resources(priv, &priv->dma_conf, queue);
+ __free_dma_tx_desc_resources(priv, priv->dma_conf, queue);
}
void stmmac_enable_tx_queue(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
struct stmmac_channel *ch = &priv->channel[queue];
unsigned long flags;
int ret;
- ret = __alloc_dma_tx_desc_resources(priv, &priv->dma_conf, queue);
+ ret = __alloc_dma_tx_desc_resources(priv, priv->dma_conf, queue);
if (ret) {
netdev_err(priv->dev, "Failed to alloc TX desc.\n");
return;
}
- ret = __init_dma_tx_desc_rings(priv, &priv->dma_conf, queue);
+ ret = __init_dma_tx_desc_rings(priv, priv->dma_conf, queue);
if (ret) {
- __free_dma_tx_desc_resources(priv, &priv->dma_conf, queue);
+ __free_dma_tx_desc_resources(priv, priv->dma_conf, queue);
netdev_err(priv->dev, "Failed to init TX desc.\n");
return;
}
stmmac_reset_tx_queue(priv, queue);
- stmmac_clear_tx_descriptors(priv, &priv->dma_conf, queue);
+ stmmac_clear_tx_descriptors(priv, priv->dma_conf, queue);
stmmac_init_tx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
tx_q->dma_tx_phy, queue);
@@ -7251,7 +7253,7 @@ void stmmac_xdp_release(struct net_device *dev)
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);
+ hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
/* Free the IRQ lines */
stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
@@ -7260,7 +7262,7 @@ void stmmac_xdp_release(struct net_device *dev)
stmmac_stop_all_dma(priv);
/* Release and free the Rx/Tx resources */
- free_dma_desc_resources(priv, &priv->dma_conf);
+ free_dma_desc_resources(priv, priv->dma_conf);
/* Disable the MAC Rx/Tx */
stmmac_mac_set(priv, priv->ioaddr, false);
@@ -7284,14 +7286,14 @@ int stmmac_xdp_open(struct net_device *dev)
u8 chan;
int ret;
- ret = alloc_dma_desc_resources(priv, &priv->dma_conf);
+ ret = alloc_dma_desc_resources(priv, priv->dma_conf);
if (ret < 0) {
netdev_err(dev, "%s: DMA descriptors allocation failed\n",
__func__);
goto dma_desc_error;
}
- ret = init_dma_desc_rings(dev, &priv->dma_conf, GFP_KERNEL);
+ ret = init_dma_desc_rings(dev, priv->dma_conf, GFP_KERNEL);
if (ret < 0) {
netdev_err(dev, "%s: DMA descriptors initialization failed\n",
__func__);
@@ -7311,7 +7313,7 @@ int stmmac_xdp_open(struct net_device *dev)
/* DMA RX Channel Configuration */
for (chan = 0; chan < rx_cnt; chan++) {
- rx_q = &priv->dma_conf.rx_queue[chan];
+ rx_q = &priv->dma_conf->rx_queue[chan];
stmmac_init_rx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
rx_q->dma_rx_phy, chan);
@@ -7326,7 +7328,7 @@ int stmmac_xdp_open(struct net_device *dev)
/* DMA TX Channel Configuration */
for (chan = 0; chan < tx_cnt; chan++) {
- tx_q = &priv->dma_conf.tx_queue[chan];
+ tx_q = &priv->dma_conf->tx_queue[chan];
stmmac_init_tx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
tx_q->dma_tx_phy, chan);
@@ -7356,10 +7358,10 @@ int stmmac_xdp_open(struct net_device *dev)
irq_error:
for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
- hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
+ hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
init_error:
- free_dma_desc_resources(priv, &priv->dma_conf);
+ free_dma_desc_resources(priv, priv->dma_conf);
dma_desc_error:
return ret;
}
@@ -7367,8 +7369,6 @@ int stmmac_xdp_open(struct net_device *dev)
int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags)
{
struct stmmac_priv *priv = netdev_priv(dev);
- struct stmmac_rx_queue *rx_q;
- struct stmmac_tx_queue *tx_q;
struct stmmac_channel *ch;
if (test_bit(STMMAC_DOWN, &priv->state) ||
@@ -7382,11 +7382,9 @@ int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags)
queue >= priv->plat->tx_queues_to_use)
return -EINVAL;
- rx_q = &priv->dma_conf.rx_queue[queue];
- tx_q = &priv->dma_conf.tx_queue[queue];
ch = &priv->channel[queue];
- if (!rx_q->xsk_pool && !tx_q->xsk_pool)
+ if (!test_bit(queue, priv->af_xdp_zc_qps))
return -EINVAL;
if (!napi_if_scheduled_mark_missed(&ch->rxtx_napi)) {
@@ -7777,8 +7775,8 @@ int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size)
if (netif_running(dev))
stmmac_release(dev);
- priv->dma_conf.dma_rx_size = rx_size;
- priv->dma_conf.dma_tx_size = tx_size;
+ priv->dma_conf->dma_rx_size = rx_size;
+ priv->dma_conf->dma_tx_size = tx_size;
if (netif_running(dev))
ret = stmmac_open(dev);
@@ -7943,6 +7941,13 @@ struct plat_stmmacenet_data *stmmac_plat_dat_alloc(struct device *dev)
}
EXPORT_SYMBOL_GPL(stmmac_plat_dat_alloc);
+static void stmmac_free_dma_conf(void *data)
+{
+ struct stmmac_priv *priv = data;
+
+ kfree(priv->dma_conf);
+}
+
static int __stmmac_dvr_probe(struct device *device,
struct plat_stmmacenet_data *plat_dat,
struct stmmac_resources *res)
@@ -7967,6 +7972,13 @@ static int __stmmac_dvr_probe(struct device *device,
priv = netdev_priv(ndev);
priv->device = device;
priv->dev = ndev;
+ /* Keep ring sizes and per-queue settings even while the device is down. */
+ priv->dma_conf = kzalloc_obj(*priv->dma_conf);
+ if (!priv->dma_conf)
+ return -ENOMEM;
+ ret = devm_add_action_or_reset(device, stmmac_free_dma_conf, priv);
+ if (ret)
+ return ret;
for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
u64_stats_init(&priv->xstats.rxq_stats[i].napi_syncp);
@@ -8389,7 +8401,7 @@ EXPORT_SYMBOL_GPL(stmmac_suspend);
static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+ struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
rx_q->cur_rx = 0;
rx_q->dirty_rx = 0;
@@ -8397,7 +8409,7 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
static void stmmac_reset_tx_queue(struct stmmac_priv *priv, u32 queue)
{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
tx_q->cur_tx = 0;
tx_q->dirty_tx = 0;
@@ -8480,7 +8492,7 @@ int stmmac_resume(struct device *dev)
stmmac_reset_queues_param(priv);
stmmac_free_tx_skbufs(priv);
- stmmac_clear_descriptors(priv, &priv->dma_conf);
+ stmmac_clear_descriptors(priv, priv->dma_conf);
ret = stmmac_hw_setup(ndev);
if (ret < 0) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index c25dc9f89270..2511339e55e4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -887,8 +887,8 @@ static int stmmac_test_flowctrl(struct stmmac_priv *priv)
struct stmmac_channel *ch = &priv->channel[i];
u32 tail;
- tail = priv->dma_conf.rx_queue[i].dma_rx_phy +
- (priv->dma_conf.dma_rx_size * sizeof(struct dma_desc));
+ tail = priv->dma_conf->rx_queue[i].dma_rx_phy +
+ (priv->dma_conf->dma_rx_size * sizeof(struct dma_desc));
stmmac_set_rx_tail_ptr(priv, priv->ioaddr, tail, i);
stmmac_start_rx(priv, priv->ioaddr, i);
@@ -1787,7 +1787,7 @@ static int stmmac_test_arpoffload(struct stmmac_priv *priv)
static int __stmmac_test_jumbo(struct stmmac_priv *priv, u16 queue)
{
struct stmmac_packet_attrs attr = { };
- int size = priv->dma_conf.dma_buf_sz;
+ int size = priv->dma_conf->dma_buf_sz;
if (!dwmac_is_xmac(priv->plat->core_type))
size -= NET_IP_ALIGN;
@@ -1873,7 +1873,7 @@ static int stmmac_test_tbs(struct stmmac_priv *priv)
/* Find first TBS enabled Queue, if any */
for (i = 0; i < priv->plat->tx_queues_to_use; i++)
- if (priv->dma_conf.tx_queue[i].tbs & STMMAC_TBS_AVAIL)
+ if (priv->dma_conf->tx_queue[i].tbs & STMMAC_TBS_AVAIL)
break;
if (i >= priv->plat->tx_queues_to_use)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 42a00446e9b4..bb1dfe2702ac 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1197,13 +1197,13 @@ static int tc_setup_etf(struct stmmac_priv *priv,
return -EOPNOTSUPP;
if (qopt->queue >= priv->plat->tx_queues_to_use)
return -EINVAL;
- if (!(priv->dma_conf.tx_queue[qopt->queue].tbs & STMMAC_TBS_AVAIL))
+ if (!(priv->dma_conf->tx_queue[qopt->queue].tbs & STMMAC_TBS_AVAIL))
return -EINVAL;
if (qopt->enable)
- priv->dma_conf.tx_queue[qopt->queue].tbs |= STMMAC_TBS_EN;
+ priv->dma_conf->tx_queue[qopt->queue].tbs |= STMMAC_TBS_EN;
else
- priv->dma_conf.tx_queue[qopt->queue].tbs &= ~STMMAC_TBS_EN;
+ priv->dma_conf->tx_queue[qopt->queue].tbs &= ~STMMAC_TBS_EN;
netdev_info(priv->dev, "%s ETF for Queue %d\n",
qopt->enable ? "enabled" : "disabled", qopt->queue);
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net v3 06/10] net: stmmac: keep DMA configurations at stable addresses
2026-09-24 17:44 ` [PATCH net v3 06/10] net: stmmac: keep DMA configurations at stable addresses James Hilliard
@ 2026-09-25 19:12 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 19:12 UTC (permalink / raw)
To: james.hilliard1
Cc: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni,
qiangqing.zhang, rmk+kernel, maxime.chevallier, andrew+netdev,
mcoquelin.stm32, alexandre.torgue, ansuelsmth, yangtiezhu,
chenhuacai, ast, daniel, hawk, john.fastabend, sdf,
fancer.lancer, quic_jsuraj, richardcochran, Joao.Pinto,
vladimir.oltean, boon.leong.ong, weifeng.voon, yoong.siang.song,
richard.genoud, alastair, mripard, netdev, linux-kernel,
linux-stm32, linux-arm-kernel, bpf, zhaojinming,
lorenzo.bianconi, dinghui1111
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 3 (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).
- [High] Baseline decision: not confirmed as a remaining issue.
- [Medium] In drivers/net/ethernet/stmicro/stmmac/stmmac_main.c, the EEE
software LPI timer can access a freed DMA configuration.
- [Medium] In drivers/net/ethernet/stmicro/stmmac/stmmac_main.c,
stmmac_features_check() (ndo_features_check) calls…
- [Medium] Baseline decision: not confirmed as a remaining issue.
Pre-existing issues:
- [Medium] Baseline decision: not confirmed as a remaining issue.
--
net: stmmac: keep DMA configurations at stable addresses
This commit turns priv->dma_conf from an embedded struct into a pointer
to the allocated DMA configuration. The goal is to keep RXQ metadata
registered with XDP and AF_XDP at a stable address. Per-queue MSI
handlers now use priv->channel[] as their cookie. stmmac_quiesce()
drains transmitters and NAPI with synchronize_net() before cancelling
the TX hrtimers. On success the open and MTU paths free the old
configuration, and on failure they restore it.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3b918cf898067..ce598b5757637 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -463,7 +463,7 @@ static bool stmmac_eee_tx_busy(struct stmmac_priv *priv)
>
> /* check if all TX queues have the work finished */
> for (queue = 0; queue < tx_cnt; queue++) {
> - struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
> + struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
>
> if (tx_q->dirty_tx != tx_q->cur_tx)
> return true; /* still unfinished work */
[Severity: Medium]
Now that dma_conf is a separate allocation that gets freed on reopen,
can the EEE software LPI timer read a freed configuration here? The
timer path is:
stmmac_eee_ctrl_timer()
stmmac_try_to_start_sw_lpi()
stmmac_eee_tx_busy()
tx_q = &priv->dma_conf->tx_queue[queue];
No lock or RCU protects the priv->dma_conf load.
__stmmac_release() calls phylink_stop() first, and that reaches
stmmac_mac_disable_tx_lpi():
priv->eee_sw_timer_en = false;
timer_delete_sync(&priv->eee_ctrl_timer);
NAPI is still running at that point. stmmac_tx_clean() checks the flag
without a lock before it re-arms the timer:
if (priv->eee_sw_timer_en && !priv->tx_path_in_lpi_mode)
stmmac_restart_sw_lpi_timer(priv);
Suppose that check passes just before the flag is cleared. Then
mod_timer() can run after timer_delete_sync() has returned. Nothing in
stmmac_quiesce() cancels eee_ctrl_timer again.
Next, suppose the callback loads the old priv->dma_conf before
__stmmac_open() stores the new one, and is then delayed (for example,
preempted in the timer softirq thread on PREEMPT_RT). It would then
dereference tx_queue[] after kfree(old_conf) in stmmac_change_mtu() or
stmmac_open(). Before this patch the configuration was embedded in
priv, so the worst case was a stale read.
Later in the series, stmmac_reconfigure_mtu() adds
timer_delete_sync(&priv->eee_ctrl_timer) after stmmac_quiesce(). The
ethtool ring and channel paths still seem to lack it:
stmmac_reinit_ringparam() / stmmac_reinit_queues()
__stmmac_release()
stmmac_reopen()
__stmmac_open()
kfree(old_conf)
The close/open path also seems to lack it. Should the EEE timer also be
cancelled on those paths after NAPI is quiesced?
[ ... ]
> @@ -3650,7 +3650,7 @@ static bool stmmac_tso_channel_permitted(struct stmmac_priv *priv,
> unsigned int chan)
> {
> /* TSO and TBS cannot co-exist */
> - return !(priv->dma_conf.tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
> + return !(priv->dma_conf->tx_queue[chan].tbs & STMMAC_TBS_AVAIL);
> }
[Severity: Medium]
stmmac_features_check(), the ndo_features_check callback, calls this.
Can it read priv->dma_conf after the old configuration has been freed?
netif_skb_features() is called from paths that neither
netif_device_detach() nor stopped TX queues exclude:
- tbf_segment() in net/sched/sch_tbf.c, and similarly taprio, cake,
netem and dualpi2
- ip_finish_output_gso() and its IPv6 counterpart
- validate_xmit_skb() in sch_direct_xmit(), before the stopped-queue
check
In stmmac_change_mtu(), the only synchronize_net() is the one inside
__stmmac_release(), and it runs before the pointer swap:
stmmac_change_mtu()
netif_device_detach(dev)
__stmmac_release(dev)
stmmac_quiesce()
synchronize_net()
__stmmac_open(dev, dma_conf)
priv->dma_conf = dma_conf;
kfree(old_conf)
A GSO feature check could load the old pointer after that drain and be
preempted until after kfree(old_conf). It would then read tbs from
freed memory.
The same ordering seems to remain at the end of the series. In
stmmac_reconfigure_mtu(), synchronize_net() runs before
priv->dma_conf = new_conf, followed by kfree(old_conf). stmmac_reopen()
does the same for ring and channel changes.
Is a grace period needed between publishing the new configuration and
freeing the old one?
[ ... ]
> @@ -6267,10 +6274,12 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
> return PTR_ERR(dma_conf);
> }
>
> + netif_device_detach(dev);
> __stmmac_release(dev);
>
> ret = __stmmac_open(dev, dma_conf);
> if (ret) {
> + priv->dma_conf = old_conf;
> free_dma_desc_resources(priv, dma_conf);
> kfree(dma_conf);
> /*
> @@ -6282,9 +6291,10 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
> return ret;
> }
>
> - kfree(dma_conf);
> + kfree(old_conf);
[Severity: High]
The commit message says:
Drain transmitters and NAPI poll tails before cancelling TX timers so
a late rearm cannot outlive the configuration containing the timer.
Can a TX hard-error interrupt restart transmitters after that drain?
__stmmac_release() runs stmmac_quiesce() and frees the IRQs only
afterwards:
stmmac_quiesce(priv);
...
stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
stmmac_stop_all_dma(priv);
free_dma_desc_resources(priv, priv->dma_conf);
In that window, stmmac_msi_intr_tx() calls stmmac_tx_err(), which ends
with:
netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, chan));
Once a queue is woken, stack transmitters can enter stmmac_xmit() or
stmmac_tso_xmit() again. They would be using the old configuration
while free_dma_desc_resources() frees its rings, and while this
kfree(old_conf) frees the struct itself.
Two later commits in the series deal with this:
- "net: stmmac: account for failed live XDP reopening" adds a second
stmmac_stop_tx_queues() after stmmac_free_irq() in
__stmmac_release().
- "net: stmmac: retain DMA resources across MTU changes" disables IRQ
handling in stmmac_reconfigure_mtu() before replacing the
configuration.
At this patch, though, can the old configuration still be in use after
kfree(old_conf)?
[ ... ]
> @@ -7251,7 +7253,7 @@ void stmmac_xdp_release(struct net_device *dev)
> 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);
> + hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
>
> /* Free the IRQ lines */
> stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
[Severity: Medium]
This isn't a bug introduced by this patch, but stmmac_xdp_release()
still cancels the TX hrtimers right after stmmac_disable_all_queues().
It does not run the synchronize_net() that stmmac_quiesce() now uses.
Can a NAPI poll tail re-arm a timer after this cancel?
After napi_complete_done() clears SCHED, stmmac_tx_timer_arm() can see
napi_is_scheduled() == false and call hrtimer_start(). stmmac_xdp_open()
then calls hrtimer_setup() on the same txtimer, which could
re-initialize an armed hrtimer.
Later in the series, "net: stmmac: account for failed live XDP
reopening" addresses this by making stmmac_xdp_release() call
stmmac_quiesce().
[ ... ]
> @@ -7382,11 +7382,9 @@ int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags)
> queue >= priv->plat->tx_queues_to_use)
> return -EINVAL;
>
> - rx_q = &priv->dma_conf.rx_queue[queue];
> - tx_q = &priv->dma_conf.tx_queue[queue];
> ch = &priv->channel[queue];
>
> - if (!rx_q->xsk_pool && !tx_q->xsk_pool)
> + if (!test_bit(queue, priv->af_xdp_zc_qps))
> return -EINVAL;
[Severity: Medium]
Is the af_xdp_zc_qps bit a strong enough guard here? In
stmmac_xdp_enable_pool(), the sequence is:
set_bit(queue, priv->af_xdp_zc_qps);
...
stmmac_enable_rx_queue(priv, queue);
stmmac_enable_tx_queue(priv, queue);
napi_enable(&ch->rxtx_napi);
err = stmmac_xsk_wakeup(priv->dev, queue, XDP_WAKEUP_RX);
Both enable helpers return void. If they fail, dma_rx, buf_pool and
xsk_pool are left NULL.
If both queue re-enables fail, the old check returned -EINVAL. The new
check lets NAPI run stmmac_rx_zc() on a NULL ring and pool.
This seems to go away later in the series with "net: stmmac: use the
tracked datapath restart for XSK pool changes". That commit switches to
stmmac_xdp_release()/stmmac_xdp_open() and clears the bit when the
reopen fails.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net v3 07/10] net: stmmac: account for failed live XDP reopening
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
` (5 preceding siblings ...)
2026-09-24 17:44 ` [PATCH net v3 06/10] net: stmmac: keep DMA configurations at stable addresses James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-25 19:12 ` netdev-bot+sashiko
2026-09-24 17:44 ` [PATCH net v3 08/10] net: stmmac: use the tracked datapath restart for XSK pool changes James Hilliard
` (2 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
The XDP-specific release/open pair bypasses the ordinary datapath
bookkeeping, and its caller ignores reopen errors. A failed allocation
therefore leaves the device marked running with NAPI disabled and rings
released. A later close disables NAPI again. An IRQ-request failure also
frees descriptors after starting DMA without first stopping it.
Detach and stop phylink while replacing the XDP datapath, update its
ownership state, and reattach only after successful reopening. Preserve
the existing PTP registration on success, but release it on failure so a
DOWN datapath has no residual PTP ownership. Drain transmitters and timer
producers after freeing IRQs which can restart TX queues, including on
the ordinary release path.
Keep the old program reference until the change has succeeded. On an
attach failure restore the old program and return the error, allowing the
core to release the proposed program. Program removal must still succeed
when the non-XDP datapath cannot restart: BPF link teardown cannot retain
the detached program. Leave the interface administratively up but detached
for an ordinary down/up recovery in either failure case.
Fixes: ac746c8520d9 ("net: stmmac: enhance XDP ZC driver level switching performance")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 47 +++++++++++++++--------
drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c | 31 +++++++++++----
2 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ce598b575763..467f471874cf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4314,12 +4314,10 @@ static int stmmac_open(struct net_device *dev)
return ret;
}
-/* Quiesce NAPI and transmit queues without releasing their resources. */
-static void stmmac_quiesce(struct stmmac_priv *priv)
+static void stmmac_stop_tx_queues(struct stmmac_priv *priv)
{
u8 chan;
- stmmac_disable_all_queues(priv);
netif_tx_disable(priv->dev);
/* A poll function can still arm a timer after napi_complete_done().
@@ -4332,6 +4330,13 @@ static void stmmac_quiesce(struct stmmac_priv *priv)
hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
}
+/* Quiesce NAPI and transmit queues without releasing their resources. */
+static void stmmac_quiesce(struct stmmac_priv *priv)
+{
+ stmmac_disable_all_queues(priv);
+ stmmac_stop_tx_queues(priv);
+}
+
static void __stmmac_release(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
@@ -4350,6 +4355,9 @@ static void __stmmac_release(struct net_device *dev)
/* Free the IRQ lines */
stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
+ /* TX error IRQs can restart a queue after the first quiescence. */
+ stmmac_stop_tx_queues(priv);
+
/* Stop TX/RX DMA after draining IRQ handlers which can restart it. */
stmmac_stop_all_dma(priv);
@@ -7244,19 +7252,15 @@ void stmmac_enable_tx_queue(struct stmmac_priv *priv, u32 queue)
void stmmac_xdp_release(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
- u8 chan;
-
- /* Ensure tx function is not running */
- netif_tx_disable(dev);
- /* Disable NAPI process */
- 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_device_detach(dev);
+ phylink_stop(priv->phylink);
+ stmmac_quiesce(priv);
+ priv->datapath = STMMAC_DATAPATH_DOWN;
/* Free the IRQ lines */
stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
+ stmmac_stop_tx_queues(priv);
/* Stop TX/RX DMA channels */
stmmac_stop_all_dma(priv);
@@ -7271,7 +7275,13 @@ void stmmac_xdp_release(struct net_device *dev)
* watchdogs during reset
*/
netif_trans_update(dev);
- netif_carrier_off(dev);
+
+ if (stmmac_fpe_supported(priv))
+ ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
+
+ /* Keep PTP across the immediately following stmmac_xdp_open(). That
+ * function releases it if reopening fails, before returning DOWN.
+ */
}
int stmmac_xdp_open(struct net_device *dev)
@@ -7350,19 +7360,22 @@ int stmmac_xdp_open(struct net_device *dev)
/* Enable NAPI process*/
stmmac_enable_all_queues(priv);
- netif_carrier_on(dev);
- netif_tx_start_all_queues(dev);
stmmac_enable_all_dma_irq(priv);
+ priv->datapath = STMMAC_DATAPATH_RUNNING;
+ phylink_start(priv->phylink);
+ netif_device_attach(dev);
return 0;
irq_error:
- for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
- hrtimer_cancel(&priv->dma_conf->tx_queue[chan].txtimer);
+ stmmac_stop_tx_queues(priv);
+ stmmac_stop_all_dma(priv);
+ stmmac_mac_set(priv, priv->ioaddr, false);
init_error:
free_dma_desc_resources(priv, priv->dma_conf);
dma_desc_error:
+ stmmac_release_ptp(priv);
return ret;
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
index 909219775507..7ecb7addd2ea 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
@@ -108,6 +108,7 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
struct bpf_prog *old_prog;
bool need_update;
bool if_running;
+ int ret;
if_running = priv->datapath == STMMAC_DATAPATH_RUNNING;
@@ -119,25 +120,41 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
return -EOPNOTSUPP;
}
- if (!prog)
- xdp_features_clear_redirect_target(dev);
-
need_update = !!priv->xdp_prog != !!prog;
if (if_running && need_update)
stmmac_xdp_release(dev);
old_prog = xchg(&priv->xdp_prog, prog);
- if (old_prog)
- bpf_prog_put(old_prog);
/* Disable RX SPH for XDP operation */
priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
- if (if_running && need_update)
- stmmac_xdp_open(dev);
+ if (if_running && need_update) {
+ ret = stmmac_xdp_open(dev);
+ if (ret) {
+ netdev_err(dev, "failed reopening after XDP change: %pe; interface remains detached\n",
+ ERR_PTR(ret));
+ if (prog) {
+ /* The core retains the old program on error and drops
+ * the reference it passed for the proposed program.
+ */
+ xchg(&priv->xdp_prog, old_prog);
+ priv->sph_active = priv->sph_capable && !old_prog;
+ return ret;
+ }
+ /* Uninstalling a BPF link must release its program even
+ * if the non-XDP datapath cannot be restarted.
+ */
+ }
+ }
+
+ if (old_prog)
+ bpf_prog_put(old_prog);
if (prog)
xdp_features_set_redirect_target(dev, false);
+ else
+ xdp_features_clear_redirect_target(dev);
return 0;
}
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net v3 07/10] net: stmmac: account for failed live XDP reopening
2026-09-24 17:44 ` [PATCH net v3 07/10] net: stmmac: account for failed live XDP reopening James Hilliard
@ 2026-09-25 19:12 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 19:12 UTC (permalink / raw)
To: james.hilliard1
Cc: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni,
qiangqing.zhang, rmk+kernel, maxime.chevallier, andrew+netdev,
mcoquelin.stm32, alexandre.torgue, ansuelsmth, yangtiezhu,
chenhuacai, ast, daniel, hawk, john.fastabend, sdf,
fancer.lancer, quic_jsuraj, richardcochran, Joao.Pinto,
vladimir.oltean, boon.leong.ong, weifeng.voon, yoong.siang.song,
richard.genoud, alastair, mripard, netdev, linux-kernel,
linux-stm32, linux-arm-kernel, bpf, zhaojinming,
lorenzo.bianconi, dinghui1111
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · 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] This patch makes every live XDP program attach or detach take
the physical link down and renegotiate it, even when the reopen…
- [Medium] The EEE software LPI timer (priv->eee_ctrl_timer) can be re-
armed after phylink cancels it, so it escapes the quiescence that this…
--
net: stmmac: account for failed live XDP reopening
This patch changes the live XDP release/open pair to track datapath
ownership state. The pair now detaches the netdev and stops/starts
phylink around the swap, and stmmac_xdp_open() gets proper error
unwinding. stmmac_xdp_set_prog() now holds the old program reference
until the reopen succeeds, and restores it when an attach fails.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ce598b5757637..467f471874cf7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -7244,19 +7252,15 @@ void stmmac_enable_tx_queue(struct stmmac_priv *priv, u32 queue)
> void stmmac_xdp_release(struct net_device *dev)
> {
> struct stmmac_priv *priv = netdev_priv(dev);
> - u8 chan;
> -
> - /* Ensure tx function is not running */
> - netif_tx_disable(dev);
>
> - /* Disable NAPI process */
> - 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_device_detach(dev);
> + phylink_stop(priv->phylink);
> + stmmac_quiesce(priv);
> + priv->datapath = STMMAC_DATAPATH_DOWN;
[Severity: Medium]
Does this make every live XDP attach or detach drop the physical link and
renegotiate it, even when stmmac_xdp_open() succeeds?
stmmac_xdp_set_prog() calls the release/open pair each time XDP is
toggled on a RUNNING datapath. Before this patch the pair only called
netif_carrier_off() and netif_carrier_on(). Now it does this:
stmmac_xdp_release()
phylink_stop()
sfp_upstream_stop()
phy_stop() -> PHY_HALTED -> phy_link_down(), then phy_suspend()
(PCS disabled)
stmmac_xdp_open()
phylink_start()
phylink_mac_initial_config()
phy_start() -> autonegotiation restarts
As a result, both the link partner and the host would see the link go
down and come back up. The link stays down for the whole negotiation.
The commit in the Fixes: tag, ac746c8520d9, was written to avoid exactly
this. Its message says the old close/open would "tear down the PHY device
and causes undesirable autonegotiation which causes a delay whenever
AFXDP ZC is setup".
As far as I can tell, phylink only has to be stopped on the failure path.
__stmmac_release() returns early for STMMAC_DATAPATH_DOWN, so otherwise
nothing would ever stop phylink. On success, could the link stay up as
it did before?
A smaller related point: stmmac_xdp_open() now enables the MAC and starts
DMA while the PHY is powered down, so its RX clock may be stopped. stmmac
sets mac_requires_rxc, but stmmac_xdp_open() does not call
phylink_prepare_resume() the way __stmmac_open() does. This path does no
DMA software reset, so the effect may be harmless.
This behaviour is still present at the end of the series. The later XSK
pool patch also sends pool changes through the same release/open pair.
>
> /* Free the IRQ lines */
> stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
> + stmmac_stop_tx_queues(priv);
[Severity: Medium]
Can priv->eee_ctrl_timer be re-armed after phylink cancels it, so that
this drain misses it?
phylink_stop() now runs before stmmac_quiesce() disables NAPI. It cancels
the EEE timer through this chain:
phylink_stop()
phylink_link_down()
phylink_deactivate_lpi()
stmmac_mac_disable_tx_lpi()
priv->eee_sw_timer_en = false;
timer_delete_sync(&priv->eee_ctrl_timer);
That runs under priv->lock. A NAPI poll on another CPU in stmmac_tx_clean()
holds only the TX queue lock, and it may already have passed this check:
stmmac_tx_clean() {
...
if (priv->eee_sw_timer_en && !priv->tx_path_in_lpi_mode)
stmmac_restart_sw_lpi_timer(priv);
...
}
If so, its mod_timer() can run after timer_delete_sync(). Nothing later
catches it:
- netif_device_detach() does not drain NAPI.
- stmmac_quiesce() and stmmac_stop_tx_queues() only cancel the per-queue
txtimer hrtimers.
The re-armed timer calls stmmac_eee_ctrl_timer() and then
stmmac_try_to_start_sw_lpi(), which does not check eee_sw_timer_en. It
would either force LPI on a MAC where phylink has disabled EEE, or re-arm
itself while dirty_tx != cur_tx.
Suppose stmmac_xdp_open() then fails before stmmac_reset_queues_param(),
for example in alloc_dma_desc_resources(). cur_tx and dirty_tx are never
reset, so the timer could keep re-arming while the device is DOWN.
Nothing deletes eee_ctrl_timer on close or remove, so could it fire on
freed priv once the device is removed?
The ordinary release path has had the same phylink_stop()-before-NAPI
ordering since before this series. This patch adds that ordering to the
XDP swap path.
The last patch in the series adds timer_delete_sync(&priv->eee_ctrl_timer)
after stmmac_quiesce() in stmmac_reconfigure_mtu(). stmmac_xdp_release()
still does not have it at the end of the series. Is the same cancellation
needed here?
>
> /* Stop TX/RX DMA channels */
> stmmac_stop_all_dma(priv);
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net v3 08/10] net: stmmac: use the tracked datapath restart for XSK pool changes
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
` (6 preceding siblings ...)
2026-09-24 17:44 ` [PATCH net v3 07/10] net: stmmac: account for failed live XDP reopening James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-25 19:12 ` netdev-bot+sashiko
2026-09-24 17:44 ` [PATCH net v3 09/10] net: stmmac: retain PHY and PM ownership during ethtool reopening James Hilliard
2026-09-24 17:44 ` [PATCH net v3 10/10] net: stmmac: retain DMA resources across MTU changes James Hilliard
9 siblings, 1 reply; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
Live XSK pool changes free and replace individual RX/TX queues through
void helpers. An allocation failure is only logged and the caller enables
NAPI over missing rings. The TX queue and its timer also remain available
while their storage is being freed.
Use the XDP datapath release/open pair instead. Quiesce all NAPI contexts,
transmitters, IRQ handlers and timers while the old program and pool bitmap
still describe the active layout. Only then change the bitmap and rebuild
the queues. This pauses all queues during a pool change, but avoids a
partially running datapath and shares its failure and ownership handling
with program changes instead of maintaining a second queue-level lifetime.
Undo the new pool's bitmap bit and DMA mapping if reopening for attachment
fails. For removal, free every reference to the pool before unmapping it
and return success even if rebuilding the ordinary queues fails: socket
teardown must not leave the departing pool reachable. In both cases failed
reopening leaves the interface detached for down/up recovery.
Remove the unused per-queue allocation/enable helpers. Preserve their TBS
programming when using the full XDP reopen path, and clear the released TX
pool pointer alongside its RX counterpart.
Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 4 -
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 105 +---------------------
drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c | 44 +++++----
3 files changed, 24 insertions(+), 129 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index ab5157fccc46..f07623a1285c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -437,10 +437,6 @@ static inline bool stmmac_xdp_is_enabled(struct stmmac_priv *priv)
return !!priv->xdp_prog;
}
-void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue);
-void stmmac_enable_rx_queue(struct stmmac_priv *priv, u32 queue);
-void stmmac_disable_tx_queue(struct stmmac_priv *priv, u32 queue);
-void stmmac_enable_tx_queue(struct stmmac_priv *priv, u32 queue);
int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags);
struct timespec64 stmmac_calc_tas_basetime(ktime_t old_base_time,
ktime_t current_time,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 467f471874cf..d56b7f911db0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2232,6 +2232,7 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
tx_q->dma_etx = NULL;
tx_q->dma_entx = NULL;
tx_q->dma_tx = NULL;
+ tx_q->xsk_pool = NULL;
}
static void free_dma_tx_desc_resources(struct stmmac_priv *priv,
@@ -7148,107 +7149,6 @@ static int stmmac_xdp_xmit(struct net_device *dev, int num_frames,
return nxmit;
}
-void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue)
-{
- struct stmmac_channel *ch = &priv->channel[queue];
- unsigned long flags;
-
- spin_lock_irqsave(&ch->lock, flags);
- stmmac_disable_dma_irq(priv, priv->ioaddr, queue, 1, 0);
- spin_unlock_irqrestore(&ch->lock, flags);
-
- stmmac_stop_rx_dma(priv, queue);
- __free_dma_rx_desc_resources(priv, priv->dma_conf, queue);
-}
-
-void stmmac_enable_rx_queue(struct stmmac_priv *priv, u32 queue)
-{
- struct stmmac_rx_queue *rx_q = &priv->dma_conf->rx_queue[queue];
- struct stmmac_channel *ch = &priv->channel[queue];
- unsigned long flags;
- int ret;
-
- ret = __alloc_dma_rx_desc_resources(priv, priv->dma_conf, queue);
- if (ret) {
- netdev_err(priv->dev, "Failed to alloc RX desc.\n");
- return;
- }
-
- ret = __init_dma_rx_desc_rings(priv, priv->dma_conf, queue, GFP_KERNEL);
- if (ret) {
- __free_dma_rx_desc_resources(priv, priv->dma_conf, queue);
- netdev_err(priv->dev, "Failed to init RX desc.\n");
- return;
- }
-
- stmmac_reset_rx_queue(priv, queue);
- stmmac_clear_rx_descriptors(priv, priv->dma_conf, queue);
-
- stmmac_init_rx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
- rx_q->dma_rx_phy, queue);
-
- stmmac_set_queue_rx_tail_ptr(priv, rx_q, queue, rx_q->buf_alloc_num);
-
- stmmac_set_queue_rx_buf_size(priv, rx_q, queue);
-
- stmmac_start_rx_dma(priv, queue);
-
- spin_lock_irqsave(&ch->lock, flags);
- stmmac_enable_dma_irq(priv, priv->ioaddr, queue, 1, 0);
- spin_unlock_irqrestore(&ch->lock, flags);
-}
-
-void stmmac_disable_tx_queue(struct stmmac_priv *priv, u32 queue)
-{
- struct stmmac_channel *ch = &priv->channel[queue];
- unsigned long flags;
-
- spin_lock_irqsave(&ch->lock, flags);
- stmmac_disable_dma_irq(priv, priv->ioaddr, queue, 0, 1);
- spin_unlock_irqrestore(&ch->lock, flags);
-
- stmmac_stop_tx_dma(priv, queue);
- __free_dma_tx_desc_resources(priv, priv->dma_conf, queue);
-}
-
-void stmmac_enable_tx_queue(struct stmmac_priv *priv, u32 queue)
-{
- struct stmmac_tx_queue *tx_q = &priv->dma_conf->tx_queue[queue];
- struct stmmac_channel *ch = &priv->channel[queue];
- unsigned long flags;
- int ret;
-
- ret = __alloc_dma_tx_desc_resources(priv, priv->dma_conf, queue);
- if (ret) {
- netdev_err(priv->dev, "Failed to alloc TX desc.\n");
- return;
- }
-
- ret = __init_dma_tx_desc_rings(priv, priv->dma_conf, queue);
- if (ret) {
- __free_dma_tx_desc_resources(priv, priv->dma_conf, queue);
- netdev_err(priv->dev, "Failed to init TX desc.\n");
- return;
- }
-
- stmmac_reset_tx_queue(priv, queue);
- stmmac_clear_tx_descriptors(priv, priv->dma_conf, queue);
-
- stmmac_init_tx_chan(priv, priv->ioaddr, priv->plat->dma_cfg,
- tx_q->dma_tx_phy, queue);
-
- if (tx_q->tbs & STMMAC_TBS_AVAIL)
- stmmac_enable_tbs(priv, priv->ioaddr, 1, queue);
-
- stmmac_set_queue_tx_tail_ptr(priv, tx_q, queue, 0);
-
- stmmac_start_tx_dma(priv, queue);
-
- spin_lock_irqsave(&ch->lock, flags);
- stmmac_enable_dma_irq(priv, priv->ioaddr, queue, 0, 1);
- spin_unlock_irqrestore(&ch->lock, flags);
-}
-
void stmmac_xdp_release(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
@@ -7345,6 +7245,9 @@ int stmmac_xdp_open(struct net_device *dev)
stmmac_set_queue_tx_tail_ptr(priv, tx_q, chan, 0);
+ if (tx_q->tbs & STMMAC_TBS_AVAIL)
+ stmmac_enable_tbs(priv, priv->ioaddr, 1, chan);
+
hrtimer_setup(&tx_q->txtimer, stmmac_tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
index 7ecb7addd2ea..907ac49a1b76 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
@@ -9,7 +9,6 @@
static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
struct xsk_buff_pool *pool, u16 queue)
{
- struct stmmac_channel *ch = &priv->channel[queue];
bool need_update;
u32 frame_size;
int err;
@@ -34,23 +33,23 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
stmmac_xdp_is_enabled(priv);
- if (need_update) {
- napi_disable(&ch->rx_napi);
- napi_disable(&ch->tx_napi);
- stmmac_disable_rx_queue(priv, queue);
- stmmac_disable_tx_queue(priv, queue);
- }
+ if (need_update)
+ stmmac_xdp_release(priv->dev);
set_bit(queue, priv->af_xdp_zc_qps);
if (need_update) {
- stmmac_enable_rx_queue(priv, queue);
- stmmac_enable_tx_queue(priv, queue);
- napi_enable(&ch->rxtx_napi);
-
- err = stmmac_xsk_wakeup(priv->dev, queue, XDP_WAKEUP_RX);
- if (err)
+ err = stmmac_xdp_open(priv->dev);
+ if (err) {
+ clear_bit(queue, priv->af_xdp_zc_qps);
+ xsk_pool_dma_unmap(pool, STMMAC_RX_DMA_ATTR);
+ netdev_err(priv->dev, "failed reopening after XSK pool attach: %pe; interface remains detached\n",
+ ERR_PTR(err));
return err;
+ }
+
+ /* The pool is installed even if link resolution is still pending. */
+ napi_schedule(&priv->channel[queue].rxtx_napi);
}
return 0;
@@ -58,9 +57,9 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
{
- struct stmmac_channel *ch = &priv->channel[queue];
struct xsk_buff_pool *pool;
bool need_update;
+ int err;
if (queue >= priv->plat->rx_queues_to_use ||
queue >= priv->plat->tx_queues_to_use)
@@ -73,24 +72,21 @@ static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
stmmac_xdp_is_enabled(priv);
- if (need_update) {
- napi_disable(&ch->rxtx_napi);
- stmmac_disable_rx_queue(priv, queue);
- stmmac_disable_tx_queue(priv, queue);
- synchronize_rcu();
- }
+ if (need_update)
+ stmmac_xdp_release(priv->dev);
xsk_pool_dma_unmap(pool, STMMAC_RX_DMA_ATTR);
clear_bit(queue, priv->af_xdp_zc_qps);
if (need_update) {
- stmmac_enable_rx_queue(priv, queue);
- stmmac_enable_tx_queue(priv, queue);
- napi_enable(&ch->rx_napi);
- napi_enable(&ch->tx_napi);
+ err = stmmac_xdp_open(priv->dev);
+ if (err)
+ netdev_err(priv->dev, "failed reopening after XSK pool removal: %pe; interface remains detached\n",
+ ERR_PTR(err));
}
+ /* Socket teardown must be able to unmap and free the removed pool. */
return 0;
}
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net v3 08/10] net: stmmac: use the tracked datapath restart for XSK pool changes
2026-09-24 17:44 ` [PATCH net v3 08/10] net: stmmac: use the tracked datapath restart for XSK pool changes James Hilliard
@ 2026-09-25 19:12 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 19:12 UTC (permalink / raw)
To: james.hilliard1
Cc: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni,
qiangqing.zhang, rmk+kernel, maxime.chevallier, andrew+netdev,
mcoquelin.stm32, alexandre.torgue, ansuelsmth, yangtiezhu,
chenhuacai, ast, daniel, hawk, john.fastabend, sdf,
fancer.lancer, quic_jsuraj, richardcochran, Joao.Pinto,
vladimir.oltean, boon.leong.ong, weifeng.voon, yoong.siang.song,
richard.genoud, alastair, mripard, netdev, linux-kernel,
linux-stm32, linux-arm-kernel, bpf, zhaojinming,
lorenzo.bianconi, dinghui1111
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 (1) · Low: 1
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] Every live AF_XDP zero-copy pool bind or unbind now bounces
the physical link, and the commit message does not say so.
- [Medium] Attaching an XSK pool now leaves rxtx NAPI repolling
continuously for as long as carrier is down, even when there is no…
- [Low] stmmac.h keeps a stale external prototype for
stmmac_xsk_wakeup().
Pre-existing issues:
- [Medium] stmmac_xdp_release() calls stmmac_stop_all_dma() and then
immediately frees the descriptor rings and buffers with…
--
net: stmmac: use the tracked datapath restart for XSK pool changes
Live XSK pool attach and removal used per-queue disable/enable helpers.
This patch replaces them with the full stmmac_xdp_release() and
stmmac_xdp_open() pair. It undoes the bitmap bit and DMA mapping if
attach fails, and always returns success on removal. The unused per-queue
helpers are removed, and their TBS programming moves into
stmmac_xdp_open().
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index ab5157fccc465..f07623a1285c9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -437,10 +437,6 @@ static inline bool stmmac_xdp_is_enabled(struct stmmac_priv *priv)
> return !!priv->xdp_prog;
> }
>
> -void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue);
> -void stmmac_enable_rx_queue(struct stmmac_priv *priv, u32 queue);
> -void stmmac_disable_tx_queue(struct stmmac_priv *priv, u32 queue);
> -void stmmac_enable_tx_queue(struct stmmac_priv *priv, u32 queue);
> int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags);
[Severity: Low]
This isn't a bug, but should this prototype be removed as well?
The only caller outside stmmac_main.c was stmmac_xdp_enable_pool(), and
this patch replaces that call with napi_schedule(). After this patch,
stmmac_xsk_wakeup() is referenced only by its definition and the
.ndo_xsk_wakeup entry in stmmac_main.c. It could be made static there.
[ ... ]
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index 7ecb7addd2ea7..907ac49a1b764 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
[ ... ]
> @@ -34,23 +33,23 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
> need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
> stmmac_xdp_is_enabled(priv);
>
> - if (need_update) {
> - napi_disable(&ch->rx_napi);
> - napi_disable(&ch->tx_napi);
> - stmmac_disable_rx_queue(priv, queue);
> - stmmac_disable_tx_queue(priv, queue);
> - }
> + if (need_update)
> + stmmac_xdp_release(priv->dev);
[Severity: Medium]
Does this bounce the physical link on every live XSK pool bind and
unbind?
An earlier patch in this series ("net: stmmac: account for failed live
XDP reopening") made stmmac_xdp_release() call phylink_stop() and
stmmac_xdp_open() call phylink_start(). The attach path looks like this:
stmmac_xdp_enable_pool()
stmmac_xdp_release()
phylink_stop()
phy_stop()
PHY_HALTED: phy_link_down(), PHY_STATE_WORK_SUSPEND
_phy_state_machine_post_work(): phy_suspend()
stmmac_xdp_open()
phylink_start()
netif_carrier_off()
autonegotiation restarts
stmmac_xdp_disable_pool() follows the same path on every socket close.
Each bind or unbind would then cause a carrier down/up event and a PHY
power-down followed by renegotiation, which typically takes seconds at
1000BASE-T. Traffic stops on every queue. The rings of all other queues
are freed and rebuilt, including queues serving other live AF_XDP
sockets.
The previous per-queue path touched only one queue and left the link
alone.
The commit message only says "This pauses all queues during a pool
change". Since the patch carries a Fixes: tag, could the commit message
mention the link flap? Alternatively, could pool changes avoid the
phylink stop/start?
>
> set_bit(queue, priv->af_xdp_zc_qps);
>
> if (need_update) {
> - stmmac_enable_rx_queue(priv, queue);
> - stmmac_enable_tx_queue(priv, queue);
> - napi_enable(&ch->rxtx_napi);
> -
> - err = stmmac_xsk_wakeup(priv->dev, queue, XDP_WAKEUP_RX);
> - if (err)
> + err = stmmac_xdp_open(priv->dev);
> + if (err) {
> + clear_bit(queue, priv->af_xdp_zc_qps);
> + xsk_pool_dma_unmap(pool, STMMAC_RX_DMA_ATTR);
> + netdev_err(priv->dev, "failed reopening after XSK pool attach: %pe; interface remains detached\n",
> + ERR_PTR(err));
> return err;
> + }
> +
> + /* The pool is installed even if link resolution is still pending. */
> + napi_schedule(&priv->channel[queue].rxtx_napi);
[Severity: Medium]
Can this leave rxtx_napi repolling nonstop while the carrier is down?
The old code went through stmmac_xsk_wakeup(), which returns -ENETDOWN
when !netif_carrier_ok(). At this point the carrier is always down: the
PHY was just halted, and phylink_start() inside stmmac_xdp_open() has just
called netif_carrier_off(). If no cable is connected, it stays down
indefinitely.
Because tx_q->xsk_pool is set, the poll goes through:
stmmac_napi_poll_rxtx()
stmmac_tx_clean()
stmmac_xdp_xmit_zc()
stmmac_xdp_xmit_zc() checks the carrier before it looks at the XSK TX
ring:
if (unlikely(stmmac_tx_avail(priv, queue) < STMMAC_TX_XSK_AVAIL) ||
!netif_carrier_ok(priv->dev)) {
work_done = false;
break;
}
stmmac_tx_clean() then turns that into a full budget:
if (work_done)
xmits = budget - 1;
else
xmits = budget;
...
return max(count, xmits);
stmmac_napi_poll_rxtx() then returns without completing NAPI:
if (rxtx_done >= budget)
return budget;
Wouldn't this spin a CPU in softirq or ksoftirqd, even with an empty XSK
TX ring, until the carrier comes up or the interface is closed?
> }
>
> return 0;
[ ... ]
> @@ -73,24 +72,21 @@ static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
> need_update = priv->datapath == STMMAC_DATAPATH_RUNNING &&
> stmmac_xdp_is_enabled(priv);
>
> - if (need_update) {
> - napi_disable(&ch->rxtx_napi);
> - stmmac_disable_rx_queue(priv, queue);
> - stmmac_disable_tx_queue(priv, queue);
> - synchronize_rcu();
> - }
> + if (need_update)
> + stmmac_xdp_release(priv->dev);
>
> xsk_pool_dma_unmap(pool, STMMAC_RX_DMA_ATTR);
[Severity: Medium]
This is a pre-existing issue, but is the hardware DMA guaranteed to be
idle when the pool is unmapped here?
stmmac_xdp_release() does:
stmmac_stop_all_dma(priv);
/* Release and free the Rx/Tx resources */
free_dma_desc_resources(priv, priv->dma_conf);
/* Disable the MAC Rx/Tx */
stmmac_mac_set(priv, priv->ioaddr, false);
dwmac4_dma_stop_rx() and dwmac4_dma_stop_tx() only clear the SR/ST bits.
They never poll for the Stopped state, and the MAC receiver is disabled
only after the rings have been freed.
A later commit in this series ("net: stmmac: retain DMA resources across
MTU changes") notes that "clearing a DMA enable bit need not finish an
in-flight frame immediately". An in-flight frame or descriptor writeback
could therefore land in freed descriptors or in the XSK buffers unmapped
here.
__stmmac_release(), the XDP program change path and the removed
per-queue helpers all follow the same stop-then-free pattern, so this
patch did not introduce it. The old synchronize_rcu() before the unmap
did not drain DMA either.
>
> clear_bit(queue, priv->af_xdp_zc_qps);
>
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net v3 09/10] net: stmmac: retain PHY and PM ownership during ethtool reopening
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
` (7 preceding siblings ...)
2026-09-24 17:44 ` [PATCH net v3 08/10] net: stmmac: use the tracked datapath restart for XSK pool changes James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-25 19:12 ` netdev-bot+sashiko
2026-09-24 17:44 ` [PATCH net v3 10/10] net: stmmac: retain DMA resources across MTU changes James Hilliard
9 siblings, 1 reply; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
Changing ring sizes or channel counts calls the complete ndo_stop/open
pair while the interface remains administratively up. If reopening fails,
the PHY attachment and runtime-PM reference have already been dropped, but
the next ordinary close drops them again. The device also remains present,
allowing an MTU change to quiesce NAPI which is already disabled.
Detach the device and release only the datapath during these live changes.
Use a common reopen helper which retains the PHY attachment and runtime-PM
reference and reattaches only on success. Restore the previous ring sizes
or channel topology after failure so a subsequent down/up retries the old
configuration. Leave the failed device detached and reject further live
ethtool reconfiguration until administrative recovery.
No additional outer-lifetime flag is needed: PHY and PM ownership continue
to follow the successful ndo_open/ndo_stop pair instead of being changed
by live datapath reconfiguration.
Fixes: aa042f60e496 ("net: stmmac: Add support to Ethtool get/set ring parameters")
Fixes: 0366f7e06a6b ("net: stmmac: add ethtool support for get/set channels")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 76 ++++++++++++++++++++---
1 file changed, 68 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d56b7f911db0..d17c68f55caf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4361,6 +4361,8 @@ static void __stmmac_release(struct net_device *dev)
/* Stop TX/RX DMA after draining IRQ handlers which can restart it. */
stmmac_stop_all_dma(priv);
+ /* Link resolution need not have reached mac_link_up() yet. */
+ stmmac_mac_set(priv, priv->ioaddr, false);
/* Release and free the Rx/Tx resources */
free_dma_desc_resources(priv, priv->dma_conf);
@@ -7658,13 +7660,37 @@ static void stmmac_napi_del(struct net_device *dev)
}
}
-int stmmac_reinit_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
+/* Rebuild only the datapath. The administratively-up device still owns its
+ * PHY attachment and runtime-PM reference, even if this reopen fails.
+ */
+static int stmmac_reopen(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
- int ret = 0, i;
+ struct stmmac_dma_conf *old_conf = priv->dma_conf;
+ struct stmmac_dma_conf *dma_conf;
+ int ret;
- if (netif_running(dev))
- stmmac_release(dev);
+ dma_conf = stmmac_setup_dma_desc(priv, dev->mtu);
+ if (IS_ERR(dma_conf))
+ return PTR_ERR(dma_conf);
+
+ ret = __stmmac_open(dev, dma_conf);
+ if (ret) {
+ priv->dma_conf = old_conf;
+ free_dma_desc_resources(priv, dma_conf);
+ kfree(dma_conf);
+ return ret;
+ }
+
+ kfree(old_conf);
+ netif_device_attach(dev);
+ return 0;
+}
+
+static void stmmac_set_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
+{
+ struct stmmac_priv *priv = netdev_priv(dev);
+ int i;
stmmac_napi_del(dev);
@@ -7676,9 +7702,31 @@ int stmmac_reinit_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
rx_cnt);
stmmac_napi_add(dev);
+}
+
+int stmmac_reinit_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
+{
+ struct stmmac_priv *priv = netdev_priv(dev);
+ u8 old_rx = priv->plat->rx_queues_to_use;
+ u8 old_tx = priv->plat->tx_queues_to_use;
+ int ret = 0;
+
+ if (netif_running(dev)) {
+ if (!netif_device_present(dev))
+ return -ENETDOWN;
+ netif_device_detach(dev);
+ __stmmac_release(dev);
+ }
+
+ stmmac_set_queues(dev, rx_cnt, tx_cnt);
if (netif_running(dev))
- ret = stmmac_open(dev);
+ ret = stmmac_reopen(dev);
+ if (ret) {
+ stmmac_set_queues(dev, old_rx, old_tx);
+ netdev_err(dev, "failed reopening after channel change: %pe; interface remains detached\n",
+ ERR_PTR(ret));
+ }
return ret;
}
@@ -7686,16 +7734,28 @@ int stmmac_reinit_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size)
{
struct stmmac_priv *priv = netdev_priv(dev);
+ u32 old_rx = priv->dma_conf->dma_rx_size;
+ u32 old_tx = priv->dma_conf->dma_tx_size;
int ret = 0;
- if (netif_running(dev))
- stmmac_release(dev);
+ if (netif_running(dev)) {
+ if (!netif_device_present(dev))
+ return -ENETDOWN;
+ netif_device_detach(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);
+ ret = stmmac_reopen(dev);
+ if (ret) {
+ priv->dma_conf->dma_rx_size = old_rx;
+ priv->dma_conf->dma_tx_size = old_tx;
+ netdev_err(dev, "failed reopening after ring change: %pe; interface remains detached\n",
+ ERR_PTR(ret));
+ }
return ret;
}
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net v3 09/10] net: stmmac: retain PHY and PM ownership during ethtool reopening
2026-09-24 17:44 ` [PATCH net v3 09/10] net: stmmac: retain PHY and PM ownership during ethtool reopening James Hilliard
@ 2026-09-25 19:12 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 19:12 UTC (permalink / raw)
To: james.hilliard1
Cc: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni,
qiangqing.zhang, rmk+kernel, maxime.chevallier, andrew+netdev,
mcoquelin.stm32, alexandre.torgue, ansuelsmth, yangtiezhu,
chenhuacai, ast, daniel, hawk, john.fastabend, sdf,
fancer.lancer, quic_jsuraj, richardcochran, Joao.Pinto,
vladimir.oltean, boon.leong.ong, weifeng.voon, yoong.siang.song,
richard.genoud, alastair, mripard, netdev, linux-kernel,
linux-stm32, linux-arm-kernel, bpf, zhaojinming,
lorenzo.bianconi, dinghui1111
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 2
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] In commit "net: stmmac: retain PHY and PM ownership during
ethtool reopening", a failed hardware setup during a live ethtool…
- [Low] The commit message says the old behaviour lets 'an MTU change ...
quiesce NAPI which is already disabled'.
- [Low] After a failed live channel change, the netdev core and the
driver disagree on queue counts.
--
net: stmmac: retain PHY and PM ownership during ethtool reopening
This commit changes the stmmac ethtool ring size and channel count paths.
They now detach the device and release only the datapath, then reopen
through a new stmmac_reopen() helper that keeps the PHY attachment and the
runtime-PM reference. On failure the previous ring sizes or queue counts are
restored and the device stays detached.
> the PHY attachment and runtime-PM reference have already been dropped, but
> the next ordinary close drops them again. The device also remains present,
> allowing an MTU change to quiesce NAPI which is already disabled.
[Severity: Low]
Is this description accurate for this tree?
With the old code, a failed ethtool reopen is stmmac_release() followed by a
failed stmmac_open(). After that, priv->datapath stays
STMMAC_DATAPATH_DOWN. A later jumbo MTU change calls __stmmac_release(),
which returns early:
if (priv->datapath == STMMAC_DATAPATH_DOWN)
return;
An earlier patch in this series added that guard ("net: stmmac: keep
datapath state coherent after reinitialization failure"). So
stmmac_quiesce() and napi_disable() never run. MTU changes that don't need
jumbo buffers don't touch the datapath at all.
The real hazard looks different. stmmac_change_mtu() goes on to call
__stmmac_open(), which restarts DMA, IRQs and phylink. At that point
stmmac_release() has already disconnected the PHY and dropped the
runtime-PM reference.
This patch does close that, because dev_set_mtu_ext() returns -ENODEV for a
detached device. Could the commit message describe that consequence instead?
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d56b7f911db0a..d17c68f55caf6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -7658,13 +7660,37 @@ static void stmmac_napi_del(struct net_device *dev)
> }
> }
>
> -int stmmac_reinit_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
> +/* Rebuild only the datapath. The administratively-up device still owns its
> + * PHY attachment and runtime-PM reference, even if this reopen fails.
> + */
> +static int stmmac_reopen(struct net_device *dev)
> {
> struct stmmac_priv *priv = netdev_priv(dev);
> - int ret = 0, i;
> + struct stmmac_dma_conf *old_conf = priv->dma_conf;
> + struct stmmac_dma_conf *dma_conf;
> + int ret;
>
> - if (netif_running(dev))
> - stmmac_release(dev);
> + dma_conf = stmmac_setup_dma_desc(priv, dev->mtu);
> + if (IS_ERR(dma_conf))
> + return PTR_ERR(dma_conf);
> +
> + ret = __stmmac_open(dev, dma_conf);
> + if (ret) {
> + priv->dma_conf = old_conf;
> + free_dma_desc_resources(priv, dma_conf);
> + kfree(dma_conf);
> + return ret;
> + }
[Severity: Medium]
Can this failure path leave the PHY powered up while phylink is stopped and
the MAC is disabled?
Before the reopen, __stmmac_release() goes through phylink_stop() and
phy_stop(). That suspends the PHY unless WoL is set.
__stmmac_open() then resumes the PHY before hardware setup. stmmac always
sets config->mac_requires_rxc = true, so phylink_prepare_resume() calls
phy_resume():
__stmmac_open()
phylink_prepare_resume(priv->phylink);
...
ret = stmmac_hw_setup(dev, false);
if (ret < 0) {
...
goto init_error;
}
...
init_error:
stmmac_stop_all_dma(priv);
stmmac_mac_set(priv, priv->ioaddr, false);
return ret;
stmmac_hw_setup() can fail, for example when the DMA software reset times
out. The init_error label then skips phylink_stop(), and the error branch
above only frees the replacement dma_conf.
The old path was stmmac_open() error -> phylink_disconnect_phy() ->
phy_detach() -> phy_suspend(). That put the PHY back into power-down.
With this change the PHY can stay powered and bring up a link with the
partner, while the host has no carrier and the MAC is off. This seems to
last until the interface is taken down. At that point __stmmac_release()
returns early on STMMAC_DATAPATH_DOWN, and only phylink_disconnect_phy() in
stmmac_release() suspends the PHY.
phylink_stop() already handles an instance that is already stopped. The
commit "net: phylink: allow stopping a suspended instance" added this
branch:
drivers/net/phy/phylink.c:phylink_stop() {
if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
if (pl->phydev)
phy_suspend(pl->phydev);
return;
}
...
}
Would calling phylink_stop() on this failure path undo the power-up and
still keep the PHY attached and the runtime-PM reference held?
stmmac_reopen() is not changed later in the series, so this remains at the
end of the series.
> +
> + kfree(old_conf);
> + netif_device_attach(dev);
> + return 0;
> +}
[ ... ]
> @@ -7676,9 +7702,31 @@ int stmmac_reinit_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
> rx_cnt);
>
> stmmac_napi_add(dev);
> +}
> +
> +int stmmac_reinit_queues(struct net_device *dev, u8 rx_cnt, u8 tx_cnt)
> +{
> + struct stmmac_priv *priv = netdev_priv(dev);
> + u8 old_rx = priv->plat->rx_queues_to_use;
> + u8 old_tx = priv->plat->tx_queues_to_use;
> + int ret = 0;
> +
> + if (netif_running(dev)) {
> + if (!netif_device_present(dev))
> + return -ENETDOWN;
> + netif_device_detach(dev);
> + __stmmac_release(dev);
> + }
> +
> + stmmac_set_queues(dev, rx_cnt, tx_cnt);
>
> if (netif_running(dev))
> - ret = stmmac_open(dev);
> + ret = stmmac_reopen(dev);
> + if (ret) {
> + stmmac_set_queues(dev, old_rx, old_tx);
[Severity: Low]
Does this leave dev->real_num_rx_queues and dev->real_num_tx_queues at the
new counts while the driver goes back to the old ones?
__stmmac_open() can fail after stmmac_hw_setup() has already succeeded, for
example when stmmac_request_irq() fails. By then stmmac_hw_setup() has
already run this with the new counts:
netif_set_real_num_rx_queues(dev, priv->plat->rx_queues_to_use);
netif_set_real_num_tx_queues(dev, priv->plat->tx_queues_to_use);
stmmac_set_queues(dev, old_rx, old_tx) restores
priv->plat->rx/tx_queues_to_use, the RSS table and the NAPI instances. It
does not restore the core real_num_* values.
Until the next ndo_open re-syncs them in stmmac_hw_setup(), the core (sysfs
queue kobjects, XPS/RPS, xsk qid bounds) sees the new topology and the
driver sees the old one. The old code never restored the plat values, so
both sides stayed consistent.
Does this partial restore match what the commit message describes as
restoring the "channel topology after failure"?
> + netdev_err(dev, "failed reopening after channel change: %pe; interface remains detached\n",
> + ERR_PTR(ret));
> + }
>
> return ret;
> }
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net v3 10/10] net: stmmac: retain DMA resources across MTU changes
2026-09-24 17:44 [PATCH net v3 00/10] net: stmmac: preserve datapath state across MTU and resume failures James Hilliard
` (8 preceding siblings ...)
2026-09-24 17:44 ` [PATCH net v3 09/10] net: stmmac: retain PHY and PM ownership during ethtool reopening James Hilliard
@ 2026-09-24 17:44 ` James Hilliard
2026-09-25 19:12 ` netdev-bot+sashiko
9 siblings, 1 reply; 18+ messages in thread
From: James Hilliard @ 2026-09-24 17:44 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joakim Zhang,
Russell King (Oracle),
Maxime Chevallier, Andrew Lunn, Maxime Coquelin,
Alexandre Torgue, Christian Marangi, Tiezhu Yang, Huacai Chen,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, Serge Semin, Suraj Jaiswal,
Richard Cochran, Joao Pinto, Vladimir Oltean, Ong Boon Leong,
Voon Weifeng, Song, Yoong Siang
Cc: Richard Genoud, Alastair D'Silva, Maxime Ripard, netdev,
linux-kernel, linux-stm32, linux-arm-kernel, bpf, ZhaoJinming,
Lorenzo Bianconi, Ding Hui, James Hilliard
Releasing the current datapath before reopening with a new MTU loses the
working configuration when hardware setup fails. Keep the old rings and
IRQ registrations until replacement hardware setup succeeds, so an
ordinary setup error can restore the old MTU without allocating new rings
or requesting interrupts again.
The preceding changes provide safe allocation unwind, stable DMA/IRQ
objects and a no-reset path for normal-size MTUs. For jumbo transitions,
prepare the replacement first, then quiesce the old queues and drain IRQ
handlers and network readers. Fill holes in the old RX buffer array before
resetting DMA, without altering its descriptors. Rollback can then rebuild
the old rings without allocations.
Only a successful reset permits old descriptors to be rewritten or retired:
clearing a DMA enable bit need not finish an in-flight frame immediately.
Separate buffer allocation from descriptor initialization so preparation
can fail without changing the active rings.
Publish the prospective MTU before MAC setup, since dwmac1000 uses it to
select receive limits. On error, restore the old MTU and configuration
before resetting and rebuilding the old rings. If that reset also fails,
retain the old rings and leave the administratively-up interface detached,
with IRQs released and NAPI stopped, until ordinary down/up recovery.
Distinguish this HALTED state from SUSPENDED, which still owns its IRQs.
Preserve coalescing, PHC registration and packet timestamp filters instead
of running a complete close/open. Reinitialize timestamp hardware after
reset without creating a new PHC. This does not preserve continuous PHC
time across the reset.
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 | 327 +++++++++++++++++-----
2 files changed, 257 insertions(+), 72 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index f07623a1285c..8ebf40789568 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -265,6 +265,8 @@ enum stmmac_datapath_state {
STMMAC_DATAPATH_RUNNING,
/* Resources retained, NAPI and DMA stopped; also after failed resume. */
STMMAC_DATAPATH_SUSPENDED,
+ /* Failed MTU rollback: rings retained, but no IRQs or running NAPI. */
+ STMMAC_DATAPATH_HALTED,
};
struct stmmac_priv {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d17c68f55caf..1a552455a633 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1629,24 +1629,10 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv,
stmmac_clear_tx_descriptors(priv, dma_conf, queue);
}
-/**
- * stmmac_init_rx_buffers - init the RX descriptor buffer.
- * @priv: driver private structure
- * @dma_conf: structure to take the dma data
- * @p: descriptor pointer
- * @i: descriptor index
- * @flags: gfp flag
- * @queue: RX queue index
- * Description: this function is called to allocate a receive buffer, perform
- * the DMA mapping and init the descriptor.
- */
-static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
- struct stmmac_dma_conf *dma_conf,
- struct dma_desc *p,
- int i, gfp_t flags, u32 queue)
+static int stmmac_alloc_rx_buffer(struct stmmac_priv *priv,
+ struct stmmac_rx_queue *rx_q,
+ struct stmmac_rx_buffer *buf)
{
- struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
- struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
gfp_t gfp = (GFP_ATOMIC | __GFP_NOWARN);
if (priv->dma_cap.host_dma_width <= 32)
@@ -1663,19 +1649,49 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
if (!buf->sec_page)
return -ENOMEM;
-
buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
- stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
- } else {
- buf->sec_page = NULL;
- stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
}
+ return 0;
+}
+
+static void stmmac_init_rx_buffer_desc(struct stmmac_priv *priv,
+ struct stmmac_dma_conf *dma_conf,
+ struct dma_desc *p,
+ struct stmmac_rx_buffer *buf)
+{
+ if (buf->sec_page)
+ buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
+ stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, !!buf->sec_page);
buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
stmmac_set_desc_addr(priv, p, buf->addr);
if (dma_conf->dma_buf_sz == BUF_SIZE_16KiB)
stmmac_init_desc3(priv, p);
+}
+
+/**
+ * stmmac_init_rx_buffers - allocate a receive buffer and init its descriptor
+ * @priv: driver private structure
+ * @dma_conf: structure to take the dma data
+ * @p: descriptor pointer
+ * @i: descriptor index
+ * @flags: gfp flag
+ * @queue: RX queue index
+ */
+static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
+ struct stmmac_dma_conf *dma_conf,
+ struct dma_desc *p,
+ int i, gfp_t flags, u32 queue)
+{
+ struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
+ struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
+ int ret;
+
+ ret = stmmac_alloc_rx_buffer(priv, rx_q, buf);
+ if (ret)
+ return ret;
+ stmmac_init_rx_buffer_desc(priv, dma_conf, p, buf);
return 0;
}
@@ -2132,6 +2148,63 @@ static void stmmac_free_tx_skbufs(struct stmmac_priv *priv)
dma_free_tx_skbufs(priv, priv->dma_conf, queue);
}
+/* NAPI is stopped, but DMA may still be using the old rings. Fill holes in
+ * the software buffer array without changing any descriptors. If allocation
+ * fails, the old rings can continue unchanged. Otherwise rollback after a
+ * reset will not need to allocate buffers.
+ */
+static int stmmac_prepare_rx_buffers(struct stmmac_priv *priv)
+{
+ struct stmmac_dma_conf *dma_conf = priv->dma_conf;
+ u32 queue, i;
+ int ret;
+
+ for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
+ struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
+
+ for (i = 0; i < dma_conf->dma_rx_size; i++) {
+ ret = stmmac_alloc_rx_buffer(priv, rx_q, &rx_q->buf_pool[i]);
+ if (ret)
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+/* Only after a successful DMA reset, and with all RX buffers prepared. */
+static void stmmac_reinit_dma_desc(struct stmmac_priv *priv)
+{
+ struct stmmac_dma_conf *dma_conf = priv->dma_conf;
+ u32 queue, i;
+
+ stmmac_free_tx_skbufs(priv);
+ stmmac_reset_queues_param(priv);
+ init_dma_tx_desc_rings(priv->dev, dma_conf);
+ stmmac_clear_descriptors(priv, dma_conf);
+
+ for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
+ struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
+
+ if (rx_q->state_saved)
+ dev_kfree_skb_any(rx_q->state.skb);
+ rx_q->state.skb = NULL;
+ rx_q->state_saved = 0;
+ rx_q->rx_count_frames = 0;
+ rx_q->buf_alloc_num = dma_conf->dma_rx_size;
+
+ for (i = 0; i < dma_conf->dma_rx_size; i++)
+ stmmac_init_rx_buffer_desc(priv, dma_conf,
+ stmmac_get_rx_desc(priv, rx_q, i),
+ &rx_q->buf_pool[i]);
+
+ if (priv->descriptor_mode == STMMAC_CHAIN_MODE)
+ stmmac_mode_init(priv, stmmac_get_rx_desc(priv, rx_q, 0),
+ rx_q->dma_rx_phy, dma_conf->dma_rx_size,
+ priv->extend_desc);
+ }
+}
+
/**
* __free_dma_rx_desc_resources - free RX dma desc resources (per queue)
* @priv: private structure
@@ -3285,12 +3358,13 @@ static int stmmac_prereset_configure(struct stmmac_priv *priv)
/**
* stmmac_init_dma_engine - DMA init.
* @priv: driver private structure
+ * @reinit: rebuild the retained rings after a successful reset
* Description:
* It inits the DMA invoking the specific MAC/GMAC callback.
* Some DMA parameters can be passed from the platform;
* in case of these are not passed a default is kept for the MAC or GMAC.
*/
-static int stmmac_init_dma_engine(struct stmmac_priv *priv)
+static int stmmac_init_dma_engine(struct stmmac_priv *priv, bool reinit)
{
u8 rx_channels_count = priv->plat->rx_queues_to_use;
u8 tx_channels_count = priv->plat->tx_queues_to_use;
@@ -3310,6 +3384,9 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
return ret;
}
+ if (reinit)
+ stmmac_reinit_dma_desc(priv);
+
/* DMA Configuration */
stmmac_dma_init(priv, priv->ioaddr, priv->plat->dma_cfg);
@@ -3657,6 +3734,7 @@ static bool stmmac_tso_channel_permitted(struct stmmac_priv *priv,
/**
* stmmac_hw_setup - setup mac in a usable state.
* @dev : pointer to the device structure.
+ * @reinit: rebuild retained descriptor rings after the DMA reset
* Description:
* this is the main function to setup the HW in a usable state because the
* dma engine is reset, the core registers are configured (e.g. AXI,
@@ -3666,7 +3744,7 @@ static bool stmmac_tso_channel_permitted(struct stmmac_priv *priv,
* 0 on success and an appropriate (-)ve integer as defined in errno.h
* file on failure.
*/
-static int stmmac_hw_setup(struct net_device *dev)
+static int stmmac_hw_setup(struct net_device *dev, bool reinit)
{
struct stmmac_priv *priv = netdev_priv(dev);
u8 rx_cnt = priv->plat->rx_queues_to_use;
@@ -3688,7 +3766,7 @@ static int stmmac_hw_setup(struct net_device *dev)
phylink_rx_clk_stop_block(priv->phylink);
/* DMA initialization and SW reset */
- ret = stmmac_init_dma_engine(priv);
+ ret = stmmac_init_dma_engine(priv, reinit);
if (ret < 0) {
phylink_rx_clk_stop_unblock(priv->phylink);
netdev_err(priv->dev, "%s: DMA engine initialization failed\n",
@@ -3804,8 +3882,7 @@ static void stmmac_free_irq(struct net_device *dev,
for (j = irq_idx - 1; msi && j >= 0; j--) {
if (msi->tx_irq[j] > 0) {
irq_set_affinity_hint(msi->tx_irq[j], NULL);
- free_irq(msi->tx_irq[j],
- &priv->channel[j]);
+ free_irq(msi->tx_irq[j], &priv->channel[j]);
}
}
irq_idx = priv->plat->rx_queues_to_use;
@@ -3814,8 +3891,7 @@ static void stmmac_free_irq(struct net_device *dev,
for (j = irq_idx - 1; msi && j >= 0; j--) {
if (msi->rx_irq[j] > 0) {
irq_set_affinity_hint(msi->rx_irq[j], NULL);
- free_irq(msi->rx_irq[j],
- &priv->channel[j]);
+ free_irq(msi->rx_irq[j], &priv->channel[j]);
}
}
@@ -4080,14 +4156,40 @@ static int stmmac_request_irq(struct net_device *dev)
return ret;
}
+/* Balance disable_irq()/enable_irq() for every registered IRQ, including
+ * shared lines. Unlike freeing and requesting IRQs, this cannot fail.
+ */
+static void stmmac_set_irq_state(struct stmmac_priv *priv, bool enable)
+{
+ void (*set_state)(unsigned int) = enable ? enable_irq : disable_irq;
+ struct stmmac_msi *msi = priv->msi;
+ int irq = priv->dev->irq;
+ u32 i;
+
+ set_state(irq);
+ if (priv->wol_irq > 0 && priv->wol_irq != irq)
+ set_state(priv->wol_irq);
+ if (priv->sfty_irq > 0 && priv->sfty_irq != irq)
+ set_state(priv->sfty_irq);
+ if (!msi)
+ return;
+ if (msi->sfty_ce_irq > 0 && msi->sfty_ce_irq != irq)
+ set_state(msi->sfty_ce_irq);
+ if (msi->sfty_ue_irq > 0 && msi->sfty_ue_irq != irq)
+ set_state(msi->sfty_ue_irq);
+ for (i = 0; i < priv->plat->rx_queues_to_use; i++)
+ if (msi->rx_irq[i] > 0)
+ set_state(msi->rx_irq[i]);
+ for (i = 0; i < priv->plat->tx_queues_to_use; i++)
+ if (msi->tx_irq[i] > 0)
+ set_state(msi->tx_irq[i]);
+}
+
/**
- * stmmac_setup_dma_desc - Generate a dma_conf and allocate DMA queue
- * @priv: driver private structure
- * @mtu: MTU to setup the dma queue and buf with
- * Description: Allocate and generate a dma_conf based on the provided MTU.
- * Allocate the Tx/Rx DMA queue and init them.
- * Return value:
- * the dma_conf allocated struct on success and an appropriate ERR_PTR on failure.
+ * stmmac_setup_dma_desc - allocate and initialize a DMA configuration
+ * @priv: driver private structure
+ * @mtu: MTU to size the receive buffers for
+ * Return: the allocated configuration, or an ERR_PTR on failure
*/
static struct stmmac_dma_conf *
stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
@@ -4220,7 +4322,7 @@ static int __stmmac_open(struct net_device *dev,
stmmac_reset_queues_param(priv);
- ret = stmmac_hw_setup(dev);
+ ret = stmmac_hw_setup(dev, false);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
goto init_error;
@@ -4341,8 +4443,9 @@ static void stmmac_quiesce(struct stmmac_priv *priv)
static void __stmmac_release(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
+ enum stmmac_datapath_state state = priv->datapath;
- /* A failed MTU reopen has already released the data path. */
+ /* There may be no resources left after detached XDP reconfiguration. */
if (priv->datapath == STMMAC_DATAPATH_DOWN)
return;
@@ -4354,7 +4457,8 @@ static void __stmmac_release(struct net_device *dev)
priv->datapath = STMMAC_DATAPATH_DOWN;
/* Free the IRQ lines */
- stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
+ if (state != STMMAC_DATAPATH_HALTED)
+ stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
/* TX error IRQs can restart a queue after the first quiescence. */
stmmac_stop_tx_queues(priv);
@@ -6234,6 +6338,113 @@ static void stmmac_set_rx_mode(struct net_device *dev)
stmmac_set_filter(priv, priv->hw, dev);
}
+static int stmmac_reconfigure_mtu(struct net_device *dev, int mtu)
+{
+ struct stmmac_priv *priv = netdev_priv(dev);
+ struct stmmac_dma_conf *old_conf = priv->dma_conf;
+ struct stmmac_dma_conf *new_conf;
+ int old_mtu = dev->mtu;
+ int ret, restore_ret;
+ u32 chan;
+
+ new_conf = stmmac_setup_dma_desc(priv, mtu);
+ if (IS_ERR(new_conf))
+ return PTR_ERR(new_conf);
+
+ netif_device_detach(dev);
+ phylink_stop(priv->phylink);
+ stmmac_quiesce(priv);
+ timer_delete_sync(&priv->eee_ctrl_timer);
+ if (stmmac_fpe_supported(priv))
+ ethtool_mmsv_stop(&priv->fpe_cfg.mmsv);
+
+ /* An IRQ can recover a TX error and restart a queue. Drain handlers
+ * before the final TX stop, and keep the registrations for rollback.
+ */
+ stmmac_set_irq_state(priv, false);
+ netif_tx_disable(dev);
+ synchronize_net();
+
+ ret = stmmac_prepare_rx_buffers(priv);
+ if (ret)
+ goto restart;
+
+ stmmac_stop_all_dma(priv);
+ phylink_prepare_resume(priv->phylink);
+
+ /* MAC receive limits must be programmed for the prospective MTU. */
+ WRITE_ONCE(dev->mtu, mtu);
+ priv->dma_conf = new_conf;
+ stmmac_reset_queues_param(priv);
+ ret = stmmac_hw_setup(dev, false);
+ if (ret) {
+ stmmac_stop_all_dma(priv);
+ stmmac_mac_set(priv, priv->ioaddr, false);
+ priv->dma_conf = old_conf;
+ WRITE_ONCE(dev->mtu, old_mtu);
+
+ /* Reuse the retained rings. Reinitialize them only after reset
+ * has completed, not merely after clearing the DMA enable bits.
+ */
+ restore_ret = stmmac_hw_setup(dev, true);
+ if (restore_ret) {
+ stmmac_stop_all_dma(priv);
+ stmmac_mac_set(priv, priv->ioaddr, false);
+ stmmac_set_irq_state(priv, true);
+ stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
+ netif_tx_disable(dev);
+ stmmac_stop_all_dma(priv);
+ priv->datapath = STMMAC_DATAPATH_HALTED;
+ netdev_err(dev, "MTU rollback failed: %pe; interface remains detached\n",
+ ERR_PTR(restore_ret));
+ goto free_new;
+ }
+ } else {
+ /* Hardware setup completed its reset before using the new rings.
+ * The old DMA allocations can now be released safely.
+ */
+ free_dma_desc_resources(priv, old_conf);
+ kfree(old_conf);
+ for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
+ hrtimer_setup(&new_conf->tx_queue[chan].txtimer,
+ stmmac_tx_timer, CLOCK_MONOTONIC,
+ HRTIMER_MODE_REL);
+ }
+
+ /* Restore timestamping without registering a new PHC or resetting the
+ * user's packet timestamp filters. Timestamping can also be used without
+ * a registered PHC (CONFIG_PTP_1588_CLOCK=n).
+ */
+ if ((priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp) &&
+ priv->plat->clk_ptp_rate) {
+ unsigned long flags;
+
+ write_lock_irqsave(&priv->ptp_lock, flags);
+ stmmac_init_tstamp_counter(priv, priv->systime_flags);
+ if (priv->plat->flags & STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY)
+ stmmac_hwtstamp_correct_latency(priv, priv);
+ write_unlock_irqrestore(&priv->ptp_lock, flags);
+ }
+ stmmac_set_rx_mode(dev);
+ stmmac_vlan_restore(priv);
+
+restart:
+ stmmac_enable_all_queues(priv);
+ stmmac_enable_all_dma_irq(priv);
+ stmmac_set_irq_state(priv, true);
+ phylink_start(priv->phylink);
+ netif_device_attach(dev);
+ for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
+ stmmac_tx_timer_arm(priv, chan);
+ if (!ret)
+ return 0;
+
+free_new:
+ free_dma_desc_resources(priv, new_conf);
+ kfree(new_conf);
+ return ret;
+}
+
/**
* stmmac_change_mtu - entry point to change MTU size for the device.
* @dev : device pointer.
@@ -6248,9 +6459,7 @@ static void stmmac_set_rx_mode(struct net_device *dev)
static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
{
struct stmmac_priv *priv = netdev_priv(dev);
- struct stmmac_dma_conf *old_conf = priv->dma_conf;
int txfifosz = priv->plat->tx_fifo_size;
- struct stmmac_dma_conf *dma_conf;
const int mtu = new_mtu;
int ret;
@@ -6276,36 +6485,9 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
*/
if (netif_running(dev) &&
(dev->mtu > ETH_DATA_LEN || mtu > ETH_DATA_LEN)) {
- netdev_dbg(priv->dev, "restarting interface to change its MTU\n");
- /* Try to allocate the new DMA conf with the new mtu */
- dma_conf = stmmac_setup_dma_desc(priv, mtu);
- if (IS_ERR(dma_conf)) {
- netdev_err(priv->dev, "failed allocating new dma conf for new MTU %d\n",
- mtu);
- return PTR_ERR(dma_conf);
- }
-
- netif_device_detach(dev);
- __stmmac_release(dev);
-
- ret = __stmmac_open(dev, dma_conf);
- if (ret) {
- priv->dma_conf = old_conf;
- 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");
+ ret = stmmac_reconfigure_mtu(dev, mtu);
+ if (ret)
return ret;
- }
-
- kfree(old_conf);
-
- stmmac_set_rx_mode(dev);
- netif_device_attach(dev);
}
WRITE_ONCE(dev->mtu, mtu);
@@ -7091,7 +7273,8 @@ static int stmmac_bpf(struct net_device *dev, struct netdev_bpf *bpf)
* 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)
+ if (priv->datapath == STMMAC_DATAPATH_SUSPENDED ||
+ priv->datapath == STMMAC_DATAPATH_HALTED)
__stmmac_release(dev);
switch (bpf->command) {
@@ -8470,7 +8653,7 @@ int stmmac_resume(struct device *dev)
stmmac_free_tx_skbufs(priv);
stmmac_clear_descriptors(priv, priv->dma_conf);
- ret = stmmac_hw_setup(ndev);
+ ret = stmmac_hw_setup(ndev, false);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
goto error_stop_dma;
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net v3 10/10] net: stmmac: retain DMA resources across MTU changes
2026-09-24 17:44 ` [PATCH net v3 10/10] net: stmmac: retain DMA resources across MTU changes James Hilliard
@ 2026-09-25 19:12 ` netdev-bot+sashiko
0 siblings, 0 replies; 18+ messages in thread
From: netdev-bot+sashiko @ 2026-09-25 19:12 UTC (permalink / raw)
To: james.hilliard1
Cc: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni,
qiangqing.zhang, rmk+kernel, maxime.chevallier, andrew+netdev,
mcoquelin.stm32, alexandre.torgue, ansuelsmth, yangtiezhu,
chenhuacai, ast, daniel, hawk, john.fastabend, sdf,
fancer.lancer, quic_jsuraj, richardcochran, Joao.Pinto,
vladimir.oltean, boon.leong.ong, weifeng.voon, yoong.siang.song,
richard.genoud, alastair, mripard, netdev, linux-kernel,
linux-stm32, linux-arm-kernel, bpf, zhaojinming,
lorenzo.bianconi, dinghui1111
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 3 · Low: 2
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] After a rollback, stmmac_reinit_dma_desc() initializes the RX
descriptors in the wrong order.
- [Medium] stmmac_reconfigure_mtu() leaves out the platform PTP clock-
frequency hook after the MAC/DMA reset.
- [Medium] The PHC now stays registered across the jumbo MTU reset, but
the PTP hardware state that users configured through it is silently…
- [Medium] stmmac_set_irq_state(priv, false) calls disable_irq() on
dev->irq, wol_irq and sfty_irq.
- [Low] Two comments were not updated when STMMAC_DATAPATH_HALTED was
added: - In stmmac_bpf() (stmmac_main.c:7271-7275), the comment talks…
- [Low] A comment in __stmmac_open() (stmmac_main.c:4314-4315) gives
'e.g. on MTU change' as the case where the interface is reopened…
--
net: stmmac: retain DMA resources across MTU changes
This patch changes how stmmac handles an MTU change across ETH_DATA_LEN on
a running interface. It no longer does a full close/open. Instead it
prepares a new DMA configuration, quiesces the old queues with their IRQs
disabled, and resets the hardware onto the new rings. On failure it rolls
back to the old rings, reinitializing them after a successful reset. If
that rollback also fails, it leaves the interface in a new HALTED state.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d17c68f55caf6..1a552455a633d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> +/* Only after a successful DMA reset, and with all RX buffers prepared. */
> +static void stmmac_reinit_dma_desc(struct stmmac_priv *priv)
> +{
> + struct stmmac_dma_conf *dma_conf = priv->dma_conf;
> + u32 queue, i;
> +
> + stmmac_free_tx_skbufs(priv);
> + stmmac_reset_queues_param(priv);
> + init_dma_tx_desc_rings(priv->dev, dma_conf);
> + stmmac_clear_descriptors(priv, dma_conf);
> +
> + for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
> + struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
> +
> + if (rx_q->state_saved)
> + dev_kfree_skb_any(rx_q->state.skb);
> + rx_q->state.skb = NULL;
> + rx_q->state_saved = 0;
> + rx_q->rx_count_frames = 0;
> + rx_q->buf_alloc_num = dma_conf->dma_rx_size;
> +
> + for (i = 0; i < dma_conf->dma_rx_size; i++)
> + stmmac_init_rx_buffer_desc(priv, dma_conf,
> + stmmac_get_rx_desc(priv, rx_q, i),
> + &rx_q->buf_pool[i]);
[Severity: High]
Can this order leave the RX descriptors without the OWN bit on dwmac4 and
xgmac cores?
On dwmac4, stmmac_clear_descriptors() reaches dwmac4_rd_init_rx_desc() and
then dwmac4_set_rx_owner(). That ORs RDES3_OWN, RDES3_BUFFER1_VALID_ADDR
and the IOC bit into des3.
The loop above then calls stmmac_init_rx_buffer_desc() for every
descriptor. That function always calls stmmac_set_desc_sec_addr(), which
on dwmac4 is:
drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:dwmac4_set_sec_addr() {
p->des2 = cpu_to_le32(lower_32_bits(addr));
p->des3 = cpu_to_le32(upper_32_bits(addr));
...
}
dwxgmac2_set_sec_addr() does the same. The plain assignment to des3 clears
OWN, BUF1V and IOC, and nothing sets them again. After that,
stmmac_init_dma_engine() programs the RX tail pointer with
buf_alloc_num = dma_rx_size, and stmmac_hw_setup() starts RX DMA.
The existing init_dma_desc_rings() writes the buffer addresses first and
sets ownership last:
ret = init_dma_rx_desc_rings(dev, dma_conf, flags);
...
ret = init_dma_tx_desc_rings(dev, dma_conf);
stmmac_clear_descriptors(priv, dma_conf);
Take the case where the first stmmac_hw_setup(dev, false) in
stmmac_reconfigure_mtu() fails and the rollback succeeds. Would the
interface then be reattached with no RX descriptors owned by the hardware
on GMAC4/5 and XGMAC? enh_desc and norm_desc keep OWN in des0, so they do
not seem to be affected.
Should stmmac_clear_descriptors() move after the RX loop, as in
init_dma_desc_rings()?
> +
> + if (priv->descriptor_mode == STMMAC_CHAIN_MODE)
> + stmmac_mode_init(priv, stmmac_get_rx_desc(priv, rx_q, 0),
> + rx_q->dma_rx_phy, dma_conf->dma_rx_size,
> + priv->extend_desc);
> + }
> +}
[ ... ]
> @@ -4220,7 +4322,7 @@ static int __stmmac_open(struct net_device *dev,
>
> stmmac_reset_queues_param(priv);
>
> - ret = stmmac_hw_setup(dev);
> + ret = stmmac_hw_setup(dev, false);
[Severity: Low]
This isn't a bug, but the comment earlier in __stmmac_open() still gives
an MTU change as its example:
/* The PHY is suspended when the interface is reopened without
* disconnecting the PHY, e.g. on MTU change. IEEE 802.3 allows PHYs
After this patch, stmmac_change_mtu() no longer reaches __stmmac_open(),
and stmmac_reconfigure_mtu() calls phylink_prepare_resume() itself. The
remaining callers are stmmac_open() and stmmac_reopen(). Could the example
be updated?
> if (ret < 0) {
> netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
> goto init_error;
[ ... ]
> +static int stmmac_reconfigure_mtu(struct net_device *dev, int mtu)
> +{
[ ... ]
> + /* An IRQ can recover a TX error and restart a queue. Drain handlers
> + * before the final TX stop, and keep the registrations for rollback.
> + */
> + stmmac_set_irq_state(priv, false);
[Severity: Medium]
Does disabling these lines also block other devices that share them?
In single-IRQ mode, stmmac_request_irq_single() requests the lines with
IRQF_SHARED:
ret = request_irq(dev->irq, stmmac_interrupt,
IRQF_SHARED, dev->name, dev);
The comment on stmmac_set_irq_state() also says it covers shared lines.
disable_irq() masks the whole line at the interrupt controller, so any
other device on dev->irq, wol_irq or sfty_irq gets no interrupts from here
until the restart label.
That window includes:
synchronize_net()
the RX buffer refill in stmmac_prepare_rx_buffers()
the DMA software reset poll, which can wait a long time without an
RX clock
a full stmmac_hw_setup()
on failure, a second reset for the rollback
Would it work to mask the device's own DMA/MAC interrupt enables and then
call synchronize_irq(), instead of disabling a shared line?
> + netif_tx_disable(dev);
> + synchronize_net();
[ ... ]
> + /* Restore timestamping without registering a new PHC or resetting the
> + * user's packet timestamp filters. Timestamping can also be used without
> + * a registered PHC (CONFIG_PTP_1588_CLOCK=n).
> + */
> + if ((priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp) &&
> + priv->plat->clk_ptp_rate) {
> + unsigned long flags;
> +
> + write_lock_irqsave(&priv->ptp_lock, flags);
> + stmmac_init_tstamp_counter(priv, priv->systime_flags);
> + if (priv->plat->flags & STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY)
> + stmmac_hwtstamp_correct_latency(priv, priv);
> + write_unlock_irqrestore(&priv->ptp_lock, flags);
> + }
[Severity: Medium]
Is the platform ptp_clk_freq_config() hook skipped here?
Before this patch, the jumbo MTU path went through
__stmmac_open()->stmmac_setup_ptp()->stmmac_init_timestamping(). That
function calls the hook after the reset done in stmmac_hw_setup():
if (priv->plat->ptp_clk_freq_config)
priv->plat->ptp_clk_freq_config(priv);
stmmac_resume() also calls stmmac_init_timestamping() after its reset.
On Intel mGBE, intel_mgbe_ptp_clk_freq_config() selects the PTP reference
clock with the GPO bits in GMAC_GPIO_STATUS, and the software reset clears
them. On PSE, 200 MHz needs GMAC_GPO0 | GMAC_GPO3, while the zero value
(PSE_PTP_CLK_FREQ_256MHZ) selects 256 MHz.
stmmac_init_tstamp_counter() still computes the sub-second increment and
addend from clk_ptp_rate (200 MHz). Would the PHC and packet timestamps
then run at the wrong rate until the next down/up?
[Severity: Medium]
The PHC now stays registered across the reset. What happens to the PTP
state that users configured through it?
The DMA software reset in stmmac_hw_setup() also resets the MAC timestamp
and PPS block, but this code only restores the counter:
stmmac_init_tstamp_counter()->stmmac_update_subsecond_increment()
writes priv->default_addend. That drops any adjfine correction that
stmmac_adjust_freq() had programmed into the addend register.
Flexible PPS/PEROUT outputs that stmmac_enable() set up with
stmmac_flex_pps_config() and recorded in priv->pps[] are not
re-applied.
EXTTS auxiliary snapshot enables in PTP_ACR are not restored, although
STMMAC_FLAG_EXT_SNAPSHOT_EN stays set in priv->plat->flags.
Before the patch, the PHC was unregistered, so consumers could see that
this state was gone. Now /dev/ptpN stays valid while PPS output and EXTTS
events stop without notice, and the frequency cached by the PTP core no
longer matches the hardware.
The commit message only says:
This does not preserve continuous PHC time across the reset.
Should these settings be replayed after the reset, or at least be listed
in the commit message?
> + stmmac_set_rx_mode(dev);
> + stmmac_vlan_restore(priv);
[ ... ]
> @@ -7091,7 +7273,8 @@ static int stmmac_bpf(struct net_device *dev, struct netdev_bpf *bpf)
> * 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)
> + if (priv->datapath == STMMAC_DATAPATH_SUSPENDED ||
> + priv->datapath == STMMAC_DATAPATH_HALTED)
> __stmmac_release(dev);
[Severity: Low]
This isn't a bug, but the comment above still mentions only "a failed
resume" and "suspended rings". The condition now also covers the HALTED
state left by a failed MTU rollback.
The same applies in __stmmac_release(), where the comment on skipping the
quiesce mentions only suspend:
/* Suspend retains the resources, but has already stopped activity. */
if (priv->datapath == STMMAC_DATAPATH_RUNNING)
stmmac_quiesce(priv);
HALTED also takes this path, and it additionally skips stmmac_free_irq().
Could both comments be updated to cover HALTED?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com
^ permalink raw reply [flat|nested] 18+ messages in thread