Re: [PATCH net v2] net: stmmac: propagate PTP addend and system time programming errors

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 4 (1) · Low: 2

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).

- [High] The fix stops at returning an error.
- [Medium] The patch is titled 'propagate PTP addend and system time
  programming errors', but the PHC callbacks that call the same hooks…
- [Medium] The new failure paths leave timestamping enabled with stale
  state.
- [Medium] The patch reorders stmmac_dl_ts_coarse_set() so that
  priv->systime_flags is copied into a local variable at entry, all…
- [Medium] stmmac_restore_subsecond_increment() calls
  stmmac_config_addend() unconditionally and ignores its result,…
- [Low] stmmac_dl_ts_coarse_set() can now return -EINVAL (sub-second
  increment computed as 0) or -ETIMEDOUT (TSADDREG poll timeout in…
- [Low] The new rollback in stmmac_update_subsecond_increment() (line
  658) passes priv->default_addend to…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the devlink
  'phc_coarse_adj' param (priv->tsfupdt_coarse) is not honoured when…
- [Medium] This problem wasn't introduced by this patch, but
  stmmac_update_subsecond_increment() and the new…

--

net: stmmac: propagate PTP addend and system time programming errors

This patch makes stmmac_update_subsecond_increment(),
stmmac_init_tstamp_counter() and stmmac_dl_ts_coarse_set() return the
errors from addend and system time programming instead of always
returning success. It also adds stmmac_restore_subsecond_increment() to
roll back the PTP configuration when programming fails.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..fae2a90283ce4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -601,31 +601,63 @@ static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
>  	}
>  }
>  
> -static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
> +static void stmmac_restore_subsecond_increment(struct stmmac_priv *priv,
> +					       u32 default_addend,
> +					       u32 systime_flags)
>  {
>  	bool xmac = dwmac_is_xmac(priv->plat->core_type);
>  	u32 sec_inc = 0;
> +
> +	stmmac_config_addend(priv, priv->ptpaddr, default_addend);
> +	stmmac_config_hw_tstamping(priv, priv->ptpaddr, systime_flags);
> +	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
> +					   priv->plat->clk_ptp_rate,
> +					   xmac, &sec_inc);
> +	priv->default_addend = default_addend;
> +	priv->sub_second_inc = sec_inc;
> +}

[Severity: Medium]
Is it safe to call stmmac_config_addend() again here and ignore its
result?

When this rollback runs because config_addend() has just timed out in
stmmac_update_subsecond_increment(), PTP_TCR_TSADDREG may still be set.
One way to get there is a PTP ref clock domain that is not running,
since stmmac_setup_ptp() only warns when clk_prepare_enable() fails.
config_addend() then overwrites PTP_TAR and sets TSADDREG again while
the previous update is still pending:

stmmac_hwtstamp.c:config_addend() {
	writel(addend, ioaddr + PTP_TAR);
	/* issue command to update the addend value */
	value = readl(ioaddr + PTP_TCR);
	value |= PTP_TCR_TSADDREG;
	writel(value, ioaddr + PTP_TCR);
	...
}

The DWMAC programming model expects this bit to read 0 before it is set
again. The call also adds a second 100 ms readl_poll_timeout_atomic()
busy-wait, so the failing devlink path spins for about 200 ms.

If the pending update latches later, TAR could end up holding the old
value (or 0 on the first open after probe) rather than the one just
computed.

On the TSINIT timeout path in stmmac_init_tstamp_counter(), the TCR
read-modify-write in config_addend() also writes the still-pending
TSINIT bit back as 1, so the pending system time load is not cancelled.

In every case, the helper then sets priv->default_addend and
priv->sub_second_inc as if the old configuration had been restored in
hardware.

[Severity: Medium]
This isn't a bug introduced by this patch, but
stmmac_update_subsecond_increment() and this new helper program
PTP_TCR, PTP_SSIR and PTP_TAR, and update priv->default_addend and
priv->sub_second_inc, without holding priv->ptp_lock. Both run from the
devlink set path while the PHC is registered.

The PHC callbacks use that lock around the same registers:

stmmac_ptp.c:stmmac_adjust_freq() {
	...
	write_lock_irqsave(&priv->ptp_lock, flags);
	stmmac_config_addend(priv, priv->ptpaddr, addend);
	write_unlock_irqrestore(&priv->ptp_lock, flags);
	...
}

