Re: [PATCH net-next v5 15/19] net: stmmac: restore TC offloads before restarting DMA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: restoring failed TAPRIO schedule uses stale base time
--

commit 8c820a1d24ead26f459c2d60dccc5c98e5feaf2f
Author: James Hilliard <james.hilliard1@gmail.com>

net: stmmac: restore TC offloads before restarting DMA

This commit adjusts the restoration of TC offloads in stmmac to occur
before restarting DMA during hardware setup or resume. It also modifies
TAPRIO configuration to validate replacements separately, only publishing
the new schedule upon success and falling back to the old schedule if
programming fails.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
[ ... ]
> @@ -1032,48 +1091,43 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
[ ... ]
> -	ret = __stmmac_setup_est(priv);
> +	mutex_lock(&priv->est_lock);
> +	ret = __stmmac_setup_est(priv, est);
>  	if (ret)
> -		goto disable;
> +		goto restore;
>  
>  	ret = tc_config_preemption(priv, extack, qopt->mqprio.preemptible_tcs);
>  	if (ret)
> -		goto disable;
> -
> -	priv->est.enable = true;
> -	kfree(gcl);
> +		goto restore;
>  
> +	priv->est = *est;
>  	mutex_unlock(&priv->est_lock);
> +free_est:
> +	kfree(est);
> +	return ret;
>  
> -	return 0;
> +restore:
> +	/* A failed hardware update must not publish the rejected schedule. */
> +	if (stmmac_est_configure(priv, priv, &priv->est,
> +				 priv->plat->clk_ptp_rate, priv->est.enable))

[Severity: Medium]
Will this restore operation use a stale base time that is in the past?

If __stmmac_setup_est() succeeds with the new schedule but the subsequent
tc_config_preemption() fails, the code jumps to the restore label.

This restore path calls stmmac_est_configure() directly on the old
priv->est. Because it bypasses __stmmac_setup_est() for the restoration,
it skips calculating a valid future base time via stmmac_calc_tas_basetime()
and appears to write the old base time register values back to the hardware.

Could this lead to hardware schedule malfunctions, such as loop errors
or a failure to start, since the old base time lies in the past?

> +		netdev_err(priv->dev, "failed to restore EST\n");
> +	mutex_unlock(&priv->est_lock);
> +	goto free_est;
>  
>  disable:
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927-submit-stmmac-reset-fixes-v1-v5-0-feec6c14dd06@gmail.com?part=15