Re: [PATCH net v5 2/3] net: stmmac: pass the desired EST enable state to est_configure()

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

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

> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit says "No functional change intended", but there is a
>   change when stmmac_adjust_time() runs concurrently.
> 
> Pre-existing issues:
> - [High] This happens at this commit in tc_taprio_configure()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c).
> - [High] At this commit stmmac_adjust_time() (stmmac_ptp.c:72) checks
>   priv->est->enable without est_lock.
> - [Medium] est_configure()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_est.c) rewrites…
> - [Medium] At this commit stmmac_adjust_time() decides est_rst from an
>   unlocked read of priv->est->enable (stmmac_ptp.c:72-73) and drops…
> 
> --
> 
> net: stmmac: pass the desired EST enable state to est_configure()
> 
> This patch adds an explicit enable argument to est_configure() so it no
> longer reads cfg->enable. The callers in stmmac_adjust_time() and
> tc_taprio_configure() now pass true or false directly, and they still set
> priv->est->enable around the EST programming as before.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> > index afc516059b897..f15d4d046aa74 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_est.c
> 
> [ ... ]
> 
> > @@ -62,7 +62,7 @@ static int est_configure(struct stmmac_priv *priv, struct stmmac_est *cfg,
> >  		ctrl |= ((NSEC_PER_SEC / ptp_rate) * EST_GMAC5_PTOV_MUL) <<
> >  			 EST_GMAC5_PTOV_SHIFT;
> >  	}
> > -	if (cfg->enable)
> > +	if (enable)
> >  		ctrl |= EST_EEST | EST_SSWL | EST_DFBS;
> >  	else
> >  		ctrl &= ~EST_EEST;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but when enable is false,
> est_configure() still rewrites BTR, TER, LLR, CTR and the whole GCL
> through est_write() before it gets here:

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

> 
>     ret |= est_write(est_addr, EST_BTR_LOW, cfg->btr[0], false);
>     ...
>     if (ret)
>         return ret;
> 
>     for (i = 0; i < cfg->gcl_size; i++) {
>         ret = est_write(est_addr, i, cfg->gcl[i], true);
>         if (ret)
>             return ret;
>     }
> 
> If one of the SRWO polls in est_write() times out, the function returns
> before it clears EST_EEST here and before it zeroes EST_INT_EN.
> 
> The disable callers ignore that return value and set
> priv->est->enable = false anyway. These are the disable: label in
> tc_taprio_configure() and the quiesce block in stmmac_adjust_time().
> 
> On TAPRIO_CMD_DESTROY, tc_taprio_configure() returns only the result of
> stmmac_fpe_map_preemption_class(), so taprio treats the offload as
> removed.
> 
> Could the MAC keep gating TX with the old schedule, with EST interrupts
> still on, while software thinks EST is off? This ordering is unchanged at
> the end of the series.
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > index e54a3c8878265..9a32af00d7d2d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> > @@ -74,7 +74,7 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> >  		mutex_lock(&priv->est_lock);
> >  		priv->est->enable = false;
> >  		stmmac_est_configure(priv, priv, priv->est,
> > -				     priv->plat->clk_ptp_rate);
> > +				     priv->plat->clk_ptp_rate, false);
> >  		mutex_unlock(&priv->est_lock);
> >  	}
> 
> [Severity: High]
> This is a pre-existing issue, but the check that guards this block reads
> priv->est->enable without holding est_lock:

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) {
>         est_rst = true;
>         mutex_lock(&priv->est_lock);
> 
> A TAPRIO_CMD_REPLACE in tc_taprio_configure() drops est_lock between
> steps:
> 
>     mutex_lock(&priv->est_lock);
>     memset(priv->est, 0, sizeof(*priv->est));
>     mutex_unlock(&priv->est_lock);
>     ...
>     mutex_lock(&priv->est_lock);
>     priv->est->gcl_size = size;
>     priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
>     mutex_unlock(&priv->est_lock);
> 
>     for (i = 0; i < size; i++) {
>         ...
>         priv->est->gcl[i] = delta_ns | (gates << wid);
>     }
> 
>     mutex_lock(&priv->est_lock);
>     /* ctr[] and btr_reserve[] are filled only here */
> 
> Suppose the second est_lock section of stmmac_adjust_time() runs in that
> gap. It then computes:
> 
>     cycle_time = (u64)priv->est->ctr[1] * NSEC_PER_SEC +
>                  priv->est->ctr[0];    /* still 0 */
>     time = stmmac_calc_tas_basetime(basetime, current_time_ns,
>                                     cycle_time);
> 
> and stmmac_calc_tas_basetime() does:
> 
>     n = div64_s64(ktime_sub_ns(current_time, old_base_time),
>                   cycle_time);
> 
> Can this divide by zero and oops on x86? It would also program a GCL
> that is still half written through est_configure(..., true).
> 
> This looks to be addressed later in the series by "net: stmmac: re-apply
> taprio offload in __stmmac_open() and stmmac_resume()". That commit:
> 
>   - holds est_lock across the whole validate, memset and program
>     sequence in tc_taprio_configure()
>   - sets enable only after programming succeeds
>   - checks enable under est_lock in stmmac_adjust_time()
> 
> > @@ -105,7 +105,7 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> >  		priv->est->btr[1] = (u32)time.tv_sec;
> >  		priv->est->enable = true;
> >  		ret = stmmac_est_configure(priv, priv, priv->est,
> > -					   priv->plat->clk_ptp_rate);
> > +					   priv->plat->clk_ptp_rate, true);
> >  		mutex_unlock(&priv->est_lock);
> >  		if (ret)
> >  			netdev_err(priv->dev, "failed to configure EST\n");
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but est_rst comes from the
> unlocked check before the first critical section. Nothing is re-checked
> here after est_lock was dropped around the systime step.

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

