[PATCH net-next v10 0/3] net: stmmac: dwxgmac2: timestamp interrupt support + Agilex5 fix

[PATCH net-next v10 0/3] net: stmmac: dwxgmac2: timestamp interrupt support + Agilex5 fix

[Zxyan Zhu]

This series adds auxiliary snapshot (EXTTS) interrupt support to
DWXGMAC2/DWXLGMAC2, fixes a stale TSIS race on the Agilex5
cross-timestamp path that the new handler would otherwise expose, and
guards the shared aux snapshot handler against a zero channel mask.

Patch 1 makes smtg_crosststamp() complete on the persistent ATSNS
count instead of the transient TSIS bit, waits for the FIFO clear to
complete, and holds aux_ts_lock across the whole trigger/poll/drain
sequence so a concurrent EXTTS request cannot flush the snapshot FIFO
mid-flight.

Patch 2 guards the shared aux snapshot handler against a zero channel
mask: ilog2() is applied to the mask without a zero check, and
ilog2(0) yields an out-of-range event index that ptp_clock_event()
feeds to test_bit() unchecked from hard IRQ context.

Patch 3 adds a dedicated DWXGMAC2 timestamp interrupt handler.  The
XGMAC hwif entries used the generic ops, whose callback read the wrong
register and whose enable mask omitted the timestamp interrupt, so the
clock advertised the aux snapshot channels but no event was ever
delivered.  XGMAC_TSIE is not enabled by default; it is armed on demand
from the EXTTS enable/disable path through a new callback, and the
handler only touches the FIFO while the interrupt is armed and while no
cross-timestamp owns it.  The interrupt is disarmed on suspend and
unregister and re-armed on resume.

The failure fixed by patch 3 was hit while bringing up auxiliary
snapshot interrupts on a local DWXGMAC2 board (not Agilex5-class):
enabling an EXTTS channel succeeded but no PTP_CLOCK_EXTTS event ever
arrived.  After the change the same board delivers EXTTS events on the
enabled channel.  The races fixed by patches 1 and 2 were found by
reviewing the code paths that this bring-up exposed; they are narrow
race windows and were not reproduced on hardware in isolation.

---

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: https://lore.kernel.org/netdev/20260902131441.322167-1-zxyan0222@gmail.com/
v5: https://lore.kernel.org/netdev/20260910081020.86227-1-zxyan0222@gmail.com/
v6: https://lore.kernel.org/netdev/20260929073553.4136336-1-zxyan0222@gmail.com/
v7: https://lore.kernel.org/netdev/20261004120526.2728564-1-zxyan0222@gmail.com/
v8: https://lore.kernel.org/netdev/20261013033207.1684328-1-zxyan0222@gmail.com/
v9: https://lore.kernel.org/netdev/20261104125030.1183146-1-zxyan0222@gmail.com/

v10:
- Rebase on net-next from 2026-11-09 (containing 7.4-rc2, which moves
  the est_lock initialisation next to the aux_ts_lock one).
- Wrap two lines that exceeded 80 columns.

-- 
2.34.1

[PATCH net-next v10 1/3] net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS

[Zxyan Zhu]

The Agilex5 smtg_crosststamp() handler polls XGMAC_INT_STATUS for TSIS
to learn that the auxiliary snapshot is latched.  TSIS is a transient,
read-to-clear status bit, so the poll is racy in two ways: a stale TSIS
from an unrelated event satisfies it before the snapshot is latched,
leaving *device unwritten while the call reports success, and a
concurrent reader of XGMAC_TIMESTAMP_STATUS can clear it and make the
poll time out.

The FIFO level is also reported by the ATSNS count in
XGMAC_TIMESTAMP_STATUS, which reading the register does not disturb.
Poll ATSNS instead of TSIS, after waiting for the PTP_ACR_ATSFC FIFO
clear, so a stale ATSNS cannot satisfy the poll before the new snapshot
is latched.

Hold aux_ts_lock across the whole sequence, and refuse the request
with -EBUSY while a channel is enabled, so the PTP_ACR mask
programming cannot disarm it and a concurrent request cannot flush the
FIFO between the poll and the drain loop.  The lock is held across the
SMTG register reads as well, so a concurrent PTP_SYS_OFFSET_PRECISE
cannot retrigger GPO0 while they run.  The internal snapshot trigger
is disarmed on the way out, including the error paths, so it does not
stay armed in PTP_ACR after the cross-timestamp.

Fixes: fd8c4f645496 ("net: stmmac: socfpga: Add hardware supported cross-timestamp")
Cc: stable@vger.kernel.org
Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 52 +++++++++++++++----
 1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 6d4bc1fe8f75..2d3c6702e203 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -304,13 +304,15 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	int i, ret;
 	u32 v;
 
+	mutex_lock(&priv->aux_ts_lock);
 	/* Both internal crosstimestamping and external triggered event
-	 * timestamping cannot be run concurrently.
+	 * timestamping cannot be run concurrently: the PTP_ACR mask
+	 * programming below would disarm an external channel.
 	 */
-	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
+	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) {
+		mutex_unlock(&priv->aux_ts_lock);
 		return -EBUSY;
-
-	mutex_lock(&priv->aux_ts_lock);
+	}
 	/* Enable Internal snapshot trigger */
 	acr_value = readl(ptpaddr + PTP_ACR);
 	acr_value &= ~PTP_ACR_MASK;
@@ -337,8 +339,21 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	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);
+
+	/* Wait for the FIFO clear, so the poll only sees snapshots from
+	 * this trigger.
+	 */
+	ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
+				 !(acr_value & PTP_ACR_ATSFC), 10, 10000);
+	if (ret) {
+		acr_value = readl(ptpaddr + PTP_ACR);
+		acr_value &= ~PTP_ACR_MASK;
+		writel(acr_value, ptpaddr + PTP_ACR);
+		mutex_unlock(&priv->aux_ts_lock);
+		netdev_err(priv->dev, "%s: Failed to clear snapshot FIFO\n",
+			   __func__);
+		return ret;
+	}
 
 	/* Trigger Internal snapshot signal. Create a rising edge by just toggle
 	 * the GPO0 to low and back to high.
@@ -349,10 +364,17 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	gpio_value |= XGMAC_GPIO_GPO0;
 	writel(gpio_value, ioaddr + XGMAC_GPIO_STATUS);
 
-	/* Poll for time sync operation done */
-	ret = readl_poll_timeout(priv->ioaddr + XGMAC_INT_STATUS, v,
-				 (v & XGMAC_INT_TSIS), 100, 10000);
+	/* Poll ATSNS instead of the transient TSIS: the ATSNS count is
+	 * the FIFO level and is not cleared by reading the register.
+	 */
+	ret = readl_poll_timeout(ioaddr + XGMAC_TIMESTAMP_STATUS, v,
+				 FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v),
+				 100, 10000);
 	if (ret) {
+		acr_value = readl(ptpaddr + PTP_ACR);
+		acr_value &= ~PTP_ACR_MASK;
+		writel(acr_value, ptpaddr + PTP_ACR);
+		mutex_unlock(&priv->aux_ts_lock);
 		netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
 			   __func__);
 		return ret;
@@ -364,8 +386,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		.use_nsecs = false,
 	};
 
-	num_snapshot = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK,
-				 readl(ioaddr + XGMAC_TIMESTAMP_STATUS));
+	num_snapshot = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v);
 
 	/* Repeat until the timestamps are from the FIFO last segment */
 	for (i = 0; i < num_snapshot; i++) {
@@ -375,9 +396,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		read_unlock_irqrestore(&priv->ptp_lock, flags);
 	}
 
+	/* Disarm the internal snapshot trigger, so the PTP_ACR mask does
+	 * not stay armed behind the software state.
+	 */
+	acr_value = readl(ptpaddr + PTP_ACR);
+	acr_value &= ~PTP_ACR_MASK;
+	writel(acr_value, ptpaddr + PTP_ACR);
+
 	get_smtgtime(priv->mii, SMTG_MDIO_ADDR, &smtg_time);
 	system->cycles = smtg_time;
 
