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

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

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:
- Gate the handler's FIFO drain on XGMAC_TSIE being armed; the handler
  runs for every MAC interrupt and must not touch the FIFO while the
  timestamp interrupt is disarmed.
- Hold aux_ts_lock across the whole EXTTS enable, including the FIFO
  clear poll, so the re-arm cannot interleave with a suspend.
- Drain an in-flight handler with stmmac_sync_timestamp_irq() when the
  FIFO is handed to the cross-timestamp and on the EXTTS disable path.
- Roll a failed EXTTS enable back in hardware and software on the
  FIFO-clear timeout, and drop the EXTTS state when the resume re-arm
  cannot restore the channel.
- Disarm XGMAC_TSIE on suspend and unregister; re-arm it on resume.
- Initialise aux_ts_lock in probe and destroy it in remove, so it is
  valid for the whole device lifetime.
- Only clear the software state in the resume-failure teardown, since
  the auxiliary snapshot register differs on DWMAC_CORE_MAC100/GMAC.

-- 
2.43.0

[PATCH net-next v8 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 so a concurrent
PTP_CLK_REQ_EXTTS request cannot flush the FIFO between the poll and
the drain loop.

Fixes: fd8c4f645496 ("net: stmmac: socfpga: Add hardware supported cross-timestamp")
Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 29 ++++++++++++++-----
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 6d4bc1fe8f75..ee79b74bf21b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -337,8 +337,18 @@ 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) {
+		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 +359,14 @@ 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) {
+		mutex_unlock(&priv->aux_ts_lock);
 		netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
 			   __func__);
 		return ret;
@@ -364,8 +378,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,6 +388,8 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		read_unlock_irqrestore(&priv->ptp_lock, flags);
 	}
 
+	mutex_unlock(&priv->aux_ts_lock);
+
 	get_smtgtime(priv->mii, SMTG_MDIO_ADDR, &smtg_time);
 	system->cycles = smtg_time;
 
-- 
2.34.1

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: stmmac: TOCTOU race on STMMAC_FLAG_EXT_SNAPSHOT_EN check
- [High] net: stmmac: race condition in smtg_crosststamp leaves SMTG
  reads unprotected

Pre-existing issues:
- [Medium] intel_crosststamp() in
  drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c has the same…
- [Medium] smtg_crosststamp() (drivers/net/ethernet/stmicro/stmmac/dwmac-
  socfpga.c:310) tests STMMAC_FLAG_EXT_SNAPSHOT_EN before…
- [Medium] At this commit, the XGMAC/XLGMAC hwif entries use .hwtimestamp
  = &stmmac_ptp.
- [Medium] smtg_crosststamp() releases aux_ts_lock (dwmac-socfpga.c:391)
  before get_smtgtime() reads the system-counter half of the cross-…

--

net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS

This patch changes smtg_crosststamp() so that it first waits for the
PTP_ACR_ATSFC FIFO clear to finish, then polls the ATSNS count in
XGMAC_TIMESTAMP_STATUS instead of the transient TSIS bit. It also holds
aux_ts_lock across the whole clear, trigger, poll and drain sequence.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 6d4bc1fe8f751..ee79b74bf21b6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -337,8 +337,18 @@ 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);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. In
smtg_crosststamp(), the STMMAC_FLAG_EXT_SNAPSHOT_EN check runs before
aux_ts_lock is taken, and it is not checked again once the lock is held:

	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
		return -EBUSY;

	mutex_lock(&priv->aux_ts_lock);

stmmac_enable() sets that flag and programs PTP_ACR under aux_ts_lock:

	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;

A PTP_CLK_REQ_EXTTS enable can finish between the unlocked check and
mutex_lock(). One way this happens is when both callers are blocked on
the mutex behind another crosststamp. smtg_crosststamp() then goes on to
do:

	acr_value &= ~PTP_ACR_MASK;

Does this clear the external ATSEN bit? And does the ATSFC write that
follows then flush any external snapshots already latched?

If so, EXT_SNAPSHOT_EN stays set and the EXTTS request has already
returned success, but external events are no longer latched and no error
is reported. Could the EXT_SNAPSHOT_EN check be moved under aux_ts_lock?

Also, at this commit smtg_crosststamp() does not set
STMMAC_FLAG_INT_SNAPSHOT_EN, so the shared timestamp_interrupt() could
drain the FIFO during this window. A later patch in the series, "net:
stmmac: dwxgmac2: add XGMAC timestamp interrupt support", fixes that
part. The unlocked EXT_SNAPSHOT_EN check is still there at the end of
the series.

> -	/* Release the mutex */
> -	mutex_unlock(&priv->aux_ts_lock);

[Severity: High]
This isn't introduced by this patch, and the window is narrower than
before. Still, the system counter half of the cross-timestamp is read
after aux_ts_lock has been dropped.

