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

[PATCH net v4 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 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_est.c  | 41 +++++++++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  | 18 +++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 32 ++------
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 93 ++++++++++++++---------
 6 files changed, 131 insertions(+), 68 deletions(-)
---
base-commit: 6430fb7f1cc4e329e386521b7b208df443de92f2
change-id: 20261026-stmmac-est-fixes-13557ed8ab20

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

[PATCH net v4 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.

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 b20e5038eead..9e09e11cc35e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -8204,6 +8204,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);
@@ -8337,6 +8338,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 v4 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.

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 50b590d67d87..b28509555d81 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 v4 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 only marks the offload
enabled once the hardware programming succeeds, and into
stmmac_setup_est(), which takes the lock and re-applies the offload only
when it is currently enabled. Use the same helper in
stmmac_adjust_time() so the PTP adjust path shares the base time
computation.

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 rejected REPLACE 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_est.c  | 35 +++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_est.h  | 18 +++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 ++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 30 ++------
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 87 ++++++++++++++---------
 5 files changed, 122 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
index f15d4d046aa7..1a89f12f997f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
@@ -80,6 +80,41 @@ 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;
+	}
+
+	priv->est->enable = true;
+
+	return 0;
+}
+
 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..0f0f162794c8 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,18 @@
 #define EST_GCL_DATA			0x00000034
 
 extern const struct stmmac_est_ops dwmac510_est_ops;
+
+int __stmmac_setup_est(struct stmmac_priv *priv);
+static inline int stmmac_setup_est(struct stmmac_priv *priv)
+{
+	int ret = 0;
+
+	mutex_lock(&priv->est_lock);
+	if (priv->est && priv->est->enable)
+		ret = __stmmac_setup_est(priv);
+	mutex_unlock(&priv->est_lock);
+
+	return ret;
+}
+
+#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 9e09e11cc35e..a13a3790207d 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);
@@ -8518,6 +8523,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..81ee89669865 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
@@ -55,7 +56,6 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 	u32 quotient, reminder;
 	int neg_adj = 0;
 	bool xmac, est_rst = false;
-	int ret;
 
 	xmac = dwmac_is_xmac(priv->plat->core_type);
 
@@ -69,14 +69,14 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 	nsec = reminder;
 
 	/* If EST is enabled, disabled it before adjust ptp time. */
+	mutex_lock(&priv->est_lock);
 	if (priv->est && 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);
 	}
+	mutex_unlock(&priv->est_lock);
 
 	write_lock_irqsave(&priv->ptp_lock, flags);
 	stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
