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

[PATCH net] 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.

Fixes: 92ba6888510c ("stmmac: add the support for PTP hw clock driver")
Fixes: 0ad2be79f254 ("net: stmmac: Balance PTP reference clock enable/disable")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 66 ++++++++++++++++-------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b..d33c82bd3ee4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -885,6 +885,11 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
 	return 0;
 }
 
+static bool stmmac_check_timestamp_cap(struct stmmac_priv *priv)
+{
+	return priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp;
+}
+
 /**
  * stmmac_init_timestamping - initialise timestamping
  * @priv: driver private structure
@@ -900,11 +905,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,22 +937,39 @@ 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 by HW\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;
+	}
+
+	ret = stmmac_init_timestamping(priv);
+	if (ret) {
+		clk_disable_unprepare(priv->plat->clk_ptp_ref);
+		return ret;
+	}
 
-	if (stmmac_init_timestamping(priv) == 0)
-		stmmac_ptp_register(priv);
+	stmmac_ptp_register(priv);
+
+	return 0;
 }
 
 static void stmmac_release_ptp(struct stmmac_priv *priv)
 {
+	if (!stmmac_check_timestamp_cap(priv))
+		return;
+
 	stmmac_ptp_unregister(priv);
 	clk_disable_unprepare(priv->plat->clk_ptp_ref);
 }
@@ -4161,10 +4178,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);
 
@@ -4187,9 +4206,10 @@ static int __stmmac_open(struct net_device *dev,
 
 	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
 		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
-
 	stmmac_release_ptp(priv);
-init_error:
+ptp_error:
+	stmmac_stop_all_dma(priv);
+
 	return ret;
 }
 
@@ -8326,13 +8346,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 (stmmac_check_timestamp_cap(priv)) {
+		ret = stmmac_init_timestamping(priv);
+		if (ret)
+			goto error_stop_dma;
+	}
 
 	stmmac_init_coalesce(priv);
 	phylink_rx_clk_stop_block(priv->phylink);
@@ -8356,6 +8377,15 @@ int stmmac_resume(struct device *dev)
 	netif_device_attach(ndev);
 
 	return 0;
+
+error_stop_dma:
+	stmmac_stop_all_dma(priv);
+error_unlock:
+	stmmac_legacy_serdes_power_down(priv);
+	mutex_unlock(&priv->lock);
+	rtnl_unlock();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(stmmac_resume);
 

---
base-commit: 641d03105cc0d2437e32fdeec164f91a4ccef6c4
change-id: 20260904-stmmac-ptp-error-propagate-6c0147b4ce29

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

Re: [PATCH net] 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: 1 · Low: 2

- [High] Undisclosed functional regression: PTP setup failures are now
  fatal to `ndo_open()` and to system resume.
- [Medium] Unbalanced clk_ptp_ref enable/disable against the platform
  noirq PM callbacks.
- [Low] The kernel-doc block above `stmmac_init_timestamping()`
  (stmmac_main.c:893-899) still states 'this is to verify if the HW…
- [Low] Both new unwind paths halt the DMA but never undo
  `stmmac_hw_setup()`'s `stmmac_mac_set(priv, priv->ioaddr, true)`…

Pre-existing issues:
- [High] Pre-existing double free / use-after-free of the DMA descriptor
  rings, whose trigger set this patch widens.

--

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

This converts stmmac_setup_ptp() from void to int so that the PTP
reference clock enable and stmmac_init_timestamping() failures are
propagated, and adds unwind paths in __stmmac_open() and stmmac_resume()
for those failures.  The HW timestamp capability check moves out of
stmmac_init_timestamping() into the new stmmac_check_timestamp_cap()
helper.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24656b35350b1..d33c82bd3ee46 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -885,6 +885,11 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
>  	return 0;
>  }
>  
> +static bool stmmac_check_timestamp_cap(struct stmmac_priv *priv)
> +{
> +	return priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp;
> +}
> +
>  /**
>   * stmmac_init_timestamping - initialise timestamping
>   * @priv: driver private structure
> @@ -900,11 +905,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;
> -	}
> -

[Severity: Low]
Should the kernel-doc block just above stmmac_init_timestamping() be updated
in the same patch?  It still says:

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

After this hunk the HW cap. register check lives in
stmmac_check_timestamp_cap()/stmmac_setup_ptp(), which is also why
stmmac_resume() now has to open-code "if (stmmac_check_timestamp_cap(priv))",
and stmmac_ptp_register() is only called from stmmac_setup_ptp().  The
-EOPNOTSUPP return the description implies is no longer produced by any
path here, and the function's new error returns are not described.

>  	ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
>  					       PTP_TCR_TSCFUPDT);
>  	if (ret) {
> @@ -937,22 +937,39 @@ 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 by HW\n");
> +		return 0;
> +	}
> +
>  	ret = clk_prepare_enable(priv->plat->clk_ptp_ref);

[Severity: Medium]
Does this early return unbalance clk_ptp_ref against the platform noirq PM
callbacks?

Before this patch stmmac_setup_ptp() always did clk_prepare_enable() on open
and stmmac_release_ptp() always did clk_disable_unprepare() on close, so a
running interface always held one enable/prepare reference.  With the new
capability gate in both functions, hardware whose dma_cap.time_stamp and
dma_cap.atime_stamp are clear never gets the clock enabled.  Note
stmmac_hw_init() leaves priv->dma_cap all-zero when there is no HW feature
register ("No HW DMA feature register supported").

stmmac_pltfr_noirq_suspend() in stmmac_platform.c still does this
unconditionally for any running non-WoL interface:

	if (!priv->wolopts) {
		/* Disable clock in case of PWM is off */
		clk_disable_unprepare(priv->plat->clk_ptp_ref);

