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

[PATCH net v5] 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(), stopping the DMA engines when PTP setup
fails. In stmmac_resume(), re-initialise timestamping only when the
PTP clock was registered before suspend, re-apply the platform clock
rate configuration, and stop the DMA engines if re-initialisation
fails.

While at it, factor the hardware timestamping capability check into a
stmmac_check_timestamp_cap() helper and apply it to the hwtstamp get
path and the ethtool ts_info path. stmmac_setup_ptp() now enables the
PTP reference clock and runs the platform ptp_clk_freq_config()
callback before validating the resulting clock rate; if no valid rate
is available, it disables the clock and returns success without
registering the PTP clock. This keeps the interface operational on
platforms with PTP-capable silicon but an unconfigured PTP clock,
where timestamping cannot be enabled: instead of failing, the driver
proceeds without setting up timestamping. Extend the timestamping gate
on the set path (which keeps testing the core's basic/advanced
timestamping capability) and the devlink registration with the PTP
reference clock rate requirement.

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 v5:
- Introduce stmmac_init_ptp_clk_freq() routine to configure ptp clock
  frequency.
- Update commit message.
- Update stmmac_init_timestamping kernel-doc.
- Link to v4: https://lore.kernel.org/r/20260913-stmmac-ptp-error-propagate-v4-1-a947aceac928@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       |   7 ++
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   |   3 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 114 +++++++++++++++------
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   6 +-
 4 files changed, 95 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..4fc96b317d79 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,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 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..0cc6eafa19a3 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;
@@ -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;
 
@@ -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) {
@@ -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;
+	}
+
+	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 +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;
 }
 
@@ -7740,8 +7776,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) || !priv->plat->clk_ptp_rate)
 		return 0;
 
 	priv->devlink = devlink_alloc(&stmmac_devlink_ops, sizeof(*dl_priv),
@@ -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);
 	phylink_rx_clk_stop_block(priv->phylink);
 	stmmac_set_rx_mode(ndev);
@@ -8376,6 +8416,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: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
change-id: 20260904-stmmac-ptp-error-propagate-6c0147b4ce29

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 (1) · Medium: 3 (3) · Low: 0

- [High] The claimed propagation of stmmac_init_timestamping() failures
  is vacuous: after this patch stmmac_init_tstamp_counter()…
- [Medium] The patch splits the hardware-timestamping gate across peer
  sites of the same uAPI.
- [Medium] In stmmac_resume(), when priv->ptp_enabled is true but
  stmmac_init_ptp_clk_freq() fails (clk_ptp_rate came back as 0), the…
- [Medium] The patch makes 'interface operational with an unconfigured
  PTP reference clock' an explicitly supported state (stmmac_setup_ptp()…

Pre-existing issues:
- [High] The new ptp_error unwind adds another way for __stmmac_open() to
  fail *after* `memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf))`…
- [Medium] stmmac_init_timestamping() hardcodes `STMMAC_HWTS_ACTIVE |
  PTP_TCR_TSCFUPDT` (fine update), overwrites priv->systime_flags and…
- [Medium] stmmac_get_ts_info() reports `info->phc_index = 0` when
  priv->ptp_clock is NULL, whereas the ethtool core uses -1 to express…
- [Medium] If clk_prepare_enable(priv->plat->clk_ptp_ref) fails in
  stmmac_pltfr_noirq_resume() (stmmac_platform.c:990), the function…

--

net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()

This makes stmmac_setup_ptp() return an error code so that PTP setup
failures are propagated by __stmmac_open() and stmmac_resume(), factors the
timestamping capability test into stmmac_check_timestamp_cap(), moves the
platform PTP clock rate configuration into stmmac_init_ptp_clk_freq(), and
gates the noirq suspend/resume PTP reference clock handling on a new
priv->ptp_enabled flag.

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.

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.

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:

	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?

> @@ -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:

	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

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

[Lorenzo Bianconi]

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

[...]

> 
> 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

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 14 Sep 2026 17:26:44 +0200 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v5] net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()
    https://git.kernel.org/netdev/net-next/c/8181678a92f0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html