[PATCH net v5 0/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[PATCH net v5 0/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[Lorenzo Bianconi]

Reconfigure MTL_EST registers after sw reset in __stmmac_open().
Remove priv->est->enable dependency in est_configure().
Initialize est_lock at probe time.

Changes in v5:
- Reconfigure preemption_class in stmmac_setup_est().
- Bail out in stmmac_setup_est() if ptp is not enabled.
- Link to v4: https://lore.kernel.org/r/20261026-stmmac-est-fixes-v4-0-999dac4de44f@oss.qualcomm.com

Changes in v4:
- Rebase on top of net main branch

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

---
Lorenzo Bianconi (3):
      net: stmmac: initialize est_lock at probe time
      net: stmmac: pass the desired EST enable state to est_configure()
      net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

 drivers/net/ethernet/stmicro/stmmac/hwif.h        |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.c  | 61 ++++++++++++++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  |  8 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 46 +++--------
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 94 ++++++++++++++---------
 7 files changed, 148 insertions(+), 77 deletions(-)
---
base-commit: d2bfe74a8fe219a86b86c1f126e1d722a5001790
change-id: 20261026-stmmac-est-fixes-13557ed8ab20

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

[PATCH net v5 1/3] net: stmmac: initialize est_lock at probe time

[Lorenzo Bianconi]

priv->est is allocated with devm_kzalloc() on the first TAPRIO REPLACE
and priv->est_lock is initialized in the same place, which ties the
validity of the lock to whether TAPRIO has ever been configured. The
lock is also never destroyed.

Initialize priv->est_lock in __stmmac_dvr_probe() next to priv->lock
and destroy it in stmmac_dvr_remove(). This keeps the lock valid for
the entire lifetime of the device, independent of the TAPRIO state,
and removes the lazy initialization from the TAPRIO configuration
path.

No functional change intended.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9ded31150fbe..2163e79fea9a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -8213,6 +8213,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);
@@ -8346,6 +8347,7 @@ void stmmac_dvr_remove(struct device *dev)
 
 	destroy_workqueue(priv->wq);
 	mutex_destroy(&priv->lock);
+	mutex_destroy(&priv->est_lock);
 	bitmap_free(priv->af_xdp_zc_qps);
 
 	pm_runtime_disable(dev);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index e137fb9012d4..aea235a69105 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1028,8 +1028,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 					 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));

-- 
2.56.0

[PATCH net v5 2/3] net: stmmac: pass the desired EST enable state to est_configure()

[Lorenzo Bianconi]

Pass the desired EST enable state explicitly to est_configure() instead
of having it derive 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.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.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 b2e557788803..8ae95e91a36a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -620,7 +620,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 e54a3c887826..9a32af00d7d2 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);
 	}
 
@@ -105,7 +105,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 aea235a69105..ac926c60f9f9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1090,7 +1090,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");
@@ -1109,7 +1109,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.56.0

[PATCH net v5 3/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[Lorenzo Bianconi]

The core soft reset issued by stmmac_init_dma_engine(), called from
stmmac_hw_setup(), clears the MTL_EST registers. Nothing re-applies the
taprio offload afterwards, so priv->est->enable stays true while the
hardware EST block is left disabled: taprio keeps being reported as
offloaded, the TX/XDP paths keep dropping frames larger than
priv->est->max_sdu[], and the programmed base time is stale.

Re-apply the offload from __stmmac_open() and stmmac_resume() after PTP
has been brought up. The base time is recomputed from the reserved base
time and the current PTP time, because the timestamp counter has been
re-initialised and the previously programmed base time cannot be reused.

Factor the recompute-and-program sequence into __stmmac_setup_est(),
which must be called with est_lock held, and into stmmac_setup_est(),
which takes the lock and re-applies the offload only when it is
currently enabled and PTP has been brought up. Use the same helper in
stmmac_adjust_time() so the PTP adjust path shares the base time
computation.

Only tc_taprio_configure() updates priv->est->enable: it is set when a
REPLACE succeeds and cleared on DESTROY or a programming failure. The
re-apply helpers leave it untouched, so the flag records that an offload
is installed rather than the current hardware programming state.
__stmmac_setup_est() only reprograms the EST block, and
stmmac_setup_est() replays the FPE mapping afterwards. On a failed
re-apply the open and resume callers abort, while stmmac_adjust_time()
keeps the offload configured.

The same reset also discards the FPE preemption programming, and on XGMAC
stmmac_dma_tx_mode() overwrites the queue-to-traffic-class map. Save the
preemptible traffic class mask in priv->est->pclass when a REPLACE
succeeds and replay the FPE mapping from stmmac_setup_est() as well.

Re-applying the offload is best-effort with respect to PTP:
stmmac_setup_est() does nothing when PTP was not enabled (for example
when clk_ptp_rate is 0), so a missing PTP reference clock does not by
itself fail ndo_open(). If re-applying the EST or FPE programming fails
while restoring the interface, the failure is fatal to the caller:
__stmmac_open() aborts the open, and stmmac_resume() takes the same error
path already used for a failed timestamp counter restore.

Since __stmmac_open() and stmmac_resume() now trust priv->est->enable,
also make tc_taprio_configure() consistent: hold est_lock across the
whole validate-and-program sequence, build the gate list in a temporary
buffer, and only commit it and enable EST after validation and hardware
programming succeed. A REPLACE rejected during validation then leaves the
currently installed schedule intact in both software and hardware, while
a hardware or FPE mapping failure disables EST instead of leaving a stale
enable flag behind. Reject cycle times whose seconds field would not fit
in priv->est->ctr[1].

Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.c  | 55 ++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  |  8 +++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 ++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 44 +++---------
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 88 ++++++++++++++---------
 6 files changed, 139 insertions(+), 68 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 93292014c393..b76acfe0d958 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -239,6 +239,7 @@ struct stmmac_est {
 	u32 gcl[EST_GCL];
 	u32 gcl_size;
 	u32 max_sdu[MTL_MAX_TX_QUEUES];
+	u32 pclass;
 };
 
 struct stmmac_msi {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
index f15d4d046aa7..59e9a54e864d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
@@ -80,6 +80,61 @@ static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
 	return 0;
 }
 
+int __stmmac_setup_est(struct stmmac_priv *priv)
+{
+	struct timespec64 current_time, time;
+	ktime_t current_time_ns, basetime;
+	u64 cycle_time;
+	int err;
+
+	lockdep_assert_held(&priv->est_lock);
+
+	priv->ptp_clock_ops.gettimex64(&priv->ptp_clock_ops, &current_time,
+				       NULL);
+	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;
+
+	err = stmmac_est_configure(priv, priv, priv->est,
+				   priv->plat->clk_ptp_rate, true);
+	if (err) {
+		netdev_err(priv->dev, "failed to re-configure EST\n");
+		return err;
+	}
+
+	return 0;
+}
+
+int stmmac_setup_est(struct stmmac_priv *priv)
+{
+	int ret = 0;
+
+	if (!priv->ptp_enabled)
+		return 0;
+
+	mutex_lock(&priv->est_lock);
+	if (priv->est && priv->est->enable) {
+		ret = __stmmac_setup_est(priv);
+		if (ret)
+			goto unlock;
+
+		ret = stmmac_fpe_map_preemption_class(priv, priv->dev, NULL,
+						      priv->est->pclass);
+	}
+unlock:
+	mutex_unlock(&priv->est_lock);
+
+	return ret;
+}
+
 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..5e07b84957f3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
@@ -4,6 +4,9 @@
  * stmmac EST(802.3 Qbv) handling
  */
 
+#ifndef __STMMAC_EST_H__
+#define __STMMAC_EST_H__
+
 #define EST_GMAC4_OFFSET		0x00000c50
 #define EST_XGMAC_OFFSET		0x00001050
 
@@ -65,3 +68,8 @@
 #define EST_GCL_DATA			0x00000034
 
 extern const struct stmmac_est_ops dwmac510_est_ops;
+
+int __stmmac_setup_est(struct stmmac_priv *priv);
+int stmmac_setup_est(struct stmmac_priv *priv);
+
+#endif /* __STMMAC_EST_H__ */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 2163e79fea9a..d1e3d2cc7767 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>
@@ -4201,6 +4202,10 @@ static int __stmmac_open(struct net_device *dev,
 	if (ret)
 		goto ptp_error;
 
+	ret = stmmac_setup_est(priv);
+	if (ret)
+		goto est_error;
+
 	stmmac_init_coalesce(priv);
 
 	phylink_start(priv->phylink);
@@ -4224,7 +4229,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);
@@ -8527,6 +8532,10 @@ int stmmac_resume(struct device *dev)
 		ret = stmmac_init_timestamping(priv);
 		if (ret)
 			goto error_stop_dma;
+
+		ret = stmmac_setup_est(priv);
+		if (ret)
+			goto error_stop_dma;
 	}
 
 init_coalesce:
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 9a32af00d7d2..a37405534beb 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,7 @@ 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;
 
 	xmac = dwmac_is_xmac(priv->plat->core_type);
 
