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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 0

- [Medium] The `!priv->plat->clk_ptp_rate` -> -EINVAL guard is deleted
  from stmmac_init_tstamp_counter()…
- [Medium] stmmac_hwtstamp_set() (stmmac_main.c:656) and
  stmmac_hwtstamp_get() (stmmac_main.c:846) now reject hardware…
- [Medium] stmmac_update_subsecond_increment() now returns the
  stmmac_config_addend() error, but by the time that error is produced…
- [Medium] Making stmmac_setup_ptp() failure abort __stmmac_open() adds a
  new route by which open fails *after* a preceding __stmmac_release()…

--

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

This makes stmmac_setup_ptp() return an error code and propagates PTP
setup failures out of __stmmac_open() and stmmac_resume(), stopping the
DMA engines on failure.  It also propagates the addend and system time
programming errors out of stmmac_update_subsecond_increment() and
stmmac_init_tstamp_counter(), and adds a stmmac_check_timestamp_cap()
helper used to gate the timestamping paths.

Link to v2: https://lore.kernel.org/r/20260907-stmmac-ptp-error-propagate-v2-1-4a2e8e41e860@oss.qualcomm.com
Link to v1: https://lore.kernel.org/r/20260904-stmmac-ptp-error-propagate-v1-1-80f01b03dafa@oss.qualcomm.com

A few questions on the new capability gate and on the newly propagated
errors below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 7582fca637419..0a00490832f3c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -419,6 +419,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_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 5fe7e95fdd345..7f5ada1132c41 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -601,7 +601,7 @@ static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
>  	}
>  }
>  
> -static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
> +static int stmmac_update_subsecond_increment(struct stmmac_priv *priv)
>  {
>  	bool xmac = dwmac_is_xmac(priv->plat->core_type);
>  	u32 sec_inc = 0;
> @@ -625,7 +625,7 @@ static void stmmac_update_subsecond_increment(struct stmmac_priv *priv)
>  	 */
>  	temp = (u64)(temp << 32);
>  	priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);
> -	stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
> +	return stmmac_config_addend(priv, priv->ptpaddr, priv->default_addend);
>  }
>  
>  /**
> @@ -653,7 +653,7 @@ 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 (!stmmac_check_timestamp_cap(priv)) {
>  		NL_SET_ERR_MSG_MOD(extack, "No support for HW time stamping");
>  		priv->hwts_tx_en = 0;
>  		priv->hwts_rx_en = 0;
> @@ -843,7 +843,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;

[Severity: Medium]
Should the advertising side of this uAPI learn about the new
clk_ptp_rate requirement too?

Before this change stmmac_hwtstamp_get() used a predicate identical to
stmmac_get_ts_info() in stmmac_ethtool.c, so the two agreed.  That
function still keys on the dma_cap bits alone:

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:stmmac_get_ts_info() {
	if ((priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp)) {

		info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
					SOF_TIMESTAMPING_TX_HARDWARE |
					SOF_TIMESTAMPING_RX_HARDWARE |
					SOF_TIMESTAMPING_RAW_HARDWARE;
	...
}

On the platform class the commit message describes as "PTP-capable
silicon but an unconfigured PTP clock", where plat->clk_ptp_rate ends up
0, does ethtool -T then keep claiming full hardware timestamping and the
complete rx_filters/tx_types bitmaps while every SIOCSHWTSTAMP and
SIOCGHWTSTAMP returns -EOPNOTSUPP?

The commit message says the helper is applied to the get/set paths "so
they consistently report -EOPNOTSUPP when timestamping is not usable" -
should stmmac_get_ts_info() be part of that consistency?

While looking at that function, when no PHC is registered it reports
info->phc_index = 0, which is a valid index owned by some other device.
Is that intended?

> @@ -865,32 +865,32 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
>  				      u32 systime_flags)
>  {
>  	struct timespec64 now;
> -
> -	if (!priv->plat->clk_ptp_rate) {
> -		netdev_err(priv->dev, "Invalid PTP clock rate");
> -		return -EINVAL;
> -	}
> +	int ret;

[Severity: Medium]
Is the new gate a full replacement for this guard, given the ordering?

This was the only clk_ptp_rate validation that ran after
priv->plat->ptp_clk_freq_config().  stmmac_check_timestamp_cap() is
evaluated in stmmac_setup_ptp() and in stmmac_resume() before
stmmac_init_timestamping() invokes the platform hook, and that hook can
rewrite the very value the gate sampled:

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:ethqos_ptp_clk_freq_config() {
	/* Max the PTP ref clock out to get the best resolution possible */
	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);
}

Note that clk_ptp_rate is stored even when clk_set_rate() failed.  With
the guard gone, nothing revalidates the post-hook value before it is
used as a divisor:

__stmmac_open()
  stmmac_setup_ptp()			<- gate uses probe-time rate
    stmmac_init_timestamping()
      priv->plat->ptp_clk_freq_config()	<- rate rewritten here
      stmmac_init_tstamp_counter()
        stmmac_update_subsecond_increment()
          stmmac_config_sub_second_increment(priv, priv->ptpaddr,
                                             priv->plat->clk_ptp_rate, ...)
          priv->default_addend = div_u64(temp, priv->plat->clk_ptp_rate);

