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

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

[Lorenzo Bianconi]

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>
---
 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,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

the issue has been reported by sashiko.

> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by LLM and not proven at runtime). If it was triggered
>    please include the symptoms, like the stack trace or kernel messages.

this is a real issue reported by sashiko.

> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.

I tested it on a Qualcomm rb3-gen2 board.

Regards,
Lorenzo

> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Nicolai Buchwitz]

Hi Lorenzo

On 11.11.2026 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.

adjust_time() drops est_lock around the step, so this isn't quite right.
Same for "as stmmac_adjust_time() does" below. Please reword this if 
there
is a v2.

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

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

[Maxime Chevallier]

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,

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Maxime Chevallier]

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

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

[netdev-bot+sashiko]

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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 11117 bytes --]

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

This issue is already fixed in the following commit:

commit b0816917a4f7194cca81288fd339d285d8d793b5
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Date:   Mon Nov 2 18:37:56 2026 +0100

    net: stmmac: keep est_lock across PTP clock adjustment

    stmmac_adjust_time() disables EST, drops est_lock to step the PTP clock,
    and then reacquires the lock to reprogram EST using a base time computed
    from the new clock value.

    That leaves a window where a concurrent tc_taprio_configure() can program
    EST enabled with a base time derived from the pre-step clock.  The gate
    schedule then runs across the PTP step with a stale base time until
    stmmac_adjust_time() reprograms it.

    Keep est_lock held for the whole disable/step/reprogram sequence so no
    other EST user can program the block while the clock is being stepped.

    This does not introduce a new lock ordering: __stmmac_setup_est() already
    takes ptp_lock through gettimex64() while holding est_lock, and
    stmmac_adjust_systime() uses readl_poll_timeout_atomic(), so it does not
    sleep under ptp_lock.

    Cc: stable+noautosel@kernel.org # LLM report + LLM fix, not seen in real life
    Fixes: 23d96de472f7 ("net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()")
    Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
    Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
    Link: https://patch.msgid.link/20261102-stmmac-ptp-est-lock-v1-1-76fbc0cb230d@oss.qualcomm.com
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

This issue is already fixed in the following commit:

commit b0816917a4f7194cca81288fd339d285d8d793b5
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Date:   Mon Nov 2 18:37:56 2026 +0100

    net: stmmac: keep est_lock across PTP clock adjustment

    stmmac_adjust_time() disables EST, drops est_lock to step the PTP clock,
    and then reacquires the lock to reprogram EST using a base time computed
    from the new clock value.

    That leaves a window where a concurrent tc_taprio_configure() can program
    EST enabled with a base time derived from the pre-step clock.  The gate
    schedule then runs across the PTP step with a stale base time until
    stmmac_adjust_time() reprograms it.

    Keep est_lock held for the whole disable/step/reprogram sequence so no
    other EST user can program the block while the clock is being stepped.

    This does not introduce a new lock ordering: __stmmac_setup_est() already
    takes ptp_lock through gettimex64() while holding est_lock, and
    stmmac_adjust_systime() uses readl_poll_timeout_atomic(), so it does not
    sleep under ptp_lock.

    Cc: stable+noautosel@kernel.org # LLM report + LLM fix, not seen in real life
    Fixes: 23d96de472f7 ("net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()")
    Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
    Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
    Link: https://patch.msgid.link/20261102-stmmac-ptp-est-lock-v1-1-76fbc0cb230d@oss.qualcomm.com
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

I implemented the same approach used in stmmac_adjust_time() where we do not
consider the stmmac_est_configure()/__stmmac_setup_est() return value.
I think we can address it with a follow-up patch for both stmmac_adjust_time()
and stmmac_set_time().

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]