[PATCH net-next v4 0/4] net: stmmac: preserve and restore the installed EST schedule

[PATCH net-next v4 0/4] net: stmmac: preserve and restore the installed EST schedule

[James Hilliard]

Continue Lorenzo Bianconi's EST replay series with v4 of the unmerged work:
https://lore.kernel.org/r/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com

The PTP initialization fix from that series is already in the base.
Carry the remaining embedded-state and explicit-enable preparations,
and restore the installed schedule after open and resume reset.

Build TAPRIO replacements separately so rejected updates cannot
corrupt the installed schedule. Rebase the previous schedule against
the current PHC when rolling back a failed hardware update, and keep
DMA stopped until replay succeeds. These changes do not depend on
the later datapath recovery series.

Changes in v4:
- Omit the merged PTP initialization patch and adapt the remaining
  changes to probe-time PTP locking and current TAPRIO error handling.
- Separate schedule validation/rollback from open and resume replay.
- Reuse the base-time helper for replacement, PHC adjustment and
  rollback; propagate errors while preserving the installed cache.
- Start DMA only after the installed schedule has been restored.
- Link to v3: https://lore.kernel.org/r/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com

Changes in v3:
- Guard priv->est.enable update using est_lock mutex.
- Embed stmmac_est in stmmac_priv struct.
- Rework locking in tc_taprio_configure().
- Fix possible divided by zero crash in tc_taprio_configure().
- Add missing reconfiguration during stmmac_resume().
- Honor ptp error code in __stmmac_open() and stmmac_resume().
- Link to v2: https://lore.kernel.org/r/20260829-stmmac-est-reapply-after-open-v2-1-5e5ccb185e92@oss.qualcomm.com

Changes in v2:
- Rename stmmac_est_reconfigure() in stmmac_setup_est().
- Rely on stmmac_setup_est() in tc_taprio_configure().
- Link to v1: https://lore.kernel.org/r/20260825-stmmac-est-reapply-after-open-v1-1-dfa80735e0a1@oss.qualcomm.com

Assisted-by: Codex:gpt-6-astra
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>

---
James Hilliard (1):
      net: stmmac: preserve the installed EST schedule on errors

Lorenzo Bianconi (3):
      net: stmmac: embed struct stmmac_est in stmmac_priv struct
      net: stmmac: pass the desired EST enable state to est_configure()
      net: stmmac: restore EST before starting DMA on open and resume

 drivers/net/ethernet/stmicro/stmmac/hwif.h        |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.c  |  47 ++++++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  |  13 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  38 ++++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  |  59 ++++-----
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 142 +++++++++++-----------
 7 files changed, 176 insertions(+), 127 deletions(-)
---
base-commit: 1631d79ae57dce2c5f88ad278307028638a8b4d9
change-id: 20260824-stmmac-est-reapply-after-open-181d70a15eb6

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

[PATCH net-next v4 1/4] net: stmmac: embed struct stmmac_est in stmmac_priv struct

[James Hilliard]

From: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