With the enable/prepare counts at zero, does that hit the clk core
enable_count/prepare_count underflow warning?  And after
stmmac_pltfr_noirq_resume() re-enables it unconditionally, the close path
stmmac_release_ptp() now returns early for the same hardware, so does that
reference stay held for good?

These four sites appear to be the only clk_ptp_ref enable/disable users, and
stmmac_pltfr_pm_ops installs those noirq callbacks for a large number of
platform glue drivers.

> -	if (ret < 0)
> +	if (ret < 0) {
>  		netdev_warn(priv->dev,
>  			    "failed to enable PTP reference clock: %pe\n",
>  			    ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = stmmac_init_timestamping(priv);
> +	if (ret) {
> +		clk_disable_unprepare(priv->plat->clk_ptp_ref);
> +		return ret;
> +	}
>  
> -	if (stmmac_init_timestamping(priv) == 0)
> -		stmmac_ptp_register(priv);
> +	stmmac_ptp_register(priv);
> +
> +	return 0;
>  }
>  
>  static void stmmac_release_ptp(struct stmmac_priv *priv)
>  {
> +	if (!stmmac_check_timestamp_cap(priv))
> +		return;
> +
>  	stmmac_ptp_unregister(priv);
>  	clk_disable_unprepare(priv->plat->clk_ptp_ref);
>  }
> @@ -4161,10 +4178,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;

[Severity: High]
Can this make "ip link set dev ethX up" fail on configurations that work
today?

stmmac_setup_ptp() -> stmmac_init_timestamping() -> stmmac_init_tstamp_counter()
returns -EINVAL for a zero PTP rate:

	if (!priv->plat->clk_ptp_rate) {
		netdev_err(priv->dev, "Invalid PTP clock rate");
		return -EINVAL;
	}

Before this patch that only produced the "PTP init failed" warning and the
interface still came up without timestamping.

Is "timestamp capable but clk_ptp_rate == 0" a real configuration?
stmmac_register_devlink() already tolerates exactly that combination:

	if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp) ||
	    !priv->plat->clk_ptp_rate)
		return 0;

