[PATCH net v2] net: stmmac: propagate PTP addend and system time programming errors
Lorenzo Bianconi
lorenzo.bianconi at oss.qualcomm.com
Tue Sep 29 06:12:08 PDT 2026
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20260929/6e980a7f/attachment.sig>
More information about the linux-arm-kernel
mailing list