This is a preliminary change to fix EST reconfiguration in the open
and resume paths: the taprio offload must be re-applied after the DMA
soft reset clears the MTL_EST registers, but the current layout makes
that fragile.
priv->est is currently a pointer allocated with devm_kzalloc() on the
first taprio REPLACE, and the mutex guarding it (priv->est_lock) is
initialized at the same time. That ties the lock's validity to whether
taprio has ever been configured, so the EST parameters can not be
read under the lock (e.g. to check priv->est->enable in the open and
resume paths) before the first offload setup.
Embed struct stmmac_est into struct stmmac_priv and initialize the mutex
in probe(). This makes the code simpler (no logical changes added).
Moreover, the lock is now unconditionally valid, so the enable flag can
be inspected under the lock from any control path.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
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 | 17 +++----
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 22 ++++----
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 61 ++++++++++-------------
 4 files changed, 45 insertions(+), 57 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 4fc96b317d79..12f353fabd6b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -298,7 +298,7 @@ struct stmmac_priv {
 	struct plat_stmmacenet_data *plat;
 	/* Protect est parameters */
 	struct mutex est_lock;
-	struct stmmac_est *est;
+	struct stmmac_est est;
 	struct dma_features dma_cap;
 	struct stmmac_counters mmc;
 	int hw_cap_support;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3ad9252bf6ae..e93f3238be1f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2745,9 +2745,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
 		if (!xsk_tx_peek_desc(pool, &xdp_desc))
 			break;
 
-		if (priv->est && priv->est->enable &&
-		    priv->est->max_sdu[queue] &&
-		    xdp_desc.len > priv->est->max_sdu[queue]) {
+		if (priv->est.enable && priv->est.max_sdu[queue] &&
+		    xdp_desc.len > priv->est.max_sdu[queue]) {
 			priv->xstats.max_sdu_txq_drop[queue]++;
 			continue;
 		}
@@ -4843,13 +4842,12 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (skb_is_gso(skb))
 		return stmmac_tso_xmit(skb, dev);
 
-	if (priv->est && priv->est->enable &&
-	    priv->est->max_sdu[queue]) {
+	if (priv->est.enable && priv->est.max_sdu[queue]) {
 		sdu_len = skb->len;
 		/* Add VLAN tag length if VLAN tag insertion offload is requested */
 		if (priv->dma_cap.vlins && skb_vlan_tag_present(skb))
 			sdu_len += VLAN_HLEN;
-		if (sdu_len > priv->est->max_sdu[queue]) {
+		if (sdu_len > priv->est.max_sdu[queue]) {
 			priv->xstats.max_sdu_txq_drop[queue]++;
 			goto max_sdu_err;
 		}
@@ -5253,9 +5251,8 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
 	if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
 		return STMMAC_XDP_CONSUMED;
 
-	if (priv->est && priv->est->enable &&
-	    priv->est->max_sdu[queue] &&
-	    xdpf->len > priv->est->max_sdu[queue]) {
+	if (priv->est.enable && priv->est.max_sdu[queue] &&
+	    xdpf->len > priv->est.max_sdu[queue]) {
 		priv->xstats.max_sdu_txq_drop[queue]++;
 		return STMMAC_XDP_CONSUMED;
 	}
@@ -8106,6 +8103,7 @@ static int __stmmac_dvr_probe(struct device *device,
 	stmmac_napi_add(ndev);
 
 	mutex_init(&priv->lock);
+	mutex_init(&priv->est_lock);
 	rwlock_init(&priv->ptp_lock);
 
 	stmmac_fpe_init(priv);
@@ -8238,6 +8236,7 @@ void stmmac_dvr_remove(struct device *dev)
 	stmmac_mdio_unregister(ndev);
 
 	destroy_workqueue(priv->wq);
+	mutex_destroy(&priv->est_lock);
 	mutex_destroy(&priv->lock);
 	bitmap_free(priv->af_xdp_zc_qps);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 3bfcc9760dce..2a4099fe470c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -69,11 +69,11 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 	nsec = reminder;
 
 	/* If EST is enabled, disabled it before adjust ptp time. */
-	if (priv->est && priv->est->enable) {
+	if (priv->est.enable) {
 		est_rst = true;
 		mutex_lock(&priv->est_lock);
-		priv->est->enable = false;
-		stmmac_est_configure(priv, priv, priv->est,
+		priv->est.enable = false;
+		stmmac_est_configure(priv, priv, &priv->est,
 				     priv->plat->clk_ptp_rate);
 		mutex_unlock(&priv->est_lock);
 	}
@@ -91,19 +91,19 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 		mutex_lock(&priv->est_lock);
 		priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, &current_time);
 		current_time_ns = timespec64_to_ktime(current_time);
-		time.tv_nsec = priv->est->btr_reserve[0];
-		time.tv_sec = priv->est->btr_reserve[1];
+		time.tv_nsec = priv->est.btr_reserve[0];
+		time.tv_sec = priv->est.btr_reserve[1];
 		basetime = timespec64_to_ktime(time);
-		cycle_time = (u64)priv->est->ctr[1] * NSEC_PER_SEC +
-			     priv->est->ctr[0];
+		cycle_time = (u64)priv->est.ctr[1] * NSEC_PER_SEC +
+			     priv->est.ctr[0];
 		time = stmmac_calc_tas_basetime(basetime,
 						current_time_ns,
 						cycle_time);
 
-		priv->est->btr[0] = (u32)time.tv_nsec;
-		priv->est->btr[1] = (u32)time.tv_sec;
-		priv->est->enable = true;
-		ret = stmmac_est_configure(priv, priv, priv->est,
+		priv->est.btr[0] = (u32)time.tv_nsec;
+		priv->est.btr[1] = (u32)time.tv_sec;
+		priv->est.enable = true;
+		ret = stmmac_est_configure(priv, priv, &priv->est,
 					   priv->plat->clk_ptp_rate);
 		mutex_unlock(&priv->est_lock);
 		if (ret)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 42a00446e9b4..357d1eaf0d7d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -959,7 +959,7 @@ static void tc_taprio_map_maxsdu_txq(struct stmmac_priv *priv,
 		count = qopt->mqprio.qopt.count[i];
 
 		for (j = offset; j < offset + count; j++)
-			priv->est->max_sdu[j] = qopt->max_sdu[i] + ETH_HLEN - ETH_TLEN;
+			priv->est.max_sdu[j] = qopt->max_sdu[i] + ETH_HLEN - ETH_TLEN;
 	}
 }
 
@@ -1023,24 +1023,15 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	if (qopt->cycle_time_extension >= BIT(wid + 7))
 		return -ERANGE;
 
-	if (!priv->est) {
-		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
-					 GFP_KERNEL);
-		if (!priv->est)
-			return -ENOMEM;
-
-		mutex_init(&priv->est_lock);
-	} else {
-		mutex_lock(&priv->est_lock);
-		memset(priv->est, 0, sizeof(*priv->est));
-		mutex_unlock(&priv->est_lock);
-	}
+	mutex_lock(&priv->est_lock);
+	memset(&priv->est, 0, sizeof(priv->est));
+	mutex_unlock(&priv->est_lock);
 
 	size = qopt->num_entries;
 
 	mutex_lock(&priv->est_lock);
-	priv->est->gcl_size = size;
-	priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
+	priv->est.gcl_size = size;
+	priv->est.enable = qopt->cmd == TAPRIO_CMD_REPLACE;
 	mutex_unlock(&priv->est_lock);
 
 	for (i = 0; i < size; i++) {
@@ -1065,7 +1056,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 			return -EOPNOTSUPP;
 		}
 
-		priv->est->gcl[i] = delta_ns | (gates << wid);
+		priv->est.gcl[i] = delta_ns | (gates << wid);
 	}
 
 	mutex_lock(&priv->est_lock);
@@ -1075,22 +1066,22 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	time = stmmac_calc_tas_basetime(qopt->base_time, current_time_ns,
 					qopt->cycle_time);
 
-	priv->est->btr[0] = (u32)time.tv_nsec;
-	priv->est->btr[1] = (u32)time.tv_sec;
+	priv->est.btr[0] = (u32)time.tv_nsec;
+	priv->est.btr[1] = (u32)time.tv_sec;
 
 	qopt_time = ktime_to_timespec64(qopt->base_time);
-	priv->est->btr_reserve[0] = (u32)qopt_time.tv_nsec;
-	priv->est->btr_reserve[1] = (u32)qopt_time.tv_sec;
+	priv->est.btr_reserve[0] = (u32)qopt_time.tv_nsec;
+	priv->est.btr_reserve[1] = (u32)qopt_time.tv_sec;
 
 	ctr = qopt->cycle_time;
-	priv->est->ctr[0] = do_div(ctr, NSEC_PER_SEC);
-	priv->est->ctr[1] = (u32)ctr;
+	priv->est.ctr[0] = do_div(ctr, NSEC_PER_SEC);
+	priv->est.ctr[1] = (u32)ctr;
 
-	priv->est->ter = qopt->cycle_time_extension;
+	priv->est.ter = qopt->cycle_time_extension;
 
 	tc_taprio_map_maxsdu_txq(priv, qopt);
 
-	ret = stmmac_est_configure(priv, priv, priv->est,
+	ret = stmmac_est_configure(priv, priv, &priv->est,
 				   priv->plat->clk_ptp_rate);
 	mutex_unlock(&priv->est_lock);
 	if (ret) {
@@ -1106,19 +1097,17 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	return 0;
 
 disable:
-	if (priv->est) {
-		mutex_lock(&priv->est_lock);
-		priv->est->enable = false;
-		stmmac_est_configure(priv, priv, priv->est,
-				     priv->plat->clk_ptp_rate);
-		/* Reset taprio status */
-		for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
-			priv->xstats.max_sdu_txq_drop[i] = 0;
-			priv->xstats.mtl_est_txq_hlbf[i] = 0;
-			priv->xstats.mtl_est_txq_hlbs[i] = 0;
-		}
-		mutex_unlock(&priv->est_lock);
+	mutex_lock(&priv->est_lock);
+	priv->est.enable = false;
+	stmmac_est_configure(priv, priv, &priv->est,
+			     priv->plat->clk_ptp_rate);
+	/* Reset taprio status */
+	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
+		priv->xstats.max_sdu_txq_drop[i] = 0;
+		priv->xstats.mtl_est_txq_hlbf[i] = 0;
+		priv->xstats.mtl_est_txq_hlbs[i] = 0;
 	}
+	mutex_unlock(&priv->est_lock);
 
 	err = stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
 

-- 
2.53.0

[PATCH net-next v4 2/4] net: stmmac: pass the desired EST enable state to est_configure()

[James Hilliard]

From: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Pass the desired EST enable state explicitly to est_configure() instead
of having it derive the EEST/EST_INT_EN bits from cfg->enable. This
decouples the hardware programming state from the priv->est.enable
flag, which records whether the taprio offload is attached.

No functional change intended: the callers keep toggling
priv->est.enable around the EST programming, as before.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/hwif.h       | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.c | 6 +++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 4 ++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c  | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index a8a5c8fdd5ed..1fd9f1ab316e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -617,7 +617,7 @@ struct stmmac_mmc_ops {
 
 struct stmmac_est_ops {
 	int (*configure)(struct stmmac_priv *priv, struct stmmac_est *cfg,
-			 unsigned int ptp_rate);
+			 unsigned int ptp_rate, bool enable);
 	void (*irq_status)(struct stmmac_priv *priv, struct net_device *dev,
 			   struct stmmac_extra_stats *x, u32 txqcnt);
 };
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
index afc516059b89..f15d4d046aa7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
@@ -26,7 +26,7 @@ static int est_write(void __iomem *est_addr, u32 reg, u32 val, bool gcl)
 }
 
 static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
-			 unsigned int ptp_rate)
+			 unsigned int ptp_rate, bool enable)
 {
 	void __iomem *est_addr = priv->estaddr;
 	int i, ret = 0;
@@ -62,7 +62,7 @@ static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
 		ctrl |= ((NSEC_PER_SEC / ptp_rate) * EST_GMAC5_PTOV_MUL) <<
 			 EST_GMAC5_PTOV_SHIFT;
 	}
-	if (cfg->enable)
+	if (enable)
 		ctrl |= EST_EEST | EST_SSWL | EST_DFBS;
 	else
 		ctrl &= ~EST_EEST;
@@ -70,7 +70,7 @@ static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
 	writel(ctrl, est_addr + EST_CONTROL);
 
 	/* Configure EST interrupt */
-	if (cfg->enable)
+	if (enable)
 		ctrl = EST_IECGCE | EST_IEHS | EST_IEHF | EST_IEBE | EST_IECC;
 	else
 		ctrl = 0;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 2a4099fe470c..64d890664421 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -74,7 +74,7 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 		mutex_lock(&priv->est_lock);
 		priv->est.enable = false;
 		stmmac_est_configure(priv, priv, &priv->est,
-				     priv->plat->clk_ptp_rate);
+				     priv->plat->clk_ptp_rate, false);
 		mutex_unlock(&priv->est_lock);
 	}
 
@@ -104,7 +104,7 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 		priv->est.btr[1] = (u32)time.tv_sec;
 		priv->est.enable = true;
 		ret = stmmac_est_configure(priv, priv, &priv->est,
-					   priv->plat->clk_ptp_rate);
+					   priv->plat->clk_ptp_rate, true);
 		mutex_unlock(&priv->est_lock);
 		if (ret)
 			netdev_err(priv->dev, "failed to configure EST\n");
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 357d1eaf0d7d..67c6fc32d0ea 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1082,7 +1082,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	tc_taprio_map_maxsdu_txq(priv, qopt);
 
 	ret = stmmac_est_configure(priv, priv, &priv->est,
-				   priv->plat->clk_ptp_rate);
+				   priv->plat->clk_ptp_rate, true);
 	mutex_unlock(&priv->est_lock);
 	if (ret) {
 		netdev_err(priv->dev, "failed to configure EST\n");
@@ -1100,7 +1100,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	mutex_lock(&priv->est_lock);
 	priv->est.enable = false;
 	stmmac_est_configure(priv, priv, &priv->est,
-			     priv->plat->clk_ptp_rate);
+			     priv->plat->clk_ptp_rate, false);
 	/* Reset taprio status */
 	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
 		priv->xstats.max_sdu_txq_drop[i] = 0;