and stmmac_probe_config_dt() derives the rate from a clock that may be NULL:

	if (IS_ERR(plat->clk_ptp_ref)) {
		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
		plat->clk_ptp_ref = NULL;

with clk_get_rate(NULL) returning 0 (plat->stmmac_clk is left NULL on the
"Cannot get CSR clock" path, and is not fetched at all for
snps,dwc-qos-ethernet-4.10).

On such a board, would the interface become permanently un-openable for
users who never touch PTP or TAPRIO?  The same question applies to
stmmac_resume() below, where the new goto skips netif_device_attach() and
leaves a previously working link down.  Since there are two Fixes: tags this
will be picked up for stable, so it may be worth spelling the behaviour
change out in the changelog, or keeping the failure non-fatal for the
datapath and only refusing the PTP/EST-dependent features.

>  	stmmac_init_coalesce(priv);
>  
> @@ -4187,9 +4206,10 @@ static int __stmmac_open(struct net_device *dev,
>  
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> -
>  	stmmac_release_ptp(priv);
> -init_error:
> +ptp_error:
> +	stmmac_stop_all_dma(priv);
> +
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the new ptp_error exit widens
the set of ways __stmmac_open() can fail after it has already done:

	memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));

so priv->dma_conf holds its own copy of every pointer in the caller's
dma_conf.  On the MTU reopen path the caller reacts to failure like this:

stmmac_change_mtu()
	ret = __stmmac_open(dev, dma_conf);
	if (ret) {
		free_dma_desc_resources(priv, dma_conf);
		kfree(dma_conf);

The interface is left administratively up, and priv->dma_conf still points
at the freed dma_alloc_coherent() buffers and the freed tx_skbuff/rx_skbuff
arrays, since nulling pointers inside the caller's copy cannot clear the
memcpy'd copy.  A later "ip link set dev ethX down" then runs:

__stmmac_release()
	stmmac_stop_all_dma(priv);
	/* Release and free the Rx/Tx resources */
	free_dma_desc_resources(priv, &priv->dma_conf);

Does that free the same rings a second time?  The pre-existing trigger was
an stmmac_request_irq() failure via irq_error; with this patch a PTP
reference clock enable failure reaches the same state.

[Severity: Low]
Should this unwind also clear the MAC Rx/Tx enable that stmmac_hw_setup()
set?

stmmac_hw_setup() does:

	/* Enable the MAC Rx/Tx */
	stmmac_mac_set(priv, priv->ioaddr, true);

stmmac_stop_all_dma() only clears the DMA channel start bits and deinits the
channels.  The MAC enable is cleared in the mac_link_down callback, in the
reset path and in stmmac_suspend(), and on the ptp_error path
phylink_start() was never reached, so mac_link_down() cannot run.
stmmac_open() then frees the descriptor rings and drops the runtime-PM
reference, leaving the MAC clock-gated with its Rx/Tx still armed.  All
stmmac_hw_setup() failure returns happen before the stmmac_mac_set() call,
so this residue is specific to the new post-setup failure exits.

> @@ -8326,13 +8346,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 (stmmac_check_timestamp_cap(priv)) {
> +		ret = stmmac_init_timestamping(priv);
> +		if (ret)
> +			goto error_stop_dma;
> +	}
>  
>  	stmmac_init_coalesce(priv);
>  	phylink_rx_clk_stop_block(priv->phylink);
> @@ -8356,6 +8377,15 @@ int stmmac_resume(struct device *dev)
>  	netif_device_attach(ndev);
>  
>  	return 0;
> +
> +error_stop_dma:
> +	stmmac_stop_all_dma(priv);
> +error_unlock:
> +	stmmac_legacy_serdes_power_down(priv);
> +	mutex_unlock(&priv->lock);
> +	rtnl_unlock();
> +
> +	return ret;
>  }

[Severity: Low]
Same question on the resume side: stmmac_suspend() explicitly did

	stmmac_mac_set(priv, priv->ioaddr, false);

