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

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

[Zxyan Zhu]

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

Patch 1 makes smtg_crosststamp() complete on the persistent ATSNS count
instead of the transient TSIS bit, waits for the ATSFC FIFO clear to
complete before arming, and derives the pop count from the poll result,
so the completion condition is immune to the destructive reads of
XGMAC_TIMESTAMP_STATUS performed by the DWXGMAC2 timestamp interrupt
handler and the TX timestamp completion path.

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

Because v5 changes the completion mechanism, the previous Tested-by for
the mask approach no longer applies; Patch 1 needs re-verification on
Agilex5 hardware.

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:
- Replace the XGMAC_TSIE masking approach with completing the
  cross-timestamp on the persistent ATSNS count, and reorder the series
  so it is bisect-safe.
- Wait for the ATSFC FIFO clear to complete before arming, so a stale
  ATSNS count from a previous snapshot cannot be observed.

Zxyan Zhu (2):
  net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS
  net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

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

-- 
2.34.1

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

[Zxyan Zhu]

The Agilex5 smtg_crosststamp() handler arms an internal auxiliary
snapshot, toggles GPO0 and then polls XGMAC_INT_STATUS for TSIS to
learn that the snapshot is ready.  TSIS is a transient, read-to-clear
status bit: it is set by any MAC timestamp event and cleared the moment
XGMAC_TIMESTAMP_STATUS is read.

That makes the TSIS poll racy in two ways.  A stale TSIS latched by an
unrelated event satisfies the poll immediately, before the auxiliary
snapshot is latched, so the FIFO comes back empty and *device is never
written even though the call returns 0.  Conversely a concurrent reader
of XGMAC_TIMESTAMP_STATUS, such as the TX timestamp completion path, can
clear TSIS while the poll is waiting and make it time out with "Wait for
time sync operation timeout".

The auxiliary snapshot FIFO is also reported by the persistent ATSNS
count in XGMAC_TIMESTAMP_STATUS.  ATSNS is cleared only when the FIFO
clear bit (PTP_ACR_ATSFC) is set, so it is immune to the destructive
reads above.  Poll ATSNS instead of TSIS, wait for the ATSFC clear to
complete first so a stale ATSNS cannot be observed, and derive the
count for the FIFO pop loop from the value the poll read back so it is
guaranteed non-zero on the success path.

Signed-off-by: Zxyan Zhu <zxyan0222@gmail.com>
---
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 29 +++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1d7f0a57d288..ee6f28637878 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -340,6 +340,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
 	/* Release the mutex */
 	mutex_unlock(&priv->aux_ts_lock);
 
+	/* Wait for the FIFO clear to complete so a stale ATSNS count from
+	 * a previous snapshot cannot satisfy the poll below before the new
+	 * snapshot is latched.
+	 */
+	ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
+				 !(acr_value & PTP_ACR_ATSFC), 10, 10000);
+	if (ret) {
+		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,9 +361,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);
+	/* Wait for the auxiliary snapshot to be latched.  TSIS is a
+	 * transient status bit that is set by any MAC timestamp event and
+	 * cleared by reading XGMAC_TIMESTAMP_STATUS, so it is not a
+	 * reliable completion condition.  Poll the persistent ATSNS count
+	 * instead: it is cleared only by setting PTP_ACR_ATSFC, so
+	 * nothing can clear it while we wait, and it reflects exactly the
+	 * snapshot latched by this trigger.
+	 */
+	ret = readl_poll_timeout(ioaddr + XGMAC_TIMESTAMP_STATUS, v,
+				 FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v),
+				 100, 10000);
 	if (ret) {
 		netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
 			   __func__);
@@ -364,8 +384,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++) {
-- 
2.34.1

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS

This changes the Agilex5 smtg_crosststamp() handler so that it 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 in
XGMAC_INT_STATUS.  The snapshot count used for the FIFO pop loop is now
taken from the value the poll read back rather than from a fresh register
read.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 1d7f0a57d2889..ee6f28637878a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -340,6 +340,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  	/* Release the mutex */
>  	mutex_unlock(&priv->aux_ts_lock);
>  
> +	/* Wait for the FIFO clear to complete so a stale ATSNS count from
> +	 * a previous snapshot cannot satisfy the poll below before the new
> +	 * snapshot is latched.
> +	 */
> +	ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
> +				 !(acr_value & PTP_ACR_ATSFC), 10, 10000);
> +	if (ret) {
> +		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,9 +361,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);
> +	/* Wait for the auxiliary snapshot to be latched.  TSIS is a
> +	 * transient status bit that is set by any MAC timestamp event and
> +	 * cleared by reading XGMAC_TIMESTAMP_STATUS, so it is not a
> +	 * reliable completion condition.  Poll the persistent ATSNS count
> +	 * instead: it is cleared only by setting PTP_ACR_ATSFC, so
> +	 * nothing can clear it while we wait, and it reflects exactly the
> +	 * snapshot latched by this trigger.
> +	 */

