> 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 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 Reviewed-by: Maxime Chevallier Link: https://patch.msgid.link/20261102-stmmac-ptp-est-lock-v1-1-76fbc0cb230d@oss.qualcomm.com Signed-off-by: Jakub Kicinski > > > 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 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 Reviewed-by: Maxime Chevallier Link: https://patch.msgid.link/20261102-stmmac-ptp-est-lock-v1-1-76fbc0cb230d@oss.qualcomm.com Signed-off-by: Jakub Kicinski > > > + 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