[PATCH net-next v4 0/2] net: stmmac: dwxgmac2: timestamp interrupt + Agilex5 fix

[PATCH net-next v4 0/2] net: stmmac: dwxgmac2: timestamp interrupt + Agilex5 fix

[Zxyan Zhu]

This series adds auxiliary snapshot (EXTTS) interrupt support to
DWXGMAC2/DWXLGMAC2 and fixes a regression it would introduce on the
Agilex5 cross-timestamp path.

Patch 1 wires up a dedicated DWXGMAC2 timestamp interrupt handler.
Before this change the XGMAC hwif entries used the generic stmmac_ptp
ops, whose timestamp_interrupt callback read the dwmac4 offset
GMAC_TIMESTAMP_STATUS (0xb20) instead of the XGMAC register at 0xd20,
and XGMAC_TSIE was never enabled, so auxiliary snapshot events were
never reported on XGMAC platforms.

Patch 1 also unmasks XGMAC_TSIE in XGMAC_INT_DEFAULT_EN.  That makes the
new handler run from hardirq on every XGMAC timestamp event, which races
with the Agilex5 smtg_crosststamp() poll loop: the handler clears TSIS
by reading XGMAC_TIMESTAMP_STATUS before the poll loop in process
context can observe it, so getcrosststamp() times out.  Patch 2 fixes
this by masking XGMAC_TSIE for the duration of the cross-timestamp so
the handler cannot steal TSIS while smtg_crosststamp() owns the
snapshot FIFO.

Patch 2 has been verified on Agilex5 hardware: with both patches
applied, smtg_crosststamp() no longer times out (no "Wait for time sync
operation timeout" in dmesg) and the PTP clock registers and operates
normally.

The pre-existing FIFO-pop / PTP_ACR / flags races that the automated
review raised are not specific to this series (the same shape exists in
the dwmac4 handler) and are left for a separate follow-up.

v1: https://lore.kernel.org/netdev/20260806-dwxgmac2-timestamp-irq-v1-1-c051c79c9d90@gmail.com/
v2: https://lore.kernel.org/netdev/20260810100221.9166-1-zxyan0222@gmail.com/
v3: https://lore.kernel.org/netdev/20260818132722.1852876-1-zxyan0222@gmail.com/
v4:
- Rebased on the latest net-next/main (v7.4 cycle, after net-next
  reopened).

Zxyan Zhu (2):
  net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support
  net: stmmac: dwmac-socfpga: mask XGMAC_TSIE during cross-timestamp

 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 11 +++++
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  2 +-
 .../ethernet/stmicro/stmmac/dwxgmac2_core.c   | 43 +++++++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.c    |  4 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h     |  1 +
 .../ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 12 ++++++
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.h  |  1 +
 7 files changed, 71 insertions(+), 3 deletions(-)

--
2.34.1

[PATCH net-next v4 1/2] net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

[Zxyan Zhu]

DWXGMAC2 uses XGMAC_TIMESTAMP_STATUS at offset 0xd20, while the
generic stmmac PTP handler reads the dwmac4 offset GMAC_TIMESTAMP_STATUS
(0xb20).  Before this change, the DWXGMAC2 and DWXLGMAC2 hwif entries
used &stmmac_ptp, whose timestamp_interrupt callback read the wrong
register and whose config_hw_tstamping callback never enabled the
XGMAC timestamp interrupt (XGMAC_TSIE was not in XGMAC_INT_DEFAULT_EN).
As a result, auxiliary snapshot events were never reported on XGMAC
platforms.

Add a dedicated DWXGMAC2 timestamp interrupt handler that:
- reads XGMAC_TIMESTAMP_STATUS before checking
  STMMAC_FLAG_EXT_SNAPSHOT_EN, so that the timestamp interrupt status is
  cleared even when auxiliary snapshots are disabled
- derives the pending auxiliary snapshot count from the persistent
  ATSNS field instead of the transient AUXTSTRIG status bit
- generates the corresponding PTP_CLOCK_EXTTS events

Also enable XGMAC_TSIE in XGMAC_INT_DEFAULT_EN and hook the new
handler into the DWXGMAC2 and DWXLGMAC2 hwif entries.

Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  2 +-
 .../ethernet/stmicro/stmmac/dwxgmac2_core.c   | 43 +++++++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.c    |  4 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  1 +
 .../ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 12 ++++++
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.h  |  1 +
 6 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index f8ab347f7b5b..03cbae326995 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -87,7 +87,7 @@
 #define XGMAC_TSIE			BIT(12)
 #define XGMAC_LPIIE			BIT(5)
 #define XGMAC_PMTIE			BIT(4)
-#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE)
+#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE | XGMAC_TSIE)
 #define XGMAC_Qx_TX_FLOW_CTRL(x)	(0x00000070 + (x) * 4)
 #define XGMAC_PT			GENMASK(31, 16)
 #define XGMAC_TFE			BIT(1)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 1a88cbaed70c..891b7ceea2d4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -1154,6 +1154,49 @@ static int dwxgmac2_get_mac_tx_timestamp(struct mac_device_info *hw, u64 *ts)
 	return 0;
 }
 