+	mutex_unlock(&priv->aux_ts_lock);
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH net-next v10 2/3] net: stmmac: guard against a zero channel in the aux snapshot handler

[Zxyan Zhu]

The generic timestamp_interrupt() handler derives the EXTTS channel
index with ilog2() applied directly to the PTP_ACR channel mask, with
no check for a zero mask.  ilog2(0) yields -1, which ends up in
event.index as 0xffffffff, and ptp_clock_event() uses that index in
test_bit() against a PTP_MAX_CHANNELS bitmap without range validation,
reading far past the allocation from hard IRQ context.

The zero-mask window is reachable: stmmac_enable() sets
STMMAC_FLAG_EXT_SNAPSHOT_EN before it programs PTP_ACR, so an EXTTS
interrupt arriving in between passes the flag check while the mask is
still clear (snapshots can also linger in the FIFO from a previous
enable, as only PTP_ACR_ATSFC clears them).

Check the mask before applying the ilog2().

Fixes: 8851346912a1 ("net: stmmac: Assign configured channel value to EXTTS event")
Cc: stable@vger.kernel.org
Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index 870c740b4a50..fd1142344950 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -245,7 +245,10 @@ static void timestamp_interrupt(struct stmmac_priv *priv)
 		       GMAC_TIMESTAMP_ATSNS_SHIFT;
 
 	acr_value = readl(priv->ptpaddr + PTP_ACR);
