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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

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