[Severity: Low]
Is the "cleared only by setting PTP_ACR_ATSFC" part of this comment (and
the matching "ATSNS is cleared only when the FIFO clear bit
(PTP_ACR_ATSFC) is set" in the commit message) accurate?

ATSNS is the auxiliary snapshot FIFO fill level, and popping the FIFO
decrements it.  get_ptptime() in stmmac_hwtstamp.c does the pop:

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

That is also why both timestamp_interrupt() and the loop just below in
smtg_crosststamp() iterate exactly num_snapshot times:

	/* Repeat until the timestamps are from the FIFO last segment */
	for (i = 0; i < num_snapshot; i++) {
		read_lock_irqsave(&priv->ptp_lock, flags);
		stmmac_get_ptptime(priv, ptpaddr, &ptp_time);

Would it be more precise to say only that ATSNS is not cleared by reading
XGMAC_TIMESTAMP_STATUS, rather than that nothing can clear it while the
poll runs?

> +	ret = readl_poll_timeout(ioaddr + XGMAC_TIMESTAMP_STATUS, v,
> +				 FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v),
> +				 100, 10000);
>  	if (ret) {
>  		netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
>  			   __func__);
> @@ -364,8 +384,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++) {

[Severity: Medium]
Can this reuse of the polled value hand a fabricated timestamp back to
userspace?

Everything from the ATSFC-clear wait through the GPO0 trigger, the ATSNS
poll and this pop loop runs after aux_ts_lock has been dropped:

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

The STMMAC_FLAG_EXT_SNAPSHOT_EN test at the top of smtg_crosststamp():

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

is done without that mutex, while stmmac_enable() in stmmac_ptp.c sets the
flag and writes PTP_ACR under it:

	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;

The PTP core does not serialize the callback either:

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

So after the poll observes ATSNS = N, a PTP_CLK_REQ_EXTTS enable or a
second concurrent smtg_crosststamp() can set PTP_ACR_ATSFC and flush the
FIFO, or drain it itself.  The loop then still runs N iterations against an
empty FIFO, sets *device from whatever those reads return, and returns 0.

Note the read_lock_irqsave(&priv->ptp_lock, flags) inside the loop is a
reader lock, so two concurrent drains can interleave pops of the same
FIFO.

Before this patch the count was re-read immediately before the loop, so the
same race produced num_snapshot == 0 and a skipped loop, i.e. a detectable
failure rather than a stale value.  Given that, does the commit message
claim hold?

	"derive the count for the FIFO pop loop from the value the poll read
	back so it is guaranteed non-zero on the success path"

Would holding aux_ts_lock (or another writer-exclusive lock) across the
ATSFC wait, the trigger, the poll and the drain, and re-checking the count
at drain time, be needed to make that guarantee real?

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

[zhu xin]

On Tue, Sep 15, 2026 at 9:49 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS
>
> This changes the Agilex5 smtg_crosststamp() handler so that it 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 in
> XGMAC_INT_STATUS.  The snapshot count used for the FIFO pop loop is now
> taken from the value the poll read back rather than from a fresh register
> read.
>
> A few questions below.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > index 1d7f0a57d2889..ee6f28637878a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > @@ -340,6 +340,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
> >       /* Release the mutex */
> >       mutex_unlock(&priv->aux_ts_lock);
> >
> > +     /* Wait for the FIFO clear to complete so a stale ATSNS count from
> > +      * a previous snapshot cannot satisfy the poll below before the new
> > +      * snapshot is latched.
> > +      */
> > +     ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
> > +                              !(acr_value & PTP_ACR_ATSFC), 10, 10000);
> > +     if (ret) {
> > +             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,9 +361,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);
> > +     /* Wait for the auxiliary snapshot to be latched.  TSIS is a
> > +      * transient status bit that is set by any MAC timestamp event and
> > +      * cleared by reading XGMAC_TIMESTAMP_STATUS, so it is not a
> > +      * reliable completion condition.  Poll the persistent ATSNS count
> > +      * instead: it is cleared only by setting PTP_ACR_ATSFC, so
> > +      * nothing can clear it while we wait, and it reflects exactly the
> > +      * snapshot latched by this trigger.
> > +      */
>
> [Severity: Low]
> Is the "cleared only by setting PTP_ACR_ATSFC" part of this comment (and
> the matching "ATSNS is cleared only when the FIFO clear bit
> (PTP_ACR_ATSFC) is set" in the commit message) accurate?
>
> ATSNS is the auxiliary snapshot FIFO fill level, and popping the FIFO
> decrements it.  get_ptptime() in stmmac_hwtstamp.c does the pop:
>
>         ns = readl(ptpaddr + PTP_ATNR);
>         ns += (u64)readl(ptpaddr + PTP_ATSR) * NSEC_PER_SEC;
>
> That is also why both timestamp_interrupt() and the loop just below in
> smtg_crosststamp() iterate exactly num_snapshot times:
>
>         /* Repeat until the timestamps are from the FIFO last segment */
>         for (i = 0; i < num_snapshot; i++) {
>                 read_lock_irqsave(&priv->ptp_lock, flags);
>                 stmmac_get_ptptime(priv, ptpaddr, &ptp_time);
>
> Would it be more precise to say only that ATSNS is not cleared by reading
> XGMAC_TIMESTAMP_STATUS, rather than that nothing can clear it while the
> poll runs?
>
> > +     ret = readl_poll_timeout(ioaddr + XGMAC_TIMESTAMP_STATUS, v,
> > +                              FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v),
> > +                              100, 10000);
> >       if (ret) {
> >               netdev_err(priv->dev, "%s: Wait for time sync operation timeout\n",
> >                          __func__);
> > @@ -364,8 +384,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++) {
>
Agreed, the wording was wrong: ATSNS is the aux snapshot FIFO level,
so popping the FIFO via get_ptptime() decrements it as well.
v6 rewords the comment and the commit message accordingly.

> [Severity: Medium]
> Can this reuse of the polled value hand a fabricated timestamp back to
> userspace?
>
> Everything from the ATSFC-clear wait through the GPO0 trigger, the ATSNS
> poll and this pop loop runs after aux_ts_lock has been dropped:
>
>         /* Release the mutex */
>         mutex_unlock(&priv->aux_ts_lock);
>
> The STMMAC_FLAG_EXT_SNAPSHOT_EN test at the top of smtg_crosststamp():
>
>         if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
>                 return -EBUSY;
>
> is done without that mutex, while stmmac_enable() in stmmac_ptp.c sets the
> flag and writes PTP_ACR under it:
>
>         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;
>
> The PTP core does not serialize the callback either:
>
> drivers/ptp/ptp_chardev.c:ptp_ioctl() {
>         ...
>         case PTP_SYS_OFFSET_PRECISE:
>         case PTP_SYS_OFFSET_PRECISE2:
>                 return ptp_sys_offset_precise(ptp, argptr,
>                                               ptp->info->getcrosststamp);
>         ...
> }
>
> So after the poll observes ATSNS = N, a PTP_CLK_REQ_EXTTS enable or a
> second concurrent smtg_crosststamp() can set PTP_ACR_ATSFC and flush the
> FIFO, or drain it itself.  The loop then still runs N iterations against an
> empty FIFO, sets *device from whatever those reads return, and returns 0.
>
> Note the read_lock_irqsave(&priv->ptp_lock, flags) inside the loop is a
> reader lock, so two concurrent drains can interleave pops of the same
> FIFO.
>
> Before this patch the count was re-read immediately before the loop, so the
> same race produced num_snapshot == 0 and a skipped loop, i.e. a detectable
> failure rather than a stale value.  Given that, does the commit message
> claim hold?
>
>         "derive the count for the FIFO pop loop from the value the poll read
>         back so it is guaranteed non-zero on the success path"
>
> Would holding aux_ts_lock (or another writer-exclusive lock) across the
> ATSFC wait, the trigger, the poll and the drain, and re-checking the count
> at drain time, be needed to make that guarantee real?

Agreed.  v6 drops the "guaranteed non-zero" claim and instead holds
aux_ts_lock across the whole sequence: the ATSFC clear wait, the GPO0
trigger, the ATSNS poll and the drain loop all run under the mutex,
and it is only released on the way out.

v6 will be posted shortly.

Best regards,
Zxyan

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

[Zxyan Zhu]

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

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

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

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

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

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

Adds a DWXGMAC2-specific timestamp interrupt handler that reads
XGMAC_TIMESTAMP_STATUS instead of the dwmac4 offset, derives the pending
auxiliary snapshot count from ATSNS and emits PTP_CLOCK_EXTTS events.  It
also adds XGMAC_TSIE to XGMAC_INT_DEFAULT_EN and points the DWXGMAC2 and
DWXLGMAC2 hwif entries at the new hwtimestamp ops.

A few questions on the commit message and on the new handler.

[Severity: Medium]
The commit message says:

  "used &stmmac_ptp, whose timestamp_interrupt callback read the wrong
   register and whose config_hw_tstamping callback never enabled the
   XGMAC timestamp interrupt (XGMAC_TSIE was not in XGMAC_INT_DEFAULT_EN)"

Is the config_hw_tstamping part accurate?  The new ops struct installs the
very same function that stmmac_ptp uses:

  const struct stmmac_hwtimestamp dwxgmac2_ptp = {
	.config_hw_tstamping = config_hw_tstamping,

and config_hw_tstamping() only does a read-modify-write of PTP_TCR; it never
touches XGMAC_INT_EN.  The single hunk that actually unmasks the interrupt is
the XGMAC_INT_DEFAULT_EN change, which dwxgmac2_core_init() writes to
XGMAC_INT_EN.

The second bullet says the handler:

  "- derives the pending auxiliary snapshot count from the persistent
     ATSNS field instead of the transient AUXTSTRIG status bit"

Is anything changing here?  The generic handler being replaced already used
ATSNS:

  num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >>
		 GMAC_TIMESTAMP_ATSNS_SHIFT;

and no AUXTSTRIG bit is defined for XGMAC at all (only
GMAC_TIMESTAMP_AUXTSTRIG in dwmac4.h).

Could the message be reworded so it is clear which hunk is load-bearing?  As
written, someone backporting could take the ops table and handler hunks
without the XGMAC_INT_DEFAULT_EN hunk and still get no events.

[Severity: Low]
The body describes a user-visible failure:

  "As a result, auxiliary snapshot events were never reported on XGMAC
   platforms."

stmmac_ptp_register() already advertises the channels on XGMAC:

  n_ext_ts = priv->dma_cap.aux_snapshot_n;
  if (n_ext_ts)
	  priv->ptp_clock_ops.n_ext_ts = n_ext_ts;

and stmmac_enable() accepts PTP_CLK_REQ_EXTTS, so PTP_EXTTS_REQUEST succeeds
today but no events arrive.  Should this carry a Fixes: tag, or should the
subject drop "add ... support" and state explicitly that EXTTS never worked
on this hardware?  Without either, it is hard to tell which trees are
affected.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index f8ab347f7b5b9..03cbae326995b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -87,7 +87,7 @@
>  #define XGMAC_TSIE			BIT(12)
>  #define XGMAC_LPIIE			BIT(5)
>  #define XGMAC_PMTIE			BIT(4)
> -#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE)
> +#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE | XGMAC_TSIE)

[Severity: High]
Does this revert commit 30300d9f9150 ("net: stmmac: xgmac: Disable the
Timestamp interrupt by default")?  git blame on this line points at that
commit, which removed exactly this bit with the reasoning that XGMAC only
polls for timestamps in the current software implementation and that dropping
the interrupt "greatly reduces the system load by reducing the number of
interrupts".  That commit carried a Fixes: tag.

dwxgmac2_core_init() programs this unconditionally on every ifup:

  writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);

with no check of PTP capability and no check that anything consumes
auxiliary snapshots.  On platforms that never set
STMMAC_FLAG_EXT_SNAPSHOT_EN the new handler returns immediately after one
side-effecting register read, so every aux-snapshot trigger, target-time
match, seconds overflow and TX timestamp capture becomes an interrupt with
no consumer.

Is there also a missing disable counterpart?  XGMAC_INT_EN has only two
writers, dwxgmac2_core_init() and dwxgmac2_irq_modify(), and the latter is
used only for XGMAC_FPEIE and the PCS masks, so TSIE is never cleared.
__stmmac_release() frees the IRQ lines before releasing PTP:

  /* Free the IRQ lines */
  stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
  ...
  stmmac_release_ptp(priv);

which leaves the MAC timestamp source armed with nobody left to read
XGMAC_TIMESTAMP_STATUS and clear TSIS; on an IRQF_SHARED line that shows up
as unhandled interrupts for the co-owner, and stmmac_interrupt() also returns
early on STMMAC_DOWN without acknowledging the source.

Would arming this on demand from the PTP_CLK_REQ_EXTTS path via
dwxgmac2_irq_modify(), the way dwmac1000 does, avoid both problems?

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

[Severity: High]
Should this read be gated on the MAC interrupt status bit first?  The
dwmac4 implementation of the same callback checks before touching the
timestamp status register:

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

  if (!tsync_int)
	  return;

The XGMAC equivalent, XGMAC_INT_TSIS, is defined in dwxgmac2.h but after
this series git grep XGMAC_INT_TSIS matches only its own definition, and
dwxgmac2_host_irq_status() does not consume it either.

Without that gate the clear-on-read register is read on every MAC host
interrupt:

  stmmac_interrupt()
    stmmac_common_interrupt()
      stmmac_timestamp_interrupt()
        readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS)

and in the legacy shared-IRQ configuration stmmac_common_interrupt() runs
before stmmac_dma_interrupt(), so under traffic that read happens for every
RX/TX DMA, LPI, PMT, MTL and EST interrupt as well.

The same register is polled from NAPI/softirq by the pre-existing
dwxgmac2_get_mac_tx_timestamp():

  if (readl_poll_timeout_atomic(ioaddr + XGMAC_TIMESTAMP_STATUS,
				value, value & XGMAC_TXTSC, 100, 10000))
	  return -EBUSY;

reached via stmmac_get_tx_hwtstamp() and stmmac_xsk_fill_timestamp(), with
nothing serializing it against the hardirq read.

The comment above also seems to argue both sides.  If reading
XGMAC_TIMESTAMP_STATUS clears TSIS, as the comment states and as the sibling
patch "net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS"
asserts, does the whole register not go read-clear, including TXTSC?  In that
case the interrupt handler consumes the TX-timestamp-captured status before
the TX clean path sees it, and readl_poll_timeout_atomic() spins its full
10 ms budget in atomic context and returns -EBUSY, losing the MAC level TX
hardware timestamp.

If instead TXTSC really is cleared only by reading XGMAC_TXTIMESTAMP_SEC as
the comment claims, then nothing here drains it, so TSIS stays asserted
whenever a MAC level TX timestamp is captured but never fetched (descriptor
level status in use, or hwts_tx_en turned off).  With XGMAC_TSIE now
unmasked, would that not re-fire the level triggered MAC interrupt
immediately?

Could the comment be reconciled with the databook and the missing
XGMAC_INT_TSIS check added?

> +
> +	if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN))
> +		return;
> +
> +	pending_snapshots = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, ts_status);
> +	if (!pending_snapshots)
> +		return;
> +
> +	acr_value = readl(priv->ptpaddr + PTP_ACR);
> +	channel = FIELD_GET(PTP_ACR_MASK, acr_value);
> +	if (!channel)
> +		return;
> +	channel = ilog2(channel);