-- 
2.53.0

[PATCH net-next v4 3/4] net: stmmac: preserve the installed EST schedule on errors

[James Hilliard]

TAPRIO replacement clears the installed EST cache before validating
all entries. An invalid interval can leave enable set with a zero
cycle time. A later PHC adjustment then divides by zero while
calculating the next base time.

Build and validate a separate schedule and publish it only after
hardware setup succeeds. If setup fails, restore the installed
schedule using a base time advanced by whole cycles, not the expired
base time from its original installation.

Use the same checked clock-read and programming helper for new
schedules, rollback and PHC adjustment. Hold est_lock throughout a
clock step and return disable, adjustment or replay errors. Attempt
to rearm the schedule even when the clock update fails.

Based on the schedule replay helper in Lorenzo Bianconi's EST work.

Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
Co-developed-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.c |  41 ++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.h |  13 +++
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c |  57 ++++--------
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c  | 113 ++++++++++++-----------
 4 files changed, 134 insertions(+), 90 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
index f15d4d046aa7..15c2d1794e22 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
@@ -80,6 +80,47 @@ static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
 	return 0;
 }
 
+/* Program either the installed schedule or an unpublished replacement. */
+int __stmmac_setup_est(struct stmmac_priv *priv, struct stmmac_est *est)
+{
+	struct timespec64 current_time, time;
+	ktime_t current_time_ns, basetime;
+	unsigned long flags;
+	u64 now;
+	u64 cycle_time;
+	int err;
+
+	lockdep_assert_held(&priv->est_lock);
+
+	if (!priv->ptp_enabled)
+		return -EOPNOTSUPP;
+
+	read_lock_irqsave(&priv->ptp_lock, flags);
+	err = stmmac_get_systime(priv, priv->ptpaddr, &now);
+	read_unlock_irqrestore(&priv->ptp_lock, flags);
+	if (err)
+		return err;
+	current_time = ns_to_timespec64(now);
+	current_time_ns = timespec64_to_ktime(current_time);
+
+	time.tv_nsec = est->btr_reserve[0];
+	time.tv_sec = est->btr_reserve[1];
+	basetime = timespec64_to_ktime(time);
+
+	cycle_time = (u64)est->ctr[1] * NSEC_PER_SEC + est->ctr[0];
+
+	time = stmmac_calc_tas_basetime(basetime, current_time_ns, cycle_time);
+	est->btr[0] = (u32)time.tv_nsec;
+	est->btr[1] = (u32)time.tv_sec;
+
+	err = stmmac_est_configure(priv, priv, est,
+				   priv->plat->clk_ptp_rate, true);
+	if (err)
+		netdev_err(priv->dev, "failed to re-configure EST\n");
+
+	return err;
+}
+
 static void est_irq_status(struct stmmac_priv *priv, struct net_device *dev,
 			   struct stmmac_extra_stats *x, u32 txqcnt)
 {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
index f70221c9c84a..5665e7a53994 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
@@ -65,3 +65,16 @@
 #define EST_GCL_DATA			0x00000034
 
 extern const struct stmmac_est_ops dwmac510_est_ops;
+
+int __stmmac_setup_est(struct stmmac_priv *priv, struct stmmac_est *est);
+static inline int stmmac_setup_est(struct stmmac_priv *priv)
+{
+	int ret = 0;
+
+	mutex_lock(&priv->est_lock);
+	if (priv->est.enable)
+		ret = __stmmac_setup_est(priv, &priv->est);
+	mutex_unlock(&priv->est_lock);
+
+	return ret;
+}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 64d890664421..05c7e15fcb2f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -8,6 +8,7 @@
   Author: Rayagond Kokatanur <rayagond@vayavyalabs.com>
 *******************************************************************************/
 #include "stmmac.h"
+#include "stmmac_est.h"
 #include "stmmac_ptp.h"
 
 #define PTP_SAFE_TIME_OFFSET_NS	500000
@@ -54,8 +55,8 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 	u32 sec, nsec;
 	u32 quotient, reminder;
 	int neg_adj = 0;
-	bool xmac, est_rst = false;
-	int ret;
+	bool xmac;
+	int ret, err;
 
 	xmac = dwmac_is_xmac(priv->plat->core_type);
 
@@ -68,49 +69,31 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 	sec = quotient;
 	nsec = reminder;
 
-	/* If EST is enabled, disabled it before adjust ptp time. */
+	/* Keep the installed schedule stable across the entire clock step. */
+	mutex_lock(&priv->est_lock);
 	if (priv->est.enable) {
-		est_rst = true;
-		mutex_lock(&priv->est_lock);
-		priv->est.enable = false;
-		stmmac_est_configure(priv, priv, &priv->est,
-				     priv->plat->clk_ptp_rate, false);
-		mutex_unlock(&priv->est_lock);
+		ret = stmmac_est_configure(priv, priv, &priv->est,
+					   priv->plat->clk_ptp_rate, false);
+		if (ret)
+			goto out_unlock;
 	}
 
 	write_lock_irqsave(&priv->ptp_lock, flags);
-	stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
+	ret = stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
 	write_unlock_irqrestore(&priv->ptp_lock, flags);
 
-	/* Calculate new basetime and re-configured EST after PTP time adjust. */
-	if (est_rst) {
-		struct timespec64 current_time, time;
-		ktime_t current_time_ns, basetime;
-		u64 cycle_time;
-
-		mutex_lock(&priv->est_lock);
-		priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, &current_time);
-		current_time_ns = timespec64_to_ktime(current_time);
-		time.tv_nsec = priv->est.btr_reserve[0];
-		time.tv_sec = priv->est.btr_reserve[1];
-		basetime = timespec64_to_ktime(time);
-		cycle_time = (u64)priv->est.ctr[1] * NSEC_PER_SEC +
-			     priv->est.ctr[0];
-		time = stmmac_calc_tas_basetime(basetime,
-						current_time_ns,
-						cycle_time);
-
-		priv->est.btr[0] = (u32)time.tv_nsec;
-		priv->est.btr[1] = (u32)time.tv_sec;
-		priv->est.enable = true;
-		ret = stmmac_est_configure(priv, priv, &priv->est,
-					   priv->plat->clk_ptp_rate, true);
-		mutex_unlock(&priv->est_lock);
-		if (ret)
-			netdev_err(priv->dev, "failed to configure EST\n");
+	/* Also try to restore EST after a failed clock update. Keep the first
+	 * error, but do not report success if only schedule replay failed.
+	 */
+	if (priv->est.enable) {
+		err = __stmmac_setup_est(priv, &priv->est);
+		if (!ret)
+			ret = err;
 	}
 
-	return 0;
+out_unlock:
+	mutex_unlock(&priv->est_lock);
+	return ret;
 }
 
 /**
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 67c6fc32d0ea..58dda3282973 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -10,6 +10,7 @@
 #include "dwmac4.h"
 #include "dwmac5.h"
 #include "stmmac.h"
+#include "stmmac_est.h"
 
 static void tc_fill_all_pass_entry(struct stmmac_tc_entry *entry)
 {
@@ -942,7 +943,7 @@ struct timespec64 stmmac_calc_tas_basetime(ktime_t old_base_time,
 	return time;
 }
 
-static void tc_taprio_map_maxsdu_txq(struct stmmac_priv *priv,
+static void tc_taprio_map_maxsdu_txq(struct stmmac_est *est,
 				     struct tc_taprio_qopt_offload *qopt)
 {
 	u32 num_tc = qopt->mqprio.qopt.num_tc;
@@ -959,7 +960,7 @@ static void tc_taprio_map_maxsdu_txq(struct stmmac_priv *priv,
 		count = qopt->mqprio.qopt.count[i];
 
 		for (j = offset; j < offset + count; j++)
-			priv->est.max_sdu[j] = qopt->max_sdu[i] + ETH_HLEN - ETH_TLEN;
+			est->max_sdu[j] = qopt->max_sdu[i] + ETH_HLEN - ETH_TLEN;
 	}
 }
 
@@ -968,10 +969,10 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 {
 	u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep;
 	struct netlink_ext_ack *extack = qopt->mqprio.extack;
-	struct timespec64 time, current_time, qopt_time;
-	ktime_t current_time_ns;
-	int err, i, ret = 0;
-	u64 ctr;
+	struct timespec64 qopt_time;
+	u64 ctr = qopt->cycle_time;
+	struct stmmac_est *est;
+	int i, ret, err;
 
 	if (qopt->base_time < 0)
 		return -ERANGE;
@@ -979,6 +980,9 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	if (!priv->dma_cap.estsel)
 		return -EOPNOTSUPP;
 
+	if (ctr > (u64)U32_MAX * NSEC_PER_SEC)
+		return -ERANGE;
+
 	switch (wid) {
 	case 0x1:
 		wid = 16;
@@ -1015,6 +1019,8 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 
 	if (qopt->cmd == TAPRIO_CMD_DESTROY)
 		goto disable;
+	if (!priv->ptp_enabled || !priv->ptp_clock_ops.gettime64)
+		return -EOPNOTSUPP;
 
 	if (qopt->num_entries > dep)
 		return -EINVAL;
@@ -1023,25 +1029,27 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	if (qopt->cycle_time_extension >= BIT(wid + 7))
 		return -ERANGE;
 
-	mutex_lock(&priv->est_lock);
-	memset(&priv->est, 0, sizeof(priv->est));
-	mutex_unlock(&priv->est_lock);
+	/* Build the replacement without changing the installed schedule. An
+	 * entry rejected below must not leave an enabled, zero-cycle cache for
+	 * PHC adjustment or reset replay to consume.
+	 */
+	est = kzalloc_obj(*est);
+	if (!est)
+		return -ENOMEM;
 
 	size = qopt->num_entries;