The SMTG block latches the ARM arch counter on the GPO0 rising edge
that smtg_crosststamp() generates. get_smtgtime() then fetches that
latched value with four separate mdiobus_read() calls, from
SMTG_TSC_WORD3 down to SMTG_TSC_WORD0. Each MDIO read is serialized
on its own, but nothing ties the four reads to the trigger that
produced the value.

Neither ptp_sys_offset_precise() nor get_device_system_crosststamp()
serializes calls into the getcrosststamp path. Two processes issuing
PTP_SYS_OFFSET_PRECISE on the same /dev/ptpX, for example phc2sys and
chronyd, can therefore run smtg_crosststamp() concurrently.

Suppose the first caller has dropped aux_ts_lock but has not finished
get_smtgtime(). The second caller can take aux_ts_lock, clear the FIFO
and toggle GPO0 in that window, which relatches the SMTG registers
under the first caller. The first caller then pairs its PTP device
time with the second capture, or with words torn across the two
captures, and still reports success.

That silently breaks the precision PTP_SYS_OFFSET_PRECISE is meant to
provide. Would it work to move this mutex_unlock() after
get_smtgtime()? Then the trigger, the FIFO drain and the SMTG read
would all happen under aux_ts_lock. Holding the mutex across the MDIO
accesses should be fine, since this path can already sleep. Is there
any path that takes aux_ts_lock with the MDIO bus lock held?

[Severity: Medium]
This isn't a bug introduced by this patch, but intel_crosststamp() in
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c still has the pattern
that this patch removes from the socfpga version:

dwmac-intel.c:intel_crosststamp() {
    ...
	acr_value |= PTP_ACR_ATSFC;
	writel(acr_value, ptpaddr + PTP_ACR);
	/* Release the mutex */
	mutex_unlock(&priv->aux_ts_lock);
    ...
}

It never waits for the ATSFC clear to complete. It also waits on the
transient TSIE bit through stmmac_cross_ts_isr():

	return (readl(priv->ioaddr + GMAC_INT_STATUS) & GMAC_INT_TSIE);

After that it reads ATSNS from GMAC_TIMESTAMP_STATUS and drains the aux
FIFO without holding aux_ts_lock:

	num_snapshot = (readl(ioaddr + GMAC_TIMESTAMP_STATUS) &
			GMAC_TIMESTAMP_ATSNS_MASK) >>
			GMAC_TIMESTAMP_ATSNS_SHIFT;

stmmac_enable() handles PTP_CLK_REQ_EXTTS by taking aux_ts_lock,
rewriting PTP_ACR and setting ATSFC. Can that flush or re-arm the FIFO
between the TSIE wait and the drain loop? A stale TSIE, or a FIFO clear
that is still in progress, could also produce a stale or missing device
sample.

If num_snapshot ends up as 0, system->cycles stays 0 and
get_device_system_crosststamp() returns -EINVAL. So the result is a
mismatched sample or a spurious error, not uninitialized data.

Would it make sense to apply the same fix to intel_crosststamp() in a
follow-up?

> +
> +	/* Wait for the FIFO clear, so the poll only sees snapshots from
> +	 * this trigger.
> +	 */

[ ... ]

> @@ -349,10 +359,14 @@ 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);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. At
this commit, the shared timestamp ISR does not use this register on
XGMAC. The XGMAC and XLGMAC entries in hwif.c use:

		.hwtimestamp = &stmmac_ptp,

and timestamp_interrupt() in stmmac_hwtstamp.c reads the dwmac4 offset:

	ts_status = readl(priv->ioaddr + GMAC_TIMESTAMP_STATUS);

GMAC_TIMESTAMP_STATUS is 0xb20, while XGMAC_TIMESTAMP_STATUS is 0xd20
with ATSNS at GENMASK(29, 25). On XGMAC cores, does the EXTTS ISR read
an unrelated register both to clear the timestamp status and to get the
aux FIFO level? That would give wrong or missing EXTTS events.

This looks like it is fixed later in the series by "net: stmmac:
dwxgmac2: add XGMAC timestamp interrupt support". That patch adds
dwxgmac2_ptp, whose dwxgmac2_timestamp_interrupt() reads
XGMAC_TIMESTAMP_STATUS, and switches both XGMAC hwif entries to
&dwxgmac2_ptp.

[ ... ]

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

[Severity: Medium]
This isn't a bug introduced by this patch, but the system counter half
of the cross-timestamp is still read after aux_ts_lock is dropped. The
SMTG IP latches the ARM arch counter on the GPO0 trigger. get_smtgtime()
then reads it with four separate MDIO reads:

	ns = mdiobus_read(mii, smtg_addr, SMTG_TSC_WORD3);
	...
	ns |= mdiobus_read(mii, smtg_addr, SMTG_TSC_WORD0);