+void dwxgmac2_timestamp_interrupt(struct stmmac_priv *priv)
+{
+	u32 ts_status, pending_snapshots, acr_value, channel;
+	struct ptp_clock_event event;
+	unsigned long flags;
+	u64 ptp_time;
+	int i;
+
+	/* Read XGMAC_TIMESTAMP_STATUS to get the AUX snapshot
+	 * count.  This read also clears the TSIS bit in
+	 * XGMAC_INT_STATUS.
+	 * TX timestamp polling may have already cleared TSIS
+	 * and AUXTSTRIG, so rely on ATSNS instead.
+	 * TXTSC is cleared by XGMAC_TXTIMESTAMP_SEC, not by
+	 * this register, so there is no conflict.
+	 */
+	ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
+
+	if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN))
+		return;
+
+	pending_snapshots = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, ts_status);
+	if (!pending_snapshots)
+		return;
+
+	acr_value = readl(priv->ptpaddr + PTP_ACR);
+	channel = FIELD_GET(PTP_ACR_MASK, acr_value);
+	if (!channel)
+		return;
+	channel = ilog2(channel);
+
+	for (i = 0; i < pending_snapshots; i++) {
+		read_lock_irqsave(&priv->ptp_lock, flags);
+		stmmac_get_ptptime(priv, priv->ptpaddr, &ptp_time);
+		read_unlock_irqrestore(&priv->ptp_lock, flags);
+
+		event.type = PTP_CLOCK_EXTTS;
+		event.index = channel;
+		event.timestamp = ptp_time;
+		ptp_clock_event(priv->ptp_clock, &event);
+	}
+}
+
 static int dwxgmac2_flex_pps_config(void __iomem *ioaddr, int index,
 				    struct stmmac_pps_cfg *cfg, bool enable,
 				    u32 sub_second_inc, u32 systime_flags)
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
index 265671170bf6..eba87410f985 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
@@ -258,7 +258,7 @@ static const struct stmmac_hwif_entry {
 		.dma = &dwxgmac210_dma_ops,
 		.mac = &dwxgmac210_ops,
 		.vlan = &dwxgmac210_vlan_ops,
-		.hwtimestamp = &stmmac_ptp,
+		.hwtimestamp = &dwxgmac2_ptp,
 		.ptp = &stmmac_ptp_clock_ops,
 		.mode = NULL,
 		.tc = &dwmac510_tc_ops,
@@ -280,7 +280,7 @@ static const struct stmmac_hwif_entry {
 		.dma = &dwxgmac210_dma_ops,
 		.mac = &dwxlgmac2_ops,
 		.vlan = &dwxlgmac2_vlan_ops,
-		.hwtimestamp = &stmmac_ptp,
+		.hwtimestamp = &dwxgmac2_ptp,
 		.ptp = &stmmac_ptp_clock_ops,
 		.mode = NULL,
 		.tc = &dwmac510_tc_ops,
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index fe18f98fae45..5a308e79db4a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -672,6 +672,7 @@ extern const struct stmmac_desc_ops ndesc_ops;
 
 extern const struct stmmac_hwtimestamp stmmac_ptp;
 extern const struct stmmac_hwtimestamp dwmac1000_ptp;
+extern const struct stmmac_hwtimestamp dwxgmac2_ptp;
 
 extern const struct stmmac_mode_ops ring_mode_ops;
 extern const struct stmmac_mode_ops chain_mode_ops;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index b9a985fa772c..9d7d24259abd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -277,3 +277,15 @@ const struct stmmac_hwtimestamp dwmac1000_ptp = {
 	.get_ptptime = dwmac1000_get_ptptime,
 	.timestamp_interrupt = dwmac1000_timestamp_interrupt,
 };
+
+const struct stmmac_hwtimestamp dwxgmac2_ptp = {
+	.config_hw_tstamping = config_hw_tstamping,
+	.init_systime = init_systime,
+	.config_sub_second_increment = config_sub_second_increment,
+	.config_addend = config_addend,
+	.adjust_systime = adjust_systime,
+	.get_systime = get_systime,
+	.get_ptptime = get_ptptime,
+	.timestamp_interrupt = dwxgmac2_timestamp_interrupt,
+	.hwtstamp_correct_latency = hwtstamp_correct_latency,
+};
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
index 3fe0e3a80e80..dade09614163 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
@@ -103,6 +103,7 @@ int dwmac1000_ptp_enable(struct ptp_clock_info *ptp,
 
 void dwmac1000_get_ptptime(void __iomem *ptpaddr, u64 *ptp_time);
 void dwmac1000_timestamp_interrupt(struct stmmac_priv *priv);
+void dwxgmac2_timestamp_interrupt(struct stmmac_priv *priv);
 
 extern const struct ptp_clock_info stmmac_ptp_clock_ops;
 extern const struct ptp_clock_info dwmac1000_ptp_clock_ops;
-- 
2.34.1

[PATCH net-next v4 2/2] net: stmmac: dwmac-socfpga: mask XGMAC_TSIE during cross-timestamp

[Zxyan Zhu]

The Agilex5 smtg_crosststamp() handler arms an internal auxiliary
snapshot, toggles GPO0 and then polls XGMAC_INT_STATUS for TSIS in
process context to learn that the snapshot is ready.  Unlike the Intel
cross-timestamp path it does not set STMMAC_FLAG_INT_SNAPSHOT_EN, so it
relies on nothing else consuming TSIS.

Once XGMAC_TSIE is unmasked (done by a companion change that enables it
in XGMAC_INT_DEFAULT_EN), the DWXGMAC2 timestamp interrupt handler runs
from hardirq on every timestamp event and clears TSIS by reading
XGMAC_TIMESTAMP_STATUS.  That read can win the race against the poll
loop, which then times out and makes PTP_SYS_OFFSET_PRECISE fail with
"Wait for time sync operation timeout".

Mask XGMAC_TSIE around the snapshot trigger and FIFO read so the hardirq
handler cannot clear TSIS while smtg_crosststamp() owns it, and restore
it on every return path.

Tested-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1d7f0a57d288..a4d00bf81423 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -310,6 +310,13 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
 		return -EBUSY;
 
+	/* The XGMAC timestamp interrupt handler clears TSIS by reading
+	 * XGMAC_TIMESTAMP_STATUS, which would race with the TSIS poll
+	 * below.  Mask XGMAC_TSIE for the duration of the cross-timestamp
+	 * so the handler does not run while we own the snapshot FIFO.
+	 */
+	stmmac_mac_irq_modify(priv, XGMAC_TSIE, 0);
+
 	mutex_lock(&priv->aux_ts_lock);
 	/* Enable Internal snapshot trigger */
 	acr_value = readl(ptpaddr + PTP_ACR);
@@ -329,6 +336,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		break;
 	default:
 		mutex_unlock(&priv->aux_ts_lock);
+		stmmac_mac_irq_modify(priv, 0, XGMAC_TSIE);
 		return -EINVAL;
 	}
 	writel(acr_value, ptpaddr + PTP_ACR);
@@ -353,6 +361,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	ret = readl_poll_timeout(priv->ioaddr + XGMAC_INT_STATUS, v,
 				 (v & XGMAC_INT_TSIS), 100, 10000);
 	if (ret) {
+		stmmac_mac_irq_modify(priv, 0, XGMAC_TSIE);
 		netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
 			   __func__);
 		return ret;
@@ -375,6 +384,8 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		read_unlock_irqrestore(&priv->ptp_lock, flags);
 	}
 
+	stmmac_mac_irq_modify(priv, 0, XGMAC_TSIE);
+
 	get_smtgtime(priv->mii, SMTG_MDIO_ADDR, &smtg_time);
 	system->cycles = smtg_time;
 
-- 
2.34.1

Re: [PATCH net-next v4 1/2] net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

[Maxime Chevallier]

Hi,

On 9/2/26 15:14, Zxyan Zhu wrote:
> DWXGMAC2 uses XGMAC_TIMESTAMP_STATUS at offset 0xd20, while the
> generic stmmac PTP handler reads the dwmac4 offset GMAC_TIMESTAMP_STATUS
> (0xb20).  Before this change, the DWXGMAC2 and DWXLGMAC2 hwif entries
> used &stmmac_ptp, whose timestamp_interrupt callback read the wrong
> register and whose config_hw_tstamping callback never enabled the
> XGMAC timestamp interrupt (XGMAC_TSIE was not in XGMAC_INT_DEFAULT_EN).
> As a result, auxiliary snapshot events were never reported on XGMAC
> platforms.
> 
> Add a dedicated DWXGMAC2 timestamp interrupt handler that:
> - reads XGMAC_TIMESTAMP_STATUS before checking
>   STMMAC_FLAG_EXT_SNAPSHOT_EN, so that the timestamp interrupt status is
>   cleared even when auxiliary snapshots are disabled
> - derives the pending auxiliary snapshot count from the persistent
>   ATSNS field instead of the transient AUXTSTRIG status bit
> - generates the corresponding PTP_CLOCK_EXTTS events
> 
> Also enable XGMAC_TSIE in XGMAC_INT_DEFAULT_EN and hook the new
> handler into the DWXGMAC2 and DWXLGMAC2 hwif entries.
> 
> Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>

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

Maxime

Re: [PATCH net-next v4 1/2] net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 1

- [High] dwxgmac2_timestamp_interrupt()
  (drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:1157) performs an…
- [Medium] dwxgmac2_timestamp_interrupt() samples the pending snapshot
  count from XGMAC_TIMESTAMP_STATUS (dwxgmac2_core.c:1178), reads…
- [Low] The commit message asserts a concrete pre-existing user-visible
  defect ("the generic stmmac PTP handler reads the dwmac4 offset…

--

net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

This adds a DWXGMAC2 specific timestamp interrupt handler that reads
XGMAC_TIMESTAMP_STATUS (0xd20), derives the pending auxiliary snapshot
count from ATSNS and emits PTP_CLOCK_EXTTS events.  It also adds
XGMAC_TSIE to XGMAC_INT_DEFAULT_EN and points the DWXGMAC2 and DWXLGMAC2
hwif entries at the new dwxgmac2_ptp ops instead of &stmmac_ptp.

A couple of questions below, starting with the changelog.

> DWXGMAC2 uses XGMAC_TIMESTAMP_STATUS at offset 0xd20, while the
> generic stmmac PTP handler reads the dwmac4 offset GMAC_TIMESTAMP_STATUS
> (0xb20).
[ ... ]
> As a result, auxiliary snapshot events were never reported on XGMAC
> platforms.

[Severity: Low]
The description here is of previously broken functionality (wrong register
offset, timestamp interrupt never unmasked), but the patch is titled "add
... support" and carries no Fixes: tag.  Should one be added so that PTP
EXTTS breakage on DWXGMAC2/DWXLGMAC2 is bisectable and stable selection is
unambiguous?

Two candidates that exist in the tree:

  f4da56529da6 ("net: stmmac: Add support for external trigger timestamping")
  bd7f77dae695 ("net: stmmac: fix incorrect flag check in timestamp interrupt")

Is this intended for net or net-next?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index f8ab347f7b5b..03cbae326995 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -87,7 +87,7 @@
>  #define XGMAC_TSIE			BIT(12)
>  #define XGMAC_LPIIE			BIT(5)
>  #define XGMAC_PMTIE			BIT(4)
> -#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE)
> +#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE | XGMAC_TSIE)
>  #define XGMAC_Qx_TX_FLOW_CTRL(x)	(0x00000070 + (x) * 4)
>  #define XGMAC_PT			GENMASK(31, 16)
>  #define XGMAC_TFE			BIT(1)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index 1a88cbaed70c..891b7ceea2d4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -1154,6 +1154,49 @@ static int dwxgmac2_get_mac_tx_timestamp(struct mac_device_info *hw, u64 *ts)
>  	return 0;
>  }
>  
> +void dwxgmac2_timestamp_interrupt(struct stmmac_priv *priv)
> +{
> +	u32 ts_status, pending_snapshots, acr_value, channel;
> +	struct ptp_clock_event event;
> +	unsigned long flags;
> +	u64 ptp_time;
> +	int i;
> +
> +	/* Read XGMAC_TIMESTAMP_STATUS to get the AUX snapshot
> +	 * count.  This read also clears the TSIS bit in
> +	 * XGMAC_INT_STATUS.
> +	 * TX timestamp polling may have already cleared TSIS
> +	 * and AUXTSTRIG, so rely on ATSNS instead.
> +	 * TXTSC is cleared by XGMAC_TXTIMESTAMP_SEC, not by
> +	 * this register, so there is no conflict.
> +	 */
> +	ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);

[Severity: High]
This read of the clear-on-read status register is unconditional, unlike the
generic handler in stmmac_hwtstamp.c which first checks whether a timestamp
interrupt is actually pending:

	tsync_int = readl(priv->ioaddr + GMAC_INT_STATUS) & GMAC_INT_TSIE;
	if (!tsync_int)
		return;

stmmac_common_interrupt() invokes stmmac_timestamp_interrupt(priv, priv) for
every xmac core interrupt, and on platforms that do not set
STMMAC_FLAG_MULTI_MSI_EN the single shared handler runs it ahead of DMA
processing:

stmmac_interrupt()
	stmmac_common_interrupt();   /* -> readl(0xd20), clears TSIS */

	/* To handle DMA interrupts */
	stmmac_dma_interrupt(priv);

The Agilex5 XGMAC is such a platform: socfpga_agilex5_setup_plat_dat() sets

	plat_dat->core_type = DWMAC_CORE_XGMAC;

and never sets STMMAC_FLAG_MULTI_MSI_EN, so it uses the single-IRQ path and
is one of the two hwif entries switched to &dwxgmac2_ptp here.  Its
cross-timestamp path detects snapshot completion by polling the very bit
that this read clears:

drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:smtg_crosststamp() {
	...
	ret = readl_poll_timeout(priv->ioaddr + XGMAC_INT_STATUS, v,
				 (v & XGMAC_INT_TSIS), 100, 10000);
	...
}

Can this handler consume TSIS behind smtg_crosststamp(), so that
PTP_SYS_OFFSET_PRECISE fails with -ETIMEDOUT and a netdev_err, or reads
ATSNS == 0 and returns a zero device timestamp?  With the TSIE unmasking in
XGMAC_INT_DEFAULT_EN above, plus one handler run per RX/TX interrupt on the
shared IRQ, this looks like it would happen under ordinary traffic.

The later patch in the series ("net: stmmac: dwmac-socfpga: mask XGMAC_TSIE
during cross-timestamp") masks XGMAC_TSIE around the snapshot, but at the
end of the series dwxgmac2_timestamp_interrupt() still reads 0xd20
unconditionally, so any other MAC or DMA interrupt during the up-to-10 ms
poll window still clears TSIS.

Would gating this read on XGMAC_INT_STATUS & XGMAC_INT_TSIS, as the dwmac4
handler does, close that window instead?

> +
> +	if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN))
> +		return;
> +
> +	pending_snapshots = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, ts_status);
> +	if (!pending_snapshots)
> +		return;
> +
> +	acr_value = readl(priv->ptpaddr + PTP_ACR);
> +	channel = FIELD_GET(PTP_ACR_MASK, acr_value);
> +	if (!channel)
> +		return;
> +	channel = ilog2(channel);
> +
> +	for (i = 0; i < pending_snapshots; i++) {
> +		read_lock_irqsave(&priv->ptp_lock, flags);
> +		stmmac_get_ptptime(priv, priv->ptpaddr, &ptp_time);
> +		read_unlock_irqrestore(&priv->ptp_lock, flags);

[Severity: Medium]
The ATSNS count and the PTP_ACR channel mask are sampled once, and the
auxiliary snapshot FIFO is then popped pending_snapshots times while holding
only a read lock on priv->ptp_lock, which protects the systime/addend
registers and does not exclude other readers.

struct stmmac_priv documents a different lock for these registers:

	/* Protects auxiliary snapshot registers from concurrent access. */
	struct mutex aux_ts_lock;

The process-context writer publishes the enable flag and issues the FIFO
clear under that mutex, then waits for completion after dropping it:

drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c:stmmac_enable() {
	...
			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;

			/* Enable External snapshot trigger */
			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
			acr_value |= PTP_ACR_ATSFC;
	...
		mutex_unlock(&priv->aux_ts_lock);
		/* wait for auxts fifo clear to finish */
		ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
					 !(acr_value & PTP_ACR_ATSFC),
					 10, 10000);
	...
}

