Re: [PATCH net v5 3/3] net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] A taprio REPLACE that fails the FPE callback's software
  validation destroys the installed hardware schedule, even though the…
- [Low] stmmac_adjust_time() still uses two separate est_lock critical
  sections: it disables EST, runs adjust_systime under ptp_lock only,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  a new way to reach it.
- [Low] The TX/XDP hot paths (stmmac_main.c:2753, 4861, 5286) read
  priv->est, priv->est->enable and priv->est->max_sdu[] with no lock and…

--

net: stmmac: re-apply taprio offload in __stmmac_open() and stmmac_resume()

This commit re-applies the taprio EST offload and the FPE preemption
mapping after the core soft reset, in __stmmac_open() and stmmac_resume().
The base time recompute moves into __stmmac_setup_est() and
stmmac_setup_est().

tc_taprio_configure() now holds est_lock across validation and
programming, builds the gate list in a temporary buffer, and sets
priv->est->enable only after success.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2163e79fea9a4..d1e3d2cc77670 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -4201,6 +4202,10 @@ static int __stmmac_open(struct net_device *dev,
>  	if (ret)
>  		goto ptp_error;
>  
> +	ret = stmmac_setup_est(priv);
> +	if (ret)
> +		goto est_error;
> +
>  	stmmac_init_coalesce(priv);

[Severity: High]
This issue predates the patch, but the new est_error exit gives it one
more trigger. If stmmac_setup_est() fails during an MTU change, is the
running netdev left with freed rings and TX hrtimers that were never
initialised?

stmmac_change_mtu() calls __stmmac_release() and then
__stmmac_open(dev, dma_conf), which memcpy()s the kzalloc'd dma_conf into
priv->dma_conf. The est_error exit returns before stmmac_init_coalesce(),
so hrtimer_setup() never runs on the txtimers.

stmmac_change_mtu() then does:

	ret = __stmmac_open(dev, dma_conf);
	if (ret) {
		free_dma_desc_resources(priv, dma_conf);
		kfree(dma_conf);
		...
		return ret;
	}

and the interface stays up. On the next ifdown, __stmmac_release() calls
hrtimer_cancel() on priv->dma_conf.tx_queue[chan].txtimer, and
hrtimer_active() dereferences a NULL timer->base. After that,
free_dma_desc_resources(priv, &priv->dma_conf) frees the same rings a
second time.

Failures in stmmac_hw_setup() and stmmac_setup_ptp() already reach this
path. A failed EST/FPE replay is a new way in.

>  
>  	phylink_start(priv->phylink);
> @@ -4224,7 +4229,7 @@ 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);
> -
> +est_error:
>  	stmmac_release_ptp(priv);
>  ptp_error:
>  	stmmac_stop_all_dma(priv);

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 9a32af00d7d2d..a37405534bebf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c

[ ... ]

> @@ -69,47 +69,21 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
>  	nsec = reminder;
>  
>  	/* If EST is enabled, disabled it before adjust ptp time. */
> -	if (priv->est && priv->est->enable) {
> -		est_rst = true;
> -		mutex_lock(&priv->est_lock);
> -		priv->est->enable = false;
> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable)
>  		stmmac_est_configure(priv, priv, priv->est,
>  				     priv->plat->clk_ptp_rate, false);
> -		mutex_unlock(&priv->est_lock);
> -	}
> +	mutex_unlock(&priv->est_lock);
>  
>  	write_lock_irqsave(&priv->ptp_lock, flags);
>  	stmmac_adjust_systime(priv, priv->ptpaddr, sec, nsec, neg_adj, xmac);
>  	write_unlock_irqrestore(&priv->ptp_lock, flags);
>  
>  	/* Calculate new basetime and re-configured EST after PTP time adjust. */

[ ... ]

> +	mutex_lock(&priv->est_lock);
> +	if (priv->est && priv->est->enable)
> +		__stmmac_setup_est(priv);
> +	mutex_unlock(&priv->est_lock);

[Severity: Low]
Can stmmac_setup_est() turn EST back on between these two est_lock
sections?

