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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 10572 bytes --]

> 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?

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

> 
> 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?

This is the same issue reported in patch 2/3. As pointed out before this issue
has not been introduced by this patch and it needs to be fixed with a dedicated
patch.

> 
> > 
> >  	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():

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

> 
> 	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?

This issue has not been introduced by this patch and it needs to be fixed with
a dedicated patch.

Regards,
Lorenzo

> 
> > +	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]