Since aux_ts_lock is a mutex and this handler runs in hardirq, the two paths
share no lock at all.  If a timestamp interrupt lands in that window, can it
see the already-published STMMAC_FLAG_EXT_SNAPSHOT_EN together with a stale
ATSNS count or channel mask and drain a FIFO that is being flushed or
re-channelled, reporting stale or mis-indexed PTP_CLOCK_EXTTS events?

PTP_ACR is also read only once outside the loop, so a channel change part
way through the loop would be reported under the old index.

The same unlocked pattern exists in the dwmac4 and dwmac1000 handlers, but
this patch creates the new code and makes the path reachable on DWXGMAC2 and
DWXLGMAC2 by unmasking XGMAC_TSIE and wiring the handler in.

> +
> +		event.type = PTP_CLOCK_EXTTS;
> +		event.index = channel;
> +		event.timestamp = ptp_time;
> +		ptp_clock_event(priv->ptp_clock, &event);
> +	}
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902131441.322167-1-zxyan0222%40gmail.com

Re: [PATCH net-next v4 2/2] net: stmmac: dwmac-socfpga: mask XGMAC_TSIE during cross-timestamp

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 2 · Low: 0

- [High] The fix does not close the race it claims to close: masking
  XGMAC_INT_EN.TSIE does not prevent the destructive read of…