If the post-hook rate is 0, can this divide by zero, or on arm64 where
udiv yields 0 program PTP_SSIR = 0 and addend = 0 while
stmmac_setup_ptp() returns 0 and stmmac_ptp_register() exposes a PHC
whose counter never advances?  That is the non-running counter state the
commit message says it eliminates.

There is also the mirror case: on a platform whose probe-time
clk_get_rate() reads 0 but whose ptp_clk_freq_config() hook would
program a valid rate, does the hook now never run at all, so the port is
permanently reported as PTP-less where before it configured itself at
first open?

>  	stmmac_config_hw_tstamping(priv, priv->ptpaddr, systime_flags);
>  	priv->systime_flags = systime_flags;
>  
> -	stmmac_update_subsecond_increment(priv);
> +	ret = stmmac_update_subsecond_increment(priv);
> +	if (ret)
> +		return ret;
>  
>  	/* initialize system time */
>  	ktime_get_real_ts64(&now);
>  
>  	/* lower 32 bits of tv_sec are safe until y2106 */
> -	stmmac_init_systime(priv, priv->ptpaddr, (u32)now.tv_sec, now.tv_nsec);
> -
> -	return 0;
> +	return stmmac_init_systime(priv, priv->ptpaddr, (u32)now.tv_sec,
> +				   now.tv_nsec);
>  }

[ ... ]

> @@ -900,11 +900,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 +932,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\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);
> +
> +	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 +4173,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: Medium]
Can this new failure route leave the clk_ptp_ref enable/prepare count
below zero?

stmmac_change_mtu() releases and reopens the interface, and on reopen
failure it only frees the new dma_conf while the netdev stays up:

drivers/net/ethernet/stmicro/stmmac/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;
	}
}

So with a PTP programming failure, for example an init_systime() or
config_addend() poll timeout:

__stmmac_release() -> stmmac_release_ptp() -> clk_disable_unprepare()   (count 0)
__stmmac_open() -> stmmac_setup_ptp() -> clk_prepare_enable()           (count 1)
                   stmmac_init_timestamping() fails
                   clk_disable_unprepare()                             (count 0)
                   goto ptp_error, error returned, netdev still up
later ip link set down -> __stmmac_release() -> stmmac_release_ptp()
                       -> clk_disable_unprepare()                      (count 0 already)

Since stmmac_release_ptp() decides purely from the re-evaluated
capability predicate, and dma_cap plus clk_ptp_rate are unchanged, does
the second clk_disable_unprepare() hit WARN_ON(core->enable_count == 0)
and underflow prepare_count?  A system suspend in the same window looks
like it reaches the same double disable through
stmmac_pltfr_noirq_suspend(), which now disables clk_ptp_ref whenever
netif_running() and the predicate are true.

Given the Fixes: 0ad2be79f254 ("net: stmmac: Balance PTP reference clock
enable/disable") tag, would tracking whether the clock was actually
acquired, or tearing the interface down when the reopen fails, be a
better fit than keying the disable off the capability predicate?

>  
>  	stmmac_init_coalesce(priv);
>  
> @@ -4189,7 +4203,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;
>  }
>  
> @@ -7685,9 +7702,7 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
>  	/* In Coarse mode, we can use a smaller subsecond increment, let's
>  	 * reconfigure the systime, subsecond increment and addend.
>  	 */
> -	stmmac_update_subsecond_increment(priv);
> -
> -	return 0;
> +	return stmmac_update_subsecond_increment(priv);
>  }

[Severity: Medium]
Should the software state be rolled back when this now-propagated error
fires?

By the time the error is produced, stmmac_dl_ts_coarse_set() has already
committed priv->tsfupdt_coarse and flipped PTP_TCR_TSCFUPDT in
priv->systime_flags, and stmmac_update_subsecond_increment() has already
written PTP_TCR and PTP_SSIR and cached priv->sub_second_inc and
priv->default_addend.  The only failure mode of the addend write is a
poll timeout:

drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:config_addend() {
	return readl_poll_timeout_atomic(ioaddr + PTP_TCR, value,
					 !(value & PTP_TCR_TSADDREG),
					 10, 100000);
}

so the addend register keeps its previous value while the update-mode
bit and the sub-second increment have been reprogrammed.  In fine update
mode, does the system time counter then advance at a rate matching
neither the old nor the new configuration, with priv->default_addend no
longer describing the register contents?

Also, devlink is told the set failed, but stmmac_dl_ts_coarse_get()
unconditionally reports the already-mutated boolean:

	ctx->val.vbool = priv->tsfupdt_coarse;

so a rejected set reads back as applied.  Would committing
priv->tsfupdt_coarse and priv->systime_flags only after successful
programming, or restoring them on failure, be preferable here?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910-stmmac-ptp-error-propagate-v3-1-4f386e8256b6%40oss.qualcomm.com