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

[Jakub Kicinski <kuba@kernel.org>]

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?