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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- 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 --]