@@ -69,47 +69,21 @@ 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) {
-		est_rst = true;
-		mutex_lock(&priv->est_lock);
-		priv->est->enable = false;
+	mutex_lock(&priv->est_lock);
+	if (priv->est && priv->est->enable)
 		stmmac_est_configure(priv, priv, priv->est,
 				     priv->plat->clk_ptp_rate, false);
-		mutex_unlock(&priv->est_lock);
-	}
+	mutex_unlock(&priv->est_lock);
 
 	write_lock_irqsave(&priv->ptp_lock, flags);
 	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.gettimex64(&priv->ptp_clock_ops,
-					       &current_time, NULL);
-		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");
-	}
+	mutex_lock(&priv->est_lock);
+	if (priv->est && priv->est->enable)
+		__stmmac_setup_est(priv);
+	mutex_unlock(&priv->est_lock);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index ac926c60f9f9..10b61fa2c27c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -966,11 +966,12 @@ static void tc_taprio_map_maxsdu_txq(struct stmmac_priv *priv,
 static int tc_taprio_configure(struct stmmac_priv *priv,
 			       struct tc_taprio_qopt_offload *qopt)
 {
-	u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep;
+	u32 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;
+	u32 *gcl = NULL;
 	u64 ctr;
 
 	if (qopt->base_time < 0)
@@ -1013,42 +1014,46 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 		return -EOPNOTSUPP;
 	}
 
+	mutex_lock(&priv->est_lock);
+
 	if (qopt->cmd == TAPRIO_CMD_DESTROY)
 		goto disable;
 
-	if (qopt->num_entries > dep)
-		return -EINVAL;
-	if (!qopt->cycle_time)
-		return -ERANGE;
-	if (qopt->cycle_time_extension >= BIT(wid + 7))
-		return -ERANGE;
+	if (qopt->num_entries > dep) {
+		ret = -EINVAL;
+		goto error_unlock;
+	}
 
-	if (!priv->est) {
-		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
-					 GFP_KERNEL);
-		if (!priv->est)
-			return -ENOMEM;
-	} else {
-		mutex_lock(&priv->est_lock);
-		memset(priv->est, 0, sizeof(*priv->est));
-		mutex_unlock(&priv->est_lock);
+	if (!qopt->cycle_time ||
+	    qopt->cycle_time > (u64)U32_MAX * NSEC_PER_SEC) {
+		ret = -ERANGE;
+		goto error_unlock;
 	}
 
-	size = qopt->num_entries;
+	if (qopt->cycle_time_extension >= BIT(wid + 7)) {
+		ret = -ERANGE;
+		goto error_unlock;
+	}
 
-	mutex_lock(&priv->est_lock);
-	priv->est->gcl_size = size;
-	priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
-	mutex_unlock(&priv->est_lock);
+	gcl = kcalloc(qopt->num_entries, sizeof(*gcl), GFP_KERNEL);
+	if (!gcl) {
+		ret = -ENOMEM;
+		goto error_unlock;
+	}
 
-	for (i = 0; i < size; i++) {
+	for (i = 0; i < qopt->num_entries; 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)) {
+			ret = -ERANGE;
+			goto error_unlock;
+		}
+
+		if (gates > GENMASK(31 - wid, 0)) {
+			ret = -ERANGE;
+			goto error_unlock;
+		}
 
 		switch (qopt->entries[i].command) {
 		case TC_TAPRIO_CMD_SET_GATES:
@@ -1060,13 +1065,26 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 			gates &= ~BIT(0);
 			break;
 		default:
-			return -EOPNOTSUPP;
+			ret = -EOPNOTSUPP;
+			goto error_unlock;
 		}
 
-		priv->est->gcl[i] = delta_ns | (gates << wid);
+		gcl[i] = delta_ns | (gates << wid);
 	}
 
-	mutex_lock(&priv->est_lock);
+	if (!priv->est) {
+		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
+					 GFP_KERNEL);
+		if (!priv->est) {
+			ret = -ENOMEM;
+			goto error_unlock;
+		}
+	} else {
+		memset(priv->est, 0, sizeof(*priv->est));
+	}
+	memcpy(priv->est->gcl, gcl, sizeof(*gcl) * qopt->num_entries);
+	priv->est->gcl_size = qopt->num_entries;
+
 	/* Adjust for real system time */
 	priv->ptp_clock_ops.gettimex64(&priv->ptp_clock_ops, &current_time,
 				       NULL);
@@ -1091,7 +1109,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 
 	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;
@@ -1102,11 +1119,16 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	if (ret)
 		goto disable;
 
+	priv->est->pclass = qopt->mqprio.preemptible_tcs;
+	priv->est->enable = true;
+
+	mutex_unlock(&priv->est_lock);
+	kfree(gcl);
+
 	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, false);
@@ -1116,10 +1138,12 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 			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);
+error_unlock:
+	mutex_unlock(&priv->est_lock);
+	kfree(gcl);
 
 	return qopt->cmd == TAPRIO_CMD_DESTROY ? err : ret;
 }

-- 
2.56.0

