Re: [PATCH net] net: stmmac: re-base EST after PTP settime64

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · 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 gets stmmac_adjust_time() wrong, and it
  uses that function as the model for the new code.
- [Medium] stmmac_adjust_time() and stmmac_set_time() are the .adjtime
  and .settime64 callbacks in the same ops tables (stmmac_ptp.c lines…
- [Medium] In stmmac_set_time() the new code ignores the return values of
  both stmmac_est_configure(..., false) and __stmmac_setup_est(), and…

--

net: stmmac: re-base EST after PTP settime64

This patch makes stmmac_set_time() take est_lock and disable EST before
it steps the PHC with stmmac_init_systime(). Afterwards it re-bases EST
through __stmmac_setup_est(). The goal is to stop EST from running with
a stale base time after clock_settime() on the PHC.

> stmmac_set_time() initializes the PTP clock through
> stmmac_init_systime() (TSINIT) while holding only ptp_lock.  It neither
> takes est_lock nor disables and re-bases EST around the step, unlike
> stmmac_adjust_time(), which performs TSUPDT through
> stmmac_adjust_systime() under est_lock.

[ ... ]

> Take est_lock for the whole disable/step/reprogram sequence, as
> stmmac_adjust_time() does: disable EST before initializing the clock and
> call __stmmac_setup_est() afterwards so the base time is recomputed from
> the reserved taprio base time and the new clock.

[Severity: Medium]
Does this match what stmmac_adjust_time() does? In stmmac_ptp.c,
stmmac_adjust_time() releases est_lock before the step and takes it
again afterwards:

stmmac_adjust_time() {
    ...
	mutex_lock(&priv->est_lock);
	if (priv->est && priv->est->enable)
		stmmac_est_configure(priv, priv, priv->est,
				     priv->plat->clk_ptp_rate, false);
	mutex_unlock(&priv->est_lock);

	write_lock_irqsave(&priv->ptp_lock, flags);
	stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
	write_unlock_irqrestore(&priv->ptp_lock, flags);

	mutex_lock(&priv->est_lock);
	if (priv->est && priv->est->enable)
		__stmmac_setup_est(priv);
	mutex_unlock(&priv->est_lock);
    ...
}

So TSUPDT runs under ptp_lock only, not under est_lock. The new
stmmac_set_time() holds est_lock across the write side of ptp_lock, and
stmmac_adjust_time() never nests the locks that way.

The est_lock -> ptp_lock (read) ordering does already exist through
__stmmac_setup_est()->gettimex64(), and stmmac_init_systime() does use
readl_poll_timeout_atomic(). That part of the ordering argument holds. The
precedent of stmmac_adjust_time() holding est_lock across the step does
not.

Could the commit message describe what stmmac_adjust_time() actually
does?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 31a66e855d6b1..6370bdc6c58b5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -129,10 +129,19 @@ static int stmmac_set_time(struct ptp_clock_info *ptp,
>  	    container_of(ptp, struct stmmac_priv, ptp_clock_ops);
>  	unsigned long flags;
>  
> +	mutex_lock(&priv->est_lock);

[Severity: Medium]
stmmac_set_time() now holds est_lock for the whole disable/step/re-base
sequence. Its sibling .adjtime callback, stmmac_adjust_time(), still
releases est_lock between disabling EST and the TSUPDT step (see the
snippet above).

Can tc_taprio_configure() still get into that window?

CPU1                                  CPU2
stmmac_adjust_time()
  mutex_lock(&priv->est_lock)
  stmmac_est_configure(..., false)
  mutex_unlock(&priv->est_lock)
                                      tc_taprio_configure()
                                        mutex_lock(&priv->est_lock)
                                        gettimex64()  /* pre-step clock */
                                        stmmac_est_configure(..., true)
                                        priv->est->enable = true
                                        mutex_unlock(&priv->est_lock)
  write_lock_irqsave(&priv->ptp_lock)
  stmmac_adjust_systime()  /* EST enabled in hardware */
  write_unlock_irqrestore(&priv->ptp_lock)
  mutex_lock(&priv->est_lock)
  __stmmac_setup_est()

In that case EST is running in hardware while the clock is stepped. That
goes against the "If EST is enabled, disabled it before adjust ptp time"
comment in stmmac_adjust_time(). The later __stmmac_setup_est() re-base
corrects the base time, so the effect lasts only as long as the window.

The commit message treats stmmac_adjust_time() as already covering this
case. Should stmmac_adjust_time() also hold est_lock across the step, so
that the .adjtime and .settime64 paths behave the same way?

> +	if (priv->est && priv->est->enable)
> +		stmmac_est_configure(priv, priv, priv->est,
> +				     priv->plat->clk_ptp_rate, false);
> +
>  	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);
>  
> +	if (priv->est && priv->est->enable)
> +		__stmmac_setup_est(priv);
> +	mutex_unlock(&priv->est_lock);
> +
>  	return 0;
>  }

[Severity: Medium]
Should the return values of stmmac_est_configure(..., false) and
__stmmac_setup_est() be checked here?

est_configure() in stmmac_est.c writes each register through est_write().
est_write() polls with readl_poll_timeout() and can return -ETIMEDOUT. On
any failure, est_configure() returns early, before it writes EST_CONTROL
and EST_INT_EN:

est_configure() {
    ...
	if (ret)
		return ret;

	for (i = 0; i < cfg->gcl_size; i++) {
		ret = est_write(est_addr, i, cfg->gcl[i], true);
		if (ret)
			return ret;
	}
    ...
}

If the disable call fails, EEST stays set and stmmac_init_systime()
steps the clock while EST is still running. That is the situation this
patch is meant to avoid.

If the disable succeeds but the re-enable inside __stmmac_setup_est()
fails, EST stays off in hardware. __stmmac_setup_est() only logs the
error:

__stmmac_setup_est() {
    ...
	if (err) {
		netdev_err(priv->dev, "failed to re-configure EST\n");
		return err;
	}
    ...
}

priv->est->enable is still true and stmmac_set_time() returns 0. Taprio
then believes the gate schedule is active until taprio is reconfigured.
On a similar failure, tc_taprio_configure() clears priv->est->enable, and
stmmac_setup_est() passes the error up.

stmmac_adjust_time() already ignores these return values in the same way.
This patch adds the same pattern to the settime64 path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-stmmac_set_time-est_lock-v1-1-da1554f233e6%40oss.qualcomm.com