- [Medium] Series ordering leaves a knowingly broken intermediate commit
  and an undocumented backport dependency.
- [Medium] The new mask/unmask pair in smtg_crosststamp() is neither
  state-preserving nor nestable.

Pre-existing issues:
- [High] smtg_crosststamp() uses the aggregate TSIS bit as its completion
  condition without ever clearing it before arming the snapshot, and…

--

net: stmmac: dwmac-socfpga: mask XGMAC_TSIE during cross-timestamp

This masks the XGMAC timestamp interrupt enable bit (XGMAC_TSIE) around the
auxiliary snapshot trigger and FIFO read in smtg_crosststamp(), and restores
it on the -EINVAL, timeout and success return paths.  The stated goal is to
keep the DWXGMAC2 timestamp interrupt handler from clearing TSIS while the
poll loop in smtg_crosststamp() is waiting for it.

No Link: tags are present in the commit header.

Before the diff, a couple of notes about the commit message and the series
ordering.

[Severity: Medium]

The commit message says:

  "Once XGMAC_TSIE is unmasked (done by a companion change that enables it
  in XGMAC_INT_DEFAULT_EN)"

Could that companion change be named explicitly?  The preceding commit in
this series, f4fbb4b15db4 ("net: stmmac: dwxgmac2: add XGMAC timestamp
interrupt support"), is the one that adds XGMAC_TSIE to
XGMAC_INT_DEFAULT_EN (at the baseline the macro is only
(XGMAC_LPIIE | XGMAC_PMTIE)).

That ordering means a bisect that lands on f4fbb4b15db4 alone has Agilex5
PTP_SYS_OFFSET_PRECISE failing with the exact message quoted here.  Since
f4fbb4b15db4 fixes a user-visible problem on its own, it looks like a
plausible stable backport candidate, and neither commit carries a Fixes: or
dependency tag.

Reversing the order does not help either, because the exit paths added here
unconditionally set XGMAC_TSIE, which would enable it on trees where the
default mask deliberately omits it.  Would folding this masking into
f4fbb4b15db4 (or at minimum adding "Fixes: f4fbb4b15db4" and naming the
dependency) be preferable?

Separately, the driver already has a generic mechanism for this exact
problem: STMMAC_FLAG_INT_SNAPSHOT_EN together with priv->tstamp_busy_wait,
as used by intel_crosststamp() in dwmac-intel.c.  Could the commit message
explain why a second, platform-private mechanism is added instead of reusing
that one?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 1d7f0a57d288..a4d00bf81423 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -310,6 +310,13 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
>  		return -EBUSY;
>  
> +	/* The XGMAC timestamp interrupt handler clears TSIS by reading
> +	 * XGMAC_TIMESTAMP_STATUS, which would race with the TSIS poll
> +	 * below.  Mask XGMAC_TSIE for the duration of the cross-timestamp
> +	 * so the handler does not run while we own the snapshot FIFO.
> +	 */
> +	stmmac_mac_irq_modify(priv, XGMAC_TSIE, 0);
> +

[Severity: High]

Does masking XGMAC_TSIE actually stop the read that clears TSIS?

dwmac-socfpga does not set STMMAC_FLAG_MULTI_MSI_EN, so Agilex5 runs the
legacy shared handler, and the dispatch to the timestamp callback is not
gated on TSIS or on XGMAC_INT_EN:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_interrupt() {
	...
	stmmac_common_interrupt(priv);
	...
}

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_common_interrupt() {
	...
	stmmac_timestamp_interrupt(priv, priv);
	...
}

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_timestamp_interrupt() {
	ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
	...
}

The clear-on-read of XGMAC_TIMESTAMP_STATUS is the first statement of
dwxgmac2_timestamp_interrupt(), before the STMMAC_FLAG_EXT_SNAPSHOT_EN early
return.  Since stmmac_common_interrupt() is reached on every device
interrupt, including ordinary RX/TX DMA completions, wouldn't any interrupt
during the up-to-10 ms readl_poll_timeout() window still clear TSIS and make
the poll return -ETIMEDOUT with the same "Wait for time sync operation
timeout" message?

There is a second consumer of the same clear-on-read register that no
interrupt mask can affect:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_get_tx_hwtstamp() {
	...
	} else if (!stmmac_get_mac_tx_timestamp(priv, priv->hw, &ns)) {
	...
}

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_get_mac_tx_timestamp() {
	if (readl_poll_timeout_atomic(ioaddr + XGMAC_TIMESTAMP_STATUS,
				      value, value & XGMAC_TXTSC, 100, 10000))
	...
}

That runs from the TX completion path whenever TX hardware timestamping is
enabled, which is the configuration in which cross-timestamping is used.

Two smaller points on the same mechanism.  There is no synchronize_irq()
after the mask, so a handler already in flight on another CPU still performs
the destructive read.  And dwxgmac2_core_init() writes the whole enable mask
directly:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_core_init() {
	...
	writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
	...
}

which now includes XGMAC_TSIE and bypasses hw->irq_ctrl_lock, so it can
re-enable TSIE inside the masked window.

Would gating the destructive read in software (an ownership flag, as the
Intel path does with STMMAC_FLAG_INT_SNAPSHOT_EN), or keying completion off
the persistent ATSNS count rather than the transient TSIS bit, replace this
approach rather than extend it?

>  	mutex_lock(&priv->aux_ts_lock);
>  	/* Enable Internal snapshot trigger */
>  	acr_value = readl(ptpaddr + PTP_ACR);
> @@ -329,6 +336,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  		break;
>  	default:
>  		mutex_unlock(&priv->aux_ts_lock);
> +		stmmac_mac_irq_modify(priv, 0, XGMAC_TSIE);
>  		return -EINVAL;
>  	}
>  	writel(acr_value, ptpaddr + PTP_ACR);