Re: [PATCH net v5 0/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[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 v5 0/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[Lorenzo Bianconi]

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

On Oct 29, 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.

The issue has been reported by sashiko

> 
>  - 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.

This is a real issue reproduced on a Qualcomm rb3-gen2 baord. Flipping the
interface down/up the EST MTL configuration is lost

> 
>  - 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 tested the fix on a Qualcomm rb3-gen2 baord.

Regards,
Lorenzo

> 
> 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.

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

Re: [PATCH net v5 3/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[Maxime Chevallier]

Hi Lorenzo,

On 10/29/26 11:33, Lorenzo Bianconi wrote:
> The core soft reset issued by stmmac_init_dma_engine(), called from
> stmmac_hw_setup(), clears the MTL_EST registers. Nothing re-applies the
> taprio offload afterwards, so priv->est->enable stays true while the
> hardware EST block is left disabled: taprio keeps being reported as
> offloaded, the TX/XDP paths keep dropping frames larger than
> priv->est->max_sdu[], and the programmed base time is stale.
> 
> Re-apply the offload from __stmmac_open() and stmmac_resume() after PTP
> has been brought up. The base time is recomputed from the reserved base
> time and the current PTP time, because the timestamp counter has been
> re-initialised and the previously programmed base time cannot be reused.
> 
> Factor the recompute-and-program sequence into __stmmac_setup_est(),
> which must be called with est_lock held, and into stmmac_setup_est(),
> which takes the lock and re-applies the offload only when it is
> currently enabled and PTP has been brought up. Use the same helper in
> stmmac_adjust_time() so the PTP adjust path shares the base time
> computation.
> 
> Only tc_taprio_configure() updates priv->est->enable: it is set when a
> REPLACE succeeds and cleared on DESTROY or a programming failure. The
> re-apply helpers leave it untouched, so the flag records that an offload
> is installed rather than the current hardware programming state.
> __stmmac_setup_est() only reprograms the EST block, and
> stmmac_setup_est() replays the FPE mapping afterwards. On a failed
> re-apply the open and resume callers abort, while stmmac_adjust_time()
> keeps the offload configured.
> 
> The same reset also discards the FPE preemption programming, and on XGMAC
> stmmac_dma_tx_mode() overwrites the queue-to-traffic-class map. Save the
> preemptible traffic class mask in priv->est->pclass when a REPLACE
> succeeds and replay the FPE mapping from stmmac_setup_est() as well.
> 
> Re-applying the offload is best-effort with respect to PTP:
> stmmac_setup_est() does nothing when PTP was not enabled (for example
> when clk_ptp_rate is 0), so a missing PTP reference clock does not by
> itself fail ndo_open(). If re-applying the EST or FPE programming fails
> while restoring the interface, the failure is fatal to the caller:
> __stmmac_open() aborts the open, and stmmac_resume() takes the same error
> path already used for a failed timestamp counter restore.
> 
> Since __stmmac_open() and stmmac_resume() now trust priv->est->enable,
> also make tc_taprio_configure() consistent: hold est_lock across the
> whole validate-and-program sequence, build the gate list in a temporary
> buffer, and only commit it and enable EST after validation and hardware
> programming succeed. A REPLACE rejected during validation then leaves the
> currently installed schedule intact in both software and hardware, while
> a hardware or FPE mapping failure disables EST instead of leaving a stale
> enable flag behind. Reject cycle times whose seconds field would not fit
> in priv->est->ctr[1].
> 
> Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

LGTM, I ran a quick test on AgileX5 (dwxgmac w/ EST) and imx8mp, seems
to solve the EST config being lost after link flap/MTU changes :)

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks :)

Maxime

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  1 +
>  drivers/net/ethernet/stmicro/stmmac/stmmac_est.c  | 55 ++++++++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  |  8 +++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 ++-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 44 +++---------
>  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 88 ++++++++++++++---------
>  6 files changed, 139 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 93292014c393..b76acfe0d958 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -239,6 +239,7 @@ struct stmmac_est {
>  	u32 gcl[EST_GCL];
>  	u32 gcl_size;
>  	u32 max_sdu[MTL_MAX_TX_QUEUES];
> +	u32 pclass;
>  };
>  
>  struct stmmac_msi {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> index f15d4d046aa7..59e9a54e864d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> @@ -80,6 +80,61 @@ static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
>  	return 0;
>  }
>  
> +int __stmmac_setup_est(struct stmmac_priv *priv)
> +{
> +	struct timespec64 current_time, time;
> +	ktime_t current_time_ns, basetime;
> +	u64 cycle_time;
> +	int err;
> +
> +	lockdep_assert_held(&priv->est_lock);
> +
> +	priv->ptp_clock_ops.gettimex64(&priv->ptp_clock_ops, &current_time,
> +				       NULL);
> +	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;
> +
> +	err = stmmac_est_configure(priv, priv, priv->est,
> +				   priv->plat->clk_ptp_rate, true);
> +	if (err) {
> +		netdev_err(priv->dev, "failed to re-configure EST\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +int stmmac_setup_est(struct stmmac_priv *priv)
> +{
> +	int ret = 0;
> +
> +	if (!priv->ptp_enabled)
> +		return 0;
> +
> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable) {
> +		ret = __stmmac_setup_est(priv);
> +		if (ret)
> +			goto unlock;
> +
> +		ret = stmmac_fpe_map_preemption_class(priv, priv->dev, NULL,
> +						      priv->est->pclass);
> +	}
> +unlock:
> +	mutex_unlock(&priv->est_lock);
> +
> +	return ret;
> +}
> +
>  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..5e07b84957f3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> @@ -4,6 +4,9 @@
>   * stmmac EST(802.3 Qbv) handling
>   */
>  
> +#ifndef __STMMAC_EST_H__
> +#define __STMMAC_EST_H__
> +
>  #define EST_GMAC4_OFFSET		0x00000c50
>  #define EST_XGMAC_OFFSET		0x00001050
>  
> @@ -65,3 +68,8 @@
>  #define EST_GCL_DATA			0x00000034
>  
>  extern const struct stmmac_est_ops dwmac510_est_ops;
> +
> +int __stmmac_setup_est(struct stmmac_priv *priv);
> +int stmmac_setup_est(struct stmmac_priv *priv);
> +
> +#endif /* __STMMAC_EST_H__ */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2163e79fea9a..d1e3d2cc7767 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>
> @@ -4201,6 +4202,10 @@ static int __stmmac_open(struct net_device *dev,
>  	if (ret)
>  		goto ptp_error;
>  
> +	ret = stmmac_setup_est(priv);
> +	if (ret)
> +		goto est_error;
> +
>  	stmmac_init_coalesce(priv);
>  
>  	phylink_start(priv->phylink);
> @@ -4224,7 +4229,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);
> @@ -8527,6 +8532,10 @@ int stmmac_resume(struct device *dev)
>  		ret = stmmac_init_timestamping(priv);
>  		if (ret)
>  			goto error_stop_dma;
> +
> +		ret = stmmac_setup_est(priv);
> +		if (ret)
> +			goto error_stop_dma;
>  	}
>  
>  init_coalesce:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 9a32af00d7d2..a37405534beb 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,7 @@ 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;
>  
>  	xmac = dwmac_is_xmac(priv->plat->core_type);
>  
> @@ -69,47 +69,21 @@ 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) {
> -		est_rst = true;
> -		mutex_lock(&priv->est_lock);
> -		priv->est->enable = false;
> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable)
>  		stmmac_est_configure(priv, priv, priv->est,
>  				     priv->plat->clk_ptp_rate, false);
> -		mutex_unlock(&priv->est_lock);
> -	}
> +	mutex_unlock(&priv->est_lock);
>  
>  	write_lock_irqsave(&priv->ptp_lock, flags);
>  	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.gettimex64(&priv->ptp_clock_ops,
> -					       &current_time, NULL);
> -		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");
> -	}
> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable)
> +		__stmmac_setup_est(priv);
> +	mutex_unlock(&priv->est_lock);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index ac926c60f9f9..10b61fa2c27c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -966,11 +966,12 @@ static void tc_taprio_map_maxsdu_txq(struct stmmac_priv *priv,
>  static int tc_taprio_configure(struct stmmac_priv *priv,
>  			       struct tc_taprio_qopt_offload *qopt)
>  {
> -	u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep;
> +	u32 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;
> +	u32 *gcl = NULL;
>  	u64 ctr;
>  
>  	if (qopt->base_time < 0)
> @@ -1013,42 +1014,46 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	mutex_lock(&priv->est_lock);
> +
>  	if (qopt->cmd == TAPRIO_CMD_DESTROY)
>  		goto disable;
>  
> -	if (qopt->num_entries > dep)
> -		return -EINVAL;
> -	if (!qopt->cycle_time)
> -		return -ERANGE;
> -	if (qopt->cycle_time_extension >= BIT(wid + 7))
> -		return -ERANGE;
> +	if (qopt->num_entries > dep) {
> +		ret = -EINVAL;
> +		goto error_unlock;
> +	}
>  
> -	if (!priv->est) {
> -		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
> -					 GFP_KERNEL);
> -		if (!priv->est)
> -			return -ENOMEM;
> -	} else {
> -		mutex_lock(&priv->est_lock);
> -		memset(priv->est, 0, sizeof(*priv->est));
> -		mutex_unlock(&priv->est_lock);
> +	if (!qopt->cycle_time ||
> +	    qopt->cycle_time > (u64)U32_MAX * NSEC_PER_SEC) {
> +		ret = -ERANGE;
> +		goto error_unlock;
>  	}
>  
> -	size = qopt->num_entries;
> +	if (qopt->cycle_time_extension >= BIT(wid + 7)) {
> +		ret = -ERANGE;
> +		goto error_unlock;
> +	}
>  
> -	mutex_lock(&priv->est_lock);
> -	priv->est->gcl_size = size;
> -	priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
> -	mutex_unlock(&priv->est_lock);
> +	gcl = kcalloc(qopt->num_entries, sizeof(*gcl), GFP_KERNEL);
> +	if (!gcl) {
> +		ret = -ENOMEM;
> +		goto error_unlock;
> +	}
>  
> -	for (i = 0; i < size; i++) {
> +	for (i = 0; i < qopt->num_entries; 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)) {
> +			ret = -ERANGE;
> +			goto error_unlock;
> +		}
> +
> +		if (gates > GENMASK(31 - wid, 0)) {
> +			ret = -ERANGE;
> +			goto error_unlock;
> +		}
>  
>  		switch (qopt->entries[i].command) {
>  		case TC_TAPRIO_CMD_SET_GATES:
> @@ -1060,13 +1065,26 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  			gates &= ~BIT(0);
>  			break;
>  		default:
> -			return -EOPNOTSUPP;
> +			ret = -EOPNOTSUPP;
> +			goto error_unlock;
>  		}
>  
> -		priv->est->gcl[i] = delta_ns | (gates << wid);
> +		gcl[i] = delta_ns | (gates << wid);
>  	}
>  
> -	mutex_lock(&priv->est_lock);
> +	if (!priv->est) {
> +		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
> +					 GFP_KERNEL);
> +		if (!priv->est) {
> +			ret = -ENOMEM;
> +			goto error_unlock;
> +		}
> +	} else {
> +		memset(priv->est, 0, sizeof(*priv->est));
> +	}
> +	memcpy(priv->est->gcl, gcl, sizeof(*gcl) * qopt->num_entries);
> +	priv->est->gcl_size = qopt->num_entries;
> +
>  	/* Adjust for real system time */
>  	priv->ptp_clock_ops.gettimex64(&priv->ptp_clock_ops, &current_time,
>  				       NULL);
> @@ -1091,7 +1109,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  
>  	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;
> @@ -1102,11 +1119,16 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	if (ret)
>  		goto disable;
>  
> +	priv->est->pclass = qopt->mqprio.preemptible_tcs;
> +	priv->est->enable = true;
> +
> +	mutex_unlock(&priv->est_lock);
> +	kfree(gcl);
> +
>  	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, false);
> @@ -1116,10 +1138,12 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  			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);
> +error_unlock:
> +	mutex_unlock(&priv->est_lock);
> +	kfree(gcl);
>  
>  	return qopt->cmd == TAPRIO_CMD_DESTROY ? err : ret;
>  }
> 