but error_stop_dma/error_unlock stops the DMA and calls
stmmac_legacy_serdes_power_down() while the MAC Rx/Tx enabled by
stmmac_hw_setup() is still set, so the SerDes is removed under an enabled
MAC.  Would adding stmmac_mac_set(priv, priv->ioaddr, false) to this unwind
restore the state that stmmac_suspend() had established?

Also, on this path netif_device_attach() is never reached while
netif_running() is still true, so the netdev stays detached after a resume
failure.  Is that the intended outcome for a PTP init error?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-stmmac-ptp-error-propagate-v1-1-80f01b03dafa%40oss.qualcomm.com

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 (1) · Medium: 1 · Low: 2
> 
> - [High] Undisclosed functional regression: PTP setup failures are now
>   fatal to `ndo_open()` and to system resume.
> - [Medium] Unbalanced clk_ptp_ref enable/disable against the platform
>   noirq PM callbacks.
> - [Low] The kernel-doc block above `stmmac_init_timestamping()`
>   (stmmac_main.c:893-899) still states 'this is to verify if the HW…
> - [Low] Both new unwind paths halt the DMA but never undo
>   `stmmac_hw_setup()`'s `stmmac_mac_set(priv, priv->ioaddr, true)`…
> 
> Pre-existing issues:
> - [High] Pre-existing double free / use-after-free of the DMA descriptor
>   rings, whose trigger set this patch widens.
> 
> --
> 
> net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()

I think I have already posted a v2 for this series:
https://patchwork.kernel.org/project/netdevbpf/patch/20260907-stmmac-ptp-error-propagate-v2-1-4a2e8e41e860@oss.qualcomm.com/

This one is marked as Superseded in patchwork.

Regards,
Lorenzo

