[PATCH net v4] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()
Lorenzo Bianconi
lorenzo.bianconi at oss.qualcomm.com
Mon Sep 14 07:27:09 PDT 2026
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 3 (3) · Low: 2
>
> - [Medium] stmmac_setup_ptp()
> (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:940) now gates the…
> - [Medium] The patch unifies two of the three hardware-timestamping ABI
> gates on stmmac_check_timestamp_cap() ((dma_cap.time_stamp ||…
> - [Medium] In stmmac_resume(), when the newly propagated
> stmmac_init_timestamping() failure is taken (stmmac_main.c:8371-8375…
> - [Low] The commit message claims that after applying
> stmmac_check_timestamp_cap() to the hwtstamp get path and the ethtool…
> - [Low] The kernel-doc of stmmac_init_timestamping()
> (stmmac_main.c:889-895) still reads 'Description: this is to verify if…
I will fix the above issues in v5.
Regards,
Lorenzo
>
> Pre-existing issues:
> - [Medium] stmmac_get_ts_info() (stmmac_ethtool.c:1010-1019) sets
> info->phc_index = 0 when priv->ptp_clock is NULL while still…
> - [Medium] stmmac_change_mtu() (stmmac_main.c:6181-6199) tears the
> running interface down with __stmmac_release() and then calls…
> - [Medium] priv->ptp_enabled is used as the single ownership token for
> the clk_ptp_ref prepare/enable reference, but the platform noirq PM…
>
> --
>
> net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()
>
> stmmac_setup_ptp() now returns an error code so that the PTP reference clock
> enable and stmmac_init_timestamping() failures are propagated by
> __stmmac_open() and stmmac_resume(), which stop the DMA engines and disable
> the MAC on failure. A new stmmac_check_timestamp_cap() helper folds the
> hardware timestamping capability and a non-zero PTP reference clock rate
> into one test used by the hwtstamp get path, the ethtool ts_info path and
> the devlink registration. The platform noirq PM callbacks gate the
> clk_ptp_ref enable/disable on the new priv->ptp_enabled flag.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > index 7582fca637419..c781cf3441fc3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>
> [ ... ]
>
> > @@ -419,6 +421,17 @@ 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)
> > +{
> > + if (!priv->dma_cap.time_stamp && !priv->dma_cap.atime_stamp)
> > + return false;
> > +
> > + if (!priv->plat->clk_ptp_rate)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > 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: Low]
>
> The commit message says that after applying the helper to the hwtstamp get
> path and the ethtool ts_info path, "so all of them consistently report
> -EOPNOTSUPP when timestamping is not usable".
>
> Does the ethtool path do that? When stmmac_check_timestamp_cap() is false,
> stmmac_get_ts_info() takes the else branch and calls
> ethtool_op_get_ts_info(), which succeeds:
>
> net/ethtool/ioctl.c:ethtool_op_get_ts_info() {
> info->so_timestamping =
> SOF_TIMESTAMPING_TX_SOFTWARE |
> SOF_TIMESTAMPING_RX_SOFTWARE |
> SOF_TIMESTAMPING_SOFTWARE;
> info->phc_index = -1;
> return 0;
> }
>
> So ethtool -T returns 0 with software-only capabilities rather than
> -EOPNOTSUPP. Could that sentence be adjusted?
>
> [Severity: Medium]
>
> This isn't a bug introduced by this patch, but while the gate here is being
> narrowed, the phc_index assignment just below it still reports a valid index
> when no PTP clock exists:
>
> if (priv->ptp_clock)
> info->phc_index = ptp_clock_index(priv->ptp_clock);
> else
> info->phc_index = 0;
>
> The ethtool convention for "no associated PHC" is -1, as
> ethtool_op_get_ts_info() does. Index 0 points userspace at some other
> device's PTP clock.
>
> This branch is reachable before the first ifup, since the gate is a pure
> capability plus clk_ptp_rate test independent of PTP registration, and also
> after ptp_clock_register() fails, where stmmac_ptp_register() leaves
> priv->ptp_clock NULL.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 62c3441911e71..9d2c6bf68575f 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]
>
> Is the remaining divergence between this predicate and
> stmmac_check_timestamp_cap() intended? The set path tests priv->adv_ts,
> while the get and ethtool paths now test priv->dma_cap.atime_stamp.
>
> priv->adv_ts is not a capability bit, it is derived in
> stmmac_init_timestamping():
>
> priv->adv_ts = 0;
> if (xmac && priv->dma_cap.atime_stamp)
> priv->adv_ts = 1;
> else if (priv->extend_desc && priv->dma_cap.atime_stamp)
> priv->adv_ts = 1;
>
> On a non-xmac core that reports atime_stamp=1 with time_stamp=0 and runs
> without extended descriptors, adv_ts stays 0, so ethtool -T and
> SIOCGHWTSTAMP advertise hardware timestamping while SIOCSHWTSTAMP returns
> -EOPNOTSUPP with "No support for HW time stamping".
>
> adv_ts is also only assigned once PTP setup has run, so this gate answers
> differently before and after the first ifup, whereas the other two are
> static.
>
> Does that fit the commit message claim that "all of them consistently
> report -EOPNOTSUPP when timestamping is not usable"?
I think this is just a small inaccuracy in the commit message I can fix if I
need to repost.
>
> > @@ -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;
> > @@ -900,11 +901,6 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
> > 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: Low]
>
> With the capability check removed here, should the kernel-doc directly above
> stmmac_init_timestamping() be refreshed as well? It still reads:
>
> /**
> * 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.
> */
>
> Both statements now belong to stmmac_setup_ptp(), which holds the
> stmmac_check_timestamp_cap() test and the stmmac_ptp_register() call. This
> matters for readers of stmmac_resume(), which calls
> stmmac_init_timestamping() directly under the new priv->ptp_enabled gate.
>
> > @@ -937,24 +933,43 @@ 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;
> > + }
> > +
>
> [Severity: Medium]
>
> Can this early return permanently disable PTP on platforms whose PTP rate is
> programmed by the very code the gate skips?
>
> stmmac_check_timestamp_cap() returns false when priv->plat->clk_ptp_rate is
> zero, but the only run-time writer of that field is
> plat->ptp_clk_freq_config(), which runs downstream in
> stmmac_init_timestamping():
>
> if (priv->plat->ptp_clk_freq_config)
> priv->plat->ptp_clk_freq_config(priv);
>
> On dwmac-qcom-ethqos that callback is what establishes the rate:
>
> dwmac-qcom-ethqos.c:ethqos_ptp_clk_freq_config() {
> err = clk_set_rate(plat_dat->clk_ptp_ref, ULONG_MAX);
> if (err)
> netdev_err(priv->dev, "Failed to max out clk_ptp_ref: %d\n", err);
> plat_dat->clk_ptp_rate = clk_get_rate(plat_dat->clk_ptp_ref);
> }
>
> The value the gate samples comes from probe time:
>
> stmmac_platform.c:stmmac_probe_config_dt() {
> plat->clk_ptp_rate = clk_get_rate(plat->clk_ptp_ref);
> }
>
> If that probe-time read is zero, the pre-patch flow still recovered: the ref
> clock was enabled unconditionally, ptp_clk_freq_config() programmed the rate,
> and the !clk_ptp_rate check in stmmac_init_tstamp_counter() then passed.
> With this gate, clk_ptp_ref is never enabled, stmmac_init_tstamp_counter()
> and stmmac_ptp_register() never run, priv->ptp_enabled stays false so
> stmmac_resume() skips timestamping too, and hwtstamp get plus ethtool -T
> report no hardware timestamping. Nothing is surfaced because
> stmmac_setup_ptp() returns 0.
>
> Would it be safer to evaluate clk_ptp_rate only after
> plat->ptp_clk_freq_config() has had a chance to program it?
>
> > 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);
> > + 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;
> > }
> >
> > static void stmmac_legacy_serdes_power_down(struct stmmac_priv *priv)
> > @@ -4161,10 +4176,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 +4206,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: Medium]
>
> This is a pre-existing issue, not introduced by this patch, but the new PTP
> failure branch adds another way of reaching it. stmmac_change_mtu() tears
> the live interface down and reopens it:
>
> stmmac_main.c:stmmac_change_mtu() {
> __stmmac_release(dev);
>
> ret = __stmmac_open(dev, dma_conf);
> if (ret) {
> free_dma_desc_resources(priv, dma_conf);
> kfree(dma_conf);
> netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
> return ret;
> }
> }
>
> There is no rollback to the previous configuration and no transition out of
> the running state, so netif_running() stays true while queues, IRQs,
> descriptors, phylink, MAC and DMA are all down. In the baseline this was
> already reachable via the stmmac_hw_setup() and stmmac_request_irq()
> failures; after this change a clk_prepare_enable(clk_ptp_ref) or
> stmmac_init_timestamping() failure during an MTU change can put the
> interface in the same state.
>
> [ ... ]
>
> > @@ -8346,13 +8365,14 @@ 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) {
> > + ret = stmmac_init_timestamping(priv);
> > + if (ret)
> > + goto error_stop_dma;
> > + }
>
> [Severity: Medium]
>
> Should this failure path also drop the PTP-side state? The open path is now
> consistent, since a stmmac_setup_ptp() failure leaves priv->ptp_enabled
> false with no PHC registered, but the resume path is not.
>
> On the goto error_stop_dma path, priv->ptp_enabled stays true, the PTP clock
> stays registered, and priv->systime_flags, sub_second_inc, default_addend and
> tstamp_config still describe the pre-suspend hardware state that the DMA
> software reset in stmmac_hw_setup() has just cleared. ethtool ts_info,
> SIOCGHWTSTAMP and /dev/ptpN then keep advertising working hardware
> timestamping over a non-running counter.
>
> That looks like the exact divergence the commit message describes: "the MAC
> system time counter is left in its post-reset, non-running state, while the
> driver keeps operating as if timestamping were up", including the EST base
> time derived from a non-advancing counter.
>
> One way to reach it is stmmac_init_tstamp_counter() failing on its
> !plat->clk_ptp_rate check after plat->ptp_clk_freq_config() re-reads a zero
> rate during resume.
>
> >
> > stmmac_init_coalesce(priv);
> > phylink_rx_clk_stop_block(priv->phylink);
> > @@ -8376,6 +8396,16 @@ int stmmac_resume(struct device *dev)
> > netif_device_attach(ndev);
> >
> > return 0;
> > +
> > +error_stop_dma:
> > + stmmac_stop_all_dma(priv);
> > + stmmac_mac_set(priv, priv->ioaddr, false);
> > +error_unlock:
> > + stmmac_legacy_serdes_power_down(priv);
> > + mutex_unlock(&priv->lock);
> > + rtnl_unlock();
> > +
> > + return ret;
> > }
> > EXPORT_SYMBOL_GPL(stmmac_resume);
> >
> > 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
> > @@ -959,7 +959,8 @@ static int __maybe_unused stmmac_pltfr_noirq_suspend(struct device *dev)
> >
> > if (!priv->wolopts) {
> > /* Disable clock in case of PWM is off */
> > - clk_disable_unprepare(priv->plat->clk_ptp_ref);
> > + if (priv->ptp_enabled)
> > + clk_disable_unprepare(priv->plat->clk_ptp_ref);
> >
> > ret = pm_runtime_force_suspend(dev);
> > if (ret)
> > @@ -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 imbalance rather than something this patch
> introduces, but since priv->ptp_enabled now becomes the ownership token for
> the clk_ptp_ref reference, can the two error paths here leave it out of sync
> with the reference actually held?
>
> In stmmac_pltfr_noirq_suspend(), the clock is dropped and then
> pm_runtime_force_suspend() may fail and return, so ptp_enabled stays true
> with no reference held, and the failing device is not resumed by
> dpm_resume_noirq().
>
> In stmmac_pltfr_noirq_resume(), a failing clk_prepare_enable() returns while
> ptp_enabled is still true, and a failing resume_noirq does not abort the PM
> resume sequence.
>
> In both cases the next teardown drops a reference that was never acquired:
>
> stmmac_main.c:stmmac_release_ptp() {
> if (!priv->ptp_enabled)
> return;
>
> stmmac_ptp_unregister(priv);
> clk_disable_unprepare(priv->plat->clk_ptp_ref);
> }
>
> The same underflow existed in the baseline, where both callbacks and
> stmmac_release_ptp() touched clk_ptp_ref unconditionally, so the gate
> reproduces the old behaviour whenever ptp_enabled is true. Would it be
> worth clearing or restoring the flag on these two error returns while the
> ownership rules are being written down?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913-stmmac-ptp-error-propagate-v4-1-a947aceac928%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/20260914/2449aeab/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list