Re: [PATCH net v5 2/3] net: stmmac: pass the desired EST enable state to est_configure()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit says "No functional change intended", but there is a
  change when stmmac_adjust_time() runs concurrently.

Pre-existing issues:
- [High] This happens at this commit in tc_taprio_configure()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c).
- [High] At this commit stmmac_adjust_time() (stmmac_ptp.c:72) checks
  priv->est->enable without est_lock.
- [Medium] est_configure()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_est.c) rewrites…
- [Medium] At this commit stmmac_adjust_time() decides est_rst from an
  unlocked read of priv->est->enable (stmmac_ptp.c:72-73) and drops…

--

net: stmmac: pass the desired EST enable state to est_configure()

This patch adds an explicit enable argument to est_configure() so it no
longer reads cfg->enable. The callers in stmmac_adjust_time() and
tc_taprio_configure() now pass true or false directly, and they still set
priv->est->enable around the EST programming as before.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> index afc516059b897..f15d4d046aa74 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c

[ ... ]

> @@ -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;

[Severity: Medium]
This isn't a bug introduced by this patch, but when enable is false,
est_configure() still rewrites BTR, TER, LLR, CTR and the whole GCL
through est_write() before it gets here:

    ret |= est_write(est_addr, EST_BTR_LOW, cfg->btr[0], false);
    ...
    if (ret)
        return ret;

    for (i = 0; i < cfg->gcl_size; i++) {
        ret = est_write(est_addr, i, cfg->gcl[i], true);
        if (ret)
            return ret;
    }

If one of the SRWO polls in est_write() times out, the function returns
before it clears EST_EEST here and before it zeroes EST_INT_EN.

The disable callers ignore that return value and set
priv->est->enable = false anyway. These are the disable: label in
tc_taprio_configure() and the quiesce block in stmmac_adjust_time().

On TAPRIO_CMD_DESTROY, tc_taprio_configure() returns only the result of
stmmac_fpe_map_preemption_class(), so taprio treats the offload as
removed.

Could the MAC keep gating TX with the old schedule, with EST interrupts
still on, while software thinks EST is off? This ordering is unchanged at
the end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index e54a3c8878265..9a32af00d7d2d 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);
>  	}