-
-	mutex_lock(&priv->est_lock);
-	priv->est.gcl_size = size;
-	priv->est.enable = qopt->cmd == TAPRIO_CMD_REPLACE;
-	mutex_unlock(&priv->est_lock);
+	est->gcl_size = size;
+	est->enable = true;
 
 	for (i = 0; i < size; i++) {
 		s64 delta_ns = qopt->entries[i].interval;
 		u32 gates = qopt->entries[i].gate_mask;
 
-		if (delta_ns > GENMASK(wid - 1, 0))
-			return -ERANGE;
-		if (gates > GENMASK(31 - wid, 0))
-			return -ERANGE;
+		if (delta_ns > GENMASK(wid - 1, 0) ||
+		    gates > GENMASK(31 - wid, 0)) {
+			ret = -ERANGE;
+			goto free_est;
+		}
 
 		switch (qopt->entries[i].command) {
 		case TC_TAPRIO_CMD_SET_GATES:
@@ -1053,55 +1061,56 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 			gates &= ~BIT(0);
 			break;
 		default:
-			return -EOPNOTSUPP;
+			ret = -EOPNOTSUPP;
+			goto free_est;
 		}
 
-		priv->est.gcl[i] = delta_ns | (gates << wid);
+		est->gcl[i] = delta_ns | (gates << wid);
 	}
 
-	mutex_lock(&priv->est_lock);
-	/* Adjust for real system time */
-	priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, &current_time);
-	current_time_ns = timespec64_to_ktime(current_time);
-	time = stmmac_calc_tas_basetime(qopt->base_time, current_time_ns,
-					qopt->cycle_time);
-
-	priv->est.btr[0] = (u32)time.tv_nsec;
-	priv->est.btr[1] = (u32)time.tv_sec;
-
 	qopt_time = ktime_to_timespec64(qopt->base_time);
-	priv->est.btr_reserve[0] = (u32)qopt_time.tv_nsec;
-	priv->est.btr_reserve[1] = (u32)qopt_time.tv_sec;
-
-	ctr = qopt->cycle_time;
-	priv->est.ctr[0] = do_div(ctr, NSEC_PER_SEC);
-	priv->est.ctr[1] = (u32)ctr;
+	est->btr_reserve[0] = (u32)qopt_time.tv_nsec;
+	est->btr_reserve[1] = (u32)qopt_time.tv_sec;
+	est->ctr[0] = do_div(ctr, NSEC_PER_SEC);
+	est->ctr[1] = (u32)ctr;
+	est->ter = qopt->cycle_time_extension;
 
-	priv->est.ter = qopt->cycle_time_extension;
+	tc_taprio_map_maxsdu_txq(est, qopt);
 
-	tc_taprio_map_maxsdu_txq(priv, qopt);
-
-	ret = stmmac_est_configure(priv, priv, &priv->est,
-				   priv->plat->clk_ptp_rate, true);
-	mutex_unlock(&priv->est_lock);
-	if (ret) {
-		netdev_err(priv->dev, "failed to configure EST\n");
-		goto disable;
-	}
+	mutex_lock(&priv->est_lock);
+	ret = __stmmac_setup_est(priv, est);
+	if (ret)
+		goto restore;
 
 	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
-					      qopt->mqprio.preemptible_tcs);
+					     qopt->mqprio.preemptible_tcs);
 	if (ret)
-		goto disable;
+		goto restore;
 
-	return 0;
+	priv->est = *est;
+	mutex_unlock(&priv->est_lock);
+free_est:
+	kfree(est);
+	return ret;
+
+restore:
+	/* A failed hardware update must not publish the rejected schedule. */
+	if (priv->est.enable)
+		err = __stmmac_setup_est(priv, &priv->est);
+	else
+		err = stmmac_est_configure(priv, priv, &priv->est,
+					   priv->plat->clk_ptp_rate, false);
+	if (err)
+		netdev_err(priv->dev, "failed to restore EST\n");
+	mutex_unlock(&priv->est_lock);
+	goto free_est;
 
 disable:
 	mutex_lock(&priv->est_lock);
 	priv->est.enable = false;
 	stmmac_est_configure(priv, priv, &priv->est,
 			     priv->plat->clk_ptp_rate, false);
-	/* Reset taprio status */
+	/* Reset taprio stats */
 	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
 		priv->xstats.max_sdu_txq_drop[i] = 0;
 		priv->xstats.mtl_est_txq_hlbf[i] = 0;
@@ -1109,9 +1118,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	}
 	mutex_unlock(&priv->est_lock);
 
-	err = stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
-
-	return qopt->cmd == TAPRIO_CMD_DESTROY ? err : ret;
+	return stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
 }
 
 static void tc_taprio_stats(struct stmmac_priv *priv,

-- 
2.53.0

Re: [PATCH net-next v4 3/4] net: stmmac: preserve the installed EST schedule on errors

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (3) · Low: 6

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] tc_taprio_configure() (stmmac_tc.c:1022-1023) now rejects
  TAPRIO_CMD_REPLACE with -EOPNOTSUPP whenever priv->ptp_enabled is…
- [Medium] The new rollback in tc_taprio_configure() rewrites the EST
  registers right after the replacement schedule has been armed.
