[PATCH net v5] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()
Lorenzo Bianconi
lorenzo.bianconi at oss.qualcomm.com
Wed Sep 16 10:59:12 PDT 2026
[...]
>
> A few questions below.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > index 7582fca637419..4fc96b317d79c 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>
> [ ... ]
>
> > @@ -419,6 +421,11 @@ int stmmac_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
> >
> > struct plat_stmmacenet_data *stmmac_plat_dat_alloc(struct device *dev);
> >
> > +static inline bool stmmac_check_timestamp_cap(struct stmmac_priv *priv)
> > +{
> > + return priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp;
> > +}
> > +
> > static inline bool stmmac_xdp_is_enabled(struct stmmac_priv *priv)
> > {
> > return !!priv->xdp_prog;
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > index 154cc0c7623da..7758b854700a3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > @@ -1007,8 +1007,7 @@ static int stmmac_get_ts_info(struct net_device *dev,
> > {
> > struct stmmac_priv *priv = netdev_priv(dev);
> >
> > - if ((priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp)) {
> > -
> > + if (stmmac_check_timestamp_cap(priv)) {
> > info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> > SOF_TIMESTAMPING_TX_HARDWARE |
> > SOF_TIMESTAMPING_RX_HARDWARE |
>
> [Severity: Medium]
> This is a pre-existing issue and not something this patch introduces, but
> this branch of stmmac_get_ts_info() reports phc_index = 0 when
> priv->ptp_clock is NULL:
>
> if (priv->ptp_clock)
> info->phc_index = ptp_clock_index(priv->ptp_clock);
> else
> info->phc_index = 0;
>
> The ethtool core expresses "no PHC" with -1 (ethtool_init_tsinfo() and
> ethtool_op_get_ts_info() both set info->phc_index = -1), so index 0 is
> reported to userspace as a real PHC that usually belongs to another
> device.
This issue is already fixed in the following commit:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=f0ef4b1eaed000a304726a43091588e8426ba08a
>
> The patch makes the unregistered-PHC case a normal outcome, since
> stmmac_setup_ptp() now returns success without registering the clock when
> no rate is available. Should the else branch leave the core's -1 in place?
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 62c3441911e71..0cc6eafa19a3d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -653,7 +653,8 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> > u32 ts_master_en = 0;
> > u32 ts_event_en = 0;
> >
> > - if (!(priv->dma_cap.time_stamp || priv->adv_ts)) {
> > + if (!priv->plat->clk_ptp_rate ||
> > + !(priv->dma_cap.time_stamp || priv->adv_ts)) {
> > NL_SET_ERR_MSG_MOD(extack, "No support for HW time stamping");
> > priv->hwts_tx_en = 0;
> > priv->hwts_rx_en = 0;
>
> [Severity: Medium]
> Should the clk_ptp_rate requirement be applied to the peer sites of the
> same uAPI too?
>
> The set path now returns -EOPNOTSUPP when clk_ptp_rate is zero, while
> stmmac_hwtstamp_get() and stmmac_get_ts_info() were converted to
> stmmac_check_timestamp_cap(), which only tests dma_cap.time_stamp /
> dma_cap.atime_stamp.
I have not changed the logic in stmmac_get_ts_info() since I do not think it is
required (priv->tstamp_config is set just in stmmac_set_ts_info()).
>
> On the configuration this patch newly declares supported (PTP-capable
> silicon, no PTP clock rate, so stmmac_setup_ptp() returns 0 without
> registering the PHC) ethtool -T still advertises
> SOF_TIMESTAMPING_TX_HARDWARE / SOF_TIMESTAMPING_RX_HARDWARE plus the full
> rx_filters list and SIOCGHWTSTAMP still succeeds, but every SIOCSHWTSTAMP
> is rejected.
>
> That state looks reachable at probe time, since stmmac_probe_config_dt()
> does:
>
> plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
>
> which yields 0 when the CSR clock lookup only warned and stmmac_clk is
> NULL and no ptp_ref clock is described.
>
> > @@ -843,7 +844,7 @@ static int stmmac_hwtstamp_get(struct net_device *dev,
> > {
> > struct stmmac_priv *priv = netdev_priv(dev);
> >
> > - if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp))
> > + if (!stmmac_check_timestamp_cap(priv))
> > return -EOPNOTSUPP;
> >
> > *config = priv->tstamp_config;
> > @@ -866,11 +867,6 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
> > {
> > struct timespec64 now;
> >
> > - if (!priv->plat->clk_ptp_rate) {
> > - netdev_err(priv->dev, "Invalid PTP clock rate");
> > - return -EINVAL;
> > - }
> > -
> > stmmac_config_hw_tstamping(priv, priv->ptpaddr, systime_flags);
> > priv->systime_flags = systime_flags;
>
> [Severity: High]
> With this check moved out, does stmmac_init_tstamp_counter() still have any
> failure path left? The remainder is:
I will improve stmmac_init_tstamp_counter() error path in a subsequent patch
>
> stmmac_config_hw_tstamping(priv, priv->ptpaddr, systime_flags);
> priv->systime_flags = systime_flags;
>
> stmmac_update_subsecond_increment(priv);
>
> ktime_get_real_ts64(&now);
>
> stmmac_init_systime(priv, priv->ptpaddr, (u32)now.tv_sec, now.tv_nsec);
>
> return 0;
>
> stmmac_update_subsecond_increment() is void and discards the status of
> config_addend(), and the int returned by the stmmac_init_systime() callback
> is dropped, so the function looks like it can only return 0.
>
> If that is the case, stmmac_init_timestamping() can only return 0 as well,
> and then the netdev_warn("PTP init failed") branch, the
>
> ret = stmmac_init_timestamping(priv);
> if (ret) {
> clk_disable_unprepare(priv->plat->clk_ptp_ref);
> return ret;
> }
>
> block in stmmac_setup_ptp(), the goto error_stop_dma in stmmac_resume() and
> the whole new error_stop_dma label are unreachable.
>
> The failure named in the commit message ("the MAC system time counter is
> left in its post-reset, non-running state") is exactly what init_systime()
> reports:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:init_systime() {
> ...
> return readl_poll_timeout_atomic(ioaddr + PTP_TCR, value,
> !(value & PTP_TCR_TSINIT),
> 10, 100000);
> }
>
> When that times out with -ETIMEDOUT, execution still reaches
> stmmac_ptp_register() and priv->ptp_enabled = true, so the interface comes
> up with a stopped time base that TAPRIO/EST offload will arm against.
> Should the hwif return values be checked so the only genuinely propagated
> error is not just the clk_prepare_enable() one?
This change is part of a dedicated patch (the one containing
stmmac_init_tstamp_counter() error path changes) that I will post as follow-up
>
> > @@ -885,26 +881,37 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
> > return 0;
> > }
> >
> > +static int stmmac_init_ptp_clk_freq(struct stmmac_priv *priv)
> > +{
> > + if (priv->plat->ptp_clk_freq_config)
> > + priv->plat->ptp_clk_freq_config(priv);
> > +
> > + if (!priv->plat->clk_ptp_rate) {
> > + netdev_info(priv->dev, "PTP clock rate not configured\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * stmmac_init_timestamping - initialise timestamping
> > * @priv: driver private structure
> > - * Description: this is to verify if the HW supports the PTPv1 or PTPv2.
> > - * This is done by looking at the HW cap. register.
> > - * This function also registers the ptp driver.
> > + *
> > + * Description: initialise the hardware timestamping counter, reset the
> > + * timestamping configuration and derive the advanced timestamping flags from
> > + * the HW capabilities. The caller must have ensured a valid PTP reference
> > + * clock rate (see stmmac_init_ptp_clk_freq()); the configured state is valid
> > + * as long as the interface is open and not suspended, and this function is
> > + * re-run on resume.
> > + *
> > + * Return: 0 on success, a negative errno otherwise.
> > */
> > static int stmmac_init_timestamping(struct stmmac_priv *priv)
> > {
> > bool xmac = dwmac_is_xmac(priv->plat->core_type);
> > int ret;
> >
> > - if (priv->plat->ptp_clk_freq_config)
> > - priv->plat->ptp_clk_freq_config(priv);
> > -
> > - if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp)) {
> > - netdev_info(priv->dev, "PTP not supported by HW\n");
> > - return -EOPNOTSUPP;
> > - }
> > -
> > ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
> > PTP_TCR_TSCFUPDT);
> > if (ret) {
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but since the resume path is
> being reworked to re-run stmmac_init_timestamping() under
> if (priv->ptp_enabled), the hardcoded PTP_TCR_TSCFUPDT here reverts a
> devlink-configured coarse mode after every suspend/resume, while
> stmmac_dl_ts_coarse_get() keeps reporting the old value:
As pointed out by sashiko, this issue is not introduced by this patch and it
will be fixed with a dedicated patch.
Regards,
Lorenzo
>
> priv->tsfupdt_coarse = ctx->val.vbool;
> ...
> ctx->val.vbool = priv->tsfupdt_coarse;
>
> The same function also does:
>
> memset(&priv->tstamp_config, 0, sizeof(priv->tstamp_config));
> priv->hwts_tx_en = 0;
> priv->hwts_rx_en = 0;
>
> so an active SIOCSHWTSTAMP configuration is dropped across suspend as
> well. I see from the v4/v5 notes that the subsecond increment rework was
> deliberately deferred, so this is noted only for completeness.
>
> > @@ -937,24 +944,48 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
> > return 0;
> > }
> >
> > -static void stmmac_setup_ptp(struct stmmac_priv *priv)
> > +static int stmmac_setup_ptp(struct stmmac_priv *priv)
> > {
> > int ret;
> >
> > + if (!stmmac_check_timestamp_cap(priv)) {
> > + netdev_info(priv->dev, "PTP not supported\n");
> > + return 0;
> > + }
> > +
> > ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> > - if (ret < 0)
> > + if (ret < 0) {
> > netdev_warn(priv->dev,
> > "failed to enable PTP reference clock: %pe\n",
> > ERR_PTR(ret));
> > + return ret;
> > + }
> >
> > - if (stmmac_init_timestamping(priv) == 0)
> > - stmmac_ptp_register(priv);
> > + if (stmmac_init_ptp_clk_freq(priv)) {
> > + clk_disable_unprepare(priv->plat->clk_ptp_ref);
> > + return 0;
> > + }
>
> [Severity: Medium]
> This path makes "interface up with clk_ptp_rate == 0 and clk_ptp_ref
> disabled" a supported state. Is the devlink runtime setter still safe in
> that state?
>
> stmmac_dl_ts_coarse_set() ("phc_coarse_adj", CAP_NET_ADMIN) has no
> clk_ptp_rate or ptp_enabled check:
>
> /* In Coarse mode, we can use a smaller subsecond increment, let's
> * reconfigure the systime, subsecond increment and addend.
> */
> stmmac_update_subsecond_increment(priv);
>
> and that helper divides by the rate:
>
> priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
>
> as does the hwif callback it invokes:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:config_sub_second_increment() {
> if (value & PTP_TCR_TSCFUPDT)
> data = (2000000000ULL / ptp_clock);
> else
> data = (1000000000ULL / ptp_clock);
> ...
> }
>
> Devlink is only registered when clk_ptp_rate was nonzero at probe, but a
> platform ptp_clk_freq_config() callback can turn it into zero later, e.g.
>
> drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:ethqos_ptp_clk_freq_config() {
> ...
> plat_dat->clk_ptp_rate = clk_get_rate(plat_dat->clk_ptp_ref);
> }
>
> after which this return 0 (and the goto init_coalesce in stmmac_resume())
> keeps the interface and its registered devlink params alive. A subsequent
> devlink dev param set then divides by zero and programs PTP_SSIR/addend
> with clk_ptp_ref disabled. Should the rate requirement be extended to the
> devlink setter as well?
>
> > +
> > + ret = stmmac_init_timestamping(priv);
> > + if (ret) {
> > + clk_disable_unprepare(priv->plat->clk_ptp_ref);
> > + return ret;
> > + }
> > +
> > + stmmac_ptp_register(priv);
> > + priv->ptp_enabled = true;
> > +
> > + return 0;
> > }
> >
> > static void stmmac_release_ptp(struct stmmac_priv *priv)
> > {
> > + if (!priv->ptp_enabled)
> > + return;
> > +
> > stmmac_ptp_unregister(priv);
> > clk_disable_unprepare(priv->plat->clk_ptp_ref);
> > + priv->ptp_enabled = false;
> > }
>
> [ ... ]
>
> > @@ -4161,10 +4192,12 @@ static int __stmmac_open(struct net_device *dev,
> > ret = stmmac_hw_setup(dev);
> > if (ret < 0) {
> > netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
> > - goto init_error;
> > + return ret;
> > }
> >
> > - stmmac_setup_ptp(priv);
> > + ret = stmmac_setup_ptp(priv);
> > + if (ret)
> > + goto ptp_error;
> >
> > stmmac_init_coalesce(priv);
> >
> > @@ -4189,7 +4222,10 @@ static int __stmmac_open(struct net_device *dev,
> > hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> >
> > stmmac_release_ptp(priv);
> > -init_error:
> > +ptp_error:
> > + stmmac_stop_all_dma(priv);
> > + stmmac_mac_set(priv, priv->ioaddr, false);
> > +
> > return ret;
> > }
>
> [Severity: High]
> This is a pre-existing issue that the patch does not introduce, but the new
> ptp_error return adds one more way to leave __stmmac_open() after
>
> memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
>
> has aliased priv->dma_conf to the caller's descriptor and page pool
> pointers. On the reopen path the caller then frees them:
>
> stmmac_change_mtu() {
> ...
> free_dma_desc_resources(priv, dma_conf);
> kfree(dma_conf);
> ...
> return ret;
> }
>
> __free_dma_rx_desc_resources() only clears buf_alloc_num and xsk_pool, so
> dma_rx/dma_erx, buf_pool and page_pool stay set in priv->dma_conf. Since
> the netdev remains netif_running() after a failed ndo_change_mtu, a later
> ip link set down runs:
>
> __stmmac_release() {
> ...
> for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> ...
> free_dma_desc_resources(priv, &priv->dma_conf);
> }
>
> Can this free the same dma_free_coherent()/buf_pool/page_pool objects a
> second time? The ptp_error path also skips stmmac_init_coalesce(), so the
> txtimers in the freshly zeroed dma_conf were never passed to
> hrtimer_setup() before hrtimer_cancel() runs on them.
>
> [ ... ]
>
> > @@ -8346,14 +8381,19 @@ int stmmac_resume(struct device *dev)
> > ret = stmmac_hw_setup(ndev);
> > if (ret < 0) {
> > netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
> > - stmmac_legacy_serdes_power_down(priv);
> > - mutex_unlock(&priv->lock);
> > - rtnl_unlock();
> > - return ret;
> > + goto error_unlock;
> > }
> >
> > - stmmac_init_timestamping(priv);
> > + if (priv->ptp_enabled) {
> > + if (stmmac_init_ptp_clk_freq(priv))
> > + goto init_coalesce;
> >
> > + ret = stmmac_init_timestamping(priv);
> > + if (ret)
> > + goto error_stop_dma;
> > + }
> > +
> > +init_coalesce:
> > stmmac_init_coalesce(priv);
>
> [Severity: Medium]
> When priv->ptp_enabled is true and stmmac_init_ptp_clk_freq() fails here,
> the interface is resumed via init_coalesce with an info-level log only.
> Does that leave software and hardware state inconsistent?
>
> stmmac_hw_setup() has just done the DMA/MAC software reset, so PTP_TCR
> (TSENA), PTP_SSIR, the addend and the system time registers are back at
> reset values and the counter is stopped, yet:
>
> - priv->ptp_clock stays registered and priv->ptp_enabled stays true, so a
> PHC whose time never advances remains exposed to userspace
> - stmmac_pltfr_noirq_resume() re-enables clk_ptp_ref because
> ptp_enabled is true
> - priv->hwts_tx_en/hwts_rx_en/systime_flags/sub_second_inc/default_addend
> still describe the pre-suspend hardware configuration
> - priv->plat->clk_ptp_rate stays 0 for the code that divides by it, e.g.
> stmmac_update_subsecond_increment()
>
> The open path handles the same failure differently: it also disables
> clk_ptp_ref and leaves the PHC unregistered, so software and hardware
> agree. Should resume unregister the PHC and clear ptp_enabled instead of
> continuing?
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 6128ed1bd521a..19d4815544990 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>
> [ ... ]
>
> > @@ -984,6 +985,9 @@ static int __maybe_unused stmmac_pltfr_noirq_resume(struct device *dev)
> > if (ret)
> > return ret;
> >
> > + if (!priv->ptp_enabled)
> > + return 0;
> > +
> > ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> > if (ret < 0) {
> > netdev_warn(priv->dev,
>
> [Severity: Medium]
> This is a pre-existing issue rather than something the patch adds, but with
> the new gating the two flags can now disagree. If clk_prepare_enable() here
> fails, the function returns the error with priv->ptp_enabled still true, so
> no PTP clock reference is held while the flag says one is.
>
> A later close or driver removal then passes the new gate in
> stmmac_release_ptp():
>
> if (!priv->ptp_enabled)
> return;
>
> stmmac_ptp_unregister(priv);
> clk_disable_unprepare(priv->plat->clk_ptp_ref);
>
> and calls clk_disable_unprepare() without a matching enable. Should the
> failure path clear priv->ptp_enabled?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-stmmac-ptp-error-propagate-v5-1-81149897e65d%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/20260916/79a88c4e/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list