[Severity: High]
This is a pre-existing issue, but the check that guards this block reads
priv->est->enable without holding est_lock:

    if (priv->est && priv->est->enable) {
        est_rst = true;
        mutex_lock(&priv->est_lock);

A TAPRIO_CMD_REPLACE in tc_taprio_configure() drops est_lock between
steps:

    mutex_lock(&priv->est_lock);
    memset(priv->est, 0, sizeof(*priv->est));
    mutex_unlock(&priv->est_lock);
    ...
    mutex_lock(&priv->est_lock);
    priv->est->gcl_size = size;
    priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
    mutex_unlock(&priv->est_lock);

    for (i = 0; i < size; i++) {
        ...
        priv->est->gcl[i] = delta_ns | (gates << wid);
    }

    mutex_lock(&priv->est_lock);
    /* ctr[] and btr_reserve[] are filled only here */

Suppose the second est_lock section of stmmac_adjust_time() runs in that
gap. It then computes:

    cycle_time = (u64)priv->est->ctr[1] * NSEC_PER_SEC +
                 priv->est->ctr[0];    /* still 0 */
    time = stmmac_calc_tas_basetime(basetime, current_time_ns,
                                    cycle_time);

and stmmac_calc_tas_basetime() does:

    n = div64_s64(ktime_sub_ns(current_time, old_base_time),
                  cycle_time);

Can this divide by zero and oops on x86? It would also program a GCL
that is still half written through est_configure(..., true).

This looks to be addressed later in the series by "net: stmmac: re-apply
taprio offload in __stmmac_open() and stmmac_resume()". That commit:

  - holds est_lock across the whole validate, memset and program
    sequence in tc_taprio_configure()
  - sets enable only after programming succeeds
  - checks enable under est_lock in stmmac_adjust_time()

> @@ -105,7 +105,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");

[Severity: Medium]
This isn't a bug introduced by this patch, but est_rst comes from the
unlocked check before the first critical section. Nothing is re-checked
here after est_lock was dropped around the systime step.

A TAPRIO_CMD_DESTROY can land in that gap:

CPU1 (stmmac_adjust_time)        CPU2 (tc_taprio_configure, DESTROY)
est_rst = true
priv->est->enable = false
est_configure(..., false)
mutex_unlock(&priv->est_lock)
                                 disable:
                                 priv->est->enable = false
                                 est_configure(..., false)
                                 returns success
stmmac_adjust_systime()
mutex_lock(&priv->est_lock)
priv->est->enable = true
est_configure(..., true)

Does this bring back a schedule that taprio has already removed? The
xmit paths would then also enforce the priv->est->max_sdu[] drops again.

The same later commit, "net: stmmac: re-apply taprio offload in
__stmmac_open() and stmmac_resume()", appears to fix this. It re-checks
priv->est && priv->est->enable under est_lock before calling
__stmmac_setup_est(), and stmmac_adjust_time() no longer writes the
enable flag.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index aea235a69105e..ac926c60f9f9a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -1090,7 +1090,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);

[Severity: Low]
The commit message says:

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

Does that still hold when stmmac_adjust_time() runs at the same time?

Before this patch, this call read cfg->enable under est_lock. The REPLACE
path sets enable = true in an earlier critical section and then drops
est_lock while it fills the GCL. Consider this sequence:

CPU1 (tc_taprio_configure, REPLACE)  CPU2 (stmmac_adjust_time)
priv->est->enable = true
mutex_unlock(&priv->est_lock)
fill priv->est->gcl[]
                                     priv->est->enable = false
                                     est_configure(..., false)
                                     mutex_unlock(&priv->est_lock)
mutex_lock(&priv->est_lock)
est_configure(..., true)
                                     stmmac_adjust_systime()

With the old code, the REPLACE would program EST with EEST cleared,
because cfg->enable was false. EST then stayed off across the time step.

With the hard-coded true, EST is turned on with a BTR computed from the
pre-step time. The PHC step then happens with EST active while
priv->est->enable is false, until the second section of
stmmac_adjust_time() reprograms it.

The underlying race is pre-existing, since no lock covers the time step
together with the EST programming. Still, this interleaving behaves
differently after this patch, and the window remains at the end of the
series.

>  	mutex_unlock(&priv->est_lock);
>  	if (ret) {
>  		netdev_err(priv->dev, "failed to configure EST\n");
> @@ -1109,7 +1109,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);

[Severity: High]
This is a pre-existing issue, but the entry validation loop earlier in
tc_taprio_configure() returns directly instead of going through this
disable: label:

    priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
    mutex_unlock(&priv->est_lock);

    for (i = 0; i < size; i++) {
        ...
        if (delta_ns > GENMASK(wid - 1, 0))
            return -ERANGE;
        if (gates > GENMASK(31 - wid, 0))
            return -ERANGE;
        ...
        default:
            return -EOPNOTSUPP;
        }

By that point priv->est has already been memset. So a REPLACE with one
invalid interval, gate mask or command leaves enable == true while ctr[]
and btr_reserve[] are zero.

taprio_disable_offload() does not call back into the driver when the
first offload attempt failed, so nothing clears the flag later.

A later clock_adjtime(ADJ_SETOFFSET) on the PHC then does:

stmmac_adjust_time()
  cycle_time = 0
  stmmac_calc_tas_basetime(0, now, 0)
    div64_s64(now, 0)

Can this divide by zero and oops on x86?

This also looks to be fixed later in the series by "net: stmmac: re-apply
taprio offload in __stmmac_open() and stmmac_resume()". That commit
validates every entry into a temporary kcalloc() buffer before it touches
priv->est, and sets enable only after programming succeeds.

>  		/* Reset taprio status */
>  		for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
>  			priv->xstats.max_sdu_txq_drop[i] = 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-stmmac-est-fixes-v5-0-85b830a795a9%40oss.qualcomm.com

Re: [PATCH net v5 3/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1 (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] A taprio REPLACE that fails the FPE callback's software
  validation destroys the installed hardware schedule, even though the…
- [Low] stmmac_adjust_time() still uses two separate est_lock critical
  sections: it disables EST, runs adjust_systime under ptp_lock only,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  a new way to reach it.
- [Low] The TX/XDP hot paths (stmmac_main.c:2753, 4861, 5286) read
  priv->est, priv->est->enable and priv->est->max_sdu[] with no lock and…

--

net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

This commit re-applies the taprio EST offload and the FPE preemption
mapping after the core soft reset, in __stmmac_open() and stmmac_resume().
The base time recompute moves into __stmmac_setup_est() and
stmmac_setup_est().

tc_taprio_configure() now holds est_lock across validation and
programming, builds the gate list in a temporary buffer, and sets
priv->est->enable only after success.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2163e79fea9a4..d1e3d2cc77670 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4201,6 +4202,10 @@ static int __stmmac_open(struct net_device *dev,
>  	if (ret)
>  		goto ptp_error;
>  
> +	ret = stmmac_setup_est(priv);
> +	if (ret)
> +		goto est_error;
> +
>  	stmmac_init_coalesce(priv);

[Severity: High]
This issue predates the patch, but the new est_error exit gives it one
more trigger. If stmmac_setup_est() fails during an MTU change, is the
running netdev left with freed rings and TX hrtimers that were never
initialised?

stmmac_change_mtu() calls __stmmac_release() and then
__stmmac_open(dev, dma_conf), which memcpy()s the kzalloc'd dma_conf into
priv->dma_conf. The est_error exit returns before stmmac_init_coalesce(),
so hrtimer_setup() never runs on the txtimers.

stmmac_change_mtu() then does:

	ret = __stmmac_open(dev, dma_conf);
	if (ret) {
		free_dma_desc_resources(priv, dma_conf);
		kfree(dma_conf);
		...
		return ret;
	}

and the interface stays up. On the next ifdown, __stmmac_release() calls
hrtimer_cancel() on priv->dma_conf.tx_queue[chan].txtimer, and
hrtimer_active() dereferences a NULL timer->base. After that,
free_dma_desc_resources(priv, &priv->dma_conf) frees the same rings a
second time.

Failures in stmmac_hw_setup() and stmmac_setup_ptp() already reach this
path. A failed EST/FPE replay is a new way in.

>  
>  	phylink_start(priv->phylink);
> @@ -4224,7 +4229,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);

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 9a32af00d7d2d..a37405534bebf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c

[ ... ]

> @@ -69,47 +69,21 @@ 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) {
> -		est_rst = true;
> -		mutex_lock(&priv->est_lock);
> -		priv->est->enable = false;
> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable)
>  		stmmac_est_configure(priv, priv, priv->est,
>  				     priv->plat->clk_ptp_rate, false);
> -		mutex_unlock(&priv->est_lock);
> -	}
> +	mutex_unlock(&priv->est_lock);
>  
>  	write_lock_irqsave(&priv->ptp_lock, flags);
>  	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. */