- [Low] This concern said that stmmac_setup_est() in stmmac_est.h has no
  callers and that the tc_taprio_configure() comment refers to a 'reset…
- [Low] The `!priv->ptp_clock_ops.gettime64` test in
  tc_taprio_configure() (stmmac_tc.c:1022) is now dead and misleading.
- [Low] The comment on __stmmac_setup_est() (stmmac_est.c:83) says it
  programs 'either the installed schedule or an unpublished…
- [Low] stmmac_est.h has no include guard.
- [Low] The new scratch buffer `est = kzalloc_obj(*est)`
  (stmmac_tc.c:1036) holds a struct stmmac_est.
- [Low] stmmac_setup_ptp() registers the PHC (stmmac_main.c:975) before
  it sets priv->ptp_enabled = true (line 976).

Pre-existing issues:
- [Medium] stmmac_set_time() (the settime64 op, stmmac_ptp.c:133-145)
  steps the PHC under ptp_lock only.
- [Medium] The TX hot paths (stmmac_xmit, stmmac_xdp_xmit_zc,
  stmmac_xdp_xmit_xdpf) read priv->est.enable and priv->est.max_sdu[]…
- [Medium] This concern said that after stmmac_hw_setup() ->
  stmmac_init_dma_engine() -> stmmac_reset() clears the EST block,…

--

net: stmmac: preserve the installed EST schedule on errors

This patch builds a TAPRIO replacement schedule in a separately allocated
struct stmmac_est. It copies that schedule to priv->est only after the
hardware accepts it, and restores the installed schedule on failure. A
shared __stmmac_setup_est() helper is used for new schedules, rollback
and stmmac_adjust_time(), which now holds est_lock across the whole clock
step.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> index f15d4d046aa74..15c2d1794e229 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> @@ -80,6 +80,47 @@ static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
>  	return 0;
>  }
>  
> +/* Program either the installed schedule or an unpublished replacement. */
> +int __stmmac_setup_est(struct stmmac_priv *priv, struct stmmac_est *est)
> +{

[ ... ]

> +	lockdep_assert_held(&priv->est_lock);
> +
> +	if (!priv->ptp_enabled)
> +		return -EOPNOTSUPP;

[Severity: Low]
stmmac_setup_ptp() registers the PHC before it sets ptp_enabled:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_setup_ptp() {
    ...
	stmmac_ptp_register(priv);
	priv->ptp_enabled = true;
    ...
}

Suppose an adjtime lands in that window while priv->est.enable is set.
stmmac_adjust_time() then disables EST and steps the clock, and this
check returns -EOPNOTSUPP, which leaves EST off in hardware.

At this commit, stmmac_hw_setup() has already reset EST before
stmmac_setup_ptp() runs. The later patch "net: stmmac: restore EST before
starting DMA on open and resume" re-arms EST via stmmac_setup_est() right
after stmmac_setup_ptp(). What remains is that adjtime returns
-EOPNOTSUPP for a clock step that was actually applied.

Is that return value intended?

> +
> +	read_lock_irqsave(&priv->ptp_lock, flags);
> +	err = stmmac_get_systime(priv, priv->ptpaddr, &now);
> +	read_unlock_irqrestore(&priv->ptp_lock, flags);

[Severity: Medium]
This is a pre-existing issue, but stmmac_set_time() (the settime64 op)
steps the PHC while holding only ptp_lock:

drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c:stmmac_set_time() {
    ...
	write_lock_irqsave(&priv->ptp_lock, flags);
	stmmac_init_systime(priv, priv->ptpaddr, ts->tv_sec, ts->tv_nsec);
	write_unlock_irqrestore(&priv->ptp_lock, flags);
    ...
}

It does not take est_lock, does not disable EST and does not replay the
schedule. Can a settime land between this PHC read and the EST register
writes below? Can a settime after a schedule is installed leave the
hardware BTR stale?

The commit message's "Hold est_lock throughout a clock step" covers only
the adjtime path. The old tc_taprio_configure() and stmmac_adjust_time()
were not serialized against settime64 either.

[ ... ]

> +	err = stmmac_est_configure(priv, priv, est,
> +				   priv->plat->clk_ptp_rate, true);
> +	if (err)
> +		netdev_err(priv->dev, "failed to re-configure EST\n");

[Severity: Low]
This isn't a bug, but this helper now also does the first install of a
new schedule from tc_taprio_configure(). The message removed there was
"failed to configure EST".

A failed initial install is now logged as a re-configure failure. On the
rollback path, "failed to restore EST" follows this message. Would
"failed to configure EST" be more accurate here?

> +
> +	return err;
> +}
> +
>  static void est_irq_status(struct stmmac_priv *priv, struct net_device *dev,
>  			   struct stmmac_extra_stats *x, u32 txqcnt)
>  {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> index f70221c9c84af..5665e7a539948 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> @@ -65,3 +65,16 @@
>  #define EST_GCL_DATA			0x00000034
>  
>  extern const struct stmmac_est_ops dwmac510_est_ops;
> +
> +int __stmmac_setup_est(struct stmmac_priv *priv, struct stmmac_est *est);
> +static inline int stmmac_setup_est(struct stmmac_priv *priv)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&priv->est_lock);
> +	if (priv->est.enable)
> +		ret = __stmmac_setup_est(priv, &priv->est);
> +	mutex_unlock(&priv->est_lock);
> +
> +	return ret;
> +}

[Severity: Low]
stmmac_setup_est() has no callers at this commit. The new comment in
tc_taprio_configure() also refers to a "reset replay" consumer that does
not exist yet.

The last patch in the series, "net: stmmac: restore EST before starting
DMA on open and resume", adds callers in __stmmac_open() and
stmmac_resume(). At the end of the series the helper and the comment are
both accurate.

Would it make sense to add this helper in that patch instead?

[Severity: Low]
stmmac_est.h has no include guard. Until now it held only macros and an
extern declaration, which are safe to include more than once.

This adds a static inline body that dereferences priv->est_lock and
priv->est and calls mutex_lock(). Would a second inclusion cause a
redefinition error? Does the header now silently depend on stmmac.h being
included first?

All current includers include stmmac.h first and include this header
once, so the build works today.

[Severity: Medium]
This is a pre-existing issue, but stmmac_hw_setup() clears the EST block:

stmmac_hw_setup()->stmmac_init_dma_engine()->stmmac_reset()

At this commit, nothing re-programs EST afterwards while priv->est.enable
stays set.