[Severity: Medium]
Can this read of PTP_ACR see a half-programmed value?  stmmac_enable() does
its read-modify-write of PTP_ACR under priv->aux_ts_lock, and sets the flag
before the register write:

  priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
  ...
  writel(acr_value, ptpaddr + PTP_ACR);

priv->aux_ts_lock is a mutex ("Protects auxiliary snapshot registers from
concurrent access." in stmmac.h), so a hardirq handler cannot take it.  An
interrupt landing in that window sees the flag set with a stale PTP_ACR and
reports the snapshot with the wrong event.index; on the disable path it can
see the flag still set with the mask already cleared, take the
if (!channel) return; exit and drop the pending ATSNS snapshots.

priv->plat->flags itself is now a plain non-atomic |= / &= shared with a
hardirq reader, with no READ_ONCE()/WRITE_ONCE() or barrier.  Is that
intentional?

[Severity: High]
This isn't a bug introduced by this patch, but the guard added here is
missing from the shared dwmac4 handler that this new code was modelled on.
In timestamp_interrupt() in stmmac_hwtstamp.c:

  channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));

For a non-constant u32, ilog2() expands to __ilog2_u32(n) = fls(n) - 1, so
ilog2(0) is -1.  Stored into the u32 channel and then into event.index it
becomes -1, and ptp_clock_event() uses it without range validation:

  drivers/ptp/ptp_clock.c:ptp_clock_event() {
	  ...
	  if (test_bit((unsigned int)event->index, tsevq->mask))
	  ...
  }