[ ... ]

> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable)
> +		__stmmac_setup_est(priv);
> +	mutex_unlock(&priv->est_lock);

[Severity: Low]
Can stmmac_setup_est() turn EST back on between these two est_lock
sections?

The first section no longer clears priv->est->enable, so any est_lock
user that runs while the time is being stepped still sees
enable == true.

In __stmmac_open(), stmmac_setup_est() runs right after
stmmac_setup_ptp() has registered the PHC. clock_adjtime() is not
serialised by rtnl:

CPU0 (stmmac_adjust_time)          CPU1 (__stmmac_open)
  disable EST, drop est_lock
                                     stmmac_setup_est()
                                       sees enable == true
                                       __stmmac_setup_est() enables EST
                                       with a pre-step base time
  stmmac_adjust_systime()
    (EST running across the step)
  __stmmac_setup_est()

A taprio REPLACE can race with adjust_time in the same way.

Before this patch, clearing enable during the window kept other paths
from re-arming EST. Would holding est_lock across the disable, the
adjust and the re-enable close this window?

> 
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index ac926c60f9f9a..10b61fa2c27c5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -1060,13 +1065,26 @@ static int tc_taprio_configure(struct stmmac_priv *priv,

[ ... ]

> -		priv->est->gcl[i] = delta_ns | (gates << wid);
> +		gcl[i] = delta_ns | (gates << wid);
>  	}
>  
> -	mutex_lock(&priv->est_lock);
> +	if (!priv->est) {
> +		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
> +					 GFP_KERNEL);
> +		if (!priv->est) {
> +			ret = -ENOMEM;
> +			goto error_unlock;
> +		}
> +	} else {
> +		memset(priv->est, 0, sizeof(*priv->est));
> +	}

[Severity: Low]
This is a pre-existing issue that the patch did not introduce.
stmmac_xmit(), stmmac_xdp_xmit_xdpf() and the other TX paths read
priv->est, priv->est->enable and priv->est->max_sdu[] without est_lock
and without READ_ONCE():

	if (priv->est && priv->est->enable &&
	    priv->est->max_sdu[queue]) {

If a REPLACE runs while traffic is being sent, can this memset() and the
plain stores after it race with those readers? That could drop a frame
by mistake, or let one through past max_sdu, while the schedule is being
reconfigured.

Setting enable last in this patch does make the window smaller.

> +	memcpy(priv->est->gcl, gcl, sizeof(*gcl) * qopt->num_entries);
> +	priv->est->gcl_size = qopt->num_entries;
> +

[ ... ]

> @@ -1091,7 +1109,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  
>  	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;
> @@ -1102,11 +1119,16 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	if (ret)
>  		goto disable;
>  

[Severity: Medium]
If a REPLACE fails FPE validation, is the installed schedule still torn
down?

The commit message says:

  A REPLACE rejected during validation then leaves the currently
  installed schedule intact in both software and hardware

However, by the time stmmac_fpe_map_preemption_class() runs, priv->est
has already been memset and overwritten, and
stmmac_est_configure(..., true) has programmed the new schedule.

dwmac5_fpe_map_preemption_class() only does software validation, and it
can return -EINVAL. One example:

	if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
		NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
		return -EINVAL;
	}

It can also fail on a TXQ weight mismatch. Either error takes the
disable path, which clears priv->est->enable and turns off EST in
hardware.

Meanwhile taprio_change() sees the error, jumps to free_sched, and keeps
the old schedule along with its full-offload state.

For example, on DWMAC5 with FPE: install an all-express schedule that
has a multi-queue TC, then REPLACE it with that TC marked preemptible.
The user gets -EINVAL and the qdisc still reports the old schedule as
offloaded. But hardware gating and max_sdu enforcement are gone, and a
later reopen or resume will not bring them back because enable is now
false.

Could the FPE validation run before priv->est is changed and EST is
programmed?

> +	priv->est->pclass = qopt->mqprio.preemptible_tcs;
> +	priv->est->enable = true;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-stmmac-est-fixes-v5-0-85b830a795a9%40oss.qualcomm.com

Re: [PATCH net v5 2/3] net: stmmac: pass the desired EST enable state to est_configure()

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit says "No functional change intended", but there is a
>   change when stmmac_adjust_time() runs concurrently.
> 
> Pre-existing issues:
> - [High] This happens at this commit in tc_taprio_configure()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c).
> - [High] At this commit stmmac_adjust_time() (stmmac_ptp.c:72) checks
>   priv->est->enable without est_lock.
> - [Medium] est_configure()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_est.c) rewrites…
> - [Medium] At this commit stmmac_adjust_time() decides est_rst from an
>   unlocked read of priv->est->enable (stmmac_ptp.c:72-73) and drops…
> 
> --
> 
> net: stmmac: pass the desired EST enable state to est_configure()
> 
> This patch adds an explicit enable argument to est_configure() so it no
> longer reads cfg->enable. The callers in stmmac_adjust_time() and
> tc_taprio_configure() now pass true or false directly, and they still set
> priv->est->enable around the EST programming as before.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> > index afc516059b897..f15d4d046aa74 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> 
> [ ... ]
> 
> > @@ -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;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but when enable is false,
> est_configure() still rewrites BTR, TER, LLR, CTR and the whole GCL
> through est_write() before it gets here:

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

> 
>     ret |= est_write(est_addr, EST_BTR_LOW, cfg->btr[0], false);
>     ...
>     if (ret)
>         return ret;
> 
>     for (i = 0; i < cfg->gcl_size; i++) {
>         ret = est_write(est_addr, i, cfg->gcl[i], true);
>         if (ret)
>             return ret;
>     }
> 
> If one of the SRWO polls in est_write() times out, the function returns
> before it clears EST_EEST here and before it zeroes EST_INT_EN.
> 
> The disable callers ignore that return value and set
> priv->est->enable = false anyway. These are the disable: label in
> tc_taprio_configure() and the quiesce block in stmmac_adjust_time().
> 
> On TAPRIO_CMD_DESTROY, tc_taprio_configure() returns only the result of
> stmmac_fpe_map_preemption_class(), so taprio treats the offload as
> removed.
> 
> Could the MAC keep gating TX with the old schedule, with EST interrupts
> still on, while software thinks EST is off? This ordering is unchanged at
> the end of the series.
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > index e54a3c8878265..9a32af00d7d2d 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);
> >  	}
> 
> [Severity: High]
> This is a pre-existing issue, but the check that guards this block reads
> priv->est->enable without holding est_lock:

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