stmmac_enable() also reads priv->sub_second_inc and
priv->systime_flags under ptp_lock for PEROUT.

Could a concurrent adjfine and phc_coarse_adj devlink set lose a TCR
mode change, or program a TAR derived from a stale default_addend? This
error path adds one more unlocked config_addend() plus
config_hw_tstamping() sequence.

> +
> +static int stmmac_update_subsecond_increment(struct stmmac_priv *priv,
> +					     u32 systime_flags)
> +{

[ ... ]

> +	ret = stmmac_config_addend(priv, priv->ptpaddr, val);
> +	if (ret)
> +		goto error;
> +
> +	priv->sub_second_inc = sec_inc;
> +	priv->default_addend = val;
> +
> +	return 0;
> +error:
> +	/* Restore previous configuration */
> +	stmmac_restore_subsecond_increment(priv, priv->default_addend,
> +					   priv->systime_flags);

[Severity: Low]
Is priv->default_addend really the previous configuration here?
stmmac_adjust_freq() programs an adjusted addend into PTP_TAR but does
not update default_addend:

	addend = adjust_by_scaled_ppm(priv->default_addend, scaled_ppm);

If a phc_coarse_adj devlink set fails (for example with sec_inc == 0),
the rollback writes the nominal addend into PTP_TAR. That drops the
frequency correction the servo had applied, so a failed operation still
changes the PHC frequency, which the "Restore previous configuration"
comment doesn't reflect.

> +	return ret;
>  }
>  
>  /**
> @@ -864,25 +896,37 @@ static int stmmac_hwtstamp_get(struct net_device *dev,
>  static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
>  				      u32 systime_flags)
>  {

[ ... ]

>  	/* lower 32 bits of tv_sec are safe until y2106 */
> -	stmmac_init_systime(priv, priv->ptpaddr, (u32)now.tv_sec, now.tv_nsec);
> +	ret = stmmac_init_systime(priv, priv->ptpaddr, (u32)now.tv_sec,
> +				  now.tv_nsec);
> +	if (ret)
> +		goto error;

[Severity: Medium]
The subject says addend and system time programming errors are now
propagated, but the PHC callbacks that call the same hooks still drop
the result:

stmmac_ptp.c:stmmac_set_time() {
	...
	write_lock_irqsave(&priv->ptp_lock, flags);
	stmmac_init_systime(priv, priv->ptpaddr, ts->tv_sec, ts->tv_nsec);
	write_unlock_irqrestore(&priv->ptp_lock, flags);

	return 0;
}

stmmac_adjust_freq() also ignores the return of stmmac_config_addend()
and returns 0.

When the TSINIT or TSADDREG poll times out, clock_settime() or
clock_adjtime() from ptp4l or phc2sys still reports success. Should
settime64 and adjfine return the error too, since the ptp_clock_info
callbacks are allowed to fail?

> +
> +	priv->systime_flags = systime_flags;

[Severity: High]
This is a pre-existing issue, but this path is being reworked here:
stmmac_init_timestamping() always sets PTP_TCR_TSCFUPDT and ignores
priv->tsfupdt_coarse:

	ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
					       PTP_TCR_TSCFUPDT);

Suppose phc_coarse_adj is set to true and then the interface goes down
and up (or through suspend/resume). config_sub_second_increment() then
programs the fine mode SSINC (2e9 / clk_ptp_rate) and the ~2^31 addend.
priv->tsfupdt_coarse stays true, so stmmac_dl_ts_coarse_get() still
reports coarse mode.

The next SIOCSHWTSTAMP clears TSCFUPDT in stmmac_hwtstamp_set():

	priv->systime_flags = STMMAC_HWTS_ACTIVE;
	if (!priv->tsfupdt_coarse)
		priv->systime_flags |= PTP_TCR_TSCFUPDT;

It then only calls stmmac_config_hw_tstamping(), and SSIR and TAR are
not reprogrammed.

Does that leave the hardware in coarse mode, adding twice the clock
period on every cycle, so the PHC runs about twice as fast?

>  
>  	return 0;
> +error:
> +	/* Restore previous configuration */
> +	stmmac_restore_subsecond_increment(priv, default_addend,
> +					   priv->systime_flags);
> +
> +	return ret;
>  }

[Severity: Medium]
Can this failure path leave timestamping enabled with stale state?

stmmac_hwtstamp_set() sets priv->hwts_rx_en, priv->hwts_tx_en,
priv->tstamp_config and a priv->systime_flags value that includes TSENA
and the RX filter bits. None of these is cleared on close or suspend.