The last patch in the series, "net: stmmac: restore EST before starting
DMA on open and resume", fixes this. It calls stmmac_setup_est() in
__stmmac_open() after stmmac_setup_ptp(), and in stmmac_resume() before
stmmac_start_all_dma(). Those cover both callers of stmmac_hw_setup().

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 67c6fc32d0ea6..58dda3282973e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -979,6 +980,9 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	if (!priv->dma_cap.estsel)
>  		return -EOPNOTSUPP;
>  
> +	if (ctr > (u64)U32_MAX * NSEC_PER_SEC)
> +		return -ERANGE;
> +
>  	switch (wid) {
>  	case 0x1:
>  		wid = 16;
> @@ -1015,6 +1019,8 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  
>  	if (qopt->cmd == TAPRIO_CMD_DESTROY)
>  		goto disable;
> +	if (!priv->ptp_enabled || !priv->ptp_clock_ops.gettime64)
> +		return -EOPNOTSUPP;

[Severity: Medium]
Does this reject TAPRIO_CMD_REPLACE whenever the interface is down?

priv->ptp_enabled is set only in stmmac_setup_ptp(), called from
__stmmac_open(), and stmmac_release_ptp() clears it on close. Neither
stmmac_setup_tc() nor tc_setup_taprio() checks netif_running().

Before this patch, a full-offload taprio replace on a down interface read
the PHC through gettime64, programmed EST and succeeded. Now it fails
with -EOPNOTSUPP on a MAC that has estsel set:

  ip link set dev X down
  tc qdisc replace dev X root taprio ... flags 0x2
    -> tc_taprio_configure()
       -> return -EOPNOTSUPP

An installed schedule already survives a down/up cycle, and the later
"restore EST ... on open" patch replays it on open. Could a replace while
down be cached in the same way instead of being rejected?

The commit message mentions neither this gate nor the new -ERANGE check
for cycle_time > U32_MAX * NSEC_PER_SEC above.

[Severity: Low]
This isn't a bug, but is the gettime64 test still needed?

__stmmac_setup_est() no longer reads the PHC through gettime64. It checks
priv->ptp_enabled itself and uses stmmac_get_systime().

Also, priv->ptp_clock_ops is copied at hwif init from
stmmac_ptp_clock_ops or dwmac1000_ptp_clock_ops. Both set .gettime64, so
this condition can never be true. stmmac_adjust_time() calls the same
helper without this check.

>  
>  	if (qopt->num_entries > dep)
>  		return -EINVAL;
> @@ -1023,25 +1029,27 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	if (qopt->cycle_time_extension >= BIT(wid + 7))
>  		return -ERANGE;
>  
> -	mutex_lock(&priv->est_lock);
> -	memset(&priv->est, 0, sizeof(priv->est));
> -	mutex_unlock(&priv->est_lock);
> +	/* Build the replacement without changing the installed schedule. An
> +	 * entry rejected below must not leave an enabled, zero-cycle cache for
> +	 * PHC adjustment or reset replay to consume.
> +	 */
> +	est = kzalloc_obj(*est);
> +	if (!est)
> +		return -ENOMEM;

[Severity: Low]
This isn't a bug, but struct stmmac_est is about 8268 bytes because
gcl_unaligned[] and gcl[] each hold EST_GCL (1024) u32 entries.

That is larger than the biggest kmalloc cache on 4K page systems, so every
taprio replace makes a physically contiguous order-2 allocation. The
buffer is short-lived and allocated in sleepable context. Would
kvzalloc_obj() with kvfree() be a better fit?

[ ... ]

> +	mutex_lock(&priv->est_lock);
> +	ret = __stmmac_setup_est(priv, est);
> +	if (ret)
> +		goto restore;
>  
>  	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
> -					      qopt->mqprio.preemptible_tcs);
> +					     qopt->mqprio.preemptible_tcs);
>  	if (ret)
> -		goto disable;
> +		goto restore;
>  
> -	return 0;
> +	priv->est = *est;

[Severity: Medium]
This is a pre-existing issue, but stmmac_xmit(), stmmac_xdp_xmit_zc() and
stmmac_xdp_xmit_xdpf() read priv->est.enable and priv->est.max_sdu[] with
no est_lock and no READ_ONCE().

Can a packet sent during this struct copy be checked against a mix of old
and new max_sdu values?

The old memset() followed by field-by-field updates had the same race,
with intermediate states at least as bad, so the struct copy does not
make it worse.

> +	mutex_unlock(&priv->est_lock);
> +free_est:
> +	kfree(est);
> +	return ret;
> +
> +restore:
> +	/* A failed hardware update must not publish the rejected schedule. */
> +	if (priv->est.enable)
> +		err = __stmmac_setup_est(priv, &priv->est);
> +	else
> +		err = stmmac_est_configure(priv, priv, &priv->est,
> +					   priv->plat->clk_ptp_rate, false);

[Severity: Medium]
When stmmac_fpe_map_preemption_class() fails, __stmmac_setup_est(priv,
est) has already armed the replacement through est_configure(..., true):

	if (enable)
		ctrl |= EST_EEST | EST_SSWL | EST_DFBS;

SSWL asks the hardware to switch lists at the programmed BTR, and it stays
set until the switch happens. When base_time is in the past, that is
usually about one cycle later.

This restore rewrites the GCL and the other EST registers microseconds
later. It does not clear EEST or wait for SSWL first.

An admin can trigger the FPE failure. dwmac5_fpe_map_preemption_class()
returns -EINVAL with SP scheduling when a preemptible TC spans several
queues, or when queue weights differ.

If the hardware aborts indirect GCL writes while SSWL is pending (the
Synopsys EST programming model describes this), est_write() would not
notice, because it only polls SRWO:

	return readl_poll_timeout(est_addr + EST_GCL_CONTROL, ctrl,
				  !(ctrl & EST_SRWO), 100, 5000);

Could the restore then return 0 while the hardware later switches to the
rejected schedule, and priv->est still describes the old one?

Would checking the FPE mapping before arming the replacement, or waiting
for SSWL to clear, avoid this?

> +	if (err)
> +		netdev_err(priv->dev, "failed to restore EST\n");
> +	mutex_unlock(&priv->est_lock);
> +	goto free_est;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930-stmmac-est-reapply-after-open-v4-0-9d4366587b73%40gmail.com

[PATCH net-next v4 4/4] net: stmmac: restore EST before starting DMA on open and resume

[James Hilliard]

From: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

The core reset clears the MTL_EST registers while the cached TAPRIO
schedule remains installed. Reapply that schedule after timestamp
initialization in open and resume, advancing its base time by whole
cycles through the shared replay helper.

Move DMA start out of hardware setup and into its callers after EST
replay succeeds. An EST setup failure must not expose an active
datapath without its installed gate schedule.

Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e93f3238be1f..2104eaef8806 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -48,6 +48,7 @@
 #include "stmmac_ptp.h"
 #include "stmmac_fpe.h"
 #include "stmmac.h"
+#include "stmmac_est.h"
 #include "stmmac_pcs.h"
 #include "stmmac_xdp.h"
 #include <linux/reset.h>
@@ -3779,8 +3780,6 @@ static int stmmac_hw_setup(struct net_device *dev)
 	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);
 
-	/* Start the ball rolling... */
-	stmmac_start_all_dma(priv);
 
 	phylink_rx_clk_stop_block(priv->phylink);
 	stmmac_set_hw_vlan_mode(priv, priv->hw);
@@ -4200,12 +4199,22 @@ static int __stmmac_open(struct net_device *dev,
 	if (ret)
 		goto ptp_error;
 
+	/* The core soft reset in stmmac_hw_setup() clears the MTL_EST
+	 * registers, so re-apply the taprio offload after PTP is up.
+	 */
+	ret = stmmac_setup_est(priv);
+	if (ret < 0)
+		goto est_error;
+
 	stmmac_init_coalesce(priv);
 
 	phylink_start(priv->phylink);
 
 	stmmac_vlan_restore(priv);
 
+	/* Restore the installed schedule before starting DMA. */
+	stmmac_start_all_dma(priv);
+
 	ret = stmmac_request_irq(dev);
 	if (ret)
 		goto irq_error;
@@ -4224,6 +4233,7 @@ static int __stmmac_open(struct net_device *dev,
 	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
 		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
 
+est_error:
 	stmmac_release_ptp(priv);
 ptp_error:
 	stmmac_stop_all_dma(priv);
@@ -8420,6 +8430,10 @@ int stmmac_resume(struct device *dev)
 	}
 
 init_coalesce:
+	ret = stmmac_setup_est(priv);
+	if (ret < 0)
+		goto error_stop_dma;
+
 	stmmac_init_coalesce(priv);
 	phylink_rx_clk_stop_block(priv->phylink);
 	stmmac_set_rx_mode(ndev);
@@ -8427,6 +8441,9 @@ int stmmac_resume(struct device *dev)
 
 	stmmac_vlan_restore(priv);
 
+	/* Restore the installed schedule before starting DMA. */
+	stmmac_start_all_dma(priv);
+
 	stmmac_enable_all_queues(priv);
 	stmmac_enable_all_dma_irq(priv);
 

-- 
2.53.0

Re: [PATCH net-next v4 4/4] net: stmmac: restore EST before starting DMA on open and resume

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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] The patch gives stmmac_resume() a new way to fail: if
  stmmac_setup_est() returns an error, resume jumps to error_stop_dma…
- [Medium] __stmmac_open() can now fail at the new est_error exit
  (stmmac_main.c:4205-4207).
- [Medium] In __stmmac_open(), stmmac_start_all_dma() now runs after
  phylink_start() (stmmac_main.c:4211 -> 4216).

--

net: stmmac: restore EST before starting DMA on open and resume

