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