On Nov 12, Maxime Chevallier wrote:
> 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.

I used the same approach used in stmmac_adjust_time(). I guess we can update
both of them if required.

Regards,
Lorenzo

> 
> 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,
>