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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Lorenzo,

On 11/11/26 10:54, Lorenzo Bianconi wrote:
> 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.
> 
> Both operations change the PHC system time, so both invalidate the
> absolute EST base time.  With settime64, a concurrent
> tc_taprio_configure() can read the pre-step clock through gettimex64()
> and program EST with a base time that is already stale once TSINIT
> lands.  Separately, a taprio offload installed before the clock_settime()
> is never re-based and keeps running with its old base time until taprio
> is reconfigured.
> 
> 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.
> 
> This keeps the established est_lock -> ptp_lock ordering:
> stmmac_init_systime() only uses readl_poll_timeout_atomic() under
> ptp_lock, and __stmmac_setup_est() takes ptp_lock through gettimex64().
> 
> Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

This LGTM, the return values from stmmac_est_configure/__stmmac_setup_est are 
ignored though, the [cl|s]ashiko gang may not like that.

Besides one of the EST commands timing out, it seems unlikely though.

Maxime

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 31a66e855d6b..6370bdc6c58b 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);
> +	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;
>  }
>  
> 
> ---
> base-commit: 22b7cb932c6a0f7566d41072be6956cb6c8af4e1
> change-id: 20261111-stmmac_set_time-est_lock-f74f4ce678b7
> 
> Best regards,