The core reset clears the MTL_EST registers. This patch replays the cached
taprio/EST schedule after timestamp initialization in __stmmac_open() and
stmmac_resume(). It also moves stmmac_start_all_dma() out of
stmmac_hw_setup() and into its callers, so DMA starts only after the EST
replay succeeds.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e93f3238be1fb..2104eaef8806f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4200,12 +4199,22 @@ static int __stmmac_open(struct net_device *dev,
>  	if (ret)
>  		goto ptp_error;
>  
> +	/* The core soft reset in stmmac_hw_setup() clears the MTL_EST
> +	 * registers, so re-apply the taprio offload after PTP is up.
> +	 */
> +	ret = stmmac_setup_est(priv);
> +	if (ret < 0)
> +		goto est_error;

[Severity: Medium]
What state is the interface left in if this new exit is taken during an MTU
change?

The est_error exit comes after the memcpy() at the top of __stmmac_open()
has overwritten priv->dma_conf. It also comes before stmmac_init_coalesce(),
phylink_start() and the NAPI enable.

stmmac_change_mtu() reopens the device in place:

stmmac_change_mtu() {
    ...
		__stmmac_release(dev);

		ret = __stmmac_open(dev, dma_conf);
		if (ret) {
			free_dma_desc_resources(priv, dma_conf);
			kfree(dma_conf);
			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
			return ret;
		}
    ...
}

After this failure the interface stays IFF_UP. priv->dma_conf still holds
the descriptor, buffer and page pool pointers that were just freed. The
txtimers hold the zeroed hrtimers copied from the new dma_conf, because
stmmac_init_coalesce() was skipped.

The next close, ring parameter change or XDP change goes through
__stmmac_release(). Every NAPI is already disabled at that point. Would
stmmac_disable_all_queues() then hang forever in napi_disable() under
RTNL?

If execution got past that point, hrtimer_cancel() would run on
uninitialized hrtimers. free_dma_desc_resources(priv, &priv->dma_conf)
would then free the same rings a second time. Until then, readers such as
the debugfs descriptors_status file would dereference the freed
descriptor memory.

The ptp_error and irq_error exits already used this weak unwind. What
changes is that an EST register write timeout (readl_poll_timeout() in
est_write()) during a routine MTU change can now reach it. Before this
patch the device kept running, just without its EST schedule.

>  	stmmac_init_coalesce(priv);
>  
>  	phylink_start(priv->phylink);
>  
>  	stmmac_vlan_restore(priv);
>  
> +	/* Restore the installed schedule before starting DMA. */
> +	stmmac_start_all_dma(priv);

[Severity: Medium]
Can starting DMA after phylink_start() race with the phylink resolve
worker?

phylink_start() calls phylink_enable_and_run_resolve(), which queues
pl->resolve. phylink_resolve() does not take RTNL, so it can call
stmmac_mac_link_up() while __stmmac_open() is still running here.

stmmac_mac_link_up() sets speed and duplex with an unlocked
read-modify-write of MAC_CTRL_REG:

	old_ctrl = readl(priv->ioaddr + MAC_CTRL_REG);
	ctrl = old_ctrl & ~priv->hw->link.speed_mask;
	...
	if (ctrl != old_ctrl)
		writel(ctrl, priv->ioaddr + MAC_CTRL_REG);

For each channel, stmmac_start_all_dma() does its own unlocked
read-modify-write of the same register. On dwmac4/5:

dwmac4_dma_start_tx() {
    ...
	value = readl(ioaddr + GMAC_CONFIG);
	value |= GMAC_CONFIG_TE;
	writel(value, ioaddr + GMAC_CONFIG);
}

dwmac4_dma_start_rx() does the same for GMAC_CONFIG_RE. GMAC_CONFIG has
the same offset as MAC_CTRL_REG. On XGMAC, dwxgmac2_dma_start_tx() does the
same on XGMAC_TX_CONFIG, which holds the speed field.

Consider this interleaving:

__stmmac_open()                      phylink_resolve worker
dwmac4_dma_start_tx()
  value = readl(GMAC_CONFIG)
                                     stmmac_mac_link_up()
                                       writel(ctrl, MAC_CTRL_REG)
  writel(value | TE, GMAC_CONFIG)

The open path writes back the stale speed and duplex. stmmac_mac_set() in
the worker only checks that TE/RE are set, so it does not correct this.
Would the link then report up while the MAC runs at the wrong speed or
duplex until the next link change?

Before this patch, DMA started inside stmmac_hw_setup(), before
phylink_start(), so these accesses could not overlap. stmmac_resume() is
not affected because DMA starts there before phylink_resume().

Would moving stmmac_start_all_dma() before phylink_start() close this
window and still replay EST before DMA starts?

>  	ret = stmmac_request_irq(dev);
>  	if (ret)
>  		goto irq_error;

[ ... ]

> @@ -8420,6 +8430,10 @@ int stmmac_resume(struct device *dev)
>  	}
>  
>  init_coalesce:
> +	ret = stmmac_setup_est(priv);
> +	if (ret < 0)
> +		goto error_stop_dma;

[Severity: Medium]
Does this error path leave the device half resumed?

error_stop_dma stops DMA, clears MAC TX/RX, powers down the serdes and
returns. It does not re-enable NAPI or call netif_device_attach().

stmmac_suspend() had already done this:

	netif_device_detach(ndev);

	stmmac_disable_all_queues(priv);

The interface is still IFF_UP. A later "ip link set down" would then run
this on NAPIs that are already disabled:

__stmmac_release()
  stmmac_disable_all_queues()
    napi_disable()
      napi_disable_locked()

napi_disable_locked() would then loop forever while holding RTNL:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}

A stmmac_init_timestamping() failure could already reach this incomplete
unwind. This patch adds an EST trigger, most realistically an est_write()
register timeout. A failed EST reprogram used to be harmless at resume,
but now it leaves the device in this state.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930-stmmac-est-reapply-after-open-v4-0-9d4366587b73%40gmail.com

Re: [PATCH net-next v4 0/4] net: stmmac: preserve and restore the installed EST schedule

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next v4 0/4] net: stmmac: preserve and restore the installed EST schedule

[James Hilliard]

On Wed, Sep 30, 2026 at 10:19 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

My additional replacement/error-path fixes were found by source review
while integrating Lorenzo's EST work into the larger stmmac MTU/resume
recovery series.

>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

These were source-review findings, not failures observed on an
EST-capable device.

>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.

I have not tested these EST paths on real hardware.

> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH net-next v4 0/4] net: stmmac: preserve and restore the installed EST schedule

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3669 bytes --]

> Continue Lorenzo Bianconi's EST replay series with v4 of the unmerged work:
> https://lore.kernel.org/r/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com

Hi James,

My plan is to continue working on this series (the code is available in [0]),
I am just waiting other patches to be merged upstream before reposting in order
to avoid any conflict. I guess it is correct to publicly ask before posting
other people patches.
Moreover, this is thecnically a fix so this seires should targed net tree.

Regards,
Lorenzo

[0] https://github.com/LorenzoBianconi/net-next/tree/b4/stmmac-est-reapply-after-open

