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

[Maxime Chevallier]

Hi,

On 9/7/26 13:50, Lorenzo Bianconi 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.
> 
> Make stmmac_setup_ptp() return an error code and propagate the
> failure in __stmmac_open() and stmmac_resume(), stopping the DMA
> engines when PTP setup fails.
> 
> Extend the same error propagation to the timestamping counter
> initialisation: stmmac_update_subsecond_increment() and
> stmmac_init_tstamp_counter() now return the addend and system time
> programming errors instead of discarding them, so a counter that
> cannot be configured is reported as a failure rather than silently
> left non-running.
> 
> While at it, factor the timestamping availability check into a
> stmmac_check_timestamp_cap() helper that requires both the hardware
> timestamping capability and a valid PTP reference clock rate. This
> keeps the interface operational on platforms with PTP-capable
> silicon but an unconfigured PTP clock, where timestamping cannot be
> enabled: those are treated as PTP-less rather than failing to open
> or resume. Apply the same helper to the hwtstamp get/set paths so
> they consistently report -EOPNOTSUPP when timestamping is not usable.
> 
> 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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 96 +++++++++++++++--------
>  1 file changed, 64 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 07a6fab6460e..99d4fbccc300 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,18 @@ 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);
> +}
> +
> +static 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;
>  }
>  
>  /**
> @@ -653,7 +664,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)) {

This isn't equivalent, as here we check for adv_ts. adv_ts is more restrictive than
just checking the atime_stamp cap, as on dwmac1000 we need both extend descriptors
and atime_stamp capa to set adv_ts.

Now we do have a discrepancy between the _set and _get timestamping ops, as the _get
part only checks the atime_stamp capa.

It seems to me that your fix is the correct one though.

To me the patch looks OK,

Reviewed-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>

Maxime