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

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

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

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

This is a pre-existing issue and it needs to be addressed with a dedicated
patch.

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

The reported issue is valid, but it is a pre-existing problem out of scope for
this patch (it needs to be addressed with a dedicated patch).

Regards,
Lorenzo

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

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