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

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

Hi,

On 11/13/26 09:48, Lorenzo Bianconi wrote:
> 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.

Bah, I'm OK doing that as a followup then,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

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