-	channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
+	channel = FIELD_GET(PTP_ACR_MASK, acr_value);
+	if (!channel)
+		return;
+	channel = ilog2(channel);
 
 	for (i = 0; i < num_snapshot; i++) {
 		read_lock_irqsave(&priv->ptp_lock, flags);
-- 
2.34.1

[PATCH net-next v10 3/3] net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

[Zxyan Zhu]

The DWXGMAC2 hwif entries use the generic stmmac_ptp hwtimestamp ops,
whose timestamp_interrupt callback reads the dwmac4 register instead of
the XGMAC one and whose enable mask omits XGMAC_TSIE.  The PTP clock
still advertises the auxiliary snapshot channels, so PTP_EXTTS_REQUEST
succeeds but no PTP_CLOCK_EXTTS event is ever delivered.

Add a DWXGMAC2 handler that reads XGMAC_TIMESTAMP_STATUS and reports
the pending auxiliary snapshots as PTP_CLOCK_EXTTS events.  It
deliberately does not gate on XGMAC_INT_TSIS: TSIS is an aggregate,
read-to-clear bit that the TX timestamp completion path clears before
the handler can observe it, while the ATSNS count and the FIFO still
hold the events, so a dwmac4-style TSIS gate would drop them.

XGMAC_TSIE is not re-added to XGMAC_INT_DEFAULT_EN.  It is armed on
demand from the PTP_CLK_REQ_EXTTS enable/disable path through a new
optional stmmac_ops->timestamp_interrupt_cfg() callback implemented
only for DWXGMAC2/DWXLGMAC2, since stmmac_enable() is shared with
dwmac4/dwmac5, whose timestamp interrupt stays always-enabled and is
relied upon by intel_crosststamp().  The driver keeps a single
enabled EXTTS channel at a time: an enable for a channel other than the
armed one is rejected with -EBUSY, a disable for a channel other than
the armed one is a no-op, and re-enabling the armed channel stays
idempotent, disarming the interrupt across the FIFO clear like a first
enable.  stmmac_ptp_register() clamps the advertised channel count to
the four PTP_ACR ATSEN bits, which also covers any core whose
AUXSNAPNUM capability reports more than four auxiliary snapshots;
n_ext_ts reports the hardware channels, the driver serialises their
use.  Requests are also refused while an internal cross-timestamp
owns the FIFO, so the PTP_ACR programming cannot disarm its trigger.
intel_crosststamp() holds aux_ts_lock across its whole body, like
smtg_crosststamp(), and disarms its internal snapshot trigger on the
way out, on a timeout and on an interrupted wait alike, so it cannot
wipe a channel that stmmac_enable() just armed, nor leave the mask
armed behind the software state; it also refuses to run once the clock
has been invalidated by unregister, and skips the disarm write
afterwards.

The handler drains the FIFO only while XGMAC_TSIE is enabled: it runs
on every MAC interrupt, and PTP_ACR_ATSFC is only ever set while
XGMAC_TSIE is disabled, so this both keeps it off the FIFO on unrelated
interrupts and stops it racing a FIFO flush.  The interrupt is armed
only after the FIFO clear has completed, with aux_ts_lock held across
the whole enable so the arm cannot interleave with a disarm under the
same lock.
A failed enable is rolled back in hardware and software, leaving the
interrupt disarmed.

While an internal cross-timestamp owns the FIFO
(STMMAC_FLAG_INT_SNAPSHOT_EN, raised and dropped under aux_ts_lock),
the handler only clears the interrupt source.  smtg_crosststamp() calls
stmmac_sync_timestamp_irq() after raising the flag and the disable path
calls it after disarming, so no handler that is already past its check
can still use the FIFO.

aux_ts_lock is initialised in probe and destroyed in remove, so it is
valid for the whole device lifetime.  At unregister the clock is
invalidated under the lock before ptp_clock_unregister(), after which
the enable path and smtg_crosststamp() refuse to arm.

On resume stmmac_rearm_timestamp_irq() reprograms the enabled channel
under aux_ts_lock and re-arms the interrupt only if the FIFO flush
completed; otherwise, and whenever the timestamping setup cannot be
redone, the recorded EXTTS channel is dropped and its trigger
disarmed, so no stale channel survives.  Both that drop and the
unregister disarm the hardware trigger as well: PTP_ACR on the cores
whose EXTTS requests go through stmmac_enable(), and PTP_TCR on
MAC100/GMAC, whose requests go through dwmac1000_ptp_enable(), so a
leftover or re-armed trigger bit cannot reject later requests or keep
the FIFO latching behind the software state.
stmmac_suspend() disarms the interrupt and drains an in-flight handler
under aux_ts_lock before the clocks are gated.

Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c |  46 ++++--
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |  22 ++-
 .../ethernet/stmicro/stmmac/dwxgmac2_core.c   |  62 ++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.c    |   4 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |   6 +
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |   2 +
 .../ethernet/stmicro/stmmac/stmmac_hwtstamp.c |  12 ++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 138 +++++++++++++++++-
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 107 +++++++++++---
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.h  |   2 +
 include/linux/stmmac.h                        |   1 +
 11 files changed, 363 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 4308dccbf257..3f0889c30a70 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -370,19 +370,23 @@ static int intel_crosststamp(ktime_t *device,
 	u32 num_snapshot;
 	u32 gpio_value;
 	u32 acr_value;
-	int i;
+	int ret, i;
 
 	intel_priv = priv->plat->bsp_priv;
 
+	mutex_lock(&priv->aux_ts_lock);
 	/* Both internal crosstimestamping and external triggered event
-	 * timestamping cannot be run concurrently.
+	 * timestamping cannot be run concurrently: the PTP_ACR mask
+	 * programming below would disarm an external channel.
 	 */
-	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
-		return -EBUSY;
+	if ((priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) ||
+	    !priv->ptp_clock) {
+		mutex_unlock(&priv->aux_ts_lock);
+		return priv->ptp_clock ? -EBUSY : -ENODEV;
+	}
 
 	priv->plat->flags |= STMMAC_FLAG_INT_SNAPSHOT_EN;
 
-	mutex_lock(&priv->aux_ts_lock);
 	/* Enable Internal snapshot trigger */
 	acr_value = readl(ptpaddr + PTP_ACR);
 	acr_value &= ~PTP_ACR_MASK;
@@ -400,8 +404,8 @@ static int intel_crosststamp(ktime_t *device,
 		acr_value |= PTP_ACR_ATSEN3;
 		break;
 	default:
-		mutex_unlock(&priv->aux_ts_lock);
 		priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
+		mutex_unlock(&priv->aux_ts_lock);
 		return -EINVAL;
 	}
 	writel(acr_value, ptpaddr + PTP_ACR);
@@ -410,8 +414,6 @@ static int intel_crosststamp(ktime_t *device,
 	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);
 
 	/* Trigger Internal snapshot signal
 	 * Create a rising edge by just toggle the GPO1 to low
@@ -424,11 +426,22 @@ static int intel_crosststamp(ktime_t *device,
 	writel(gpio_value, ioaddr + GMAC_GPIO_STATUS);
 
 	/* Time sync done Indication - Interrupt method */
-	if (!wait_event_interruptible_timeout(priv->tstamp_busy_wait,
-					      stmmac_cross_ts_isr(priv),
-					      HZ / 100)) {
+	ret = wait_event_interruptible_timeout(priv->tstamp_busy_wait,
+					       stmmac_cross_ts_isr(priv),
+					       HZ / 100);
+	if (ret <= 0) {
+		/* Disarm the trigger on a timeout or an interrupted
+		 * wait, but do not touch the hardware once the clock has
+		 * been invalidated by an interface teardown.
+		 */
+		if (priv->ptp_clock) {
+			acr_value = readl(ptpaddr + PTP_ACR);
+			acr_value &= ~PTP_ACR_MASK;
+			writel(acr_value, ptpaddr + PTP_ACR);
+		}
 		priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
-		return -ETIMEDOUT;
+		mutex_unlock(&priv->aux_ts_lock);
+		return ret ? ret : -ETIMEDOUT;
 	}
 
 	*system = (struct system_counterval_t) {
@@ -453,7 +466,16 @@ static int intel_crosststamp(ktime_t *device,
 
 	system->cycles *= intel_priv->crossts_adj;
 
+	/* Disarm the internal snapshot trigger, so it does not stay armed
+	 * in PTP_ACR after the cross-timestamp.
+	 */
+	if (priv->ptp_clock) {
+		acr_value = readl(ptpaddr + PTP_ACR);
+		acr_value &= ~PTP_ACR_MASK;
+		writel(acr_value, ptpaddr + PTP_ACR);
+	}
 	priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
+	mutex_unlock(&priv->aux_ts_lock);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 2d3c6702e203..212aabbf1a19 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -305,14 +305,21 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	u32 v;
 
 	mutex_lock(&priv->aux_ts_lock);
-	/* Both internal crosstimestamping and external triggered event
-	 * timestamping cannot be run concurrently: the PTP_ACR mask
-	 * programming below would disarm an external channel.
+	/* Internal crosstimestamping and external triggered event
+	 * timestamping cannot run concurrently: the PTP_ACR mask
+	 * programming below would disarm an external channel.  Refuse to
+	 * arm once the clock has been invalidated by unregister.
 	 */
-	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) {
+	if ((priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) ||
+	    !priv->ptp_clock) {
 		mutex_unlock(&priv->aux_ts_lock);
-		return -EBUSY;
+		return priv->ptp_clock ? -EBUSY : -ENODEV;
 	}
+	priv->plat->flags |= STMMAC_FLAG_INT_SNAPSHOT_EN;
+	/* Wait for a handler that is already past the flag check before
+	 * taking over the snapshot FIFO.
+	 */
+	stmmac_sync_timestamp_irq(priv);
 	/* Enable Internal snapshot trigger */
 	acr_value = readl(ptpaddr + PTP_ACR);
 	acr_value &= ~PTP_ACR_MASK;
@@ -330,6 +337,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		acr_value |= PTP_ACR_ATSEN3;
 		break;
 	default:
+		priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
 		mutex_unlock(&priv->aux_ts_lock);
 		return -EINVAL;
 	}
@@ -349,6 +357,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		acr_value = readl(ptpaddr + PTP_ACR);
 		acr_value &= ~PTP_ACR_MASK;
 		writel(acr_value, ptpaddr + PTP_ACR);
+		priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
 		mutex_unlock(&priv->aux_ts_lock);
 		netdev_err(priv->dev, "%s: Failed to clear snapshot FIFO\n",
 			   __func__);
@@ -374,6 +383,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		acr_value = readl(ptpaddr + PTP_ACR);
 		acr_value &= ~PTP_ACR_MASK;
 		writel(acr_value, ptpaddr + PTP_ACR);
+		priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
 		mutex_unlock(&priv->aux_ts_lock);
 		netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
 			   __func__);
@@ -403,6 +413,8 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	acr_value &= ~PTP_ACR_MASK;
 	writel(acr_value, ptpaddr + PTP_ACR);
 
+	priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
+
 	get_smtgtime(priv->mii, SMTG_MDIO_ADDR, &smtg_time);
 	system->cycles = smtg_time;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 1a88cbaed70c..ff2956bb89e6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -42,6 +42,12 @@ static void dwxgmac2_irq_modify(struct mac_device_info *hw, u32 disable,
 	spin_unlock_irqrestore(&hw->irq_ctrl_lock, flags);
 }
 
+static void dwxgmac2_timestamp_interrupt_cfg(struct stmmac_priv *priv, bool en)
+{
+	stmmac_mac_irq_modify(priv, en ? 0 : XGMAC_TSIE,
+			      en ? XGMAC_TSIE : 0);
+}
+
 static void dwxgmac2_update_caps(struct stmmac_priv *priv)
 {
 	if (!priv->dma_cap.mbps_10_100)
@@ -1154,6 +1160,60 @@ 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;
+
+	/* Runs on every MAC interrupt: only touch the FIFO when the
+	 * timestamp interrupt was armed, and leave it to the
+	 * cross-timestamp path while that owns it.
+	 */
+	if (priv->plat->flags & STMMAC_FLAG_INT_SNAPSHOT_EN) {
+		readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
+		return;
+	}
+
+	if (!(readl(priv->ioaddr + XGMAC_INT_EN) & XGMAC_TSIE))
+		return;
+
+	/* Reading XGMAC_TIMESTAMP_STATUS clears TSIS, so use the ATSNS
+	 * count to see the pending snapshots.
+	 */
+	ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
+
+	if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) ||
+	    !priv->ptp_clock)
+		return;
+
+	pending_snapshots = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, ts_status);
+	if (!pending_snapshots)
+		return;
+
+	acr_value = readl(priv->ptpaddr + PTP_ACR);
+	/* Entries observed while the FIFO is being flushed are stale. */
+	if (acr_value & PTP_ACR_ATSFC)
+		return;
+	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)
@@ -1413,6 +1473,7 @@ static int dwxgmac2_config_l4_filter(struct mac_device_info *hw, u32 filter_no,
 const struct stmmac_ops dwxgmac210_ops = {
 	.core_init = dwxgmac2_core_init,
 	.irq_modify = dwxgmac2_irq_modify,
+	.timestamp_interrupt_cfg = dwxgmac2_timestamp_interrupt_cfg,
 	.update_caps = dwxgmac2_update_caps,
 	.set_mac = dwxgmac2_set_mac,
 	.rx_ipc = dwxgmac2_rx_ipc,
@@ -1468,6 +1529,7 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
 const struct stmmac_ops dwxlgmac2_ops = {
 	.core_init = dwxgmac2_core_init,
 	.irq_modify = dwxgmac2_irq_modify,
+	.timestamp_interrupt_cfg = dwxgmac2_timestamp_interrupt_cfg,
 	.set_mac = dwxgmac2_set_mac,
 	.rx_ipc = dwxgmac2_rx_ipc,
 	.rx_queue_enable = dwxlgmac2_rx_queue_enable,
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 8ae95e91a36a..f077c1465a0c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -317,6 +317,8 @@ struct stmmac_ops {
 	void (*update_caps)(struct stmmac_priv *priv);
 	/* Change the interrupt enable setting. Enable takes precedence. */
 	void (*irq_modify)(struct mac_device_info *hw, u32 disable, u32 enable);
+	/* Arm or disarm the timestamp interrupt on demand (optional) */
+	void (*timestamp_interrupt_cfg)(struct stmmac_priv *priv, bool en);
 	/* Enable the MAC RX/TX */
 	void (*set_mac)(void __iomem *ioaddr, bool enable);
 	/* Enable and verify that the IPC module is supported */
@@ -420,6 +422,9 @@ struct stmmac_ops {
 	stmmac_do_void_callback(__priv, mac, update_caps, __priv)
 #define stmmac_mac_irq_modify(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mac, irq_modify, (__priv)->hw, __args)
+#define stmmac_mac_timestamp_interrupt_cfg(__priv, __args...) \
+	stmmac_do_void_callback(__priv, mac, timestamp_interrupt_cfg, \
+				__priv, __args)
 #define stmmac_mac_set(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mac, set_mac, __args)
 #define stmmac_rx_ipc(__priv, __args...) \
@@ -675,6 +680,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.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index b76acfe0d958..5cde6720590e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -410,6 +410,8 @@ int stmmac_mdio_register(struct net_device *ndev);
 int stmmac_mdio_reset(struct mii_bus *mii);
 void stmmac_mdio_lock(struct stmmac_priv *priv);
 void stmmac_mdio_unlock(struct stmmac_priv *priv);
+void stmmac_sync_timestamp_irq(struct stmmac_priv *priv);
+void stmmac_drop_hw_extts(struct stmmac_priv *priv);
 int stmmac_pcs_setup(struct net_device *ndev);
 void stmmac_pcs_clean(struct net_device *ndev);
 void stmmac_set_ethtool_ops(struct net_device *netdev);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index fd1142344950..40476fa32ec1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -283,3 +283,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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a2177777727e..1abc7c696cf7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -18,6 +18,7 @@
 #include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/ip.h>
 #include <linux/tcp.h>
 #include <linux/skbuff.h>
@@ -895,6 +896,117 @@ static int stmmac_init_ptp_clk_freq(struct stmmac_priv *priv)
 	return 0;
 }
 
+/**
+ * stmmac_sync_timestamp_irq - wait for an in-flight timestamp handler
+ * @priv: driver private structure
+ * Description: called after the timestamp interrupt has been disarmed or
+ * handed over, to make sure a handler that is already past its checks has
+ * finished before the caller takes over the auxiliary snapshot FIFO.
+ */
+void stmmac_sync_timestamp_irq(struct stmmac_priv *priv)
+{
+	synchronize_irq(priv->dev->irq);
+	if (priv->wol_irq > 0 && priv->wol_irq != priv->dev->irq)
+		synchronize_irq(priv->wol_irq);
+}
+EXPORT_SYMBOL_GPL(stmmac_sync_timestamp_irq);
+
+/**
+ * stmmac_drop_hw_extts - disarm the recorded EXTTS channel in hardware
+ * @priv: driver private structure
+ * Description: clears the auxiliary snapshot trigger, in PTP_ACR for
+ * the cores whose EXTTS requests go through stmmac_enable() and in
+ * PTP_TCR for MAC100/GMAC, whose requests go through
+ * dwmac1000_ptp_enable().
+ */
+void stmmac_drop_hw_extts(struct stmmac_priv *priv)
+{
+	u32 value;
+
+	if (priv->plat->core_type == DWMAC_CORE_MAC100 ||
+	    priv->plat->core_type == DWMAC_CORE_GMAC) {
+		value = readl(priv->ptpaddr + PTP_TCR);
+		value &= ~GMAC_PTP_TCR_ATSEN0;
+		writel(value, priv->ptpaddr + PTP_TCR);
+		return;
+	}
+
+	value = readl(priv->ptpaddr + PTP_ACR);
+	value &= ~PTP_ACR_MASK;
+	writel(value, priv->ptpaddr + PTP_ACR);
+}
+
+/**
+ * stmmac_drop_extts_state - drop the recorded EXTTS channel
+ * @priv: driver private structure
+ * Description: clears the recorded EXTTS channel and disarms its
+ * trigger, so no event is reported for it and a later enable starts
+ * from a clean state.
+ */
+static void stmmac_drop_extts_state(struct stmmac_priv *priv)
+{
+	mutex_lock(&priv->aux_ts_lock);
+	stmmac_drop_hw_extts(priv);
+	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+	priv->plat->ext_snapshot_num = -1;
+	mutex_unlock(&priv->aux_ts_lock);
+	netdev_err(priv->dev,
+		   "Dropped the enabled auxiliary snapshot channel\n");
+}
+
+/**
+ * stmmac_rearm_timestamp_irq - re-arm the timestamp interrupt
+ * @priv: driver private structure
+ * Description: this re-arms the on-demand timestamp interrupt if an
+ * auxiliary snapshot channel was left enabled.
+ */
+static void stmmac_rearm_timestamp_irq(struct stmmac_priv *priv)
+{
+	u32 acr_value;
+	int ret, num;
+
+	if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) ||
+	    !priv->ptp_clock)
+		return;
+
+	/* Flush the FIFO, re-program the enabled auxiliary snapshot
+	 * trigger and re-arm the interrupt only if the flush completed.
+	 */
+	if (priv->plat->core_type != DWMAC_CORE_XGMAC)
+		return;
+
+	mutex_lock(&priv->aux_ts_lock);
+	/* Snapshot the channel under the lock: a concurrent disable may
+	 * have cleared it.
+	 */
+	num = priv->plat->ext_snapshot_num;
+	if (num < 0) {
+		mutex_unlock(&priv->aux_ts_lock);
+		return;
+	}
+	acr_value = readl(priv->ptpaddr + PTP_ACR);
+	acr_value &= ~PTP_ACR_MASK;
+	acr_value |= PTP_ACR_ATSFC;
+	writel(acr_value, priv->ptpaddr + PTP_ACR);
+	ret = readl_poll_timeout(priv->ptpaddr + PTP_ACR, acr_value,
+				 !(acr_value & PTP_ACR_ATSFC), 10, 10000);
+	if (!ret) {
+		acr_value |= PTP_ACR_ATSEN(num);
+		writel(acr_value, priv->ptpaddr + PTP_ACR);
+		stmmac_mac_timestamp_interrupt_cfg(priv, true);
+	} else {
+		/* The trigger could not be restored, so drop the software
+		 * state rather than advertise a dead channel.
+		 */
+		priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+		priv->plat->ext_snapshot_num = -1;
+		netdev_err(priv->dev,
+			   "%s: Failed to restore auxiliary snapshot channel\n",
+			   __func__);
+	}
+	mutex_unlock(&priv->aux_ts_lock);
+}
+
 /**
  * stmmac_init_timestamping - initialise timestamping
  * @priv: driver private structure
@@ -954,6 +1066,9 @@ static int stmmac_setup_ptp(struct stmmac_priv *priv)
 		return 0;
 	}
 
+	/* No EXTTS channel is recorded until one is enabled. */
+	priv->plat->ext_snapshot_num = -1;
+
 	ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
 	if (ret < 0) {
 		netdev_warn(priv->dev,
@@ -8246,6 +8361,7 @@ static int __stmmac_dvr_probe(struct device *device,
 
 	mutex_init(&priv->lock);
 	mutex_init(&priv->est_lock);
+	mutex_init(&priv->aux_ts_lock);
 	rwlock_init(&priv->ptp_lock);
 
 	stmmac_fpe_init(priv);
@@ -8380,6 +8496,7 @@ void stmmac_dvr_remove(struct device *dev)
 	destroy_workqueue(priv->wq);
 	mutex_destroy(&priv->lock);
 	mutex_destroy(&priv->est_lock);
+	mutex_destroy(&priv->aux_ts_lock);
 	bitmap_free(priv->af_xdp_zc_qps);
 
 	pm_runtime_disable(dev);
@@ -8434,6 +8551,17 @@ int stmmac_suspend(struct device *dev)
 		pinctrl_pm_select_sleep_state(priv->device);
 	}
 
+	/* Disarm the timestamp interrupt while suspended and drain an
+	 * in-flight handler before the clocks are gated;
+	 * stmmac_rearm_timestamp_irq() restores it on resume.
+	 */
+	mutex_lock(&priv->aux_ts_lock);
+	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) {
+		stmmac_mac_timestamp_interrupt_cfg(priv, false);
+		stmmac_sync_timestamp_irq(priv);
+	}
+	mutex_unlock(&priv->aux_ts_lock);
+
 	mutex_unlock(&priv->lock);
 
 	rtnl_lock();
@@ -8556,16 +8684,22 @@ int stmmac_resume(struct device *dev)
 	}
 
 	if (priv->ptp_enabled) {
-		if (stmmac_init_ptp_clk_freq(priv))
+		if (stmmac_init_ptp_clk_freq(priv)) {
+			stmmac_drop_extts_state(priv);
 			goto init_coalesce;
+		}
 
 		ret = stmmac_init_timestamping(priv);
-		if (ret)
+		if (ret) {
+			stmmac_drop_extts_state(priv);
 			goto error_stop_dma;
+		}
 
 		ret = stmmac_setup_est(priv);
 		if (ret)
 			goto error_stop_dma;
+
+		stmmac_rearm_timestamp_irq(priv);
 	}
 
 init_coalesce:
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index a37405534beb..4486c800fa71 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -201,35 +201,98 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 		u8 channel;
 
 		mutex_lock(&priv->aux_ts_lock);
+		/* Refuse to arm once the clock has been invalidated by
+		 * unregister, or while an internal cross-timestamp owns the
+		 * FIFO: the PTP_ACR programming below would disarm its
+		 * trigger.
+		 */
+		if (!priv->ptp_clock) {
+			mutex_unlock(&priv->aux_ts_lock);
+			return -ENODEV;
+		}
+		if (priv->plat->flags & STMMAC_FLAG_INT_SNAPSHOT_EN) {
+			mutex_unlock(&priv->aux_ts_lock);
+			return -EBUSY;
+		}
 		acr_value = readl(ptpaddr + PTP_ACR);
-		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
-		acr_value &= ~PTP_ACR_MASK;
+		channel = FIELD_GET(PTP_ACR_MASK, acr_value);
 
 		if (on) {
-			if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
+			if (channel && rq->extts.index != ilog2(channel)) {
 				netdev_err(priv->dev,
 					   "Cannot enable auxiliary snapshot %d as auxiliary snapshot %d is already enabled",
-					rq->extts.index, channel);
+					rq->extts.index, ilog2(channel));
 				mutex_unlock(&priv->aux_ts_lock);
 				return -EBUSY;
 			}
 
+			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
+				mutex_unlock(&priv->aux_ts_lock);
+				return -EINVAL;
+			}
+
+			/* A re-enable of the armed channel re-arms the
+			 * interrupt as well; disarm it across the FIFO
+			 * clear below and drain the handler that is
+			 * already past its checks, so no one touches the
+			 * FIFO while it is flushed.
+			 */
+			if (channel) {
+				stmmac_mac_timestamp_interrupt_cfg(priv, false);
+				stmmac_sync_timestamp_irq(priv);
+			}
+
 			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
+			priv->plat->ext_snapshot_num = rq->extts.index;
 
 			/* Enable External snapshot trigger */
 			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
 			acr_value |= PTP_ACR_ATSFC;
+		} else if (channel) {
+			/* A disable request for a channel that is not the
+			 * armed one is a no-op: the requested channel is
+			 * not armed.
+			 */
+			if (rq->extts.index != ilog2(channel)) {
+				mutex_unlock(&priv->aux_ts_lock);
+				return 0;
+			}
+
+			acr_value &= ~PTP_ACR_MASK;
+			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+			priv->plat->ext_snapshot_num = -1;
 		} else {
 			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+			priv->plat->ext_snapshot_num = -1;
 		}
 		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
 			   rq->extts.index, on ? "enabled" : "disabled");
 		writel(acr_value, ptpaddr + PTP_ACR);