ptp_sys_offset_precise() does not take pincfg_mux, and
get_device_system_crosststamp() does not serialize get_time_fn. With two
concurrent PTP_SYS_OFFSET_PRECISE callers, can the second caller take
aux_ts_lock and toggle GPO0 again before or during the first caller's
MDIO reads?

In that case the first caller pairs its device timestamp with the second
capture, or with words torn across the two captures, and still returns
success.

Before this patch the mutex was dropped even before the GPO0 trigger, so
the window was wider then. Would moving mutex_unlock() after
get_smtgtime() close the remaining window?

[ ... ]

> @@ -337,8 +337,18 @@ 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) {
> +		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.

[Severity: High]
This isn't introduced by this patch, but there is still a hole at the
top of smtg_crosststamp(). The commit message says aux_ts_lock is now
held across the whole sequence so a concurrent PTP_CLK_REQ_EXTTS
request cannot interfere. However, the STMMAC_FLAG_EXT_SNAPSHOT_EN test
that returns -EBUSY runs before mutex_lock(&priv->aux_ts_lock), and it
is not repeated once the lock is held.

stmmac_enable() sets STMMAC_FLAG_EXT_SNAPSHOT_EN and writes
PTP_ACR_ATSEN(rq->extts.index) | PTP_ACR_ATSFC into PTP_ACR under
aux_ts_lock. So this interleaving looks possible:

  smtg_crosststamp()           stmmac_enable(EXTTS, on)
  flag check passes
                               mutex_lock(aux_ts_lock)
                               set EXT_SNAPSHOT_EN
                               set ATSEN(n) | ATSFC in PTP_ACR
                               mutex_unlock(aux_ts_lock)
                               return 0
  mutex_lock(aux_ts_lock)
  acr_value &= ~PTP_ACR_MASK
  set ATSEN(int_snapshot_num)
  set ATSFC

Doesn't that drop the external ATSEN bit? And doesn't the ATSFC write
that follows flush any external snapshots already latched? Nothing
restores the external trigger afterwards. EXT_SNAPSHOT_EN stays set,
so later crosststamp calls return -EBUSY. Meanwhile the EXTTS request,
which already returned success, silently stops producing events.

Could the EXT_SNAPSHOT_EN check be moved after mutex_lock(), with an
unlock on the -EBUSY path? Then the check and the PTP_ACR update would
be done under the same lock.

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

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

[Zxyan Zhu]

timestamp_interrupt() derives the EXTTS channel index with ilog2()
applied directly to the PTP_ACR channel mask, without a zero-mask
check.  ilog2(0) yields -1, which lands 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 out of bounds
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 interrupt
arriving in between passes the flag check while the mask is still
clear.

Check the mask before applying 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 b9a985fa772c..2a076e228e9a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -242,7 +242,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 v8 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 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 concurrent suspend.
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 EXTTS state is dropped rather than advertised.
stmmac_suspend() disarms the interrupt and drains an in-flight handler
under aux_ts_lock before the clocks are gated.

Fixes: 4bb7aff9e6d0 ("net: stmmac: Add PTP support for XGMAC2")
Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |  16 +++
 .../ethernet/stmicro/stmmac/dwxgmac2_core.c   |  61 ++++++++++
 drivers/net/ethernet/stmicro/stmmac/hwif.c    |   4 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |   5 +
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |   1 +
 .../ethernet/stmicro/stmmac/stmmac_hwtstamp.c |  12 ++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 106 +++++++++++++++++-
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  |  68 ++++++++---
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.h  |   2 +
 include/linux/stmmac.h                        |   1 +
 10 files changed, 258 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index ee79b74bf21b..93c1092237fc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -311,6 +311,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		return -EBUSY;
 
 	mutex_lock(&priv->aux_ts_lock);
+	/* Refuse to arm once the clock has been invalidated by
+	 * unregister.
+	 */
+	if (!priv->ptp_clock) {
+		mutex_unlock(&priv->aux_ts_lock);
+		return -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;
@@ -328,6 +340,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;
 	}
@@ -344,6 +357,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
 				 !(acr_value & PTP_ACR_ATSFC), 10, 10000);
 	if (ret) {
+		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__);
@@ -366,6 +380,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 				 FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v),
 				 100, 10000);
 	if (ret) {
+		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__);
@@ -388,6 +403,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 		read_unlock_irqrestore(&priv->ptp_lock, flags);
 	}
 
+	priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
 	mutex_unlock(&priv->aux_ts_lock);
 
 	get_smtgtime(priv->mii, SMTG_MDIO_ADDR, &smtg_time);
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 1a88cbaed70c..3b98cd82ec2d 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,59 @@ 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 +1472,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 +1528,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 2e63614d93a4..498529433d2c 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,8 @@ 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...) \
@@ -672,6 +676,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 08b68441f9c8..06d687697e76 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -408,6 +408,7 @@ 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);
 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 2a076e228e9a..4b906bc33d26 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -280,3 +280,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 56d89a696272..c22f3dd40fca 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>
