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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Calling `tc qdisc add dev <eth> parent root taprio ...` when the interface is down causes a system crash due to uninitialized locks and disabled hardware clocks.
--

commit f82312d096c2ce72658ffac1182f1169293623b9
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

This commit re-applies the taprio offload in __stmmac_open() and 
stmmac_resume() after PTP is up because a core soft reset clears the 
MTL_EST registers. It introduces a new __stmmac_setup_est() utility 
routine to handle the hardware schedule configuration, updating how 
tc_taprio_configure() applies these settings atomically.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> index f15d4d046aa74..49edfebbc39e0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c

[ ... ]

> +int __stmmac_setup_est(struct stmmac_priv *priv)
> +{
> +	struct timespec64 current_time, time;
> +	ktime_t current_time_ns, basetime;
> +	u64 cycle_time;
> +	int err;
> +
> +	lockdep_assert_held(&priv->est_lock);
> +
> +	priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, &current_time);

[Severity: Critical]
This is a pre-existing issue, but since the hardware access is refactored
into this helper, it highlights a crash if called when the interface is down.

If tc_taprio_configure() is called while the interface is down, it reaches
here and invokes the gettime64() callback. The stmmac_get_time() function
attempts to acquire priv->ptp_lock. However, rwlock_init(&priv->ptp_lock)
only occurs in stmmac_ptp_register() during __stmmac_open().

Does this code trigger a panic (e.g., on PREEMPT_RT or with PROVE_LOCKING)
because it uses an uninitialized lock when the interface has never been
opened?

> +	current_time_ns = timespec64_to_ktime(current_time);

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index be48bcabdadca..9745c9bae33c1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -1053,51 +1067,44 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  			gates &= ~BIT(0);
>  			break;
>  		default:
> -			return -EOPNOTSUPP;
> +			ret = -EOPNOTSUPP;
> +			goto free_gcl;
>  		}
>  
> -		priv->est.gcl[i] = delta_ns | (gates << wid);
> +		gcl[i] = delta_ns | (gates << wid);
>  	}
>  
> -	mutex_lock(&priv->est_lock);
> -	/* Adjust for real system time */
> -	priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, &current_time);
> -	current_time_ns = timespec64_to_ktime(current_time);
> -	time = stmmac_calc_tas_basetime(qopt->base_time, current_time_ns,
> -					qopt->cycle_time);
> -
> -	priv->est.btr[0] = (u32)time.tv_nsec;
> -	priv->est.btr[1] = (u32)time.tv_sec;
> +	memset(&priv->est, 0, sizeof(priv->est));
> +	memcpy(priv->est.gcl, gcl, sizeof(priv->est.gcl));
> +	priv->est.gcl_size = size;
>  
> -	qopt_time = ktime_to_timespec64(qopt->base_time);
> -	priv->est.btr_reserve[0] = (u32)qopt_time.tv_nsec;
> -	priv->est.btr_reserve[1] = (u32)qopt_time.tv_sec;
> +	time = ktime_to_timespec64(qopt->base_time);
> +	priv->est.btr_reserve[0] = (u32)time.tv_nsec;
> +	priv->est.btr_reserve[1] = (u32)time.tv_sec;
>  
> -	ctr = qopt->cycle_time;
>  	priv->est.ctr[0] = do_div(ctr, NSEC_PER_SEC);
>  	priv->est.ctr[1] = (u32)ctr;
>  
>  	priv->est.ter = qopt->cycle_time_extension;
> -
>  	tc_taprio_map_maxsdu_txq(priv, qopt);
>  
> -	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");
> +	ret = __stmmac_setup_est(priv);

[Severity: Critical]
This isn't a bug introduced by this patch, but does tc_taprio_configure()
need to verify netif_running() before calling this helper?

When a user configures a TAPRIO qdisc via tc while the interface is down,
the PTP reference clock (clk_ptp_ref) is still disabled. Calling
__stmmac_setup_est() unconditionally accesses MAC PTP and EST hardware
registers. 

Can accessing these registers without their corresponding clocks enabled
cause a synchronous bus fault or system hang on some platforms?

> +	if (ret)
>  		goto disable;
> -	}
>  
>  	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
>  					      qopt->mqprio.preemptible_tcs);
>  	if (ret)
>  		goto disable;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com?part=4