@@ -84,31 +84,9 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 
 	/* 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);
+		__stmmac_setup_est(priv);
 		mutex_unlock(&priv->est_lock);
-		if (ret)
-			netdev_err(priv->dev, "failed to configure EST\n");
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index ac926c60f9f9..95c5f6100c66 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,15 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	if (ret)
 		goto disable;
 
+	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 +1137,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 v4 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 v4 0/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[Lorenzo Bianconi]

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

On Oct 26, 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 v4 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 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 0 (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] The patch says it re-applies "the taprio offload" after the
  core soft reset, but it only replays the EST block.
- [Medium] The patch handles a failed EST re-apply inconsistently between
  open and resume.
- [Medium] A taprio REPLACE that is rejected only by the software checks
  in dwmac5_fpe_map_preemption_class() still tears down the installed…
- [Medium] stmmac_adjust_time() checks and clears priv->est->enable in
  one est_lock section (stmmac_ptp.c:72-79), drops the lock while it…

Pre-existing issues:
- [Low] The TX hot paths (stmmac_xmit around stmmac_main.c:4861,
  stmmac_tso_xmit, the XSK zero-copy path around 2753, and…

--

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

The core soft reset in stmmac_hw_setup() clears the MTL_EST registers. This
patch re-programs EST from __stmmac_open() and stmmac_resume(), recomputing
the base time from the reserved base time and the current PTP time, and
reuses the same helper in stmmac_adjust_time(). It also reworks
tc_taprio_configure() to hold est_lock across validation and programming,
and to set priv->est->enable only after hardware programming succeeds.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> index f70221c9c84af..0f0f162794c84 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h

[ ... ]

> @@ -65,3 +68,18 @@
>  #define EST_GCL_DATA			0x00000034
>  
>  extern const struct stmmac_est_ops dwmac510_est_ops;
> +
> +int __stmmac_setup_est(struct stmmac_priv *priv);
> +static inline int stmmac_setup_est(struct stmmac_priv *priv)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable)
> +		ret = __stmmac_setup_est(priv);

[Severity: Medium]

The commit message says this re-applies "the taprio offload" after the core
soft reset. Does stmmac_setup_est() restore everything that taprio
programmed, or only the EST block?

On a successful TAPRIO_CMD_REPLACE, tc_taprio_configure() also calls:

	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
					      qopt->mqprio.preemptible_tcs);

On GMAC5, dwmac5_fpe_map_preemption_class() writes FPE_MTL_PREEMPTION_CLASS
in GMAC5_MTL_FPE_CTRL_STS. On XGMAC, dwxgmac3_fpe_map_preemption_class()
also writes the per-queue XGMAC_Q2TCMAP field and XGMAC_MTL_FPE_CTRL_STS,
even when pclass is 0. During stmmac_hw_setup(), dwxgmac2_dma_tx_mode() then
rewrites Q2TCMAP back to the channel index:

	value |= FIELD_PREP(XGMAC_Q2TCMAP, channel);

preemptible_tcs is not saved in priv, and stmmac_fpe_map_preemption_class()
is only called from stmmac_tc.c. That means neither __stmmac_open() nor
stmmac_resume() can replay it.

So what happens after ifdown/ifup, an MTU or ring size change, or
suspend/resume? On GMAC5, would the preemptible TCs silently turn into
express TCs? On XGMAC, would the queues go back to the default queue == TC
mapping, so the EST gate masks no longer cover the intended queues?

Meanwhile __stmmac_setup_est() sets priv->est->enable = true, so taprio
would keep reporting the full offload as installed.

> +	mutex_unlock(&priv->est_lock);
> +
> +	return ret;
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9e09e11cc35e5..a13a3790207d5 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);
>  
>  	phylink_start(priv->phylink);

[ ... ]

> @@ -8518,6 +8523,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:

[Severity: Medium]

The callers seem to handle a failed EST re-apply in different ways. There
are a few cases.

In __stmmac_open(), stmmac_setup_ptp() can return 0 without enabling PTP:

stmmac_setup_ptp() {
    ...
	if (stmmac_init_ptp_clk_freq(priv)) {
		clk_disable_unprepare(priv->plat->clk_ptp_ref);
		return 0;
	}
    ...
}

This can happen when clk_ptp_rate is 0. For example, qcom-ethqos re-reads
clk_get_rate() in ethqos_ptp_clk_freq_config() on every open, so the rate
can become 0 after an earlier successful taprio REPLACE.

stmmac_setup_est() still runs in that case. __stmmac_setup_est() reads the
time through gettimex64 with the timestamp counter never initialised, and
est_configure() returns -EINVAL for a zero ptp_rate. Wouldn't ndo_open then
fail, where it succeeded before this patch?

In stmmac_resume(), any EST programming error (for example -ETIMEDOUT from
est_write()) now goes to error_stop_dma. That path stops DMA, disables the
MAC, powers down the serdes and returns without calling
netif_device_attach(), while netif_running() is still true. Is it intended
that losing EST now leaves the interface dead after resume?

Also in stmmac_resume(), when stmmac_init_ptp_clk_freq() fails, the code
jumps to init_coalesce and skips stmmac_setup_est(). Resume then returns
success with priv->est->enable still true, even though the soft reset has
cleared MTL_EST. Isn't that the stale state the commit message says it
removes?

When __stmmac_setup_est() fails, it returns without clearing
priv->est->enable:

	if (err) {
		netdev_err(priv->dev, "failed to re-configure EST\n");
		return err;
	}

For the open and resume callers the flag was already true on entry. It
stays true while EEST is off or only partly programmed. The commit message
says the helper "only marks the offload enabled once the hardware
programming succeeds". Does that hold for these callers?

Finally, stmmac_adjust_time() ignores the return value of the same helper.
Should the three callers share one failure policy?

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

[ ... ]

> @@ -69,14 +69,14 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
>  	nsec = reminder;
>  
>  	/* If EST is enabled, disabled it before adjust ptp time. */
> +	mutex_lock(&priv->est_lock);
>  	if (priv->est && 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);
>  	}
> +	mutex_unlock(&priv->est_lock);
>  
>  	write_lock_irqsave(&priv->ptp_lock, flags);
>  	stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
> @@ -84,31 +84,9 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
>  
>  	/* 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);

[ ... ]

> -		ret = stmmac_est_configure(priv, priv, priv->est,
> -					   priv->plat->clk_ptp_rate, true);
> +		__stmmac_setup_est(priv);
>  		mutex_unlock(&priv->est_lock);
> -		if (ret)
> -			netdev_err(priv->dev, "failed to configure EST\n");
>  	}

[Severity: Medium]

est_lock is dropped between clearing priv->est->enable and calling
__stmmac_setup_est() here. est_rst is not re-checked once the lock is taken
again. Can a taprio operation that runs inside that window get overridden?

ptp_clock_adjtime() does not take RTNL, so this ordering looks possible:

CPU1 (stmmac_adjust_time)
  mutex_lock(&priv->est_lock);
  est_rst = true;
  priv->est->enable = false;
  mutex_unlock(&priv->est_lock);
  stmmac_adjust_systime();

CPU2 (tc_taprio_configure, TAPRIO_CMD_DESTROY)
  mutex_lock(&priv->est_lock);
  priv->est->enable = false;
  stmmac_est_configure(..., false);
  stmmac_fpe_map_preemption_class(..., 0);
  mutex_unlock(&priv->est_lock);

CPU1
  mutex_lock(&priv->est_lock);
  __stmmac_setup_est(priv);
    stmmac_est_configure(..., true);
    priv->est->enable = true;

A REPLACE that fails and goes to disable: would race the same way.

The disable: path leaves gcl, ctr, btr_reserve and max_sdu[] in priv->est.
Would the old schedule, or the rejected one, then be programmed back into
hardware with enable set to true after the qdisc offload is gone? If so, the
TX/XDP paths would keep dropping frames larger than the stale max_sdu[].

__stmmac_open() and stmmac_resume() now trust that flag, so the schedule
would also be re-armed on every ifup and resume. taprio would not send
another DESTROY because q->offloaded is already false.

Should the decision to re-enable be re-validated under est_lock before
__stmmac_setup_est() is called?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index ac926c60f9f9a..95c5f6100c66a 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 and was not introduced by this patch. The TX
hot paths read priv->est, priv->est->enable and priv->est->max_sdu[] with
plain loads and without est_lock. For example, in stmmac_xmit():

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

stmmac_tso_xmit(), stmmac_xdp_xmit_zc() and stmmac_xdp_xmit_xdpf() do the
same.

The writers hold only est_lock. They use plain stores, a memset() of the
live struct, and a devm_kzalloc() pointer publish with no release ordering.

Could a REPLACE that runs concurrently with transmit briefly let oversize
frames through, or drop them? Two ways this could happen: the memset
temporarily zeroes max_sdu[], or a weakly ordered CPU sees the new pointer
before its zeroing is visible.

This patch does improve the ordering somewhat, since enable is now set only
after max_sdu[] is populated. The effect is limited to transient per-packet
filtering decisions.

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

[ ... ]

> @@ -1102,11 +1119,15 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	if (ret)
>  		goto disable;

[Severity: Medium]

The commit message says "A rejected REPLACE then leaves the currently
installed schedule intact in both software and hardware". However,
stmmac_fpe_map_preemption_class() can still reject the request after
priv->est has been memset, overwritten with the new schedule and programmed
into hardware.

dwmac5_fpe_map_preemption_class() returns -EINVAL with an extack message
based only on configuration checks, before it writes any register:

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

It does the same for unequal queue weights within a TC. That error goes to
disable:, which clears priv->est->enable, disables EST in hardware and
resets the preemption class to 0.

On the taprio side, taprio_enable_offload() returns the error before it
touches q->offloaded, which stays true from the previous schedule.
taprio_change() then goes to free_sched without installing the new admin
schedule. Doesn't this leave the qdisc reporting the old schedule as
offloaded while hardware EST is off?

enable is now false, so stmmac_setup_est() will not restore the schedule on
a later open or resume either.

One way to hit this:

1. Install an express-only schedule with a TC mapped to two queues. This is
   accepted, because pclass == 0 skips the checks.
2. Run tc qdisc change to mark that TC preemptible.

Should the FPE mapping checks run before the installed configuration is
replaced?

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-stmmac-est-fixes-v4-0-999dac4de44f%40oss.qualcomm.com

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

[Maxime Chevallier]

Hi,

On 10/26/26 17:55, Lorenzo Bianconi wrote:
> 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.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

Maxime

> ---
>  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 b20e5038eead..9e09e11cc35e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8204,6 +8204,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);
> @@ -8337,6 +8338,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));
> 

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

[Maxime Chevallier]

Hi,

On 10/26/26 17:55, Lorenzo Bianconi wrote:
> 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.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

Maxime

> ---
>  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 50b590d67d87..b28509555d81 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;
> 

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 4 · Low: 0 (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] The patch says it re-applies "the taprio offload" after the
>   core soft reset, but it only replays the EST block.
> - [Medium] The patch handles a failed EST re-apply inconsistently between
>   open and resume.
> - [Medium] A taprio REPLACE that is rejected only by the software checks
>   in dwmac5_fpe_map_preemption_class() still tears down the installed…
> - [Medium] stmmac_adjust_time() checks and clears priv->est->enable in
>   one est_lock section (stmmac_ptp.c:72-79), drops the lock while it…
> 
> Pre-existing issues:
> - [Low] The TX hot paths (stmmac_xmit around stmmac_main.c:4861,
>   stmmac_tso_xmit, the XSK zero-copy path around 2753, and…
> 
> --
> 
> net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()
> 
> The core soft reset in stmmac_hw_setup() clears the MTL_EST registers. This
> patch re-programs EST from __stmmac_open() and stmmac_resume(), recomputing
> the base time from the reserved base time and the current PTP time, and
> reuses the same helper in stmmac_adjust_time(). It also reworks
> tc_taprio_configure() to hold est_lock across validation and programming,
> and to set priv->est->enable only after hardware programming succeeds.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> > index f70221c9c84af..0f0f162794c84 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.h
> 
> [ ... ]
> 
> > @@ -65,3 +68,18 @@
> >  #define EST_GCL_DATA			0x00000034
> >  
> >  extern const struct stmmac_est_ops dwmac510_est_ops;
> > +
> > +int __stmmac_setup_est(struct stmmac_priv *priv);
> > +static inline int stmmac_setup_est(struct stmmac_priv *priv)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&priv->est_lock);
> > +	if (priv->est && priv->est->enable)
> > +		ret = __stmmac_setup_est(priv);
> 
> [Severity: Medium]
> 
> The commit message says this re-applies "the taprio offload" after the core
> soft reset. Does stmmac_setup_est() restore everything that taprio
> programmed, or only the EST block?
> 
> On a successful TAPRIO_CMD_REPLACE, tc_taprio_configure() also calls:
> 
> 	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
> 					      qopt->mqprio.preemptible_tcs);
> 
> On GMAC5, dwmac5_fpe_map_preemption_class() writes FPE_MTL_PREEMPTION_CLASS
> in GMAC5_MTL_FPE_CTRL_STS. On XGMAC, dwxgmac3_fpe_map_preemption_class()
> also writes the per-queue XGMAC_Q2TCMAP field and XGMAC_MTL_FPE_CTRL_STS,
> even when pclass is 0. During stmmac_hw_setup(), dwxgmac2_dma_tx_mode() then
> rewrites Q2TCMAP back to the channel index:
> 
> 	value |= FIELD_PREP(XGMAC_Q2TCMAP, channel);
> 
> preemptible_tcs is not saved in priv, and stmmac_fpe_map_preemption_class()
> is only called from stmmac_tc.c. That means neither __stmmac_open() nor
> stmmac_resume() can replay it.
> 
> So what happens after ifdown/ifup, an MTU or ring size change, or
> suspend/resume? On GMAC5, would the preemptible TCs silently turn into
> express TCs? On XGMAC, would the queues go back to the default queue == TC
> mapping, so the EST gate masks no longer cover the intended queues?
> 
> Meanwhile __stmmac_setup_est() sets priv->est->enable = true, so taprio
> would keep reporting the full offload as installed.

I will fix it in v5.

> 
> > +	mutex_unlock(&priv->est_lock);
> > +
> > +	return ret;
> > +}
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 9e09e11cc35e5..a13a3790207d5 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);
> >  
> >  	phylink_start(priv->phylink);
> 
> [ ... ]
> 
> > @@ -8518,6 +8523,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:
> 
> [Severity: Medium]
> 
> The callers seem to handle a failed EST re-apply in different ways. There
> are a few cases.
> 
> In __stmmac_open(), stmmac_setup_ptp() can return 0 without enabling PTP:
> 
> stmmac_setup_ptp() {
>     ...
> 	if (stmmac_init_ptp_clk_freq(priv)) {
> 		clk_disable_unprepare(priv->plat->clk_ptp_ref);
> 		return 0;
> 	}
>     ...
> }
> 
> This can happen when clk_ptp_rate is 0. For example, qcom-ethqos re-reads
> clk_get_rate() in ethqos_ptp_clk_freq_config() on every open, so the rate
> can become 0 after an earlier successful taprio REPLACE.
> 
> stmmac_setup_est() still runs in that case. __stmmac_setup_est() reads the
> time through gettimex64 with the timestamp counter never initialised, and
> est_configure() returns -EINVAL for a zero ptp_rate. Wouldn't ndo_open then
> fail, where it succeeded before this patch?
> 
> In stmmac_resume(), any EST programming error (for example -ETIMEDOUT from
> est_write()) now goes to error_stop_dma. That path stops DMA, disables the
> MAC, powers down the serdes and returns without calling
> netif_device_attach(), while netif_running() is still true. Is it intended
> that losing EST now leaves the interface dead after resume?
> 
> Also in stmmac_resume(), when stmmac_init_ptp_clk_freq() fails, the code
> jumps to init_coalesce and skips stmmac_setup_est(). Resume then returns
> success with priv->est->enable still true, even though the soft reset has
> cleared MTL_EST. Isn't that the stale state the commit message says it
> removes?
> 
> When __stmmac_setup_est() fails, it returns without clearing
> priv->est->enable:
> 
> 	if (err) {
> 		netdev_err(priv->dev, "failed to re-configure EST\n");
> 		return err;
> 	}
> 
> For the open and resume callers the flag was already true on entry. It
> stays true while EEST is off or only partly programmed. The commit message
> says the helper "only marks the offload enabled once the hardware
> programming succeeds". Does that hold for these callers?
> 
> Finally, stmmac_adjust_time() ignores the return value of the same helper.
> Should the three callers share one failure policy?

I will fix it in v5.

> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > index 9a32af00d7d2d..81ee896698653 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> 
> [ ... ]
> 
> > @@ -69,14 +69,14 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> >  	nsec = reminder;
> >  
> >  	/* If EST is enabled, disabled it before adjust ptp time. */
> > +	mutex_lock(&priv->est_lock);
> >  	if (priv->est && 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);
> >  	}
> > +	mutex_unlock(&priv->est_lock);
> >  
> >  	write_lock_irqsave(&priv->ptp_lock, flags);
> >  	stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
> > @@ -84,31 +84,9 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> >  
> >  	/* 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);
> 
> [ ... ]
> 
> > -		ret = stmmac_est_configure(priv, priv, priv->est,
> > -					   priv->plat->clk_ptp_rate, true);
> > +		__stmmac_setup_est(priv);
> >  		mutex_unlock(&priv->est_lock);
> > -		if (ret)
> > -			netdev_err(priv->dev, "failed to configure EST\n");
> >  	}
> 
> [Severity: Medium]
> 
> est_lock is dropped between clearing priv->est->enable and calling
> __stmmac_setup_est() here. est_rst is not re-checked once the lock is taken
> again. Can a taprio operation that runs inside that window get overridden?
> 
> ptp_clock_adjtime() does not take RTNL, so this ordering looks possible:
> 
> CPU1 (stmmac_adjust_time)
>   mutex_lock(&priv->est_lock);
>   est_rst = true;
>   priv->est->enable = false;
>   mutex_unlock(&priv->est_lock);
>   stmmac_adjust_systime();
> 
> CPU2 (tc_taprio_configure, TAPRIO_CMD_DESTROY)
>   mutex_lock(&priv->est_lock);
>   priv->est->enable = false;
>   stmmac_est_configure(..., false);
>   stmmac_fpe_map_preemption_class(..., 0);
>   mutex_unlock(&priv->est_lock);
> 
> CPU1
>   mutex_lock(&priv->est_lock);
>   __stmmac_setup_est(priv);
>     stmmac_est_configure(..., true);
>     priv->est->enable = true;
> 
> A REPLACE that fails and goes to disable: would race the same way.
> 
> The disable: path leaves gcl, ctr, btr_reserve and max_sdu[] in priv->est.
> Would the old schedule, or the rejected one, then be programmed back into
> hardware with enable set to true after the qdisc offload is gone? If so, the
> TX/XDP paths would keep dropping frames larger than the stale max_sdu[].
> 
> __stmmac_open() and stmmac_resume() now trust that flag, so the schedule
> would also be re-armed on every ifup and resume. taprio would not send
> another DESTROY because q->offloaded is already false.
> 
> Should the decision to re-enable be re-validated under est_lock before
> __stmmac_setup_est() is called?

I will fix it in v5.

> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index ac926c60f9f9a..95c5f6100c66a 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 and was not introduced by this patch. The TX
> hot paths read priv->est, priv->est->enable and priv->est->max_sdu[] with
> plain loads and without est_lock. For example, in stmmac_xmit():
> 
> 	if (priv->est && priv->est->enable &&
> 	    priv->est->max_sdu[queue]) {
> 
> stmmac_tso_xmit(), stmmac_xdp_xmit_zc() and stmmac_xdp_xmit_xdpf() do the
> same.
> 
> The writers hold only est_lock. They use plain stores, a memset() of the
> live struct, and a devm_kzalloc() pointer publish with no release ordering.
> 
> Could a REPLACE that runs concurrently with transmit briefly let oversize
> frames through, or drop them? Two ways this could happen: the memset
> temporarily zeroes max_sdu[], or a weakly ordered CPU sees the new pointer
> before its zeroing is visible.
> 
> This patch does improve the ordering somewhat, since enable is now set only
> after max_sdu[] is populated. The effect is limited to transient per-packet
> filtering decisions.
> 
> > +	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);
> 
> [ ... ]
> 
> > @@ -1102,11 +1119,15 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> >  	if (ret)
> >  		goto disable;
> 
> [Severity: Medium]
> 
> The commit message says "A rejected REPLACE then leaves the currently
> installed schedule intact in both software and hardware". However,
> stmmac_fpe_map_preemption_class() can still reject the request after
> priv->est has been memset, overwritten with the new schedule and programmed
> into hardware.
> 
> dwmac5_fpe_map_preemption_class() returns -EINVAL with an extack message
> based only on configuration checks, before it writes any register:
> 
> 		if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
> 			NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
> 			return -EINVAL;
> 		}
> 
> It does the same for unequal queue weights within a TC. That error goes to
> disable:, which clears priv->est->enable, disables EST in hardware and
> resets the preemption class to 0.
> 
> On the taprio side, taprio_enable_offload() returns the error before it
> touches q->offloaded, which stays true from the previous schedule.
> taprio_change() then goes to free_sched without installing the new admin
> schedule. Doesn't this leave the qdisc reporting the old schedule as
> offloaded while hardware EST is off?
> 
> enable is now false, so stmmac_setup_est() will not restore the schedule on
> a later open or resume either.
> 
> One way to hit this:
> 
> 1. Install an express-only schedule with a TC mapped to two queues. This is
>    accepted, because pclass == 0 skips the checks.
> 2. Run tc qdisc change to mark that TC preemptible.
> 
> Should the FPE mapping checks run before the installed configuration is
> replaced?

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

Regards,
Lorenzo

> 
> > +
> > +	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);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-stmmac-est-fixes-v4-0-999dac4de44f%40oss.qualcomm.com

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