[Severity: Medium]

Is the restore actually a restore?  The acquire in smtg_crosststamp() does
not sample XGMAC_INT_EN, and dwxgmac2_irq_modify() in dwxgmac2_core.c is a
plain read-modify-write with no notion of a previous value or of nested
owners:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_irq_modify() {
	...
	value = readl(int_mask) & ~disable;
	value |= enable;
	...
}

All three release sites here (this -EINVAL path, the timeout path and the
success path) unconditionally set XGMAC_TSIE.  If TSIE was deliberately
masked when smtg_crosststamp() was entered, does this callback silently
force it back on, so dwxgmac2_timestamp_interrupt() starts running and
consuming XGMAC_TIMESTAMP_STATUS on a configuration that never asked for it?

>  	/* Clear FIFO */
>  	acr_value = readl(ptpaddr + PTP_ACR);
>  	acr_value |= PTP_ACR_ATSFC;
>  	writel(acr_value, ptpaddr + PTP_ACR);
>  	/* Release the mutex */
>  	mutex_unlock(&priv->aux_ts_lock);
> @@ -353,6 +361,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  	ret = readl_poll_timeout(priv->ioaddr + XGMAC_INT_STATUS, v,
>  				 (v & XGMAC_INT_TSIS), 100, 10000);
>  	if (ret) {
> +		stmmac_mac_irq_modify(priv, 0, XGMAC_TSIE);
>  		netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
>  			   __func__);
>  		return ret;