@@ -894,6 +895,88 @@ 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_extts_state - drop the recorded EXTTS channel
+ * @priv: driver private structure
+ * Description: clears the software EXTTS state so the PTP clock stops
+ * advertising a channel whose interrupt can no longer be armed.
+ */
+static void stmmac_drop_extts_state(struct stmmac_priv *priv)
+{
+	mutex_lock(&priv->aux_ts_lock);
+	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+	priv->plat->ext_snapshot_num = -1;
+	mutex_unlock(&priv->aux_ts_lock);
+}
+
+/**
+ * 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
@@ -8191,6 +8274,7 @@ static int __stmmac_dvr_probe(struct device *device,
 	stmmac_napi_add(ndev);
 
 	mutex_init(&priv->lock);
+	mutex_init(&priv->aux_ts_lock);
 	rwlock_init(&priv->ptp_lock);
 
 	stmmac_fpe_init(priv);
@@ -8324,6 +8408,7 @@ void stmmac_dvr_remove(struct device *dev)
 
 	destroy_workqueue(priv->wq);
 	mutex_destroy(&priv->lock);
+	mutex_destroy(&priv->aux_ts_lock);
 	bitmap_free(priv->af_xdp_zc_qps);
 
 	pm_runtime_disable(dev);
@@ -8378,6 +8463,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();
@@ -8497,12 +8593,18 @@ 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;
+		}
+
+		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 3bfcc9760dce..f41988f5927a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -226,6 +226,13 @@ 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.
+		 */
+		if (!priv->ptp_clock) {
+			mutex_unlock(&priv->aux_ts_lock);
+			return -ENODEV;
+		}
 		acr_value = readl(ptpaddr + PTP_ACR);
 		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
 		acr_value &= ~PTP_ACR_MASK;
@@ -239,22 +246,49 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 				return -EBUSY;
 			}
 
+			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
+				mutex_unlock(&priv->aux_ts_lock);
+				return -EINVAL;
+			}
+
 			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 {
 			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;
 	}
 
@@ -355,7 +389,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;
 
@@ -365,8 +399,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)) {
@@ -376,8 +408,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);
 }
 
 /**
@@ -388,12 +418,22 @@ 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);
+	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
+	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 v8 3/3] net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 5 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says dwxgmac2_timestamp_interrupt() cannot
  race a FIFO flush because "PTP_ACR_ATSFC is only ever set while…
- [Medium] In stmmac_enable(PTP_CLK_REQ_EXTTS), the 'already enabled'
  -EBUSY check is dead.
- [Medium] smtg_crosststamp() (dwmac-socfpga) tests
  STMMAC_FLAG_EXT_SNAPSHOT_EN before mutex_lock(&priv->aux_ts_lock), and…
- [Medium] Two handler instances can drain the auxiliary timestamp FIFO
  at the same time.
- [Medium] The commit message says TSIS is a read-to-clear bit that the
  TX timestamp completion path clears.

--

net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

This adds a DWXGMAC2 timestamp interrupt handler that drains the
auxiliary snapshot FIFO and reports the entries as PTP_CLOCK_EXTTS
events. XGMAC_TSIE is armed on demand from the EXTTS enable path through
a new timestamp_interrupt_cfg() callback. The suspend/resume,
unregister and socfpga cross-timestamp paths are updated to coordinate
FIFO ownership under aux_ts_lock.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index ee79b74bf21b6..93c1092237fc6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -311,6 +311,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  		return -EBUSY;
>  
>  	mutex_lock(&priv->aux_ts_lock);
> +	/* Refuse to arm once the clock has been invalidated by
> +	 * unregister.
> +	 */
> +	if (!priv->ptp_clock) {
> +		mutex_unlock(&priv->aux_ts_lock);
> +		return -ENODEV;
> +	}
> +	priv->plat->flags |= STMMAC_FLAG_INT_SNAPSHOT_EN;

[Severity: Medium]
The STMMAC_FLAG_EXT_SNAPSHOT_EN test that guards smtg_crosststamp() still
runs before aux_ts_lock is taken. Under the lock, only ptp_clock is
rechecked:

    if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
        return -EBUSY;

    mutex_lock(&priv->aux_ts_lock);

getcrosststamp doesn't take pincfg_mux. Can a concurrent
PTP_EXTTS_REQUEST run its whole locked section inside that window?

