Re: [PATCH net v2] net: stmmac: propagate PTP addend and system time programming errors

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

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

> Thank you for your contribution! Sashiko AI review found 7 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 4 (1) · Low: 2
> 
> 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).
> 
> - [High] The fix stops at returning an error.
> - [Medium] The patch is titled 'propagate PTP addend and system time
>   programming errors', but the PHC callbacks that call the same hooks…
> - [Medium] The new failure paths leave timestamping enabled with stale
>   state.
> - [Medium] The patch reorders stmmac_dl_ts_coarse_set() so that
>   priv->systime_flags is copied into a local variable at entry, all…
> - [Medium] stmmac_restore_subsecond_increment() calls
>   stmmac_config_addend() unconditionally and ignores its result,…
> - [Low] stmmac_dl_ts_coarse_set() can now return -EINVAL (sub-second
>   increment computed as 0) or -ETIMEDOUT (TSADDREG poll timeout in…
> - [Low] The new rollback in stmmac_update_subsecond_increment() (line
>   658) passes priv->default_addend to…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the devlink
>   'phc_coarse_adj' param (priv->tsfupdt_coarse) is not honoured when…
> - [Medium] This problem wasn't introduced by this patch, but
>   stmmac_update_subsecond_increment() and the new…
> 

I will fix the issues reported by sashiko in v3.

Regards,
Lorenzo

> --
> 
> net: stmmac: propagate PTP addend and system time programming errors
> 
> This patch makes stmmac_update_subsecond_increment(),
> stmmac_init_tstamp_counter() and stmmac_dl_ts_coarse_set() return the
> errors from addend and system time programming instead of always
> returning success. It also adds stmmac_restore_subsecond_increment() to
> roll back the PTP configuration when programming fails.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 1fb5f804ea234..fae2a90283ce4 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -601,31 +601,63 @@ static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
> >  	}
> >  }
> >  
> > -static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
> > +static void stmmac_restore_subsecond_increment(struct stmmac_priv *priv,
> > +					       u32 default_addend,
> > +					       u32 systime_flags)
> >  {
> >  	bool xmac = dwmac_is_xmac(priv->plat->core_type);
> >  	u32 sec_inc = 0;
> > +
> > +	stmmac_config_addend(priv, priv->ptpaddr, default_addend);
> > +	stmmac_config_hw_tstamping(priv, priv->ptpaddr, systime_flags);
> > +	stmmac_config_sub_second_increment(priv, priv->ptpaddr,
> > +					   priv->plat->clk_ptp_rate,
> > +					   xmac, &sec_inc);
> > +	priv->default_addend = default_addend;
> > +	priv->sub_second_inc = sec_inc;
> > +}
> 
> [Severity: Medium]
> Is it safe to call stmmac_config_addend() again here and ignore its
> result?
> 
> When this rollback runs because config_addend() has just timed out in
> stmmac_update_subsecond_increment(), PTP_TCR_TSADDREG may still be set.
> One way to get there is a PTP ref clock domain that is not running,
> since stmmac_setup_ptp() only warns when clk_prepare_enable() fails.
> config_addend() then overwrites PTP_TAR and sets TSADDREG again while
> the previous update is still pending:
> 
> stmmac_hwtstamp.c:config_addend() {
> 	writel(addend, ioaddr + PTP_TAR);
> 	/* issue command to update the addend value */
> 	value = readl(ioaddr + PTP_TCR);
> 	value |= PTP_TCR_TSADDREG;
> 	writel(value, ioaddr + PTP_TCR);
> 	...
> }
> 
> The DWMAC programming model expects this bit to read 0 before it is set
> again. The call also adds a second 100 ms readl_poll_timeout_atomic()
> busy-wait, so the failing devlink path spins for about 200 ms.
> 
> If the pending update latches later, TAR could end up holding the old
> value (or 0 on the first open after probe) rather than the one just
> computed.
> 
> On the TSINIT timeout path in stmmac_init_tstamp_counter(), the TCR
> read-modify-write in config_addend() also writes the still-pending
> TSINIT bit back as 1, so the pending system time load is not cancelled.
> 
> In every case, the helper then sets priv->default_addend and
> priv->sub_second_inc as if the old configuration had been restored in
> hardware.
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but
> stmmac_update_subsecond_increment() and this new helper program
> PTP_TCR, PTP_SSIR and PTP_TAR, and update priv->default_addend and
> priv->sub_second_inc, without holding priv->ptp_lock. Both run from the
> devlink set path while the PHC is registered.
> 
> The PHC callbacks use that lock around the same registers:
> 
> stmmac_ptp.c:stmmac_adjust_freq() {
> 	...
> 	write_lock_irqsave(&priv->ptp_lock, flags);
> 	stmmac_config_addend(priv, priv->ptpaddr, addend);
> 	write_unlock_irqrestore(&priv->ptp_lock, flags);
> 	...
> }
> 
> stmmac_enable() also reads priv->sub_second_inc and
> priv->systime_flags under ptp_lock for PEROUT.
> 
> Could a concurrent adjfine and phc_coarse_adj devlink set lose a TCR
> mode change, or program a TAR derived from a stale default_addend? This
> error path adds one more unlocked config_addend() plus
> config_hw_tstamping() sequence.
> 
> > +
> > +static int stmmac_update_subsecond_increment(struct stmmac_priv *priv,
> > +					     u32 systime_flags)
> > +{
> 
> [ ... ]
> 
> > +	ret = stmmac_config_addend(priv, priv->ptpaddr, val);
> > +	if (ret)
> > +		goto error;
> > +
> > +	priv->sub_second_inc = sec_inc;
> > +	priv->default_addend = val;
> > +
> > +	return 0;
> > +error:
> > +	/* Restore previous configuration */
> > +	stmmac_restore_subsecond_increment(priv, priv->default_addend,
> > +					   priv->systime_flags);
> 
> [Severity: Low]
> Is priv->default_addend really the previous configuration here?
> stmmac_adjust_freq() programs an adjusted addend into PTP_TAR but does
> not update default_addend:
> 
> 	addend = adjust_by_scaled_ppm(priv->default_addend, scaled_ppm);
> 
> If a phc_coarse_adj devlink set fails (for example with sec_inc == 0),
> the rollback writes the nominal addend into PTP_TAR. That drops the
> frequency correction the servo had applied, so a failed operation still
> changes the PHC frequency, which the "Restore previous configuration"
> comment doesn't reflect.
> 
> > +	return ret;
> >  }
> >  
> >  /**
> > @@ -864,25 +896,37 @@ static int stmmac_hwtstamp_get(struct net_device *dev,
> >  static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
> >  				      u32 systime_flags)
> >  {
> 
> [ ... ]
> 
> >  	/* lower 32 bits of tv_sec are safe until y2106 */
> > -	stmmac_init_systime(priv, priv->ptpaddr, (u32)now.tv_sec, now.tv_nsec);
> > +	ret = stmmac_init_systime(priv, priv->ptpaddr, (u32)now.tv_sec,
> > +				  now.tv_nsec);
> > +	if (ret)
> > +		goto error;
> 
> [Severity: Medium]
> The subject says addend and system time programming errors are now
> propagated, but the PHC callbacks that call the same hooks still drop
> the result:
> 
> 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;
> }
> 
> stmmac_adjust_freq() also ignores the return of stmmac_config_addend()
> and returns 0.
> 
> When the TSINIT or TSADDREG poll times out, clock_settime() or
> clock_adjtime() from ptp4l or phc2sys still reports success. Should
> settime64 and adjfine return the error too, since the ptp_clock_info
> callbacks are allowed to fail?
> 
> > +
> > +	priv->systime_flags = systime_flags;
> 
> [Severity: High]
> This is a pre-existing issue, but this path is being reworked here:
> stmmac_init_timestamping() always sets PTP_TCR_TSCFUPDT and ignores
> priv->tsfupdt_coarse:
> 
> 	ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
> 					       PTP_TCR_TSCFUPDT);
> 
> Suppose phc_coarse_adj is set to true and then the interface goes down
> and up (or through suspend/resume). config_sub_second_increment() then
> programs the fine mode SSINC (2e9 / clk_ptp_rate) and the ~2^31 addend.
> priv->tsfupdt_coarse stays true, so stmmac_dl_ts_coarse_get() still
> reports coarse mode.
> 
> The next SIOCSHWTSTAMP clears TSCFUPDT in stmmac_hwtstamp_set():
> 
> 	priv->systime_flags = STMMAC_HWTS_ACTIVE;
> 	if (!priv->tsfupdt_coarse)
> 		priv->systime_flags |= PTP_TCR_TSCFUPDT;
> 
> It then only calls stmmac_config_hw_tstamping(), and SSIR and TAR are
> not reprogrammed.
> 
> Does that leave the hardware in coarse mode, adding twice the clock
> period on every cycle, so the PHC runs about twice as fast?
> 
> >  
> >  	return 0;
> > +error:
> > +	/* Restore previous configuration */
> > +	stmmac_restore_subsecond_increment(priv, default_addend,
> > +					   priv->systime_flags);
> > +
> > +	return ret;
> >  }
> 
> [Severity: Medium]
> Can this failure path leave timestamping enabled with stale state?
> 
> stmmac_hwtstamp_set() sets priv->hwts_rx_en, priv->hwts_tx_en,
> priv->tstamp_config and a priv->systime_flags value that includes TSENA
> and the RX filter bits. None of these is cleared on close or suspend.
> 
> On a later open or resume, if stmmac_init_tstamp_counter() fails, this
> rollback writes the old priv->systime_flags, filter bits included, back
> into PTP_TCR. The rollback in stmmac_update_subsecond_increment() does
> the same. stmmac_init_timestamping() then returns early:
> 
> 	if (ret) {
> 		netdev_warn(priv->dev, "PTP init failed\n");
> 		return ret;
> 	}
> 
> and never reaches:
> 
> 	memset(&priv->tstamp_config, 0, sizeof(priv->tstamp_config));
> 	priv->hwts_tx_en = 0;
> 	priv->hwts_rx_en = 0;
> 
> TX timestamp requests and RX timestamp delivery in
> stmmac_get_rx_hwtstamp() would then keep running on a counter whose
> init just failed, and stmmac_hwtstamp_get() reports the stale config.
> 
> Before this patch, these paths always reached the reset, and TCR was
> left at STMMAC_HWTS_ACTIVE | PTP_TCR_TSCFUPDT with no filters.
> 
> [Severity: High]
> The error now reaches stmmac_init_timestamping(), but do any callers
> act on it?
> 
> stmmac_resume() ignores the result:
> 
> 	stmmac_init_timestamping(priv);
> 
> 	stmmac_init_coalesce(priv);
> 
> After the MAC reset in stmmac_hw_setup(), a TSADDREG or TSINIT timeout
> here leaves the PHC registered at open still in place. gettime64,
> adjfine, settime64 and tc_taprio_configure(), which calls gettime64 for
> the EST base time, keep using a counter that was never re-initialized.
> 
> On open, stmmac_setup_ptp() only skips registration:
> 
> 	if (stmmac_init_timestamping(priv) == 0)
> 		stmmac_ptp_register(priv);
> 
> The interface still comes up. stmmac_hwtstamp_set() only checks the
> timestamp capability bits and netif_running(), so SIOCSHWTSTAMP can
> still turn on packet timestamping with the uninitialized counter and
> return 0.
> 
> The commit message describes the problem as the driver that "keeps
> operating as if timestamping were up" and names TAPRIO/EST. On the
> resume path, the only visible change seems to be the "PTP init failed"
> warning. Isn't that still the situation this patch is meant to fix?
> 
> >  
> >  /**
> > @@ -7693,18 +7737,23 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
> >  {
> >  	struct stmmac_devlink_priv *dl_priv = devlink_priv(dl);
> >  	struct stmmac_priv *priv = dl_priv->stmmac_priv;
> > +	u32 systime_flags = priv->systime_flags;
> > +	int ret;
> 
> [ ... ]
> 
> > -	stmmac_update_subsecond_increment(priv);
> > +	ret = stmmac_update_subsecond_increment(priv, systime_flags);
> > +	if (ret)
> > +		return ret;
> 
> [Severity: Low]
> This now returns -EINVAL or -ETIMEDOUT to devlink without filling in
> the extack it receives. Would an NL_SET_ERR_MSG_MOD() help user space
> understand why setting phc_coarse_adj failed?
> 
> > +
> > +	priv->tsfupdt_coarse = ctx->val.vbool;
> > +	priv->systime_flags = systime_flags;
> 
> [Severity: Medium]
> Does moving these assignments after the hardware programming widen a
> race with stmmac_hwtstamp_set()? The devlink set callback runs under
> the devlink instance lock and stmmac_hwtstamp_set() runs under RTNL, so
> the two paths share no lock.
> 
> CPU0 (devlink, vbool = true)
> stmmac_dl_ts_coarse_set()
>   systime_flags = priv->systime_flags, TSCFUPDT cleared
>   stmmac_update_subsecond_increment()
>     writes TCR in coarse mode and the coarse SSIR
>     config_addend() polls TSADDREG for up to 100 ms
> 
> CPU1 (SIOCSHWTSTAMP)
> stmmac_hwtstamp_set()
>   reads priv->tsfupdt_coarse == false
>   priv->systime_flags = ACTIVE | TSCFUPDT | filters
>   writes TCR back to fine mode
> 
> CPU0
>   priv->tsfupdt_coarse = true
>   priv->systime_flags = stale copy, no TSCFUPDT and no filters
> 
> The hardware ends up in fine mode with the coarse SSIR and addend, so
> the PHC runs at the wrong rate. priv->systime_flags then matches neither
> the hardware nor the hwtstamp request, and it is used later by flex PPS
> in stmmac_enable() and by the new restore paths.
> 
> The old code set tsfupdt_coarse before the MMIO programming, so the
> window was only a few instructions. Here it covers the whole
> programming sequence, including the poll.
> 
> >  
> >  	return 0;
> >  }
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-stmmac-ptp-added-systime-error-v2-1-beb2a6b5f866%40oss.qualcomm.com

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