[Severity: Medium]

Related to the point above: can a second concurrent caller end the first
caller's masked window here?

priv->aux_ts_lock is dropped right after the FIFO clear, well before this
poll, and the PTP chardev does not serialize the callback:

drivers/ptp/ptp_chardev.c:ptp_ioctl() {
	...
	case PTP_SYS_OFFSET_PRECISE:
	case PTP_SYS_OFFSET_PRECISE2:
		return ptp_sys_offset_precise(ptp, argptr,
					      ptp->info->getcrosststamp);
	...
}

ptp_sys_offset_precise() -> stmmac_getcrosststamp() ->
get_device_system_crosststamp() -> smtg_crosststamp(), and no FMODE_WRITE is
required.  If two threads issue PTP_SYS_OFFSET_PRECISE at the same time,
thread B's unconditional stmmac_mac_irq_modify(priv, 0, XGMAC_TSIE) on its
timeout or success path re-arms the interrupt while thread A is still inside
this poll.  Would sampling XGMAC_INT_EN and restoring exactly that value, or
serializing the whole cross-timestamp sequence, address both this and the
force-enable case?

>  	}
> @@ -375,6 +384,8 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  		read_unlock_irqrestore(&priv->ptp_lock, flags);
>  	}
>  
> +	stmmac_mac_irq_modify(priv, 0, XGMAC_TSIE);
> +
>  	get_smtgtime(priv->mii, SMTG_MDIO_ADDR, &smtg_time);
>  	system->cycles = smtg_time;
>  

