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