CPU1 (PTP_SYS_OFFSET_PRECISE)    CPU2 (PTP_EXTTS_REQUEST, on=1)
smtg_crosststamp()
  EXT_SNAPSHOT_EN not set
                                 stmmac_enable()
                                   sets EXT_SNAPSHOT_EN
                                   writes ATSEN(index) | ATSFC
                                   arms XGMAC_TSIE
  mutex_lock(&priv->aux_ts_lock)
  acr_value &= ~PTP_ACR_MASK;
  sets ATSEN(int_snapshot_num)
  ...
  flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN

Would that leave EXT_SNAPSHOT_EN set, TSIE armed and ext_snapshot_num
naming the user's channel, while PTP_ACR enables only the internal
snapshot channel?

If so, the user's EXTTS channel would produce no more events, and every
later smtg_crosststamp() would return -EBUSY. dwxgmac2_timestamp_interrupt()
takes the reported index from the live PTP_ACR, so any events delivered
in that state would carry the internal channel index. On resume,
stmmac_rearm_timestamp_irq() would also program ext_snapshot_num, which
no longer matches the hardware state.

Could the EXT_SNAPSHOT_EN check move under aux_ts_lock, next to the new
ptp_clock check?

[ ... ]

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

[ ... ]

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

[ ... ]

> +	/* Reading XGMAC_TIMESTAMP_STATUS clears TSIS, so use the ATSNS
> +	 * count to see the pending snapshots.
> +	 */
> +	ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);

[Severity: Medium]
The commit message says TSIS is read-to-clear and that the TX timestamp
completion path clears it. On XGMAC that path runs for every TX
timestamp. dwxgmac2_get_tx_timestamp_status() always returns 0, so
stmmac_get_tx_hwtstamp() always falls back to
dwxgmac2_get_mac_tx_timestamp(), which polls the same register from
NAPI:

    if (readl_poll_timeout_atomic(ioaddr + XGMAC_TIMESTAMP_STATUS,
                                  value, value & XGMAC_TXTSC, 100, 10000))

Suppose that read lands after an auxiliary snapshot sets TSIS (with
TSIE armed) but before the CPU takes the MAC interrupt. Can a
level-triggered MAC IRQ then be deasserted, so the handler never runs
for that snapshot?

dwxgmac2_timestamp_interrupt() is the only consumer of the aux FIFO, and
its only caller is stmmac_common_interrupt(). Skipping the TSIS gate
only helps once the handler is already running.

Would the snapshot then stay in the FIFO until an unrelated MAC
interrupt arrives, or in single-IRQ mode the next DMA interrupt? In
multi-MSI mode only MAC interrupts reach this handler. For a one-shot
input the event could then be delayed indefinitely, or lost to FIFO
overflow.

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

[Severity: Medium]
Can two instances of this loop drain the FIFO at the same time?
stmmac_request_irq_single() registers stmmac_interrupt() on dev->irq and
also on a separate wake line:

    if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
        ret = request_irq(priv->wol_irq, stmmac_interrupt,
                          IRQF_SHARED, dev->name, dev);

Both lines reach this handler through stmmac_common_interrupt(). The
handler deliberately doesn't gate on TSIS, so two CPUs servicing the two
lines could both read the same ATSNS=N and both loop N times.

read_lock_irqsave() on ptp_lock admits both readers at once. Also,
get_ptptime() consumes each entry with two separate reads:

    ns = readl(ptpaddr + PTP_ATNR);
    ns += (u64)readl(ptpaddr + PTP_ATSR) * NSEC_PER_SEC;

Would that pop 2N entries for N snapshots? Could one reported EXTTS
timestamp combine the nanosecond and second halves of different entries?

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

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 3bfcc9760dce7..f41988f5927ad 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c

[ ... ]

> @@ -239,22 +246,49 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
>  				return -EBUSY;
>  			}
>  
> +			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return -EINVAL;
> +			}
> +
>  			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 {
>  			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
>  		}

[Severity: Medium]
The "already enabled" check just above this hunk can never fire, because
the mask is cleared before it is tested:

    acr_value = readl(ptpaddr + PTP_ACR);
    channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
    acr_value &= ~PTP_ACR_MASK;

    if (on) {
        if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
            ...
            return -EBUSY;
        }

Before calling ops->enable(), ptp_extts_request() only checks
req.extts.index >= ops->n_ext_ts. With the new single-slot
ext_snapshot_num, what happens for the sequence enable(A), enable(B),
disable(A)?

enable(B) would clear ATSEN(A), set ATSEN(B) and overwrite
ext_snapshot_num.

disable(A) doesn't look at rq->extts.index. It clears EXT_SNAPSHOT_EN,
sets ext_snapshot_num to -1, writes an empty ACR mask and disarms
XGMAC_TSIE. That stops channel B while userspace still thinks it is
enabled.

The dead check predates this patch. However, the new ext_snapshot_num
tracking, the resume rearm and the ATSFC/TSIE invariant in the commit
message all assume only one channel is ever active.