> 
> This converts stmmac_setup_ptp() from void to int so that the PTP
> reference clock enable and stmmac_init_timestamping() failures are
> propagated, and adds unwind paths in __stmmac_open() and stmmac_resume()
> for those failures.  The HW timestamp capability check moves out of
> stmmac_init_timestamping() into the new stmmac_check_timestamp_cap()
> helper.
> 
> A few questions below.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 24656b35350b1..d33c82bd3ee46 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -885,6 +885,11 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
> >  	return 0;
> >  }
> >  
> > +static bool stmmac_check_timestamp_cap(struct stmmac_priv *priv)
> > +{
> > +	return priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp;
> > +}
> > +
> >  /**
> >   * stmmac_init_timestamping - initialise timestamping
> >   * @priv: driver private structure
> > @@ -900,11 +905,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;
> > -	}
> > -
> 
> [Severity: Low]
> Should the kernel-doc block just above stmmac_init_timestamping() be updated
> in the same patch?  It still says:
> 
> 	 * 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.
> 
> After this hunk the HW cap. register check lives in
> stmmac_check_timestamp_cap()/stmmac_setup_ptp(), which is also why
> stmmac_resume() now has to open-code "if (stmmac_check_timestamp_cap(priv))",
> and stmmac_ptp_register() is only called from stmmac_setup_ptp().  The
> -EOPNOTSUPP return the description implies is no longer produced by any
> path here, and the function's new error returns are not described.
> 
> >  	ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
> >  					       PTP_TCR_TSCFUPDT);
> >  	if (ret) {
> > @@ -937,22 +937,39 @@ 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 by HW\n");
> > +		return 0;
> > +	}
> > +
> >  	ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
> 
> [Severity: Medium]
> Does this early return unbalance clk_ptp_ref against the platform noirq PM
> callbacks?
> 
> Before this patch stmmac_setup_ptp() always did clk_prepare_enable() on open
> and stmmac_release_ptp() always did clk_disable_unprepare() on close, so a
> running interface always held one enable/prepare reference.  With the new
> capability gate in both functions, hardware whose dma_cap.time_stamp and
> dma_cap.atime_stamp are clear never gets the clock enabled.  Note
> stmmac_hw_init() leaves priv->dma_cap all-zero when there is no HW feature
> register ("No HW DMA feature register supported").
> 
> stmmac_pltfr_noirq_suspend() in stmmac_platform.c still does this
> unconditionally for any running non-WoL interface:
> 
> 	if (!priv->wolopts) {
> 		/* Disable clock in case of PWM is off */
> 		clk_disable_unprepare(priv->plat->clk_ptp_ref);
> 
> With the enable/prepare counts at zero, does that hit the clk core
> enable_count/prepare_count underflow warning?  And after
> stmmac_pltfr_noirq_resume() re-enables it unconditionally, the close path
> stmmac_release_ptp() now returns early for the same hardware, so does that
> reference stay held for good?
> 
> These four sites appear to be the only clk_ptp_ref enable/disable users, and
> stmmac_pltfr_pm_ops installs those noirq callbacks for a large number of
> platform glue drivers.
> 
> > -	if (ret < 0)
> > +	if (ret < 0) {
> >  		netdev_warn(priv->dev,
> >  			    "failed to enable PTP reference clock: %pe\n",
> >  			    ERR_PTR(ret));
> > +		return ret;
> > +	}
> > +
> > +	ret = stmmac_init_timestamping(priv);
> > +	if (ret) {
> > +		clk_disable_unprepare(priv->plat->clk_ptp_ref);
> > +		return ret;
> > +	}
> >  
> > -	if (stmmac_init_timestamping(priv) == 0)
> > -		stmmac_ptp_register(priv);
> > +	stmmac_ptp_register(priv);
> > +
> > +	return 0;
> >  }
> >  
> >  static void stmmac_release_ptp(struct stmmac_priv *priv)
> >  {
> > +	if (!stmmac_check_timestamp_cap(priv))
> > +		return;
> > +
> >  	stmmac_ptp_unregister(priv);
> >  	clk_disable_unprepare(priv->plat->clk_ptp_ref);
> >  }
> > @@ -4161,10 +4178,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;
> 
> [Severity: High]
> Can this make "ip link set dev ethX up" fail on configurations that work
> today?
> 
> stmmac_setup_ptp() -> stmmac_init_timestamping() -> stmmac_init_tstamp_counter()
> returns -EINVAL for a zero PTP rate:
> 
> 	if (!priv->plat->clk_ptp_rate) {
> 		netdev_err(priv->dev, "Invalid PTP clock rate");
> 		return -EINVAL;
> 	}
> 
> Before this patch that only produced the "PTP init failed" warning and the
> interface still came up without timestamping.
> 
> Is "timestamp capable but clk_ptp_rate == 0" a real configuration?
> stmmac_register_devlink() already tolerates exactly that combination:
> 
> 	if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp) ||
> 	    !priv->plat->clk_ptp_rate)
> 		return 0;
> 
> and stmmac_probe_config_dt() derives the rate from a clock that may be NULL:
> 
> 	if (IS_ERR(plat->clk_ptp_ref)) {
> 		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
> 		plat->clk_ptp_ref = NULL;
> 
> with clk_get_rate(NULL) returning 0 (plat->stmmac_clk is left NULL on the
> "Cannot get CSR clock" path, and is not fetched at all for
> snps,dwc-qos-ethernet-4.10).
> 
> On such a board, would the interface become permanently un-openable for
> users who never touch PTP or TAPRIO?  The same question applies to
> stmmac_resume() below, where the new goto skips netif_device_attach() and
> leaves a previously working link down.  Since there are two Fixes: tags this
> will be picked up for stable, so it may be worth spelling the behaviour
> change out in the changelog, or keeping the failure non-fatal for the
> datapath and only refusing the PTP/EST-dependent features.
> 
> >  	stmmac_init_coalesce(priv);
> >  
> > @@ -4187,9 +4206,10 @@ static int __stmmac_open(struct net_device *dev,
> >  
> >  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
> >  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> > -
> >  	stmmac_release_ptp(priv);
> > -init_error:
> > +ptp_error:
> > +	stmmac_stop_all_dma(priv);
> > +
> >  	return ret;
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the new ptp_error exit widens
> the set of ways __stmmac_open() can fail after it has already done:
> 
> 	memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
> 
> so priv->dma_conf holds its own copy of every pointer in the caller's
> dma_conf.  On the MTU reopen path the caller reacts to failure like this:
> 
> stmmac_change_mtu()
> 	ret = __stmmac_open(dev, dma_conf);
> 	if (ret) {
> 		free_dma_desc_resources(priv, dma_conf);
> 		kfree(dma_conf);
> 
> The interface is left administratively up, and priv->dma_conf still points
> at the freed dma_alloc_coherent() buffers and the freed tx_skbuff/rx_skbuff
> arrays, since nulling pointers inside the caller's copy cannot clear the
> memcpy'd copy.  A later "ip link set dev ethX down" then runs:
> 
> __stmmac_release()
> 	stmmac_stop_all_dma(priv);
> 	/* Release and free the Rx/Tx resources */
> 	free_dma_desc_resources(priv, &priv->dma_conf);
> 
> Does that free the same rings a second time?  The pre-existing trigger was
> an stmmac_request_irq() failure via irq_error; with this patch a PTP
> reference clock enable failure reaches the same state.
> 
> [Severity: Low]
> Should this unwind also clear the MAC Rx/Tx enable that stmmac_hw_setup()
> set?
> 
> stmmac_hw_setup() does:
> 
> 	/* Enable the MAC Rx/Tx */
> 	stmmac_mac_set(priv, priv->ioaddr, true);
> 
> stmmac_stop_all_dma() only clears the DMA channel start bits and deinits the
> channels.  The MAC enable is cleared in the mac_link_down callback, in the
> reset path and in stmmac_suspend(), and on the ptp_error path
> phylink_start() was never reached, so mac_link_down() cannot run.
> stmmac_open() then frees the descriptor rings and drops the runtime-PM
> reference, leaving the MAC clock-gated with its Rx/Tx still armed.  All
> stmmac_hw_setup() failure returns happen before the stmmac_mac_set() call,
> so this residue is specific to the new post-setup failure exits.
> 
> > @@ -8326,13 +8346,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 (stmmac_check_timestamp_cap(priv)) {
> > +		ret = stmmac_init_timestamping(priv);
> > +		if (ret)
> > +			goto error_stop_dma;
> > +	}
> >  
> >  	stmmac_init_coalesce(priv);
> >  	phylink_rx_clk_stop_block(priv->phylink);
> > @@ -8356,6 +8377,15 @@ int stmmac_resume(struct device *dev)
> >  	netif_device_attach(ndev);
> >  
> >  	return 0;
> > +
> > +error_stop_dma:
> > +	stmmac_stop_all_dma(priv);
> > +error_unlock:
> > +	stmmac_legacy_serdes_power_down(priv);
> > +	mutex_unlock(&priv->lock);
> > +	rtnl_unlock();
> > +
> > +	return ret;
> >  }
> 
> [Severity: Low]
> Same question on the resume side: stmmac_suspend() explicitly did
> 
> 	stmmac_mac_set(priv, priv->ioaddr, false);
> 
> but error_stop_dma/error_unlock stops the DMA and calls
> stmmac_legacy_serdes_power_down() while the MAC Rx/Tx enabled by
> stmmac_hw_setup() is still set, so the SerDes is removed under an enabled
> MAC.  Would adding stmmac_mac_set(priv, priv->ioaddr, false) to this unwind
> restore the state that stmmac_suspend() had established?
> 
> Also, on this path netif_device_attach() is never reached while
> netif_running() is still true, so the netdev stays detached after a resume
> failure.  Is that the intended outcome for a PTP init error?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-stmmac-ptp-error-propagate-v1-1-80f01b03dafa%40oss.qualcomm.com

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