[PATCH net v4] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()
netdev-bot+sashiko at kernel.org
netdev-bot+sashiko at kernel.org
Mon Sep 14 01:34:13 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…
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"?
> @@ -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
More information about the linux-arm-kernel
mailing list