> 
> A TAPRIO_CMD_DESTROY can land in that gap:
> 
> CPU1 (stmmac_adjust_time)        CPU2 (tc_taprio_configure, DESTROY)
> est_rst = true
> priv->est->enable = false
> est_configure(..., false)
> mutex_unlock(&priv->est_lock)
>                                  disable:
>                                  priv->est->enable = false
>                                  est_configure(..., false)
>                                  returns success
> stmmac_adjust_systime()
> mutex_lock(&priv->est_lock)
> priv->est->enable = true
> est_configure(..., true)
> 
> Does this bring back a schedule that taprio has already removed? The
> xmit paths would then also enforce the priv->est->max_sdu[] drops again.
> 
> The same later commit, "net: stmmac: re-apply taprio offload in
> __stmmac_open() and stmmac_resume()", appears to fix this. It re-checks
> priv->est && priv->est->enable under est_lock before calling
> __stmmac_setup_est(), and stmmac_adjust_time() no longer writes the
> enable flag.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index aea235a69105e..ac926c60f9f9a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > @@ -1090,7 +1090,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> >  	tc_taprio_map_maxsdu_txq(priv, qopt);
> >  
> >  	ret = stmmac_est_configure(priv, priv, priv->est,
> > -				   priv->plat->clk_ptp_rate);
> > +				   priv->plat->clk_ptp_rate, true);
> 
> [Severity: Low]
> The commit message says:
> 
>   No functional change intended: the callers keep toggling
>   priv->est->enable around the EST programming, as before.
> 
> Does that still hold when stmmac_adjust_time() runs at the same time?
> 
> Before this patch, this call read cfg->enable under est_lock. The REPLACE
> path sets enable = true in an earlier critical section and then drops
> est_lock while it fills the GCL. Consider this sequence:
> 
> CPU1 (tc_taprio_configure, REPLACE)  CPU2 (stmmac_adjust_time)
> priv->est->enable = true
> mutex_unlock(&priv->est_lock)
> fill priv->est->gcl[]
>                                      priv->est->enable = false
>                                      est_configure(..., false)
>                                      mutex_unlock(&priv->est_lock)
> mutex_lock(&priv->est_lock)
> est_configure(..., true)
>                                      stmmac_adjust_systime()

As the one above, this issue has not been introduced by this patch and I think
we should fix it with a dedicated patch.

> 
> With the old code, the REPLACE would program EST with EEST cleared,
> because cfg->enable was false. EST then stayed off across the time step.
> 
> With the hard-coded true, EST is turned on with a BTR computed from the
> pre-step time. The PHC step then happens with EST active while
> priv->est->enable is false, until the second section of
> stmmac_adjust_time() reprograms it.
> 
> The underlying race is pre-existing, since no lock covers the time step
> together with the EST programming. Still, this interleaving behaves
> differently after this patch, and the window remains at the end of the
> series.
> 
> >  	mutex_unlock(&priv->est_lock);
> >  	if (ret) {
> >  		netdev_err(priv->dev, "failed to configure EST\n");
> > @@ -1109,7 +1109,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> >  		mutex_lock(&priv->est_lock);
> >  		priv->est->enable = false;
> >  		stmmac_est_configure(priv, priv, priv->est,
> > -				     priv->plat->clk_ptp_rate);
> > +				     priv->plat->clk_ptp_rate, false);
> 
> [Severity: High]
> This is a pre-existing issue, but the entry validation loop earlier in
> tc_taprio_configure() returns directly instead of going through this
> disable: label:

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

Regards,
Lorenzo

> 
>     priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
>     mutex_unlock(&priv->est_lock);
> 
>     for (i = 0; i < size; i++) {
>         ...
>         if (delta_ns > GENMASK(wid - 1, 0))
>             return -ERANGE;
>         if (gates > GENMASK(31 - wid, 0))
>             return -ERANGE;
>         ...
>         default:
>             return -EOPNOTSUPP;
>         }
> 
> By that point priv->est has already been memset. So a REPLACE with one
> invalid interval, gate mask or command leaves enable == true while ctr[]
> and btr_reserve[] are zero.
> 
> taprio_disable_offload() does not call back into the driver when the
> first offload attempt failed, so nothing clears the flag later.
> 
> A later clock_adjtime(ADJ_SETOFFSET) on the PHC then does:
> 
> stmmac_adjust_time()
>   cycle_time = 0
>   stmmac_calc_tas_basetime(0, now, 0)
>     div64_s64(now, 0)
> 
> Can this divide by zero and oops on x86?
> 
> This also looks to be fixed later in the series by "net: stmmac: re-apply
> taprio offload in __stmmac_open() and stmmac_resume()". That commit
> validates every entry into a temporary kcalloc() buffer before it touches
> priv->est, and sets enable only after programming succeeds.
> 
> >  		/* Reset taprio status */
> >  		for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
> >  			priv->xstats.max_sdu_txq_drop[i] = 0;
> 
> -- 
> 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 --]