tsevq->mask is bitmap_alloc(PTP_MAX_CHANNELS, ...), i.e. 2048 bits, so bit
index 0xFFFFFFFF is a read roughly 512 MB past a 256-byte allocation, taken
from hard IRQ context.

The window is the same one described above: stmmac_enable() sets
STMMAC_FLAG_EXT_SNAPSHOT_EN before writing PTP_ACR, and if a timestamp
interrupt arrives with ATSNS non-zero (snapshots left in the FIFO from a
previous session, since ATSNS is cleared only by PTP_ACR_ATSFC) the handler
sees flag set and mask zero.

Would it make sense to move the if (!channel) return; check into the shared
handler as part of this series, so dwmac4/dwmac410/dwmac510 get it too?

> +
> +	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]
Does the read side of priv->ptp_lock give any exclusion here?  This loop pops
the auxiliary snapshot FIFO, and get_ptptime() is a two-register consuming
access:

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

An rwlock read side does not exclude other readers, and on XGMAC there is a
second consumer of the same FIFO in process context, smtg_crosststamp() in
dwmac-socfpga.c:

  for (i = 0; i < num_snapshot; i++) {
	  read_lock_irqsave(&priv->ptp_lock, flags);
	  stmmac_get_ptptime(priv, ptpaddr, &ptp_time);

The only thing keeping the two apart appears to be an unlocked flag test on
each side:

  drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:smtg_crosststamp() {
	  if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
		  return -EBUSY;
	  ...
  }

Since that check happens well before the drain loop (with 10 ms
readl_poll_timeout budgets in between), can a PTP_EXTTS_REQUEST arriving
after the check arm the interrupt path while smtg_crosststamp() is still
draining, letting both pop the same FIFO and interleave their PTP_ATNR /
PTP_ATSR reads?

> +
> +		event.type = PTP_CLOCK_EXTTS;
> +		event.index = channel;

[Severity: Medium]
This is a pre-existing issue, but it interacts with the new index
derivation.  In stmmac_enable(), the PTP_CLK_REQ_EXTTS path clears the
channel mask before testing it:

  acr_value &= ~PTP_ACR_MASK;

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

so the tested field is always zero and the -EBUSY rejection of a second
active channel never fires.  Since stmmac_ptp_register() advertises
dma_cap.aux_snapshot_n channels, a second PTP_EXTTS_REQUEST succeeds and
silently replaces the channel that was enabled.

With that, does the index computed here get attached to the wrong events?
Snapshots latched while the first channel was armed would be reported under
the replacement channel's index.

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

[ ... remaining hunks in hwif.c, hwif.h, stmmac_hwtstamp.c and
stmmac_ptp.h snipped ... ]

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

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

Adds a DWXGMAC2-specific timestamp interrupt handler that reads
XGMAC_TIMESTAMP_STATUS instead of the dwmac4 offset, derives the pending
auxiliary snapshot count from ATSNS and emits PTP_CLOCK_EXTTS events.  It
also adds XGMAC_TSIE to XGMAC_INT_DEFAULT_EN and points the DWXGMAC2 and
DWXLGMAC2 hwif entries at the new hwtimestamp ops.

A few questions on the commit message and on the new handler.

[Severity: Medium]
The commit message says:

  "used &stmmac_ptp, whose timestamp_interrupt callback read the wrong
   register and whose config_hw_tstamping callback never enabled the
   XGMAC timestamp interrupt (XGMAC_TSIE was not in XGMAC_INT_DEFAULT_EN)"

Is the config_hw_tstamping part accurate?  The new ops struct installs the
very same function that stmmac_ptp uses:

  const struct stmmac_hwtimestamp dwxgmac2_ptp = {
	.config_hw_tstamping = config_hw_tstamping,

and config_hw_tstamping() only does a read-modify-write of PTP_TCR; it never
touches XGMAC_INT_EN.  The single hunk that actually unmasks the interrupt is
the XGMAC_INT_DEFAULT_EN change, which dwxgmac2_core_init() writes to
XGMAC_INT_EN.

The second bullet says the handler:

  "- derives the pending auxiliary snapshot count from the persistent
     ATSNS field instead of the transient AUXTSTRIG status bit"

Is anything changing here?  The generic handler being replaced already used
ATSNS:

  num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >>
		 GMAC_TIMESTAMP_ATSNS_SHIFT;

and no AUXTSTRIG bit is defined for XGMAC at all (only
GMAC_TIMESTAMP_AUXTSTRIG in dwmac4.h).

Could the message be reworded so it is clear which hunk is load-bearing?  As
written, someone backporting could take the ops table and handler hunks
without the XGMAC_INT_DEFAULT_EN hunk and still get no events.

[Severity: Low]
The body describes a user-visible failure:

  "As a result, auxiliary snapshot events were never reported on XGMAC
   platforms."

stmmac_ptp_register() already advertises the channels on XGMAC:

  n_ext_ts = priv->dma_cap.aux_snapshot_n;
  if (n_ext_ts)
	  priv->ptp_clock_ops.n_ext_ts = n_ext_ts;

and stmmac_enable() accepts PTP_CLK_REQ_EXTTS, so PTP_EXTTS_REQUEST succeeds
today but no events arrive.  Should this carry a Fixes: tag, or should the
subject drop "add ... support" and state explicitly that EXTTS never worked
on this hardware?  Without either, it is hard to tell which trees are
affected.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index f8ab347f7b5b9..03cbae326995b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -87,7 +87,7 @@
>  #define XGMAC_TSIE			BIT(12)
>  #define XGMAC_LPIIE			BIT(5)
>  #define XGMAC_PMTIE			BIT(4)
> -#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE)
> +#define XGMAC_INT_DEFAULT_EN		(XGMAC_LPIIE | XGMAC_PMTIE | XGMAC_TSIE)

[Severity: High]
Does this revert commit 30300d9f9150 ("net: stmmac: xgmac: Disable the
Timestamp interrupt by default")?  git blame on this line points at that
commit, which removed exactly this bit with the reasoning that XGMAC only
polls for timestamps in the current software implementation and that dropping
the interrupt "greatly reduces the system load by reducing the number of
interrupts".  That commit carried a Fixes: tag.

dwxgmac2_core_init() programs this unconditionally on every ifup:

  writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);

with no check of PTP capability and no check that anything consumes
auxiliary snapshots.  On platforms that never set
STMMAC_FLAG_EXT_SNAPSHOT_EN the new handler returns immediately after one
side-effecting register read, so every aux-snapshot trigger, target-time
match, seconds overflow and TX timestamp capture becomes an interrupt with
no consumer.

Is there also a missing disable counterpart?  XGMAC_INT_EN has only two
writers, dwxgmac2_core_init() and dwxgmac2_irq_modify(), and the latter is
used only for XGMAC_FPEIE and the PCS masks, so TSIE is never cleared.
__stmmac_release() frees the IRQ lines before releasing PTP:

  /* Free the IRQ lines */
  stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
  ...
  stmmac_release_ptp(priv);

which leaves the MAC timestamp source armed with nobody left to read
XGMAC_TIMESTAMP_STATUS and clear TSIS; on an IRQF_SHARED line that shows up
as unhandled interrupts for the co-owner, and stmmac_interrupt() also returns
early on STMMAC_DOWN without acknowledging the source.

Would arming this on demand from the PTP_CLK_REQ_EXTTS path via
dwxgmac2_irq_modify(), the way dwmac1000 does, avoid both problems?

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

[Severity: High]
Should this read be gated on the MAC interrupt status bit first?  The
dwmac4 implementation of the same callback checks before touching the
timestamp status register:

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

  if (!tsync_int)
	  return;

The XGMAC equivalent, XGMAC_INT_TSIS, is defined in dwxgmac2.h but after
this series git grep XGMAC_INT_TSIS matches only its own definition, and
dwxgmac2_host_irq_status() does not consume it either.

Without that gate the clear-on-read register is read on every MAC host
interrupt:

  stmmac_interrupt()
    stmmac_common_interrupt()
      stmmac_timestamp_interrupt()
        readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS)

and in the legacy shared-IRQ configuration stmmac_common_interrupt() runs
before stmmac_dma_interrupt(), so under traffic that read happens for every
RX/TX DMA, LPI, PMT, MTL and EST interrupt as well.

The same register is polled from NAPI/softirq by the pre-existing
dwxgmac2_get_mac_tx_timestamp():

  if (readl_poll_timeout_atomic(ioaddr + XGMAC_TIMESTAMP_STATUS,
				value, value & XGMAC_TXTSC, 100, 10000))
	  return -EBUSY;

reached via stmmac_get_tx_hwtstamp() and stmmac_xsk_fill_timestamp(), with
nothing serializing it against the hardirq read.

The comment above also seems to argue both sides.  If reading
XGMAC_TIMESTAMP_STATUS clears TSIS, as the comment states and as the sibling
patch "net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS"
asserts, does the whole register not go read-clear, including TXTSC?  In that
case the interrupt handler consumes the TX-timestamp-captured status before
the TX clean path sees it, and readl_poll_timeout_atomic() spins its full
10 ms budget in atomic context and returns -EBUSY, losing the MAC level TX
hardware timestamp.

If instead TXTSC really is cleared only by reading XGMAC_TXTIMESTAMP_SEC as
the comment claims, then nothing here drains it, so TSIS stays asserted
whenever a MAC level TX timestamp is captured but never fetched (descriptor
level status in use, or hwts_tx_en turned off).  With XGMAC_TSIE now
unmasked, would that not re-fire the level triggered MAC interrupt
immediately?

Could the comment be reconciled with the databook and the missing
XGMAC_INT_TSIS check added?

> +
> +	if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN))
> +		return;
> +
> +	pending_snapshots = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, ts_status);
> +	if (!pending_snapshots)
> +		return;
> +
> +	acr_value = readl(priv->ptpaddr + PTP_ACR);
> +	channel = FIELD_GET(PTP_ACR_MASK, acr_value);
> +	if (!channel)
> +		return;
> +	channel = ilog2(channel);