Should the busy test happen before the mask is cleared? And should the
disable path check that rq->extts.index matches ext_snapshot_num?

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

[ ... ]

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

[Severity: Medium]
The commit message says:

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

Does that hold when a second PTP_CLK_REQ_EXTTS enable arrives while a
channel is already enabled? This can come from the ioctl or from the
extts_enable sysfs attribute.

Because of the dead -EBUSY check above, the request gets through. The
writel() above then sets ATSEN(new) | PTP_ACR_ATSFC while XGMAC_TSIE is
still armed from the first enable. TSIE is not disarmed and
stmmac_sync_timestamp_irq() is not called before the flush; the sync
only happens for !on.

dwxgmac2_timestamp_interrupt() runs on any MAC interrupt, or in
single-IRQ mode any DMA interrupt. It shares no lock with this path, and
it samples the FIFO level and the flush state at different times:

    ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
    ...
    acr_value = readl(priv->ptpaddr + PTP_ACR);
    if (acr_value & PTP_ACR_ATSFC)
        return;
    ...
    for (i = 0; i < pending_snapshots; i++) {
        ...
        stmmac_get_ptptime(priv, priv->ptpaddr, &ptp_time);

If that PTP_ACR read happens before the ATSFC write, or after the
self-clearing flush completes, ATSFC reads as 0.

Could the handler then pop N entries from a FIFO that is being flushed
or is already empty? It would report N stale or bogus PTP_CLOCK_EXTTS
events, tagged with either the old or the new channel index.

[ ... ]

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

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

[zhu xin]

Hi,

Thanks for the review.

On Wed, Oct 14, 2026 at 12:03 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 5 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The commit message says dwxgmac2_timestamp_interrupt() cannot
>   race a FIFO flush because "PTP_ACR_ATSFC is only ever set while…

Valid and will be fixed in the respin.

> - [Medium] In stmmac_enable(PTP_CLK_REQ_EXTTS), the 'already enabled'
>   -EBUSY check is dead.

Valid and will be fixed in the respin.

> - [Medium] smtg_crosststamp() (dwmac-socfpga) tests
>   STMMAC_FLAG_EXT_SNAPSHOT_EN before mutex_lock(&priv->aux_ts_lock), and…

Valid and will be fixed in the respin.

> - [Medium] Two handler instances can drain the auxiliary timestamp FIFO
>   at the same time.

we would like to keep this as-is, for three reasons.

First, the hardware does not have two parallel lines for the
timestamp events: per the DWC_xgmac databook the timestamp
interrupt (TSIS) is only asserted on the common sbd_intr_o /
mci_intr_o line, while the separate pmt_intr_o output exists for
power-down mode, so the application clock can be stopped while
the MAC stays wakeable; its clear even crosses clock domains.
The concurrent-drain window therefore only arises from a
platform integration choice: wiring pmt_intr_o to its own GIC
line and describing it as "eth_wake_irq".

Second, the in-tree XGMAC platforms make that choice nowhere:
the dwxgmac users (Agilex5 etc.) list only "macirq" and the
per-queue lines, so the driver's fallback in
stmmac_platform.c assigns wol_irq = dev->irq, the second
request_irq() is skipped, and the handler is registered exactly
once.  The platforms that do wire a separate eth_wake_irq
(imx8mp, renesas r9a09, stm32mp13 ...) are GMAC4/EQOS designs
where the existing dwmac4 timestamp handler has the same
exposure and has never been serialized; this is a pre-existing
driver-wide property, not something this series introduces.

Third, fixing it properly (a single serviced line or an
in-handler lock for the timestamp path) affects every stmmac
handler and would be a series of its own; we would rather not
grow this one with it.

> - [Medium] The commit message says TSIS is a read-to-clear bit that the
>   TX timestamp completion path clears.

Correct and the consequence is a delay, not a lost event: reading
XGMAC_TIMESTAMP_STATUS does not affect the ATSNS count or the
FIFO contents, so the snapshots stay queued and are reported by
the next handler entry on any MAC interrupt (in single-IRQ mode
any DMA interrupt reaches it as well).  The handler deliberately
does not gate on TSIS for exactly this reason: TSIS is an
aggregate, read-to-clear bit that the TX timestamp completion
path (which polls the same register for TXTSC from NAPI) clears
before the handler can observe it.  A dwmac4-style TSIS gate
would turn this delay into a silent event drop, which is why
the commit message documents the trade-off.  The one-shot
"delayed indefinitely" scenario needs a completely quiet MAC
(no TX completion, no LPI, no PMT, no error interrupts) while an
EXTTS edge arrives; and if more events keep arriving the FIFO
level is reported by ATSNS, so nothing is overwritten silently.
We consider the residual delay acceptable for a one-shot input
relative to the alternative of dropping events.

>
> --
>
> net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support
>
> This adds a DWXGMAC2 timestamp interrupt handler that drains the
> auxiliary snapshot FIFO and reports the entries as PTP_CLOCK_EXTTS
> events. XGMAC_TSIE is armed on demand from the EXTTS enable path through
> a new timestamp_interrupt_cfg() callback. The suspend/resume,
> unregister and socfpga cross-timestamp paths are updated to coordinate
> FIFO ownership under aux_ts_lock.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > index ee79b74bf21b6..93c1092237fc6 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > @@ -311,6 +311,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
> >               return -EBUSY;
> >
> >       mutex_lock(&priv->aux_ts_lock);
> > +     /* Refuse to arm once the clock has been invalidated by
> > +      * unregister.
> > +      */
> > +     if (!priv->ptp_clock) {
> > +             mutex_unlock(&priv->aux_ts_lock);
> > +             return -ENODEV;
> > +     }
> > +     priv->plat->flags |= STMMAC_FLAG_INT_SNAPSHOT_EN;
>
> [Severity: Medium]
> The STMMAC_FLAG_EXT_SNAPSHOT_EN test that guards smtg_crosststamp() still
> runs before aux_ts_lock is taken. Under the lock, only ptp_clock is
> rechecked:
>
>     if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
>         return -EBUSY;
>
>     mutex_lock(&priv->aux_ts_lock);
>
> getcrosststamp doesn't take pincfg_mux. Can a concurrent
> PTP_EXTTS_REQUEST run its whole locked section inside that window?
>
> CPU1 (PTP_SYS_OFFSET_PRECISE)    CPU2 (PTP_EXTTS_REQUEST, on=1)
> smtg_crosststamp()
>   EXT_SNAPSHOT_EN not set
>                                  stmmac_enable()
>                                    sets EXT_SNAPSHOT_EN
>                                    writes ATSEN(index) | ATSFC
>                                    arms XGMAC_TSIE
>   mutex_lock(&priv->aux_ts_lock)
>   acr_value &= ~PTP_ACR_MASK;
>   sets ATSEN(int_snapshot_num)
>   ...
>   flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN
>
> Would that leave EXT_SNAPSHOT_EN set, TSIE armed and ext_snapshot_num
> naming the user's channel, while PTP_ACR enables only the internal
> snapshot channel?
>
> If so, the user's EXTTS channel would produce no more events, and every
> later smtg_crosststamp() would return -EBUSY. dwxgmac2_timestamp_interrupt()
> takes the reported index from the live PTP_ACR, so any events delivered
> in that state would carry the internal channel index. On resume,
> stmmac_rearm_timestamp_irq() would also program ext_snapshot_num, which
> no longer matches the hardware state.
>
> Could the EXT_SNAPSHOT_EN check move under aux_ts_lock, next to the new
> ptp_clock check?
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > index 1a88cbaed70ca..3b98cd82ec2d7 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
>
> [ ... ]
>
> > @@ -1154,6 +1160,59 @@ static int dwxgmac2_get_mac_tx_timestamp(struct mac_device_info *hw, u64 *ts)
> >       return 0;
> >  }
> >
> > +void dwxgmac2_timestamp_interrupt(struct stmmac_priv *priv)
> > +{
>
> [ ... ]
>
> > +     /* Reading XGMAC_TIMESTAMP_STATUS clears TSIS, so use the ATSNS
> > +      * count to see the pending snapshots.
> > +      */
> > +     ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
>
> [Severity: Medium]
> The commit message says TSIS is read-to-clear and that the TX timestamp
> completion path clears it. On XGMAC that path runs for every TX
> timestamp. dwxgmac2_get_tx_timestamp_status() always returns 0, so
> stmmac_get_tx_hwtstamp() always falls back to
> dwxgmac2_get_mac_tx_timestamp(), which polls the same register from
> NAPI:
>
>     if (readl_poll_timeout_atomic(ioaddr + XGMAC_TIMESTAMP_STATUS,
>                                   value, value & XGMAC_TXTSC, 100, 10000))
>
> Suppose that read lands after an auxiliary snapshot sets TSIS (with
> TSIE armed) but before the CPU takes the MAC interrupt. Can a
> level-triggered MAC IRQ then be deasserted, so the handler never runs
> for that snapshot?
>
> dwxgmac2_timestamp_interrupt() is the only consumer of the aux FIFO, and
> its only caller is stmmac_common_interrupt(). Skipping the TSIS gate
> only helps once the handler is already running.
>
> Would the snapshot then stay in the FIFO until an unrelated MAC
> interrupt arrives, or in single-IRQ mode the next DMA interrupt? In
> multi-MSI mode only MAC interrupts reach this handler. For a one-shot
> input the event could then be delayed indefinitely, or lost to FIFO
> overflow.
>
> > +
> > +     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);
>
> [Severity: Medium]
> Can two instances of this loop drain the FIFO at the same time?
> stmmac_request_irq_single() registers stmmac_interrupt() on dev->irq and
> also on a separate wake line:
>
>     if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
>         ret = request_irq(priv->wol_irq, stmmac_interrupt,
>                           IRQF_SHARED, dev->name, dev);
>
> Both lines reach this handler through stmmac_common_interrupt(). The
> handler deliberately doesn't gate on TSIS, so two CPUs servicing the two
> lines could both read the same ATSNS=N and both loop N times.
>
> read_lock_irqsave() on ptp_lock admits both readers at once. Also,
> get_ptptime() consumes each entry with two separate reads:
>
>     ns = readl(ptpaddr + PTP_ATNR);
>     ns += (u64)readl(ptpaddr + PTP_ATSR) * NSEC_PER_SEC;
>
> Would that pop 2N entries for N snapshots? Could one reported EXTTS
> timestamp combine the nanosecond and second halves of different entries?
>
> > +
> > +             event.type = PTP_CLOCK_EXTTS;
> > +             event.index = channel;
> > +             event.timestamp = ptp_time;
> > +             ptp_clock_event(priv->ptp_clock, &event);
> > +     }
> > +}
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > index 3bfcc9760dce7..f41988f5927ad 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
>
> [ ... ]
>
> > @@ -239,22 +246,49 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
> >                               return -EBUSY;
> >                       }
> >
> > +                     if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
> > +                             mutex_unlock(&priv->aux_ts_lock);
> > +                             return -EINVAL;
> > +                     }
> > +
> >                       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 {
> >                       priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> > +                     priv->plat->ext_snapshot_num = -1;
> >               }
>
> [Severity: Medium]
> The "already enabled" check just above this hunk can never fire, because
> the mask is cleared before it is tested:
>
>     acr_value = readl(ptpaddr + PTP_ACR);
>     channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
>     acr_value &= ~PTP_ACR_MASK;
>
>     if (on) {
>         if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
>             ...
>             return -EBUSY;
>         }
>
> Before calling ops->enable(), ptp_extts_request() only checks
> req.extts.index >= ops->n_ext_ts. With the new single-slot
> ext_snapshot_num, what happens for the sequence enable(A), enable(B),
> disable(A)?
>
> enable(B) would clear ATSEN(A), set ATSEN(B) and overwrite
> ext_snapshot_num.
>
> disable(A) doesn't look at rq->extts.index. It clears EXT_SNAPSHOT_EN,
> sets ext_snapshot_num to -1, writes an empty ACR mask and disarms
> XGMAC_TSIE. That stops channel B while userspace still thinks it is
> enabled.
>
> The dead check predates this patch. However, the new ext_snapshot_num
> tracking, the resume rearm and the ATSFC/TSIE invariant in the commit
> message all assume only one channel is ever active.
>
> Should the busy test happen before the mask is cleared? And should the
> disable path check that rq->extts.index matches ext_snapshot_num?
>
> >               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);
>
> [ ... ]
>
> > +             /* 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);
>
> [Severity: Medium]
> The commit message says:
>
>   "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."
>
> Does that hold when a second PTP_CLK_REQ_EXTTS enable arrives while a
> channel is already enabled? This can come from the ioctl or from the
> extts_enable sysfs attribute.
>
> Because of the dead -EBUSY check above, the request gets through. The
> writel() above then sets ATSEN(new) | PTP_ACR_ATSFC while XGMAC_TSIE is
> still armed from the first enable. TSIE is not disarmed and
> stmmac_sync_timestamp_irq() is not called before the flush; the sync
> only happens for !on.
>
> dwxgmac2_timestamp_interrupt() runs on any MAC interrupt, or in
> single-IRQ mode any DMA interrupt. It shares no lock with this path, and
> it samples the FIFO level and the flush state at different times:
>
>     ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
>     ...
>     acr_value = readl(priv->ptpaddr + PTP_ACR);
>     if (acr_value & PTP_ACR_ATSFC)
>         return;
>     ...
>     for (i = 0; i < pending_snapshots; i++) {
>         ...
>         stmmac_get_ptptime(priv, priv->ptpaddr, &ptp_time);
>
> If that PTP_ACR read happens before the ATSFC write, or after the
> self-clearing flush completes, ATSFC reads as 0.
>
> Could the handler then pop N entries from a FIFO that is being flushed
> or is already empty? It would report N stale or bogus PTP_CLOCK_EXTTS
> events, tagged with either the old or the new channel index.
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013033207.1684328-1-zxyan0222%40gmail.com

pw-bot: cr

Re: [PATCH net-next v8 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:

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

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

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

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

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