-		mutex_unlock(&priv->aux_ts_lock);
-		/* wait for auxts fifo clear to finish */
+		/* Keep the lock across the FIFO clear so the arm below
+		 * cannot interleave with a suspend.
+		 */
 		ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
 					 !(acr_value & PTP_ACR_ATSFC),
 					 10, 10000);
+		if (ret && on) {
+			/* Roll the failed enable back so no half-enabled
+			 * channel is left.
+			 */
+			acr_value = readl(ptpaddr + PTP_ACR);
+			acr_value &= ~PTP_ACR_ATSEN(rq->extts.index);
+			writel(acr_value, ptpaddr + PTP_ACR);
+			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+			priv->plat->ext_snapshot_num = -1;
+		}
+		/* Arm the timestamp interrupt only after the FIFO clear,
+		 * so the handler cannot see a snapshot that is about to be
+		 * discarded.
+		 */
+		stmmac_mac_timestamp_interrupt_cfg(priv, on && !ret);
+		/* Drain an in-flight handler before the channel is freed. */
+		if (!on)
+			stmmac_sync_timestamp_irq(priv);
+		mutex_unlock(&priv->aux_ts_lock);
 		break;
 	}
 
@@ -330,7 +393,7 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
 	if (pps_out_num)
 		priv->ptp_clock_ops.n_per_out = pps_out_num;
 