[Severity: Medium]
Can this read of PTP_ACR see a half-programmed value?  stmmac_enable() does
its read-modify-write of PTP_ACR under priv->aux_ts_lock, and sets the flag
before the register write:

  priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
  ...
  writel(acr_value, ptpaddr + PTP_ACR);

priv->aux_ts_lock is a mutex ("Protects auxiliary snapshot registers from
concurrent access." in stmmac.h), so a hardirq handler cannot take it.  An
interrupt landing in that window sees the flag set with a stale PTP_ACR and
reports the snapshot with the wrong event.index; on the disable path it can
see the flag still set with the mask already cleared, take the
if (!channel) return; exit and drop the pending ATSNS snapshots.

priv->plat->flags itself is now a plain non-atomic |= / &= shared with a
hardirq reader, with no READ_ONCE()/WRITE_ONCE() or barrier.  Is that
intentional?

[Severity: High]
This isn't a bug introduced by this patch, but the guard added here is
missing from the shared dwmac4 handler that this new code was modelled on.
In timestamp_interrupt() in stmmac_hwtstamp.c:

  channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));

For a non-constant u32, ilog2() expands to __ilog2_u32(n) = fls(n) - 1, so
ilog2(0) is -1.  Stored into the u32 channel and then into event.index it
becomes -1, and ptp_clock_event() uses it without range validation:

  drivers/ptp/ptp_clock.c:ptp_clock_event() {
	  ...
	  if (test_bit((unsigned int)event->index, tsevq->mask))
	  ...
  }