> 
>     if (priv->est && priv->est->enable) {
>         est_rst = true;
>         mutex_lock(&priv->est_lock);
> 
> A TAPRIO_CMD_REPLACE in tc_taprio_configure() drops est_lock between
> steps:
> 
>     mutex_lock(&priv->est_lock);
>     memset(priv->est, 0, sizeof(*priv->est));
>     mutex_unlock(&priv->est_lock);
>     ...
>     mutex_lock(&priv->est_lock);
>     priv->est->gcl_size = size;
>     priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
>     mutex_unlock(&priv->est_lock);
> 
>     for (i = 0; i < size; i++) {
>         ...
>         priv->est->gcl[i] = delta_ns | (gates << wid);
>     }
> 
>     mutex_lock(&priv->est_lock);
>     /* ctr[] and btr_reserve[] are filled only here */
> 
> Suppose the second est_lock section of stmmac_adjust_time() runs in that
> gap. It then computes:
> 
>     cycle_time = (u64)priv->est->ctr[1] * NSEC_PER_SEC +
>                  priv->est->ctr[0];    /* still 0 */
>     time = stmmac_calc_tas_basetime(basetime, current_time_ns,
>                                     cycle_time);
> 
> and stmmac_calc_tas_basetime() does:
> 
>     n = div64_s64(ktime_sub_ns(current_time, old_base_time),
>                   cycle_time);
> 
> Can this divide by zero and oops on x86? It would also program a GCL
> that is still half written through est_configure(..., true).
> 
> This looks to be addressed later in the series by "net: stmmac: re-apply
> taprio offload in __stmmac_open() and stmmac_resume()". That commit:
> 
>   - holds est_lock across the whole validate, memset and program
>     sequence in tc_taprio_configure()
>   - sets enable only after programming succeeds
>   - checks enable under est_lock in stmmac_adjust_time()
> 
> > @@ -105,7 +105,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");
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but est_rst comes from the
> unlocked check before the first critical section. Nothing is re-checked
> here after est_lock was dropped around the systime step.

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

> 
> A TAPRIO_CMD_DESTROY can land in that gap:
> 
> CPU1 (stmmac_adjust_time)        CPU2 (tc_taprio_configure, DESTROY)
> est_rst = true
> priv->est->enable = false
> est_configure(..., false)
> mutex_unlock(&priv->est_lock)
>                                  disable:
>                                  priv->est->enable = false
>                                  est_configure(..., false)
>                                  returns success
> stmmac_adjust_systime()
> mutex_lock(&priv->est_lock)
> priv->est->enable = true
> est_configure(..., true)
> 
> Does this bring back a schedule that taprio has already removed? The
> xmit paths would then also enforce the priv->est->max_sdu[] drops again.
> 
> The same later commit, "net: stmmac: re-apply taprio offload in
> __stmmac_open() and stmmac_resume()", appears to fix this. It re-checks
> priv->est && priv->est->enable under est_lock before calling
> __stmmac_setup_est(), and stmmac_adjust_time() no longer writes the
> enable flag.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index aea235a69105e..ac926c60f9f9a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > @@ -1090,7 +1090,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);
> 
> [Severity: Low]
> The commit message says:
> 
>   No functional change intended: the callers keep toggling
>   priv->est->enable around the EST programming, as before.
> 
> Does that still hold when stmmac_adjust_time() runs at the same time?
> 
> Before this patch, this call read cfg->enable under est_lock. The REPLACE
> path sets enable = true in an earlier critical section and then drops
> est_lock while it fills the GCL. Consider this sequence:
> 
> CPU1 (tc_taprio_configure, REPLACE)  CPU2 (stmmac_adjust_time)
> priv->est->enable = true
> mutex_unlock(&priv->est_lock)
> fill priv->est->gcl[]
>                                      priv->est->enable = false
>                                      est_configure(..., false)
>                                      mutex_unlock(&priv->est_lock)
> mutex_lock(&priv->est_lock)
> est_configure(..., true)
>                                      stmmac_adjust_systime()

As the one above, this issue has not been introduced by this patch and I think
we should fix it with a dedicated patch.

> 
> With the old code, the REPLACE would program EST with EEST cleared,
> because cfg->enable was false. EST then stayed off across the time step.
> 
> With the hard-coded true, EST is turned on with a BTR computed from the
> pre-step time. The PHC step then happens with EST active while
> priv->est->enable is false, until the second section of
> stmmac_adjust_time() reprograms it.
> 
> The underlying race is pre-existing, since no lock covers the time step
> together with the EST programming. Still, this interleaving behaves
> differently after this patch, and the window remains at the end of the
> series.
> 
> >  	mutex_unlock(&priv->est_lock);
> >  	if (ret) {
> >  		netdev_err(priv->dev, "failed to configure EST\n");
> > @@ -1109,7 +1109,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);
> 
> [Severity: High]
> This is a pre-existing issue, but the entry validation loop earlier in
> tc_taprio_configure() returns directly instead of going through this
> disable: label:

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

Regards,
Lorenzo

> 
>     priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
>     mutex_unlock(&priv->est_lock);
> 
>     for (i = 0; i < size; i++) {
>         ...
>         if (delta_ns > GENMASK(wid - 1, 0))
>             return -ERANGE;
>         if (gates > GENMASK(31 - wid, 0))
>             return -ERANGE;
>         ...
>         default:
>             return -EOPNOTSUPP;
>         }
> 
> By that point priv->est has already been memset. So a REPLACE with one
> invalid interval, gate mask or command leaves enable == true while ctr[]
> and btr_reserve[] are zero.
> 
> taprio_disable_offload() does not call back into the driver when the
> first offload attempt failed, so nothing clears the flag later.
> 
> A later clock_adjtime(ADJ_SETOFFSET) on the PHC then does:
> 
> stmmac_adjust_time()
>   cycle_time = 0
>   stmmac_calc_tas_basetime(0, now, 0)
>     div64_s64(now, 0)
> 
> Can this divide by zero and oops on x86?
> 
> This also looks to be fixed later in the series by "net: stmmac: re-apply
> taprio offload in __stmmac_open() and stmmac_resume()". That commit
> validates every entry into a temporary kcalloc() buffer before it touches
> priv->est, and sets enable only after programming succeeds.
> 
> >  		/* Reset taprio status */
> >  		for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
> >  			priv->xstats.max_sdu_txq_drop[i] = 0;
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-stmmac-est-fixes-v5-0-85b830a795a9%40oss.qualcomm.com

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

