[PATCH net v4] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()

[PATCH net v4] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()

[Lorenzo Bianconi]

stmmac_setup_ptp() returns void and swallows both PTP setup errors:
the PTP reference clock enable and stmmac_init_timestamping()
failures are logged but never propagated. When they fail, the MAC
system time counter is left in its post-reset, non-running state,
while the driver keeps operating as if timestamping were up.
This matters for TAPRIO/EST qdisc offloading, which derives the EST
base time from the hardware timestamp counter: arming the gate list
against a non-advancing time base would leave the schedule permanently
stuck.

Make stmmac_setup_ptp() return an error code and propagate the
failure in __stmmac_open() and stmmac_resume(), stopping the DMA
engines when PTP setup fails.

While at it, factor the timestamping availability check into a
stmmac_check_timestamp_cap() helper that requires both the hardware
timestamping capability and a valid PTP reference clock rate. This
keeps the interface operational on platforms with PTP-capable
silicon but an unconfigured PTP clock, where timestamping cannot be
enabled: those are treated as PTP-less rather than failing to open
or resume. Apply the helper to the hwtstamp get path and the
ethtool ts_info path, and keep the timestamping gate on the set path
(now also requiring a valid PTP reference clock rate), so all of
them consistently report -EOPNOTSUPP when timestamping is not usable.

Gate the PTP reference clock enable/disable in the platform noirq
suspend/resume callbacks on priv->ptp_enabled, keeping the clock
reference balanced against the new early return in
stmmac_release_ptp().

Fixes: 92ba6888510c ("stmmac: add the support for PTP hw clock driver")
Fixes: 276aae377206 ("net: stmmac: fix system hang caused by eee_ctrl_timer during suspend/resume")
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
Changes in v4:
- Drop stmmac_restore_subsecond_increment() and
  stmmac_update_subsecond_increment() changes for the moment.
- Add ptp_enabled check in stmmac_pltfr_noirq_suspend() and
  stmmac_pltfr_noirq_resume().
- Link to v3: https://lore.kernel.org/r/20260910-stmmac-ptp-error-propagate-v3-1-4f386e8256b6@oss.qualcomm.com

Changes in v3:
- Rebase on top of net main branch.
- Link to v2: https://lore.kernel.org/r/20260907-stmmac-ptp-error-propagate-v2-1-4a2e8e41e860@oss.qualcomm.com

Changes in v2:
- Check clk_ptp_rate value in stmmac_check_timestamp_cap().
- Return error code in stmmac_update_subsecond_increment() and
  stmmac_init_tstamp_counter().
- Rely on stmmac_check_timestamp_cap() in stmmac_hwtstamp_set() and
  stmmac_hwtstamp_get().
- Link to v1: https://lore.kernel.org/r/20260904-stmmac-ptp-error-propagate-v1-1-80f01b03dafa@oss.qualcomm.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       | 13 ++++
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   |  3 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 72 +++++++++++++++-------
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  6 +-
 4 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..c781cf3441fc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -329,6 +329,8 @@ struct stmmac_priv {
 	struct kernel_hwtstamp_config tstamp_config;
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_clock_ops;
+	bool ptp_enabled;
+
 	unsigned int default_addend;
 	u32 sub_second_inc;
 	u32 systime_flags;
@@ -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 154cc0c7623d..7758b854700a 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 |
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 62c3441911e7..9d2c6bf68575 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;
@@ -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) {
@@ -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;
+	}
+
 	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;
 }
 
@@ -7740,8 +7760,7 @@ static int stmmac_register_devlink(struct stmmac_priv *priv)
 	/* For now, what is exposed over devlink is only relevant when
 	 * timestamping is available and we have a valid ptp clock rate
 	 */
-	if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp) ||
-	    !priv->plat->clk_ptp_rate)
+	if (!stmmac_check_timestamp_cap(priv))
 		return 0;
 
 	priv->devlink = devlink_alloc(&stmmac_devlink_ops, sizeof(*dl_priv),
@@ -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;
+	}
 
 	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 6128ed1bd521..19d481554499 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,

---
base-commit: 6500e89132d5d510c75e1be443455af87267b40e
change-id: 20260904-stmmac-ptp-error-propagate-6c0147b4ce29

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Re: [PATCH net v4] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()

[netdev-bot+sashiko]

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

Re: [PATCH net v4] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()

[Lorenzo Bianconi]

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

> 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

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