tsevq->mask is bitmap_alloc(PTP_MAX_CHANNELS, ...), i.e. 2048 bits, so bit
index 0xFFFFFFFF is a read roughly 512 MB past a 256-byte allocation, taken
from hard IRQ context.

The window is the same one described above: stmmac_enable() sets
STMMAC_FLAG_EXT_SNAPSHOT_EN before writing PTP_ACR, and if a timestamp
interrupt arrives with ATSNS non-zero (snapshots left in the FIFO from a
previous session, since ATSNS is cleared only by PTP_ACR_ATSFC) the handler
sees flag set and mask zero.

Would it make sense to move the if (!channel) return; check into the shared
handler as part of this series, so dwmac4/dwmac410/dwmac510 get it too?

> +
> +	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]
Does the read side of priv->ptp_lock give any exclusion here?  This loop pops
the auxiliary snapshot FIFO, and get_ptptime() is a two-register consuming
access:

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

An rwlock read side does not exclude other readers, and on XGMAC there is a
second consumer of the same FIFO in process context, smtg_crosststamp() in
dwmac-socfpga.c:

  for (i = 0; i < num_snapshot; i++) {
	  read_lock_irqsave(&priv->ptp_lock, flags);
	  stmmac_get_ptptime(priv, ptpaddr, &ptp_time);

The only thing keeping the two apart appears to be an unlocked flag test on
each side:

  drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:smtg_crosststamp() {
	  if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
		  return -EBUSY;
	  ...
  }

Since that check happens well before the drain loop (with 10 ms
readl_poll_timeout budgets in between), can a PTP_EXTTS_REQUEST arriving
after the check arm the interrupt path while smtg_crosststamp() is still
draining, letting both pop the same FIFO and interleave their PTP_ATNR /
PTP_ATSR reads?

> +
> +		event.type = PTP_CLOCK_EXTTS;
> +		event.index = channel;

[Severity: Medium]
This is a pre-existing issue, but it interacts with the new index
derivation.  In stmmac_enable(), the PTP_CLK_REQ_EXTTS path clears the
channel mask before testing it:

  acr_value &= ~PTP_ACR_MASK;

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

so the tested field is always zero and the -EBUSY rejection of a second
active channel never fires.  Since stmmac_ptp_register() advertises
dma_cap.aux_snapshot_n channels, a second PTP_EXTTS_REQUEST succeeds and
silently replaces the channel that was enabled.

With that, does the index computed here get attached to the wrong events?
Snapshots latched while the first channel was armed would be reported under
the replacement channel's index.

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

[ ... remaining hunks in hwif.c, hwif.h, stmmac_hwtstamp.c and
stmmac_ptp.h snipped ... ]
-- 
This is an AI-generated review.

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

[zhu xin]

Thanks.  The content matches the review Jakub forwarded earlier.
In short, v6:

  - arms XGMAC_TSIE on demand from the PTP_CLK_REQ_EXTTS path instead
    of adding it back to XGMAC_INT_DEFAULT_EN, addressing the
    30300d9f9150 revert concern and the missing disable counterpart;
  - rewrites the patch 2 commit message (drops the inaccurate
    config_hw_tstamping and AUXTSTRIG statements) and adds
    Fixes
  - adds a third patch that moves the !channel guard into the shared
    handler;

The one point I push back on is the suggestion to gate the handler's
status read on XGMAC_INT_TSIS: TSIS is aggregate and read-to-clear,
and the TX completion path polls the same register for TXTSC, which
clears TSIS before the handler can observe it while ATSNS still holds
the pending snapshots - a TSIS gate would silently drop real events.
TXTSC itself is cleared by reading XGMAC_TXTIMESTAMP_SEC, not
XGMAC_TIMESTAMP_STATUS, so there is no conflict with the TX timestamp.

v6 will be posted shortly.

Best regards,
Zxyan

On Tue, Sep 15, 2026 at 3:08 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support
>
> Adds a DWXGMAC2-specific timestamp interrupt handler that reads
> XGMAC_TIMESTAMP_STATUS instead of the dwmac4 offset, derives the pending
> auxiliary snapshot count from ATSNS and emits PTP_CLOCK_EXTTS events.  It
> also adds XGMAC_TSIE to XGMAC_INT_DEFAULT_EN and points the DWXGMAC2 and
> DWXLGMAC2 hwif entries at the new hwtimestamp ops.
>
> A few questions on the commit message and on the new handler.
>
> [Severity: Medium]
> The commit message says:
>
>   "used &stmmac_ptp, whose timestamp_interrupt callback read the wrong
>    register and whose config_hw_tstamping callback never enabled the
>    XGMAC timestamp interrupt (XGMAC_TSIE was not in XGMAC_INT_DEFAULT_EN)"
>
> Is the config_hw_tstamping part accurate?  The new ops struct installs the
> very same function that stmmac_ptp uses:
>
>   const struct stmmac_hwtimestamp dwxgmac2_ptp = {
>         .config_hw_tstamping = config_hw_tstamping,
>
> and config_hw_tstamping() only does a read-modify-write of PTP_TCR; it never
> touches XGMAC_INT_EN.  The single hunk that actually unmasks the interrupt is
> the XGMAC_INT_DEFAULT_EN change, which dwxgmac2_core_init() writes to
> XGMAC_INT_EN.
>
> The second bullet says the handler:
>
>   "- derives the pending auxiliary snapshot count from the persistent
>      ATSNS field instead of the transient AUXTSTRIG status bit"
>
> Is anything changing here?  The generic handler being replaced already used
> ATSNS:
>
>   num_snapshot = (ts_status & GMAC_TIMESTAMP_ATSNS_MASK) >>
>                  GMAC_TIMESTAMP_ATSNS_SHIFT;
>
> and no AUXTSTRIG bit is defined for XGMAC at all (only
> GMAC_TIMESTAMP_AUXTSTRIG in dwmac4.h).
>
> Could the message be reworded so it is clear which hunk is load-bearing?  As
> written, someone backporting could take the ops table and handler hunks
> without the XGMAC_INT_DEFAULT_EN hunk and still get no events.
>
> [Severity: Low]
> The body describes a user-visible failure:
>
>   "As a result, auxiliary snapshot events were never reported on XGMAC
>    platforms."
>
> stmmac_ptp_register() already advertises the channels on XGMAC:
>
>   n_ext_ts = priv->dma_cap.aux_snapshot_n;
>   if (n_ext_ts)
>           priv->ptp_clock_ops.n_ext_ts = n_ext_ts;
>
> and stmmac_enable() accepts PTP_CLK_REQ_EXTTS, so PTP_EXTTS_REQUEST succeeds
> today but no events arrive.  Should this carry a Fixes: tag, or should the
> subject drop "add ... support" and state explicitly that EXTTS never worked
> on this hardware?  Without either, it is hard to tell which trees are
> affected.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> > index f8ab347f7b5b9..03cbae326995b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> > @@ -87,7 +87,7 @@
> >  #define XGMAC_TSIE                   BIT(12)
> >  #define XGMAC_LPIIE                  BIT(5)
> >  #define XGMAC_PMTIE                  BIT(4)
> > -#define XGMAC_INT_DEFAULT_EN         (XGMAC_LPIIE | XGMAC_PMTIE)
> > +#define XGMAC_INT_DEFAULT_EN         (XGMAC_LPIIE | XGMAC_PMTIE | XGMAC_TSIE)
>
> [Severity: High]
> Does this revert commit 30300d9f9150 ("net: stmmac: xgmac: Disable the
> Timestamp interrupt by default")?  git blame on this line points at that
> commit, which removed exactly this bit with the reasoning that XGMAC only
> polls for timestamps in the current software implementation and that dropping
> the interrupt "greatly reduces the system load by reducing the number of
> interrupts".  That commit carried a Fixes: tag.
>
> dwxgmac2_core_init() programs this unconditionally on every ifup:
>
>   writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
>
> with no check of PTP capability and no check that anything consumes
> auxiliary snapshots.  On platforms that never set
> STMMAC_FLAG_EXT_SNAPSHOT_EN the new handler returns immediately after one
> side-effecting register read, so every aux-snapshot trigger, target-time
> match, seconds overflow and TX timestamp capture becomes an interrupt with
> no consumer.
>
> Is there also a missing disable counterpart?  XGMAC_INT_EN has only two
> writers, dwxgmac2_core_init() and dwxgmac2_irq_modify(), and the latter is
> used only for XGMAC_FPEIE and the PCS masks, so TSIE is never cleared.
> __stmmac_release() frees the IRQ lines before releasing PTP:
>
>   /* Free the IRQ lines */
>   stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
>   ...
>   stmmac_release_ptp(priv);
>
> which leaves the MAC timestamp source armed with nobody left to read
> XGMAC_TIMESTAMP_STATUS and clear TSIS; on an IRQF_SHARED line that shows up
> as unhandled interrupts for the co-owner, and stmmac_interrupt() also returns
> early on STMMAC_DOWN without acknowledging the source.
>
> Would arming this on demand from the PTP_CLK_REQ_EXTTS path via
> dwxgmac2_irq_modify(), the way dwmac1000 does, avoid both problems?
>
> >  #define XGMAC_Qx_TX_FLOW_CTRL(x)     (0x00000070 + (x) * 4)
> >  #define XGMAC_PT                     GENMASK(31, 16)
> >  #define XGMAC_TFE                    BIT(1)
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > index 1a88cbaed70ca..891b7ceea2d45 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > @@ -1154,6 +1154,49 @@ static int dwxgmac2_get_mac_tx_timestamp(struct mac_device_info *hw, u64 *ts)
> >       return 0;
> >  }
> >
> > +void dwxgmac2_timestamp_interrupt(struct stmmac_priv *priv)
> > +{
> > +     u32 ts_status, pending_snapshots, acr_value, channel;
> > +     struct ptp_clock_event event;
> > +     unsigned long flags;
> > +     u64 ptp_time;
> > +     int i;
> > +
> > +     /* Read XGMAC_TIMESTAMP_STATUS to get the AUX snapshot
> > +      * count.  This read also clears the TSIS bit in
> > +      * XGMAC_INT_STATUS.
> > +      * TX timestamp polling may have already cleared TSIS
> > +      * and AUXTSTRIG, so rely on ATSNS instead.
> > +      * TXTSC is cleared by XGMAC_TXTIMESTAMP_SEC, not by
> > +      * this register, so there is no conflict.
> > +      */
> > +     ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
>
> [Severity: High]
> Should this read be gated on the MAC interrupt status bit first?  The
> dwmac4 implementation of the same callback checks before touching the
> timestamp status register:
>
>   tsync_int = readl(priv->ioaddr + GMAC_INT_STATUS) & GMAC_INT_TSIE;
>
>   if (!tsync_int)
>           return;
>
> The XGMAC equivalent, XGMAC_INT_TSIS, is defined in dwxgmac2.h but after
> this series git grep XGMAC_INT_TSIS matches only its own definition, and
> dwxgmac2_host_irq_status() does not consume it either.
>
> Without that gate the clear-on-read register is read on every MAC host
> interrupt:
>
>   stmmac_interrupt()
>     stmmac_common_interrupt()
>       stmmac_timestamp_interrupt()
>         readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS)
>
> and in the legacy shared-IRQ configuration stmmac_common_interrupt() runs
> before stmmac_dma_interrupt(), so under traffic that read happens for every
> RX/TX DMA, LPI, PMT, MTL and EST interrupt as well.
>
> The same register is polled from NAPI/softirq by the pre-existing
> dwxgmac2_get_mac_tx_timestamp():
>
>   if (readl_poll_timeout_atomic(ioaddr + XGMAC_TIMESTAMP_STATUS,
>                                 value, value & XGMAC_TXTSC, 100, 10000))
>           return -EBUSY;
>
> reached via stmmac_get_tx_hwtstamp() and stmmac_xsk_fill_timestamp(), with
> nothing serializing it against the hardirq read.
>
> The comment above also seems to argue both sides.  If reading
> XGMAC_TIMESTAMP_STATUS clears TSIS, as the comment states and as the sibling
> patch "net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS"
> asserts, does the whole register not go read-clear, including TXTSC?  In that
> case the interrupt handler consumes the TX-timestamp-captured status before
> the TX clean path sees it, and readl_poll_timeout_atomic() spins its full
> 10 ms budget in atomic context and returns -EBUSY, losing the MAC level TX
> hardware timestamp.
>
> If instead TXTSC really is cleared only by reading XGMAC_TXTIMESTAMP_SEC as
> the comment claims, then nothing here drains it, so TSIS stays asserted
> whenever a MAC level TX timestamp is captured but never fetched (descriptor
> level status in use, or hwts_tx_en turned off).  With XGMAC_TSIE now
> unmasked, would that not re-fire the level triggered MAC interrupt
> immediately?
>
> Could the comment be reconciled with the databook and the missing
> XGMAC_INT_TSIS check added?
>
> > +
> > +     if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN))
> > +             return;
> > +
> > +     pending_snapshots = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, ts_status);
> > +     if (!pending_snapshots)
> > +             return;
> > +
> > +     acr_value = readl(priv->ptpaddr + PTP_ACR);
> > +     channel = FIELD_GET(PTP_ACR_MASK, acr_value);
> > +     if (!channel)
> > +             return;
> > +     channel = ilog2(channel);
>
> [Severity: Medium]
> Can this read of PTP_ACR see a half-programmed value?  stmmac_enable() does
> its read-modify-write of PTP_ACR under priv->aux_ts_lock, and sets the flag
> before the register write:
>
>   priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
>   ...
>   writel(acr_value, ptpaddr + PTP_ACR);
>
> priv->aux_ts_lock is a mutex ("Protects auxiliary snapshot registers from
> concurrent access." in stmmac.h), so a hardirq handler cannot take it.  An
> interrupt landing in that window sees the flag set with a stale PTP_ACR and
> reports the snapshot with the wrong event.index; on the disable path it can
> see the flag still set with the mask already cleared, take the
> if (!channel) return; exit and drop the pending ATSNS snapshots.
>
> priv->plat->flags itself is now a plain non-atomic |= / &= shared with a
> hardirq reader, with no READ_ONCE()/WRITE_ONCE() or barrier.  Is that
> intentional?
>
> [Severity: High]
> This isn't a bug introduced by this patch, but the guard added here is
> missing from the shared dwmac4 handler that this new code was modelled on.
> In timestamp_interrupt() in stmmac_hwtstamp.c:
>
>   channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
>
> For a non-constant u32, ilog2() expands to __ilog2_u32(n) = fls(n) - 1, so
> ilog2(0) is -1.  Stored into the u32 channel and then into event.index it
> becomes -1, and ptp_clock_event() uses it without range validation:
>
>   drivers/ptp/ptp_clock.c:ptp_clock_event() {
>           ...
>           if (test_bit((unsigned int)event->index, tsevq->mask))
>           ...
>   }
>
> tsevq->mask is bitmap_alloc(PTP_MAX_CHANNELS, ...), i.e. 2048 bits, so bit
> index 0xFFFFFFFF is a read roughly 512 MB past a 256-byte allocation, taken
> from hard IRQ context.
>
> The window is the same one described above: stmmac_enable() sets
> STMMAC_FLAG_EXT_SNAPSHOT_EN before writing PTP_ACR, and if a timestamp
> interrupt arrives with ATSNS non-zero (snapshots left in the FIFO from a
> previous session, since ATSNS is cleared only by PTP_ACR_ATSFC) the handler
> sees flag set and mask zero.
>
> Would it make sense to move the if (!channel) return; check into the shared
> handler as part of this series, so dwmac4/dwmac410/dwmac510 get it too?
>
> > +
> > +     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]
> Does the read side of priv->ptp_lock give any exclusion here?  This loop pops
> the auxiliary snapshot FIFO, and get_ptptime() is a two-register consuming
> access:
>
>   ns = readl(ptpaddr + PTP_ATNR);
>   ns += (u64)readl(ptpaddr + PTP_ATSR) * NSEC_PER_SEC;
>
> An rwlock read side does not exclude other readers, and on XGMAC there is a
> second consumer of the same FIFO in process context, smtg_crosststamp() in
> dwmac-socfpga.c:
>
>   for (i = 0; i < num_snapshot; i++) {
>           read_lock_irqsave(&priv->ptp_lock, flags);
>           stmmac_get_ptptime(priv, ptpaddr, &ptp_time);
>
> The only thing keeping the two apart appears to be an unlocked flag test on
> each side:
>
>   drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c:smtg_crosststamp() {
>           if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
>                   return -EBUSY;
>           ...
>   }
>
> Since that check happens well before the drain loop (with 10 ms
> readl_poll_timeout budgets in between), can a PTP_EXTTS_REQUEST arriving
> after the check arm the interrupt path while smtg_crosststamp() is still
> draining, letting both pop the same FIFO and interleave their PTP_ATNR /
> PTP_ATSR reads?
>
> > +
> > +             event.type = PTP_CLOCK_EXTTS;
> > +             event.index = channel;
>
> [Severity: Medium]
> This is a pre-existing issue, but it interacts with the new index
> derivation.  In stmmac_enable(), the PTP_CLK_REQ_EXTTS path clears the
> channel mask before testing it:
>
>   acr_value &= ~PTP_ACR_MASK;
>
>   if (on) {
>           if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
>                   ... return -EBUSY;
>
> so the tested field is always zero and the -EBUSY rejection of a second
> active channel never fires.  Since stmmac_ptp_register() advertises
> dma_cap.aux_snapshot_n channels, a second PTP_EXTTS_REQUEST succeeds and
> silently replaces the channel that was enabled.
>
> With that, does the index computed here get attached to the wrong events?
> Snapshots latched while the first channel was armed would be reported under
> the replacement channel's index.
>
> > +             event.timestamp = ptp_time;
> > +             ptp_clock_event(priv->ptp_clock, &event);
> > +     }
> > +}
> > +
>
> [ ... remaining hunks in hwif.c, hwif.h, stmmac_hwtstamp.c and
> stmmac_ptp.h snipped ... ]
> --
> This is an AI-generated review.
>