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

[netdev-bot+sashiko@kernel.org]

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

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

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

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

--

net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

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

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

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

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

    mutex_lock(&priv->aux_ts_lock);

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

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

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

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

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

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

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

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

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

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

> +
> +	if (!(priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN) || !priv->ptp_clock)
> +		return;
> +
> +	pending_snapshots = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, ts_status);
> +	if (!pending_snapshots)
> +		return;
> +
> +	acr_value = readl(priv->ptpaddr + PTP_ACR);
> +	/* Entries observed while the FIFO is being flushed are stale. */
> +	if (acr_value & PTP_ACR_ATSFC)
> +		return;
> +	channel = FIELD_GET(PTP_ACR_MASK, acr_value);
> +	if (!channel)
> +		return;
> +	channel = ilog2(channel);
> +
> +	for (i = 0; i < pending_snapshots; i++) {
> +		read_lock_irqsave(&priv->ptp_lock, flags);
> +		stmmac_get_ptptime(priv, priv->ptpaddr, &ptp_time);
> +		read_unlock_irqrestore(&priv->ptp_lock, flags);

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

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

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

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

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

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

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

[ ... ]

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

[ ... ]

> @@ -239,22 +246,49 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
>  				return -EBUSY;
>  			}
>  
> +			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return -EINVAL;
> +			}
> +
>  			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = rq->extts.index;
>  
>  			/* Enable External snapshot trigger */
>  			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
>  			acr_value |= PTP_ACR_ATSFC;
>  		} else {
>  			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
>  		}

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

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

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

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

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

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

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

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

>  		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
>  			   rq->extts.index, on ? "enabled" : "disabled");
>  		writel(acr_value, ptpaddr + PTP_ACR);
> -		mutex_unlock(&priv->aux_ts_lock);
> -		/* wait for auxts fifo clear to finish */
> +		/* Keep the lock across the FIFO clear so the arm below
> +		 * cannot interleave with a suspend.
> +		 */
>  		ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
>  					 !(acr_value & PTP_ACR_ATSFC),
>  					 10, 10000);

[ ... ]

> +		/* Arm the timestamp interrupt only after the FIFO clear,
> +		 * so the handler cannot see a snapshot that is about to be
> +		 * discarded.
> +		 */
> +		stmmac_mac_timestamp_interrupt_cfg(priv, on && !ret);
> +		/* Drain an in-flight handler before the channel is freed. */
> +		if (!on)
> +			stmmac_sync_timestamp_irq(priv);
> +		mutex_unlock(&priv->aux_ts_lock);

[Severity: Medium]
The commit message says:

  "PTP_ACR_ATSFC is only ever set while XGMAC_TSIE is disabled, so this
  both keeps it off the FIFO on unrelated interrupts and stops it racing
  a FIFO flush."

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

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

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

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

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

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

[ ... ]

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