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

[zhu xin]

Hi,

Thanks for the review.  Findings 1, 3, 4 and 5 are valid, we agree and
will fix them in the
respin.

Finding 2 we'd like to keep as-is; reason below.

On Mon, Oct 5, 2026 at 8:17 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 5 (2) · 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] When the FIFO-clear poll times out in the EXTTS 'on' path of
>   stmmac_enable() (stmmac_ptp.c:262-275), the new rollback only sets…
> - [Medium] Auxiliary snapshots are only drained by
>   dwxgmac2_timestamp_interrupt(), called from stmmac_common_interrupt()…

The snapshot itself is not lost: ATSNS stays non-zero and the next entry
into the handler drains the FIFO.  What can be lost is only the
interrupt for that particular event - if the TX poll clears TSIS
entirely inside an already-active servicing window, the level is
already low at EOI and no new interrupt is latched - so delivery is
delayed, not dropped.

This is a deliberate trade-off, because the alternative (gating on
XGMAC_INT_TSIS like dwmac4) is worse: TSIS is aggregate and
read-to-clear, and dwxgmac2_get_mac_tx_timestamp() polls the same
register for TXTSC, so a TSIS gate would silently drop real EXTTS
events.  Note that switching the TX path to descriptor timestamps is
not possible on XGMAC: dwxgmac2_get_tx_timestamp_status() returns 0,
the MAC-level timestamp in XGMAC_TIMESTAMP_STATUS is the only source.
So the choice is a bounded delay vs. silent loss, and we pick the delay
(the snapshot is still in the FIFO and ATSNS tells the next handler
entry to drain it).

> - [Medium] dwxgmac2_timestamp_interrupt() (dwxgmac2_core.c:1163-1212)
>   reads STMMAC_FLAG_INT_SNAPSHOT_EN (line 1171),…

We will add the synchronize_irq().  The handler runs in hard IRQ and
cannot take aux_ts_lock, which is a mutex: each of the flush/drain paths
holds it across one or two readl_poll_timeout() calls (up to 10 ms
each), so the handler can never acquire it and the flag stays the only
exclusion the handler itself can read.

A spinlock would close the window fully, but it cannot hold these
paths: readl_poll_timeout() sleeps, so it cannot run under a spinlock.
Moving the polls out of the lock would reintroduce the concurrent
FIFO-flush race that patch 1 fixes, and busy-waiting under a spinlock
for up to 20 ms with interrupts disabled is not acceptable.  So a
spinlock is out of proportion here.

synchronize_irq() after raising STMMAC_FLAG_INT_SNAPSHOT_EN gives the
part that matters: any handler already past the flag check is drained
before the cross-timestamp proceeds, and any later handler sees the flag
set.  It needs to cover both the MAC IRQ and the separate WoL IRQ, since
stmmac_common_interrupt() runs from both.  One residual window remains:
an EXTTS enable's ATSFC flush can still complete between the handler's
ATSNS read and its ATSFC read, because that is a check-then-act within a
single handler pass that synchronize_irq() cannot serialize.