> 
> The PTP initialization fix from that series is already in the base.
> Carry the remaining embedded-state and explicit-enable preparations,
> and restore the installed schedule after open and resume reset.
> 
> Build TAPRIO replacements separately so rejected updates cannot
> corrupt the installed schedule. Rebase the previous schedule against
> the current PHC when rolling back a failed hardware update, and keep
> DMA stopped until replay succeeds. These changes do not depend on
> the later datapath recovery series.
> 
> Changes in v4:
> - Omit the merged PTP initialization patch and adapt the remaining
>   changes to probe-time PTP locking and current TAPRIO error handling.
> - Separate schedule validation/rollback from open and resume replay.
> - Reuse the base-time helper for replacement, PHC adjustment and
>   rollback; propagate errors while preserving the installed cache.
> - Start DMA only after the installed schedule has been restored.
> - Link to v3: https://lore.kernel.org/r/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com
> 
> Changes in v3:
> - Guard priv->est.enable update using est_lock mutex.
> - Embed stmmac_est in stmmac_priv struct.
> - Rework locking in tc_taprio_configure().
> - Fix possible divided by zero crash in tc_taprio_configure().
> - Add missing reconfiguration during stmmac_resume().
> - Honor ptp error code in __stmmac_open() and stmmac_resume().
> - Link to v2: https://lore.kernel.org/r/20260829-stmmac-est-reapply-after-open-v2-1-5e5ccb185e92@oss.qualcomm.com
> 
> Changes in v2:
> - Rename stmmac_est_reconfigure() in stmmac_setup_est().
> - Rely on stmmac_setup_est() in tc_taprio_configure().
> - Link to v1: https://lore.kernel.org/r/20260825-stmmac-est-reapply-after-open-v1-1-dfa80735e0a1@oss.qualcomm.com
> 
> Assisted-by: Codex:gpt-6-astra
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> 
> ---
> James Hilliard (1):
>       net: stmmac: preserve the installed EST schedule on errors
> 
> Lorenzo Bianconi (3):
>       net: stmmac: embed struct stmmac_est in stmmac_priv struct
>       net: stmmac: pass the desired EST enable state to est_configure()
>       net: stmmac: restore EST before starting DMA on open and resume
> 
>  drivers/net/ethernet/stmicro/stmmac/hwif.h        |   2 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   2 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_est.c  |  47 ++++++-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  |  13 ++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  38 ++++--
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  |  59 ++++-----
>  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 142 +++++++++++-----------
>  7 files changed, 176 insertions(+), 127 deletions(-)
> ---
> base-commit: 1631d79ae57dce2c5f88ad278307028638a8b4d9
> change-id: 20260824-stmmac-est-reapply-after-open-181d70a15eb6
> 
> Best regards,
> --  
> James Hilliard <james.hilliard1@gmail.com>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v4 0/4] net: stmmac: preserve and restore the installed EST schedule

[James Hilliard]

On Thu, Oct 1, 2026 at 1:49 AM Lorenzo Bianconi
<lorenzo.bianconi@oss.qualcomm.com> wrote:
>
> > Continue Lorenzo Bianconi's EST replay series with v4 of the unmerged work:
> > https://lore.kernel.org/r/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com
>
> Hi James,
>
> My plan is to continue working on this series (the code is available in [0]),

I compared your b4/stmmac-est-reapply-after-open branch at
e7a2792034e0 with the EST v4 I posted. The first two patches
(embedding the EST state and passing an explicit enable argument)
are equivalent apart from the additional sign-off.

The substantive differences are:

1. Preserving the installed schedule on replacement failure

Both versions validate the gate list before modifying the installed
schedule, so both preserve it when an entry fails validation.

Your version then overwrites priv->est before programming hardware.
If EST programming or preemption mapping fails, it disables EST.
However, TAPRIO keeps its previous schedule when the driver rejects
the replacement, leaving the driver and qdisc with different state.

My version builds the complete replacement in a separate object and
publishes it only after programming and preemption mapping succeed.
On failure, it retains the old configuration and attempts to restore
it, advancing the old base time by whole cycles against the current
PHC time.

2. Restoring EST before starting DMA

Your version starts DMA inside stmmac_hw_setup(), before PTP setup
and EST replay.

Mine moves DMA startup into the open and resume paths, after those
steps succeed. This keeps DMA stopped if EST restoration fails. I
wouldn't claim the earlier ordering necessarily transmits an ungated
packet, but the later startup gives us a stronger initialization
invariant.

3. Serializing EST across a PHC time adjustment

Your adjtime path releases est_lock between disabling EST and
adjusting the clock, then reacquires it for replay. It also ignores
the disable, clock-adjustment and replay errors.

Mine holds est_lock across that entire sequence, so a concurrent
TAPRIO operation cannot replace or destroy the schedule partway
through it. It checks the errors and attempts replay even after a
failed clock adjustment, while preserving the first error.

4. PTP prerequisite version

My series uses the newer PTP error-propagation fix already merged
as 8181678a92f0. Your branch carries an earlier version below the EST
series. The merged version also tracks PTP enablement and handles
platforms without a configured PTP clock rate. This is your newer
upstream work, rather than an additional change in my EST patches.

One limitation of my version is that rollback remains best-effort:
if restoring the old schedule also fails, it logs the failure but
retains the old software configuration. Hardware can therefore
still differ from the cache. Neither version guarantees recovery
from that double failure.

I'd suggest retaining the candidate-schedule handling, later DMA
startup and adjtime serialization when consolidating the series.

> I am just waiting other patches to be merged upstream before reposting in order
> to avoid any conflict. I guess it is correct to publicly ask before posting
> other people patches.

I guess I had assumed since it had been nearly a month since your
last series that you just hadn't gotten around to spinning a new one
and figured if you were still working on it then my changes might be
helpful either way since I had reworked a few things while working
on a series depends on these changes.

Feel free to take back over and incorporate my changes into your
next revision if/as appropriate.

> Moreover, this is thecnically a fix so this seires should targed net tree.

I think the fixes you may have been waiting on are in net-next, which
is the reason I did not target the net tree.

>
> Regards,
> Lorenzo
>
> [0] https://github.com/LorenzoBianconi/net-next/tree/b4/stmmac-est-reapply-after-open
>
> >
> > The PTP initialization fix from that series is already in the base.
> > Carry the remaining embedded-state and explicit-enable preparations,
> > and restore the installed schedule after open and resume reset.
> >
> > Build TAPRIO replacements separately so rejected updates cannot
> > corrupt the installed schedule. Rebase the previous schedule against
> > the current PHC when rolling back a failed hardware update, and keep
> > DMA stopped until replay succeeds. These changes do not depend on
> > the later datapath recovery series.
> >
> > Changes in v4:
> > - Omit the merged PTP initialization patch and adapt the remaining
> >   changes to probe-time PTP locking and current TAPRIO error handling.
> > - Separate schedule validation/rollback from open and resume replay.
> > - Reuse the base-time helper for replacement, PHC adjustment and
> >   rollback; propagate errors while preserving the installed cache.
> > - Start DMA only after the installed schedule has been restored.
> > - Link to v3: https://lore.kernel.org/r/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com
> >
> > Changes in v3:
> > - Guard priv->est.enable update using est_lock mutex.
> > - Embed stmmac_est in stmmac_priv struct.
> > - Rework locking in tc_taprio_configure().
> > - Fix possible divided by zero crash in tc_taprio_configure().
> > - Add missing reconfiguration during stmmac_resume().
> > - Honor ptp error code in __stmmac_open() and stmmac_resume().
> > - Link to v2: https://lore.kernel.org/r/20260829-stmmac-est-reapply-after-open-v2-1-5e5ccb185e92@oss.qualcomm.com
> >
> > Changes in v2:
> > - Rename stmmac_est_reconfigure() in stmmac_setup_est().
> > - Rely on stmmac_setup_est() in tc_taprio_configure().
> > - Link to v1: https://lore.kernel.org/r/20260825-stmmac-est-reapply-after-open-v1-1-dfa80735e0a1@oss.qualcomm.com
> >
> > Assisted-by: Codex:gpt-6-astra
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> >
> > ---
> > James Hilliard (1):
> >       net: stmmac: preserve the installed EST schedule on errors
> >
> > Lorenzo Bianconi (3):
> >       net: stmmac: embed struct stmmac_est in stmmac_priv struct
> >       net: stmmac: pass the desired EST enable state to est_configure()
> >       net: stmmac: restore EST before starting DMA on open and resume
> >
> >  drivers/net/ethernet/stmicro/stmmac/hwif.h        |   2 +-
> >  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   2 +-
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_est.c  |  47 ++++++-
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  |  13 ++
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  38 ++++--
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  |  59 ++++-----
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 142 +++++++++++-----------
> >  7 files changed, 176 insertions(+), 127 deletions(-)
> > ---
> > base-commit: 1631d79ae57dce2c5f88ad278307028638a8b4d9
> > change-id: 20260824-stmmac-est-reapply-after-open-181d70a15eb6
> >
> > Best regards,
> > --
> > James Hilliard <james.hilliard1@gmail.com>
> >