-	n_ext_ts = priv->dma_cap.aux_snapshot_n;
+	n_ext_ts = min(priv->dma_cap.aux_snapshot_n, PTP_ACR_ATSEN_NUM);
 	if (n_ext_ts)
 		priv->ptp_clock_ops.n_ext_ts = n_ext_ts;
 
@@ -340,8 +403,6 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
 	if (priv->plat->crosststamp)
 		priv->ptp_clock_ops.getcrosststamp = stmmac_getcrosststamp;
 
-	mutex_init(&priv->aux_ts_lock);
-
 	priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,
 					     priv->device);
 	if (IS_ERR(priv->ptp_clock)) {
@@ -351,8 +412,6 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
 
 	if (priv->ptp_clock)
 		netdev_info(priv->dev, "registered PTP clock\n");
-	else
-		mutex_destroy(&priv->aux_ts_lock);
 }
 
 /**
@@ -363,12 +422,24 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
  */
 void stmmac_ptp_unregister(struct stmmac_priv *priv)
 {
-	if (priv->ptp_clock) {
-		ptp_clock_unregister(priv->ptp_clock);
-		priv->ptp_clock = NULL;
-		pr_debug("Removed PTP HW clock successfully on %s\n",
-			 priv->dev->name);
+	struct ptp_clock *clock;
 
-		mutex_destroy(&priv->aux_ts_lock);
-	}
+	if (!priv->ptp_clock)
+		return;
+
+	/* Tear the hardware state down and invalidate the clock before
+	 * releasing the lock, so a concurrent request cannot re-arm a
+	 * clock that is going away.
+	 */
+	mutex_lock(&priv->aux_ts_lock);
+	clock = priv->ptp_clock;
+	priv->ptp_clock = NULL;
+	stmmac_mac_timestamp_interrupt_cfg(priv, false);
+	stmmac_drop_hw_extts(priv);
+	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+	priv->plat->ext_snapshot_num = -1;
+	mutex_unlock(&priv->aux_ts_lock);
+
+	ptp_clock_unregister(clock);
+	pr_debug("Removed PTP HW clock successfully on %s\n", priv->dev->name);
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
index 3fe0e3a80e80..a2082e1299b8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
@@ -81,6 +81,7 @@
 #define	PTP_ACR_ATSEN3		BIT(7)	/* Auxiliary Snapshot 3 Enable */
 #define	PTP_ACR_ATSEN(index)	(PTP_ACR_ATSEN0 << (index))
 #define	PTP_ACR_MASK		GENMASK(7, 4)	/* Aux Snapshot Mask */
+#define	PTP_ACR_ATSEN_NUM	4	/* Aux Snapshot 0-3 */
 #define	PMC_ART_VALUE0		0x01	/* PMC_ART[15:0] timer value */
 #define	PMC_ART_VALUE1		0x02	/* PMC_ART[31:16] timer value */
 #define	PMC_ART_VALUE2		0x03	/* PMC_ART[47:32] timer value */
@@ -103,6 +104,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;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 00be2df63d22..487dd492c4b1 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -351,6 +351,7 @@ struct plat_stmmacenet_data {
 	u8 vlan_fail_q;
 	bool provide_bus_info;
 	int int_snapshot_num;
+	int ext_snapshot_num;
 	int msi_mac_vec;
 	int msi_wol_vec;
 	int msi_sfty_ce_vec;
-- 
2.34.1

Re: [PATCH net-next v10 0/3] net: stmmac: dwxgmac2: timestamp interrupt support + Agilex5 fix

[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:

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

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

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

Re: [PATCH net-next v10 3/3] net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 7 · Low: 3

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 subject says "net: stmmac: dwxgmac2: add XGMAC timestamp
  interrupt support", but the patch also changes EXTTS and cross-…
- [Medium] The recorded EXTTS state (STMMAC_FLAG_EXT_SNAPSHOT_EN and
  plat->ext_snapshot_num) can survive resume after the hardware trigger…
- [Medium] stmmac_ptp_unregister() sets priv->ptp_clock to NULL and
  disarms the trigger under aux_ts_lock, "so a concurrent request cannot…
- [Medium] The patch adds a generic optional hook,
  stmmac_ops->timestamp_interrupt_cfg ("Arm or disarm the timestamp…
- [Medium] Who owns the auxiliary snapshot FIFO, the process-context
  cross-timestamp code (smtg_crosststamp, intel_crosststamp) or the…
- [Medium] net: stmmac: unlocked ptp_clock access in
  stmmac_ptp_register()
- [Medium] net: stmmac: dwxgmac2: concurrent FIFO pop in
  dwxgmac2_timestamp_interrupt()
- [Low] The commit message describes a user-visible bug.
- [Low] intel_crosststamp() takes aux_ts_lock at entry, returns -ENODEV
  if priv->ptp_clock is NULL, and holds the lock until it exits.
- [Low] stmmac_drop_extts_state() always clears the PTP_ACR/PTP_TCR
  trigger bits and logs netdev_err("Dropped the enabled auxiliary…

--

net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

This patch adds a DWXGMAC2 timestamp interrupt handler. The handler
reports auxiliary snapshots as PTP_CLOCK_EXTTS events. XGMAC_TSIE is armed
on demand through a new stmmac_ops->timestamp_interrupt_cfg() callback.

The patch also reworks EXTTS channel bookkeeping and the use of aux_ts_lock
in the Intel and socfpga cross-timestamp paths. It also changes how
suspend, resume and unregister handle the auxiliary snapshot trigger.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 4308dccbf2570..3f0889c30a70c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c

[ ... ]

> @@ -424,11 +426,22 @@ static int intel_crosststamp(ktime_t *device,
>  	writel(gpio_value, ioaddr + GMAC_GPIO_STATUS);
>  
>  	/* Time sync done Indication - Interrupt method */
> -	if (!wait_event_interruptible_timeout(priv->tstamp_busy_wait,
> -					      stmmac_cross_ts_isr(priv),
> -					      HZ / 100)) {
> +	ret = wait_event_interruptible_timeout(priv->tstamp_busy_wait,
> +					       stmmac_cross_ts_isr(priv),
> +					       HZ / 100);
> +	if (ret <= 0) {
> +		/* Disarm the trigger on a timeout or an interrupted
> +		 * wait, but do not touch the hardware once the clock has
> +		 * been invalidated by an interface teardown.
> +		 */
> +		if (priv->ptp_clock) {

[Severity: Low]
Can priv->ptp_clock actually be NULL here? intel_crosststamp() now takes
aux_ts_lock on entry, returns -ENODEV if priv->ptp_clock is NULL, and holds
the lock until it returns.

The only runtime path that clears the pointer is stmmac_ptp_unregister(),
and it does that under the same lock:

	mutex_lock(&priv->aux_ts_lock);
	clock = priv->ptp_clock;
	priv->ptp_clock = NULL;

That means this check, and the one before the final PTP_ACR disarm, are
always true. Could these checks be dropped, along with the comment and the
"skips the disarm write afterwards" wording in the commit message?

> +			acr_value = readl(ptpaddr + PTP_ACR);
> +			acr_value &= ~PTP_ACR_MASK;
> +			writel(acr_value, ptpaddr + PTP_ACR);
> +		}

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index 1a88cbaed70ca..ff2956bb89e62 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c

[ ... ]

> @@ -1154,6 +1160,60 @@ static int dwxgmac2_get_mac_tx_timestamp(struct mac_device_info *hw, u64 *ts)
>  	return 0;
>  }
>  
> +void dwxgmac2_timestamp_interrupt(struct stmmac_priv *priv)
> +{

[ ... ]

> +	/* Runs on every MAC interrupt: only touch the FIFO when the
> +	 * timestamp interrupt was armed, and leave it to the
> +	 * cross-timestamp path while that owns it.
> +	 */
> +	if (priv->plat->flags & STMMAC_FLAG_INT_SNAPSHOT_EN) {
> +		readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
> +		return;
> +	}
> +
> +	if (!(readl(priv->ioaddr + XGMAC_INT_EN) & XGMAC_TSIE))
> +		return;

[Severity: Medium]
Can two instances of this handler drain the FIFO at the same time?
When priv->wol_irq is a separate line, stmmac_request_irq_single() and
stmmac_request_irq_multi_msi() install stmmac_interrupt() or
stmmac_mac_interrupt() on both dev->irq and wol_irq, and both end up
in stmmac_common_interrupt() and so in this function. That is why
stmmac_sync_timestamp_irq() has to sync both lines.

The handler deliberately doesn't gate on the read-to-clear TSIS bit,
and reading XGMAC_TIMESTAMP_STATUS doesn't change ATSNS. If the
timestamp interrupt on dev->irq and a PMT event on wol_irq are
handled on two CPUs at once, both see XGMAC_TSIE armed and read the
same non-zero pending_snapshots.

The drain loop only takes ptp_lock for reading, which doesn't exclude
another reader. get_ptptime() reads PTP_ATNR and then PTP_ATSR, and
the PTP_ATSR read pops the FIFO head. With two CPUs in the loop the
reads interleave across entries, so an event can pair the nanoseconds
of one snapshot with the seconds of another. The FIFO is also popped
2 * pending_snapshots times, and the extra pops read whatever an
empty FIFO returns, which is still reported to userspace as a
PTP_CLOCK_EXTTS event.

Should the count read and the pops be serialised with a spinlock, for
example a dedicated one or ptp_lock taken for writing? Re-reading
ATSNS before each pop would stop the loop at an empty FIFO, but it
wouldn't fix the torn ATNR/ATSR pairs on its own.

[Severity: Medium]
Is STMMAC_FLAG_INT_SNAPSHOT_EN being used as a lock here rather than as
state? smtg_crosststamp() now sets it on entry and clears it on every exit,
only to keep this hardirq handler away from the snapshot FIFO:

	priv->plat->flags |= STMMAC_FLAG_INT_SNAPSHOT_EN;
	/* Wait for a handler that is already past the flag check before
	 * taking over the snapshot FIFO.
	 */
	stmmac_sync_timestamp_irq(priv);
	...
	priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;

The handler reads plat->flags with plain loads, while process context does
non-atomic |= and &= on the same word. The handler also uses the XGMAC_TSIE
bit in XGMAC_INT_EN as a second gate.

Correctness then depends on ordering spread across stmmac_enable(),
stmmac_rearm_timestamp_irq(), stmmac_suspend(), stmmac_ptp_unregister()
and core_init. Lockdep can't check any of that.

intel_crosststamp() also sets the same flag but skips the
stmmac_sync_timestamp_irq() drain. So the scheme isn't applied the same way
in both cross-timestamp paths.

Would an IRQ-safe spinlock work better? It would cover the FIFO pop here
and the ATSFC/ATSEN programming in the process-context paths.

> +
> +	/* Reading XGMAC_TIMESTAMP_STATUS clears TSIS, so use the ATSNS
> +	 * count to see the pending snapshots.
> +	 */

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index 265671170bf6e..eba87410f985a 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,

[Severity: Low]
The commit message describes a bug that users can see. With .hwtimestamp =
&stmmac_ptp on DWXGMAC2/DWXLGMAC2, PTP_EXTTS_REQUEST succeeds but no
PTP_CLOCK_EXTTS event is ever delivered. Should this have a Fixes: tag?

The same question applies to the other fixes folded in here. One is the
channel exclusivity check in stmmac_enable(), which could never fire. The
other is the interrupted wait in intel_crosststamp(), which returned a
bogus cross-timestamp. Without Fixes: tags, stable backporters can't find
these fixes or tell how far back they apply.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 8ae95e91a36ab..f077c1465a0c2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -317,6 +317,8 @@ struct stmmac_ops {
>  	void (*update_caps)(struct stmmac_priv *priv);
>  	/* Change the interrupt enable setting. Enable takes precedence. */
>  	void (*irq_modify)(struct mac_device_info *hw, u32 disable, u32 enable);
> +	/* Arm or disarm the timestamp interrupt on demand (optional) */
> +	void (*timestamp_interrupt_cfg)(struct stmmac_priv *priv, bool en);

[Severity: Medium]
Only dwxgmac210_ops and dwxlgmac2_ops implement this hook. MAC100/GMAC
already arm the timestamp interrupt on demand, through a static helper in
dwmac1000_core.c with the same signature:

static void dwmac1000_timestamp_interrupt_cfg(struct stmmac_priv *priv, bool en)
{
	...
	u32 intr_mask = readl(ioaddr + GMAC_INT_MASK);
	...
	writel(intr_mask, ioaddr + GMAC_INT_MASK);
}

That helper isn't wired into dwmac1000_ops, though. Doesn't that make the
new generic disarm calls no-ops on MAC100/GMAC?

In stmmac_ptp_unregister(), stmmac_mac_timestamp_interrupt_cfg(priv,
false) does nothing on those cores. Yet the stmmac_drop_hw_extts() call
right after it has a special case for them. ATSEN0 and the flag get
cleared, but GMAC_INT_DISABLE_TIMESTAMP stays cleared, so the interrupt
stays unmasked.

dwmac1000_ptp_enable() also sets STMMAC_FLAG_EXT_SNAPSHOT_EN, so the new
block in stmmac_suspend() runs on GMAC. But nothing actually gets disarmed
there before the clocks are gated.

The locking also differs. The XGMAC hook goes through irq_modify under
hw->irq_ctrl_lock. dwmac1000_timestamp_interrupt_cfg() does an unlocked
read-modify-write of GMAC_INT_MASK, a register that dwmac1000_irq_modify()
updates under that lock.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a2177777727ef..1abc7c696cf7e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> +static void stmmac_drop_extts_state(struct stmmac_priv *priv)
> +{
> +	mutex_lock(&priv->aux_ts_lock);
> +	stmmac_drop_hw_extts(priv);
> +	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +	priv->plat->ext_snapshot_num = -1;
> +	mutex_unlock(&priv->aux_ts_lock);
> +	netdev_err(priv->dev,
> +		   "Dropped the enabled auxiliary snapshot channel\n");
> +}

[Severity: Low]
Should this check STMMAC_FLAG_EXT_SNAPSHOT_EN or ext_snapshot_num first?
stmmac_resume() calls it whenever stmmac_init_ptp_clk_freq() or
stmmac_init_timestamping() fails. So the register write runs, and
netdev_err() says a channel was dropped, even when no EXTTS channel was
ever enabled.

stmmac_rearm_timestamp_irq() and stmmac_suspend() both check the flag
before doing anything.

> +
> +/**
> + * stmmac_rearm_timestamp_irq - re-arm the timestamp interrupt

[ ... ]

> +	/* Flush the FIFO, re-program the enabled auxiliary snapshot
> +	 * trigger and re-arm the interrupt only if the flush completed.
> +	 */
> +	if (priv->plat->core_type != DWMAC_CORE_XGMAC)
> +		return;

[Severity: Medium]
Can the recorded EXTTS state survive resume after the hardware trigger is
gone? The commit message says "so no stale channel survives". There seem
to be a few paths where it does.

On resume, stmmac_hw_setup()->stmmac_init_dma_engine()->stmmac_reset()
resets the core, and stmmac_init_timestamping() never restores PTP_ACR. On
GMAC, dwmac1000_core_init() also masks the timestamp interrupt again.

This early return affects GMAC4/dwmac5 (including Intel mGBE) and
MAC100/GMAC cores. On those cores the channel is neither restored nor
dropped. STMMAC_FLAG_EXT_SNAPSHOT_EN and ext_snapshot_num stay set, but
nothing is armed in hardware.

In stmmac_resume(), the stmmac_setup_est() failure branch skips both the
rearm and the drop:

		ret = stmmac_setup_est(priv);
		if (ret)
			goto error_stop_dma;

The two PTP failure branches just before it do call
stmmac_drop_extts_state(). The stmmac_hw_setup() failure path (goto
error_unlock) also leaves the state in place. On XGMAC, that leaves the
flag set while TSIE stays disarmed from stmmac_suspend().

The visible effects would be:

- The stmmac_ptp timestamp_interrupt() handler reads a zero channel from
  PTP_ACR and returns. The channel userspace thinks is enabled never
  delivers events.
- intel_crosststamp() and smtg_crosststamp() keep returning -EBUSY
  because of the stale flag.
- An enable for a different channel is accepted, because the PTP_ACR mask
  is empty. It then silently overwrites ext_snapshot_num.

Separately, dwmac1000_ptp_enable() sets and clears the flag but never
updates ext_snapshot_num.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index a37405534bebf..4486c800fa71d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -201,35 +201,98 @@ static int stmmac_enable(struct ptp_clock_info *ptp,

[ ... ]

>  		acr_value = readl(ptpaddr + PTP_ACR);
> -		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
> -		acr_value &= ~PTP_ACR_MASK;
> +		channel = FIELD_GET(PTP_ACR_MASK, acr_value);
>  
>  		if (on) {
> -			if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
> +			if (channel && rq->extts.index != ilog2(channel)) {
>  				netdev_err(priv->dev,
>  					   "Cannot enable auxiliary snapshot %d as auxiliary snapshot %d is already enabled",
> -					rq->extts.index, channel);
> +					rq->extts.index, ilog2(channel));
>  				mutex_unlock(&priv->aux_ts_lock);
>  				return -EBUSY;
>  			}
>  
> +			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return -EINVAL;
> +			}

[Severity: Medium]
The subject says "dwxgmac2: add XGMAC timestamp interrupt support". But
this hunk changes EXTTS behaviour that userspace can see on every core
using stmmac_ptp_clock_ops. Should the commit message call these changes
out, or should they go in separate fixes?

The old code cleared the mask before testing it, so this -EBUSY check
could never fire. A second enable silently replaced the first channel.
After this patch, dwmac4/dwmac5 users start getting -EBUSY.

Other behaviour changes folded in:

- Disabling a channel that isn't the armed one is now a silent no-op.
  Before, any disable cleared the whole ATSEN mask.
- extts.index >= 4 is rejected with -EINVAL. stmmac_ptp_register() also
  clamps n_ext_ts to 4 on every core, including MAC100/GMAC. Userspace
  sees this through PTP_CLOCK_GETCAPS.
- intel_crosststamp() used to check only for a zero return from
  wait_event_interruptible_timeout(). An interrupted wait fell through
  with -ERESTARTSYS, read the FIFO, and returned 0 with a bogus device/ART
  pair to PTP_SYS_OFFSET_PRECISE callers. It now returns an error.
- intel_crosststamp() no longer leaves its internal trigger armed in
  PTP_ACR.
- aux_ts_lock init/destroy moves from PTP register/unregister to
  probe/remove.
- stmmac_ptp_unregister() now writes PTP_ACR on every core, or PTP_TCR on
  MAC100/GMAC.

The commit message presents these as design choices for the XGMAC handler.
It doesn't describe them as fixes, or as behaviour changes on dwmac4/5,
Intel mGBE, socfpga and MAC100/GMAC.

[ ... ]

> @@ -363,12 +422,24 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
>   */
>  void stmmac_ptp_unregister(struct stmmac_priv *priv)
>  {

[ ... ]

> +	/* Tear the hardware state down and invalidate the clock before
> +	 * releasing the lock, so a concurrent request cannot re-arm a
> +	 * clock that is going away.
> +	 */
> +	mutex_lock(&priv->aux_ts_lock);
> +	clock = priv->ptp_clock;
> +	priv->ptp_clock = NULL;
> +	stmmac_mac_timestamp_interrupt_cfg(priv, false);
> +	stmmac_drop_hw_extts(priv);
> +	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +	priv->plat->ext_snapshot_num = -1;
> +	mutex_unlock(&priv->aux_ts_lock);
> +
> +	ptp_clock_unregister(clock);

[Severity: Medium]
Does this guarantee hold on MAC100/GMAC? EXTTS requests on those cores go
through dwmac1000_ptp_enable(). That function takes aux_ts_lock but never
checks priv->ptp_clock:

	case PTP_CLK_REQ_EXTTS:
		mutex_lock(&priv->aux_ts_lock);
		tcr_val = readl(ptpaddr + PTP_TCR);

		if (on) {
			tcr_val |= GMAC_PTP_TCR_ATSEN0;
			tcr_val |= GMAC_PTP_TCR_ATSFC;
			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
		...
		mutex_unlock(&priv->aux_ts_lock);

		dwmac1000_timestamp_interrupt_cfg(priv, on);

Suppose a PTP_EXTTS_REQUEST ioctl is already waiting on aux_ts_lock:

ioctl                               stmmac_ptp_unregister()
dwmac1000_ptp_enable()
  mutex_lock() blocks
                                    mutex_lock()
                                    priv->ptp_clock = NULL
                                    clear ATSEN0 and flag
                                    mutex_unlock()
  set ATSEN0|ATSFC and flag
  writel(PTP_TCR)
  mutex_unlock()
  dwmac1000_timestamp_interrupt_cfg(priv, 1)
                                    ptp_clock_unregister()
                                      waits for the ioctl here

ptp_clock_unregister() only waits for in-flight ioctls after aux_ts_lock
has been dropped. ptp_disable_all_events() doesn't undo this either.

Wouldn't that leave ATSEN0 re-armed, the GMAC timestamp interrupt unmasked
and STMMAC_FLAG_EXT_SNAPSHOT_EN set after the clock is gone? Should
dwmac1000_ptp_enable() get the same !priv->ptp_clock check that
stmmac_enable() now has?

> +	pr_debug("Removed PTP HW clock successfully on %s\n", priv->dev->name);
>  }

[ ... ]

> @@ -201,35 +201,98 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
>  		u8 channel;
>  
>  		mutex_lock(&priv->aux_ts_lock);
> +		/* Refuse to arm once the clock has been invalidated by
> +		 * unregister, or while an internal cross-timestamp owns the
> +		 * FIFO: the PTP_ACR programming below would disarm its
> +		 * trigger.
> +		 */
> +		if (!priv->ptp_clock) {
> +			mutex_unlock(&priv->aux_ts_lock);
> +			return -ENODEV;
> +		}
> +		if (priv->plat->flags & STMMAC_FLAG_INT_SNAPSHOT_EN) {
> +			mutex_unlock(&priv->aux_ts_lock);
> +			return -EBUSY;
> +		}
>  		acr_value = readl(ptpaddr + PTP_ACR);
> -		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
> -		acr_value &= ~PTP_ACR_MASK;
> +		channel = FIELD_GET(PTP_ACR_MASK, acr_value);
>  
>  		if (on) {
> -			if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
> +			if (channel && rq->extts.index != ilog2(channel)) {
>  				netdev_err(priv->dev,
>  					   "Cannot enable auxiliary snapshot %d as auxiliary snapshot %d is already enabled",
> -					rq->extts.index, channel);
> +					rq->extts.index, ilog2(channel));
>  				mutex_unlock(&priv->aux_ts_lock);
>  				return -EBUSY;
>  			}
>  
> +			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return -EINVAL;
> +			}
> +
> +			/* A re-enable of the armed channel re-arms the
> +			 * interrupt as well; disarm it across the FIFO
> +			 * clear below and drain the handler that is
> +			 * already past its checks, so no one touches the
> +			 * FIFO while it is flushed.
> +			 */
> +			if (channel) {
> +				stmmac_mac_timestamp_interrupt_cfg(priv, false);
> +				stmmac_sync_timestamp_irq(priv);
> +			}
> +
>  			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = rq->extts.index;
>  
>  			/* Enable External snapshot trigger */
>  			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
>  			acr_value |= PTP_ACR_ATSFC;
> +		} else if (channel) {
> +			/* A disable request for a channel that is not the
> +			 * armed one is a no-op: the requested channel is
> +			 * not armed.
> +			 */
> +			if (rq->extts.index != ilog2(channel)) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return 0;
> +			}
> +
> +			acr_value &= ~PTP_ACR_MASK;
> +			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
>  		} else {
>  			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
>  		}
>  		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
>  			   rq->extts.index, on ? "enabled" : "disabled");
>  		writel(acr_value, ptpaddr + PTP_ACR);
> -		mutex_unlock(&priv->aux_ts_lock);
> -		/* wait for auxts fifo clear to finish */
> +		/* Keep the lock across the FIFO clear so the arm below
> +		 * cannot interleave with a suspend.
> +		 */
>  		ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
>  					 !(acr_value & PTP_ACR_ATSFC),
>  					 10, 10000);
> +		if (ret && on) {
> +			/* Roll the failed enable back so no half-enabled
> +			 * channel is left.
> +			 */
> +			acr_value = readl(ptpaddr + PTP_ACR);
> +			acr_value &= ~PTP_ACR_ATSEN(rq->extts.index);
> +			writel(acr_value, ptpaddr + PTP_ACR);
> +			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
> +		}
> +		/* Arm the timestamp interrupt only after the FIFO clear,
> +		 * so the handler cannot see a snapshot that is about to be
> +		 * discarded.
> +		 */
> +		stmmac_mac_timestamp_interrupt_cfg(priv, on && !ret);
> +		/* Drain an in-flight handler before the channel is freed. */
> +		if (!on)
> +			stmmac_sync_timestamp_irq(priv);
> +		mutex_unlock(&priv->aux_ts_lock);
>  		break;
>  	}
>  

[Severity: Medium]
Can these checks race with stmmac_ptp_register()? ptp_clock_register()
publishes /dev/ptpN through posix_clock_register() and
cdev_device_add() before it returns. stmmac_ptp_register() only stores
the result in priv->ptp_clock afterwards, and not under aux_ts_lock.

A consumer that opens the clock as soon as it appears, such as ptp4l
or ts2phc started from a udev rule, can issue PTP_EXTTS_REQUEST or
PTP_SYS_OFFSET_PRECISE inside that window. stmmac_enable(),
intel_crosststamp() and smtg_crosststamp() then see a NULL
priv->ptp_clock and fail with -ENODEV, even though the clock is being
registered rather than torn down. Before this patch those requests
worked in that window.

This is hit on every interface open, since stmmac_setup_ptp()
registers the clock again each time the interface comes up.

Would holding aux_ts_lock across ptp_clock_register() and assigning
priv->ptp_clock from a local variable before dropping it close this?

Separately, intel_crosststamp() and smtg_crosststamp() choose the
errno by reading priv->ptp_clock again after mutex_unlock(). A
concurrent stmmac_ptp_unregister() or stmmac_ptp_register() can change
it in between, so the errno need not match what was seen under the
lock, and the read itself is a data race. Could the errno be picked
before the lock is dropped, or the two checks split the way
stmmac_enable() does them?

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

Re: [PATCH net-next v10 0/3] net: stmmac: dwxgmac2: timestamp interrupt support + Agilex5 fix

[Jakub Kicinski]

On Mon, 09 Nov 2026 13:51:54 +0000 netdev-bot+sinfo@kernel.org wrote:
>  - 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.

You are at v10 and still getting these automated emails.
What's going on?