> - [Medium] In stmmac_enable(), the new XGMAC_TSIE arm/disarm call
>   (stmmac_ptp.c:269-270, stmmac_mac_timestamp_interrupt_cfg(priv, on))…
> - [Medium] The patch arms XGMAC_TSIE on demand
>   (dwxgmac2_timestamp_interrupt_cfg, dwxgmac2_core.c:45-49) and re-arms…
>
> Pre-existing issues:
> - [Medium] In the PTP_CLK_REQ_EXTTS case of stmmac_enable(), line 231
>   clears the ATSEN bits (`acr_value &= ~PTP_ACR_MASK`) before line 234…
> - [Medium] smtg_crosststamp() checks STMMAC_FLAG_EXT_SNAPSHOT_EN (dwmac-
>   socfpga.c:310) before it takes aux_ts_lock (line 313), and…
>
> --
>
> net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support
>
> This commit adds a DWXGMAC2-specific timestamp interrupt handler. The
> handler reads XGMAC_TIMESTAMP_STATUS and reports pending auxiliary
> snapshots as PTP_CLOCK_EXTTS events.
>
> XGMAC_TSIE is armed on demand from the EXTTS path of stmmac_enable()
> through a new timestamp_interrupt_cfg() callback. It is re-armed on
> resume by stmmac_rearm_timestamp_irq(). smtg_crosststamp() now sets
> STMMAC_FLAG_INT_SNAPSHOT_EN while it owns the snapshot FIFO.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > index c5f71bfc7cf42..9030cc1cc6778 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > @@ -311,6 +311,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
> >               return -EBUSY;
> >
> >       mutex_lock(&priv->aux_ts_lock);
> > +     priv->plat->flags |= STMMAC_FLAG_INT_SNAPSHOT_EN;
>
> [Severity: Medium]
> This is a pre-existing issue, but smtg_crosststamp() checks
> STMMAC_FLAG_EXT_SNAPSHOT_EN before it takes aux_ts_lock:
>
>         if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
>                 return -EBUSY;
>
>         mutex_lock(&priv->aux_ts_lock);
>
> Also, the enable path of stmmac_enable() never checks
> STMMAC_FLAG_INT_SNAPSHOT_EN. ptp_sys_offset_precise() does not take
> pincfg_mux, so a concurrent PTP_EXTTS_REQUEST can set
> STMMAC_FLAG_EXT_SNAPSHOT_EN and ATSEN(idx) inside that window.
> smtg_crosststamp() then overwrites PTP_ACR with the internal channel.
>
> Does that leave STMMAC_FLAG_EXT_SNAPSHOT_EN set, ext_snapshot_num = idx
> and TSIE armed, while the user's channel is no longer enabled in
> PTP_ACR? Later cross-timestamps would then return -EBUSY until the
> channel is disabled and enabled again.
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > index 1a88cbaed70ca..313c24e9a49d7 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
>
> [ ... ]
>
> > @@ -1154,6 +1160,57 @@ 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;
> > +
> > +     if (priv->plat->flags & STMMAC_FLAG_INT_SNAPSHOT_EN) {
> > +             /* Read the status to clear the timestamp interrupt source;
> > +              * the FIFO belongs to the cross-timestamp path.
> > +              */
> > +             readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
> > +             return;
> > +     }
> > +
> > +     /* Reading XGMAC_TIMESTAMP_STATUS clears the TSIS and AUXTSTRIG
> > +      * bits, so the ATSNS count is the only reliable indication of
> > +      * pending auxiliary snapshots.  TXTSC is cleared by
> > +      * XGMAC_TXTIMESTAMP_SEC and is not affected by this read.
> > +      */
> > +     ts_status = readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
>
> [Severity: Medium]
> The commit message says TSIS is "an aggregate, read-to-clear bit that
> the TX timestamp completion path (which polls the same register for
> TXTSC) clears before the handler can observe it". That covers the case
> where this handler runs and finds TSIS already clear. What happens if
> the handler never runs?
>
> dwxgmac2_get_mac_tx_timestamp() polls the same register:
>
>         if (readl_poll_timeout_atomic(ioaddr + XGMAC_TIMESTAMP_STATUS,
>                                       value, value & XGMAC_TXTSC, 100, 10000))
>
> Suppose one of those reads lands after an aux snapshot has set TSIS but
> before the CPU takes the MAC interrupt. TSIS was the only enabled
> source, so a level-triggered MAC interrupt line would deassert before
> stmmac_common_interrupt() runs.
>
> Only stmmac_common_interrupt() calls this handler, and nothing else
> checks ATSNS. Would the snapshot then stay in the FIFO until some
> unrelated MAC interrupt arrives? With a 1PPS source that could be about
> a second later. In multi-vector/MSI mode, DMA interrupts do not go
> through stmmac_common_interrupt(), so it could take much longer.
>
> The trigger is EXTTS enabled together with TX hardware timestamping.
> stmmac_get_tx_hwtstamp() falls back to stmmac_get_mac_tx_timestamp()
> when the descriptor has no timestamp. Whether the interrupt is really
> lost depends on how the interrupt line is triggered.
>
> > +
> > +     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]
> Are these checks enough to keep this loop off the FIFO? Each of
> STMMAC_FLAG_INT_SNAPSHOT_EN, STMMAC_FLAG_EXT_SNAPSHOT_EN and
> PTP_ACR_ATSFC is read once without a lock. The loop then drains a
> pending_snapshots count that was sampled earlier.
>
> Three process-context paths flush or consume the same FIFO:
> smtg_crosststamp(), the ATSFC flush in stmmac_enable(), and
> stmmac_rearm_timestamp_irq(). They serialize on aux_ts_lock, which this
> handler cannot take, and none of them calls synchronize_irq().
>
> ptp_lock does not help, because this loop and smtg_crosststamp() both
> take it as readers. Arming TSIE only after the flush does not
> synchronize anything either. stmmac_common_interrupt() calls this
> handler on every common interrupt whatever the TSIE state.
> stmmac_request_irq_single() also registers stmmac_interrupt on the
> separate WoL line, so two copies of this drain can run at the same time.
>
> Two windows look possible:
>
> (A) The handler samples a stale ATSNS. A concurrent enable then sets
> STMMAC_FLAG_EXT_SNAPSHOT_EN and its flush completes. The handler sees
> EXT set and ATSFC clear, and pops entries from the flushed FIFO as
> EXTTS events.
>
> (B) The handler is already past the STMMAC_FLAG_INT_SNAPSHOT_EN check
> and inside this loop when an EXTTS disable and then smtg_crosststamp()
> run. The loop pops the internal cross-timestamp snapshot and reports it
> as an EXTTS event, and smtg_crosststamp() gets the wrong device time.
>
> Doesn't this contradict two claims in the commit message, that "a
> stale snapshot is never reported as an event" and that the handler
> "leaves the snapshot FIFO alone while an internal cross-timestamp owns
> it"?
>
> smtg_crosststamp() sets STMMAC_FLAG_INT_SNAPSHOT_EN on entry and clears
> it on exit to keep the IRQ handler out, so the flag is acting as a
> lock. Would a spinlock shared by the handler and the flush/drain paths,
> or a synchronize_irq() after raising the flag, be more robust?
>
> The windows are narrow on non-RT kernels. They are wider on PREEMPT_RT,
> where the handler runs as a preemptible thread.
>
> > +
> > +             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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 3ad9252bf6aea..53aaf2716c467 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>
> [ ... ]
>
> > @@ -8418,6 +8467,8 @@ int stmmac_resume(struct device *dev)
> >               ret = stmmac_init_timestamping(priv);
> >               if (ret)
> >                       goto error_stop_dma;
> > +
> > +             stmmac_rearm_timestamp_irq(priv);
> >       }
>
> [Severity: Medium]
> TSIE is re-armed here on resume. Is there a matching disarm on suspend?
>
> With Wake-on-LAN configured, stmmac_suspend() does:
>
>         if (priv->wolopts) {
>                 stmmac_pmt(priv, priv->hw, priv->wolopts);
>                 priv->irq_wake = 1;
>
> It leaves XGMAC_INT_EN and PTP_ACR as they are, and dwxgmac2_pmt() only
> writes XGMAC_PMT and RX_CONFIG. Clocks stay on for WoL, so each
> external trigger edge, for example a GNSS 1PPS, sets TSIS and asserts
> the MAC interrupt. stmmac_common_interrupt() then does:
>
>         if (priv->irq_wake)
>                 pm_wakeup_event(priv->device, 0);
>
> Could this abort system suspend? Could it also wake the system after
> the noirq phase when the MAC IRQ is the wake IRQ? stmmac_pci uses
> pdev->irq, and stmmac_platform falls back to the MAC IRQ.
>
> Before this patch XGMAC_TSIE was never set on XGMAC, so this would be a
> new wake source. Whether the wake after noirq happens depends on how
> the platform wires the wake IRQ and gates clocks.
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > index 3bfcc9760dce7..19fa79839e465 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > @@ -239,13 +239,20 @@ 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]
> This isn't a bug introduced by this patch, but can the -EBUSY branch
> above ever be taken? Earlier in this case:
>
>                 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)) {
>
> The ATSEN bits are cleared before they are tested, so the test is
> always false.
>
> The disable branch also ignores rq->extts.index. ptp_extts_request()
> and extts_enable_store() only check index < n_ext_ts. So an enable or
> disable request for one channel can silently retarget or tear down a
> different active channel.
>
> After this patch that teardown also resets ext_snapshot_num and
> disarms XGMAC_TSIE.
>
> >               netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
> >                          rq->extts.index, on ? "enabled" : "disabled");
> > @@ -255,6 +262,17 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
> >               ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
> >                                        !(acr_value & PTP_ACR_ATSFC),
> >                                        10, 10000);
> > +             /* Arm or disarm the timestamp interrupt only once the FIFO
> > +              * clear has completed, so the handler does not observe a
> > +              * snapshot that the clear is about to discard.
> > +              */
> > +             if (!ret) {
> > +                     stmmac_mac_timestamp_interrupt_cfg(priv, on);
>
> [Severity: Medium]
> This call runs after aux_ts_lock was dropped just before the poll:
>
>                 writel(acr_value, ptpaddr + PTP_ACR);
>                 mutex_unlock(&priv->aux_ts_lock);
>
> extts_enable_store() calls ops->enable() without pincfg_mux, so it can
> race with the ioctl path or with another sysfs writer. Can a stale on
> or off value be applied here?
>
> disable request A                  enable request B
>   clear flag and ATSEN
>   mutex_unlock()
>   <preempted>
>                                      mutex_lock()
>                                      set flag, ATSEN|ATSFC
>                                      mutex_unlock()
>                                      poll completes
>                                      timestamp_interrupt_cfg(true)
>   poll completes
>   timestamp_interrupt_cfg(false)
>
> That would leave the channel B enabled with XGMAC_TSIE disarmed.
>
> The timeout branch below has the same problem. It re-takes the lock and
> sets ext_snapshot_num = -1 unconditionally, which could overwrite the
> channel recorded by a newer successful enable.
>
> > +             } else if (on) {
> > +                     mutex_lock(&priv->aux_ts_lock);
> > +                     priv->plat->ext_snapshot_num = -1;
> > +                     mutex_unlock(&priv->aux_ts_lock);
> > +             }
>
> [Severity: Medium]
> When the ATSFC poll times out on enable, only ext_snapshot_num is
> rolled back. STMMAC_FLAG_EXT_SNAPSHOT_EN and the PTP_ACR ATSEN(index)
> bit that were set earlier stay set. Is that intended?
>
> The readers of this state now disagree with each other:
>
> dwxgmac2_timestamp_interrupt() gates on the flag and on PTP_ACR. It runs
> from every stmmac_common_interrupt() even with TSIE disarmed. If ATSFC
> clears later, it can report PTP_CLOCK_EXTTS events for a request that
> returned an error.
>
> stmmac_rearm_timestamp_irq() sees the flag set but ext_snapshot_num < 0
> and skips the restore, so the behaviour changes across suspend and
> resume.
>
> smtg_crosststamp() keeps returning -EBUSY until an explicit disable or
> close.
>
> Should the failure path roll back the flag, the ATSEN bit and
> ext_snapshot_num together?
>
> >               break;
> >       }
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004120526.2728564-1-zxyan0222%40gmail.com

pw-bot: cr