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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
- 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