Re: [PATCH v13 08/14] coresight: etm4x: exclude ss_status from drvdata->config

[Yeoreum Yun <yeoreum.yun@arm.com>]

On Sat, Oct 03, 2026 at 01:33:43AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] coresight: etm4x: state leakage of single-shot comparator status in perf sessions
> --
> 
> commit 3d98ebedda262d5221467f55a32e0267bdd2e584
> Author: Yeoreum Yun <yeoreum.yun@arm.com>
> coresight: etm4x: exclude ss_status from drvdata->config
> 
> This patch moves the single-shot comparator status (ss_status) from the
> configuration structure to the driver data structure. It updates the usage
> of ss_status to clear both TRCSSCSRn_STATUS and TRCSSCSRn_PENDING bits
> before writing to the register.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 3900b96e9c7cc..4a8c1070bbcbb 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> 
> [ ... ]
> 
> > @@ -572,11 +572,11 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> >  		etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
> >  
> >  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> > -		/* always clear status bit on restart if using single-shot */
> > +		/* always clear status and pending bits on restart if using single-shot */
> >  		if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> > -			config->ss_status[i] &= ~TRCSSCSRn_STATUS;
> 
> [Severity: Medium]
> Will this bypass the manual clearing of the TRCSSCSRn_STATUS and
> TRCSSCSRn_PENDING bits during perf sessions?
> 
> Since enabling a perf session uses a zeroed configuration, config->ss_ctrl[i]
> and config->ss_pe_cmp[i] will evaluate to false. Previously, this wasn't an
> issue because the entire config structure was zeroed via memset. Now that
> ss_status is moved to drvdata, bypassing this check means the bits are not
> cleared.
> 
> > +			drvdata->ss_status[i] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> >  		etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i));
> > -		etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i));
> > +		etm4x_relaxed_write32(csa, drvdata->ss_status[i], TRCSSCSRn(i));
> 
> Does this write a stale, unmasked drvdata->ss_status[i] directly to the
> TRCSSCSRn hardware register during perf sessions?
> 
> If the manual clearing above is skipped, this could cause state leakage and
> trigger unintended hardware trace behavior.
> 
> >  		if (etm4x_sspcicrn_present(drvdata, i))
> >  			etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i));
> >  	}

No. Since curr_config is initialized to zero and the other bits in ss_status
are RO or RES fields, clearing only the STATUS and PENDING bits has the same
effect as zeroing the entire value.


> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261002-separate_etm_cfg_v2-v13-0-fe429d8d96fb@arm.com?part=8

-- 
Sincerely,
Yeoreum Yun