[PATCH net] net: stmmac: keep est_lock across PTP clock adjustment

[PATCH net] net: stmmac: keep est_lock across PTP clock adjustment

[Lorenzo Bianconi]

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.

Fixes: 23d96de472f7 ("net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index a37405534beb..6ad6c2f52a0d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -73,14 +73,12 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
 	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);
 
 	/* Calculate new basetime and re-configured EST after PTP time adjust. */
-	mutex_lock(&priv->est_lock);
 	if (priv->est && priv->est->enable)
 		__stmmac_setup_est(priv);
 	mutex_unlock(&priv->est_lock);

---
base-commit: 7308ef8b4a6779a39f68d053c19b53a5c584a663
change-id: 20261102-stmmac-ptp-est-lock-725f8c72ff1a

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Re: [PATCH net] net: stmmac: keep est_lock across PTP clock adjustment

[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 code inspection). If it was triggered please include
   the symptoms, like the stack trace or error 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: keep est_lock across PTP clock adjustment

[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 code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

This is a theoretical 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 this patch 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: keep est_lock across PTP clock adjustment

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · 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] This patch closes the EST-vs-clock-step window only in
  stmmac_adjust_time(), which serves adjtime/ADJ_SETOFFSET.

Pre-existing issues:
- [Medium] stmmac_adjust_time() discards the return value of every step
  in its disable/step/re-base sequence and always returns 0.

--

net: stmmac: keep est_lock across PTP clock adjustment

Before this patch, stmmac_adjust_time() dropped priv->est_lock while it
stepped the PTP clock. A concurrent tc_taprio_configure() could then
program EST with a base time taken from the clock before the step. The
patch keeps est_lock held across the whole disable/step/reprogram
sequence.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index a37405534bebf..6ad6c2f52a0d2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -73,14 +73,12 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
>  	if (priv->est && priv->est->enable)
>  		stmmac_est_configure(priv, priv, priv->est,
>  				     priv->plat->clk_ptp_rate, false);

[Severity: Medium]
This isn't a bug introduced by this patch, but stmmac_adjust_time()
ignores the return value of all three steps in this sequence and always
returns 0.

The disable call above can fail before EST_EEST is cleared.
est_configure() returns early on a zero ptp_rate or on an est_write()
SRWO poll timeout, and both returns come before the EST_CONTROL write:

drivers/net/ethernet/stmicro/stmmac/stmmac_est.c:est_configure() {
	if (!ptp_rate) {
		netdev_warn(priv->dev, "Invalid PTP rate");
		return -EINVAL;
	}

	ret |= est_write(est_addr, EST_BTR_LOW, cfg->btr[0], false);
	...
	if (ret)
		return ret;
	...
	else
		ctrl &= ~EST_EEST;

	writel(ctrl, est_addr + EST_CONTROL);
	...
}

In that case, is the clock stepped with the old schedule still armed?

stmmac_adjust_systime() can return -ETIMEDOUT if PTP_TCR_TSUPDT never
clears. Even then, the code recomputes the base time and re-enables EST.

If __stmmac_setup_est() fails, it logs "failed to re-configure EST" and
the error is dropped. EST_EEST and EST_INT_EN can then stay cleared while
priv->est->enable stays true.

In all three cases, ptp_clock_adjtime() reports success to userspace.

Commit 23d96de472f7 says that the third case is intentional: "On a
failed re-apply the open and resume callers abort, while
stmmac_adjust_time() keeps the offload configured." Should the disable
and adjust_systime failures still be propagated, though?

> -	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);
>
>  	/* Calculate new basetime and re-configured EST after PTP time adjust. */
> -	mutex_lock(&priv->est_lock);
>  	if (priv->est && priv->est->enable)
>  		__stmmac_setup_est(priv);
>  	mutex_unlock(&priv->est_lock);

[Severity: Medium]
This closes the window for adjtime, but what about the settime64 path?
The commit message says:

  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.

stmmac_set_time() also steps the PHC, but it only takes ptp_lock:

drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c:stmmac_set_time() {
	...
	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);

	return 0;
}

It never takes est_lock, never disables EST before the step and never
calls __stmmac_setup_est() afterwards.

tc_taprio_configure() holds est_lock, reads the clock with gettimex64(),
computes priv->est->btr[] with stmmac_calc_tas_basetime(), and then calls
stmmac_est_configure(..., true). A concurrent
clock_settime()->ptp_clock_settime()->stmmac_set_time() is not excluded
by est_lock, so it can step the clock between that read and the
programming.

Doesn't that leave the same stale base time race open through settime64?

There is a second effect. A taprio schedule installed before a
clock_settime() is never re-based, so it keeps its old base time until
taprio is reconfigured.

Should stmmac_set_time() use the same disable/step/__stmmac_setup_est()
sequence under est_lock? Should the commit message mention this path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102-stmmac-ptp-est-lock-v1-1-76fbc0cb230d%40oss.qualcomm.com