[Severity: High]

This isn't a bug introduced by this patch, but while looking at the TSIS
poll: can smtg_crosststamp() return 0 without ever writing *device?

TSIS is set by any MAC timestamp-module event, as the new handler from the
preceding commit itself documents:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_timestamp_interrupt() {
	/* ... TX timestamp polling may have already cleared TSIS
	 * and AUXTSTRIG, so rely on ATSNS instead. */
	...
}

smtg_crosststamp() flushes only the aux snapshot FIFO via PTP_ACR_ATSFC and
never reads XGMAC_TIMESTAMP_STATUS to drain a stale or foreign TSIS.
readl_poll_timeout() evaluates its condition on the first read, so a TSIS
latched by an unrelated event satisfies the poll immediately, possibly
before the auxiliary snapshot is latched.  Then:

	num_snapshot = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK,
				 readl(ioaddr + XGMAC_TIMESTAMP_STATUS));

	for (i = 0; i < num_snapshot; i++) {
		read_lock_irqsave(&priv->ptp_lock, flags);
		stmmac_get_ptptime(priv, ptpaddr, &ptp_time);
		*device = ns_to_ktime(ptp_time);
		read_unlock_irqrestore(&priv->ptp_lock, flags);
	}
	...
	return 0;

With num_snapshot == 0 the loop body, which is the only writer of *device,
never runs, yet the function returns 0.  ptp_sys_offset_precise() zeroes its
crosststamp struct:

drivers/ptp/ptp_chardev.c:ptp_sys_offset_precise() {
	struct system_device_crosststamp xtstamp = { .clock_id = CLOCK_REALTIME };
	...
	ts = ktime_to_timespec64(xtstamp.device);
	...
}

so userspace gets a device timestamp of 0 paired with a real system time and
a success return.  The latched snapshot also stays in the FIFO with ATSEN
still set until the next invocation's FIFO clear.

Would reading XGMAC_TIMESTAMP_STATUS once before arming to clear a stale
TSIS, and treating num_snapshot == 0 as an error rather than success, be the
right shape here?  Note that the masking added by this patch removes one of
the consumers that would otherwise have drained a stale TSIS, and it keeps
TSIS rather than the persistent ATSNS count as the completion condition.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902131441.322167-1-zxyan0222%40gmail.com