Re: [PATCH net v5 3/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1 (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] A taprio REPLACE that fails the FPE callback's software
>   validation destroys the installed hardware schedule, even though the…
> - [Low] stmmac_adjust_time() still uses two separate est_lock critical
>   sections: it disables EST, runs adjust_systime under ptp_lock only,…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch adds
>   a new way to reach it.
> - [Low] The TX/XDP hot paths (stmmac_main.c:2753, 4861, 5286) read
>   priv->est, priv->est->enable and priv->est->max_sdu[] with no lock and…
> 
> --
> 
> net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()
> 
> This commit re-applies the taprio EST offload and the FPE preemption
> mapping after the core soft reset, in __stmmac_open() and stmmac_resume().
> The base time recompute moves into __stmmac_setup_est() and
> stmmac_setup_est().
> 
> tc_taprio_configure() now holds est_lock across validation and
> programming, builds the gate list in a temporary buffer, and sets
> priv->est->enable only after success.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 2163e79fea9a4..d1e3d2cc77670 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -4201,6 +4202,10 @@ static int __stmmac_open(struct net_device *dev,
> >  	if (ret)
> >  		goto ptp_error;
> >  
> > +	ret = stmmac_setup_est(priv);
> > +	if (ret)
> > +		goto est_error;
> > +
> >  	stmmac_init_coalesce(priv);
> 
> [Severity: High]
> This issue predates the patch, but the new est_error exit gives it one
> more trigger. If stmmac_setup_est() fails during an MTU change, is the
> running netdev left with freed rings and TX hrtimers that were never
> initialised?

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

> 
> stmmac_change_mtu() calls __stmmac_release() and then
> __stmmac_open(dev, dma_conf), which memcpy()s the kzalloc'd dma_conf into
> priv->dma_conf. The est_error exit returns before stmmac_init_coalesce(),
> so hrtimer_setup() never runs on the txtimers.
> 
> stmmac_change_mtu() then does:
> 
> 	ret = __stmmac_open(dev, dma_conf);
> 	if (ret) {
> 		free_dma_desc_resources(priv, dma_conf);
> 		kfree(dma_conf);
> 		...
> 		return ret;
> 	}
> 
> and the interface stays up. On the next ifdown, __stmmac_release() calls
> hrtimer_cancel() on priv->dma_conf.tx_queue[chan].txtimer, and
> hrtimer_active() dereferences a NULL timer->base. After that,
> free_dma_desc_resources(priv, &priv->dma_conf) frees the same rings a
> second time.
> 
> Failures in stmmac_hw_setup() and stmmac_setup_ptp() already reach this
> path. A failed EST/FPE replay is a new way in.
> 
> >  
> >  	phylink_start(priv->phylink);
> > @@ -4224,7 +4229,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);
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > index 9a32af00d7d2d..a37405534bebf 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> 
> [ ... ]
> 
> > @@ -69,47 +69,21 @@ 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) {
> > -		est_rst = true;
> > -		mutex_lock(&priv->est_lock);
> > -		priv->est->enable = false;
> > +	mutex_lock(&priv->est_lock);
> > +	if (priv->est && priv->est->enable)
> >  		stmmac_est_configure(priv, priv, priv->est,
> >  				     priv->plat->clk_ptp_rate, false);
> > -		mutex_unlock(&priv->est_lock);
> > -	}
> > +	mutex_unlock(&priv->est_lock);
> >  
> >  	write_lock_irqsave(&priv->ptp_lock, flags);
> >  	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. */
> 
> [ ... ]
> 
> > +	mutex_lock(&priv->est_lock);
> > +	if (priv->est && priv->est->enable)
> > +		__stmmac_setup_est(priv);
> > +	mutex_unlock(&priv->est_lock);
> 
> [Severity: Low]
> Can stmmac_setup_est() turn EST back on between these two est_lock
> sections?
> 
> The first section no longer clears priv->est->enable, so any est_lock
> user that runs while the time is being stepped still sees
> enable == true.
> 
> In __stmmac_open(), stmmac_setup_est() runs right after
> stmmac_setup_ptp() has registered the PHC. clock_adjtime() is not
> serialised by rtnl:
> 
> CPU0 (stmmac_adjust_time)          CPU1 (__stmmac_open)
>   disable EST, drop est_lock
>                                      stmmac_setup_est()
>                                        sees enable == true
>                                        __stmmac_setup_est() enables EST
>                                        with a pre-step base time
>   stmmac_adjust_systime()
>     (EST running across the step)
>   __stmmac_setup_est()
> 
> A taprio REPLACE can race with adjust_time in the same way.
> 
> Before this patch, clearing enable during the window kept other paths
> from re-arming EST. Would holding est_lock across the disable, the
> adjust and the re-enable close this window?

This is the same issue reported in patch 2/3. As pointed out before this issue
has not been introduced by this patch and it needs to be fixed with a dedicated
patch.

> 
> > 
> >  	return 0;
> >  }
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index ac926c60f9f9a..10b61fa2c27c5 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> 
> [ ... ]
> 
> > @@ -1060,13 +1065,26 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> 
> [ ... ]
> 
> > -		priv->est->gcl[i] = delta_ns | (gates << wid);
> > +		gcl[i] = delta_ns | (gates << wid);
> >  	}
> >  
> > -	mutex_lock(&priv->est_lock);
> > +	if (!priv->est) {
> > +		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
> > +					 GFP_KERNEL);
> > +		if (!priv->est) {
> > +			ret = -ENOMEM;
> > +			goto error_unlock;
> > +		}
> > +	} else {
> > +		memset(priv->est, 0, sizeof(*priv->est));
> > +	}
> 
> [Severity: Low]
> This is a pre-existing issue that the patch did not introduce.
> stmmac_xmit(), stmmac_xdp_xmit_xdpf() and the other TX paths read
> priv->est, priv->est->enable and priv->est->max_sdu[] without est_lock
> and without READ_ONCE():

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

> 
> 	if (priv->est && priv->est->enable &&
> 	    priv->est->max_sdu[queue]) {
> 
> If a REPLACE runs while traffic is being sent, can this memset() and the
> plain stores after it race with those readers? That could drop a frame
> by mistake, or let one through past max_sdu, while the schedule is being
> reconfigured.
> 
> Setting enable last in this patch does make the window smaller.
> 
> > +	memcpy(priv->est->gcl, gcl, sizeof(*gcl) * qopt->num_entries);
> > +	priv->est->gcl_size = qopt->num_entries;
> > +
> 
> [ ... ]
> 
> > @@ -1091,7 +1109,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> >  
> >  	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;
> > @@ -1102,11 +1119,16 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> >  	if (ret)
> >  		goto disable;
> >  
> 
> [Severity: Medium]
> If a REPLACE fails FPE validation, is the installed schedule still torn
> down?
> 
> The commit message says:
> 
>   A REPLACE rejected during validation then leaves the currently
>   installed schedule intact in both software and hardware
> 
> However, by the time stmmac_fpe_map_preemption_class() runs, priv->est
> has already been memset and overwritten, and
> stmmac_est_configure(..., true) has programmed the new schedule.
> 
> dwmac5_fpe_map_preemption_class() only does software validation, and it
> can return -EINVAL. One example:
> 
> 	if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
> 		NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
> 		return -EINVAL;
> 	}
> 
> It can also fail on a TXQ weight mismatch. Either error takes the
> disable path, which clears priv->est->enable and turns off EST in
> hardware.
> 
> Meanwhile taprio_change() sees the error, jumps to free_sched, and keeps
> the old schedule along with its full-offload state.
> 
> For example, on DWMAC5 with FPE: install an all-express schedule that
> has a multi-queue TC, then REPLACE it with that TC marked preemptible.
> The user gets -EINVAL and the qdisc still reports the old schedule as
> offloaded. But hardware gating and max_sdu enforcement are gone, and a
> later reopen or resume will not bring them back because enable is now
> false.
> 
> Could the FPE validation run before priv->est is changed and EST is
> programmed?

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

Regards,
Lorenzo

> 
> > +	priv->est->pclass = qopt->mqprio.preemptible_tcs;
> > +	priv->est->enable = true;
> > +
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-stmmac-est-fixes-v5-0-85b830a795a9%40oss.qualcomm.com

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

Re: [PATCH net v5 0/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 29 Oct 2026 11:33:54 +0100 you wrote:
> Reconfigure MTL_EST registers after sw reset in __stmmac_open().
> Remove priv->est->enable dependency in est_configure().
> Initialize est_lock at probe time.
> 
> Changes in v5:
> - Reconfigure preemption_class in stmmac_setup_est().
> - Bail out in stmmac_setup_est() if ptp is not enabled.
> - Link to v4: https://lore.kernel.org/r/20261026-stmmac-est-fixes-v4-0-999dac4de44f@oss.qualcomm.com
> 
> [...]

Here is the summary with links:
  - [net,v5,1/3] net: stmmac: initialize est_lock at probe time
    https://git.kernel.org/netdev/net/c/98caed39c1db
  - [net,v5,2/3] net: stmmac: pass the desired EST enable state to est_configure()
    https://git.kernel.org/netdev/net/c/e4f5b46dfb8a
  - [net,v5,3/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()
    https://git.kernel.org/netdev/net/c/23d96de472f7

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html