The first section no longer clears priv->est->enable, so any est_lock
user that runs while the time is being stepped still sees
enable == true.

In __stmmac_open(), stmmac_setup_est() runs right after
stmmac_setup_ptp() has registered the PHC. clock_adjtime() is not
serialised by rtnl:

CPU0 (stmmac_adjust_time)          CPU1 (__stmmac_open)
  disable EST, drop est_lock
                                     stmmac_setup_est()
                                       sees enable == true
                                       __stmmac_setup_est() enables EST
                                       with a pre-step base time
  stmmac_adjust_systime()
    (EST running across the step)
  __stmmac_setup_est()

A taprio REPLACE can race with adjust_time in the same way.

Before this patch, clearing enable during the window kept other paths
from re-arming EST. Would holding est_lock across the disable, the
adjust and the re-enable close this window?

> 
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index ac926c60f9f9a..10b61fa2c27c5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -1060,13 +1065,26 @@ static int tc_taprio_configure(struct stmmac_priv *priv,

[ ... ]

> -		priv->est->gcl[i] = delta_ns | (gates << wid);
> +		gcl[i] = delta_ns | (gates << wid);
>  	}
>  
> -	mutex_lock(&priv->est_lock);
> +	if (!priv->est) {
> +		priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
> +					 GFP_KERNEL);
> +		if (!priv->est) {
> +			ret = -ENOMEM;
> +			goto error_unlock;
> +		}
> +	} else {
> +		memset(priv->est, 0, sizeof(*priv->est));
> +	}

[Severity: Low]
This is a pre-existing issue that the patch did not introduce.
stmmac_xmit(), stmmac_xdp_xmit_xdpf() and the other TX paths read
priv->est, priv->est->enable and priv->est->max_sdu[] without est_lock
and without READ_ONCE():

	if (priv->est && priv->est->enable &&
	    priv->est->max_sdu[queue]) {

If a REPLACE runs while traffic is being sent, can this memset() and the
plain stores after it race with those readers? That could drop a frame
by mistake, or let one through past max_sdu, while the schedule is being
reconfigured.

Setting enable last in this patch does make the window smaller.

> +	memcpy(priv->est->gcl, gcl, sizeof(*gcl) * qopt->num_entries);
> +	priv->est->gcl_size = qopt->num_entries;
> +

[ ... ]

> @@ -1091,7 +1109,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  
>  	ret = stmmac_est_configure(priv, priv, priv->est,
>  				   priv->plat->clk_ptp_rate, true);
> -	mutex_unlock(&priv->est_lock);
>  	if (ret) {
>  		netdev_err(priv->dev, "failed to configure EST\n");
>  		goto disable;
> @@ -1102,11 +1119,16 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  	if (ret)
>  		goto disable;
>  

[Severity: Medium]
If a REPLACE fails FPE validation, is the installed schedule still torn
down?

The commit message says:

  A REPLACE rejected during validation then leaves the currently
  installed schedule intact in both software and hardware

However, by the time stmmac_fpe_map_preemption_class() runs, priv->est
has already been memset and overwritten, and
stmmac_est_configure(..., true) has programmed the new schedule.

dwmac5_fpe_map_preemption_class() only does software validation, and it
can return -EINVAL. One example:

	if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
		NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
		return -EINVAL;
	}

It can also fail on a TXQ weight mismatch. Either error takes the
disable path, which clears priv->est->enable and turns off EST in
hardware.

Meanwhile taprio_change() sees the error, jumps to free_sched, and keeps
the old schedule along with its full-offload state.

For example, on DWMAC5 with FPE: install an all-express schedule that
has a multi-queue TC, then REPLACE it with that TC marked preemptible.
The user gets -EINVAL and the qdisc still reports the old schedule as
offloaded. But hardware gating and max_sdu enforcement are gone, and a
later reopen or resume will not bring them back because enable is now
false.

Could the FPE validation run before priv->est is changed and EST is
programmed?

> +	priv->est->pclass = qopt->mqprio.preemptible_tcs;
> +	priv->est->enable = true;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-stmmac-est-fixes-v5-0-85b830a795a9%40oss.qualcomm.com