On a later open or resume, if stmmac_init_tstamp_counter() fails, this
rollback writes the old priv->systime_flags, filter bits included, back
into PTP_TCR. The rollback in stmmac_update_subsecond_increment() does
the same. stmmac_init_timestamping() then returns early:

	if (ret) {
		netdev_warn(priv->dev, "PTP init failed\n");
		return ret;
	}

and never reaches:

	memset(&priv->tstamp_config, 0, sizeof(priv->tstamp_config));
	priv->hwts_tx_en = 0;
	priv->hwts_rx_en = 0;

TX timestamp requests and RX timestamp delivery in
stmmac_get_rx_hwtstamp() would then keep running on a counter whose
init just failed, and stmmac_hwtstamp_get() reports the stale config.

Before this patch, these paths always reached the reset, and TCR was
left at STMMAC_HWTS_ACTIVE | PTP_TCR_TSCFUPDT with no filters.

[Severity: High]
The error now reaches stmmac_init_timestamping(), but do any callers
act on it?

stmmac_resume() ignores the result:

	stmmac_init_timestamping(priv);

	stmmac_init_coalesce(priv);

After the MAC reset in stmmac_hw_setup(), a TSADDREG or TSINIT timeout
here leaves the PHC registered at open still in place. gettime64,
adjfine, settime64 and tc_taprio_configure(), which calls gettime64 for
the EST base time, keep using a counter that was never re-initialized.

On open, stmmac_setup_ptp() only skips registration:

	if (stmmac_init_timestamping(priv) == 0)
		stmmac_ptp_register(priv);

The interface still comes up. stmmac_hwtstamp_set() only checks the
timestamp capability bits and netif_running(), so SIOCSHWTSTAMP can
still turn on packet timestamping with the uninitialized counter and
return 0.

The commit message describes the problem as the driver that "keeps
operating as if timestamping were up" and names TAPRIO/EST. On the
resume path, the only visible change seems to be the "PTP init failed"
warning. Isn't that still the situation this patch is meant to fix?

>  
>  /**
> @@ -7693,18 +7737,23 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
>  {
>  	struct stmmac_devlink_priv *dl_priv = devlink_priv(dl);
>  	struct stmmac_priv *priv = dl_priv->stmmac_priv;
> +	u32 systime_flags = priv->systime_flags;
> +	int ret;

[ ... ]

> -	stmmac_update_subsecond_increment(priv);
> +	ret = stmmac_update_subsecond_increment(priv, systime_flags);
> +	if (ret)
> +		return ret;

[Severity: Low]
This now returns -EINVAL or -ETIMEDOUT to devlink without filling in
the extack it receives. Would an NL_SET_ERR_MSG_MOD() help user space
understand why setting phc_coarse_adj failed?

> +
> +	priv->tsfupdt_coarse = ctx->val.vbool;
> +	priv->systime_flags = systime_flags;

[Severity: Medium]
Does moving these assignments after the hardware programming widen a
race with stmmac_hwtstamp_set()? The devlink set callback runs under
the devlink instance lock and stmmac_hwtstamp_set() runs under RTNL, so
the two paths share no lock.

CPU0 (devlink, vbool = true)
stmmac_dl_ts_coarse_set()
  systime_flags = priv->systime_flags, TSCFUPDT cleared
  stmmac_update_subsecond_increment()
    writes TCR in coarse mode and the coarse SSIR
    config_addend() polls TSADDREG for up to 100 ms

CPU1 (SIOCSHWTSTAMP)
stmmac_hwtstamp_set()
  reads priv->tsfupdt_coarse == false
  priv->systime_flags = ACTIVE | TSCFUPDT | filters
  writes TCR back to fine mode

CPU0
  priv->tsfupdt_coarse = true
  priv->systime_flags = stale copy, no TSCFUPDT and no filters

The hardware ends up in fine mode with the coarse SSIR and addend, so
the PHC runs at the wrong rate. priv->systime_flags then matches neither
the hardware nor the hwtstamp request, and it is used later by flex PPS
in stmmac_enable() and by the new restore paths.

The old code set tsfupdt_coarse before the MMIO programming, so the
window was only a few instructions. Here it covers the whole
programming sequence, including the poll.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-stmmac-ptp-added-systime-error-v2-1-beb2a6b5f866%40oss.qualcomm.com