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

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

On Thu, Sep 24, 2026 at 06:21:35PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] coresight: etm4x: patch fails to preserve perf single-shot clearing behavior
> - [Medium] coresight: etm4x: data race on drvdata->ss_status during sysfs update
> --
> 
> commit 9755069e34bb3af0812d8f23f42579079827c7ca
> Author: Yeoreum Yun <yeoreum.yun@arm.com>
> 
> coresight: etm4x: exclude ss_status from drvdata->config
> 
> The patch moves ss_status out of drvdata->config and into drvdata directly.
> It aims to separate configuration from reset and restore states, while
> intentionally preserving existing single-shot clearing behavior.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > --- 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;
> > +			drvdata->ss_status[i] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> 
> [Severity: Medium]
> Is it safe to perform a lockless read-modify-write on drvdata->ss_status
> here? This can race with or interrupt sysfs stores.

As Mike said, the purpose of ss_status's writing is to clearing the bit.
So, It is ignorable.


> 
> >  		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));
> 
> [Severity: Medium]
> Does this change preserve the previous behavior for perf sessions as
> intended?
> 
> When a perf session starts, the configuration is zero-initialized. Before
> this patch, the code skipped the bit clearing (since config->ss_ctrl[i]
> and ss_pe_cmp[i] were 0) but still wrote config->ss_status[i] to
> TRCSSCSRn(i). Because ss_status was zero-initialized, this wrote 0 and
> effectively cleared the bits.
> 
> Now, it writes drvdata->ss_status[i], which might retain previous hardware
> state (potentially 1s). Doesn't this leave the bits uncleared and alter the
> hardware state configuration instead of preserving it?

No, the matter for writing bit allow is for STATUS and PENDING bit.
other bits are for RO and RES fields. so it's not true.
> 
> >  		if (etm4x_sspcicrn_present(drvdata, i))
> >  			etm4x_relaxed_write32(csa, config->ss_pe_cmp[i], TRCSSPCICRn(i));
> >  	}
> [ ... ]
> > @@ -1078,7 +1078,7 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
> >  
> >  	/* read the status of the single shot comparators */
> >  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> > -		config->ss_status[i] =
> > +		drvdata->ss_status[i] =
> >  			etm4x_relaxed_read32(csa, TRCSSCSRn(i));
> 
> [Severity: Medium]
> Can this lockless write in hardirq context race with the RMW sysfs stores,
> leading to torn updates on drvdata->ss_status?

This is the same for last comment. sysfs's clearing while session
disable (before sysfs return) is not expected. User should wait
the sysfs return for disable and this problem is ignored by former too.

> 
> >  	}
> >  
> [ ... ]
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> [ ... ]
> > @@ -1835,8 +1835,8 @@ static ssize_t sshot_ctrl_store(struct device *dev,
> >  	raw_spin_lock(&drvdata->spinlock);
> >  	idx = config->ss_idx;
> >  	config->ss_ctrl[idx] = FIELD_PREP(TRCSSCCRn_SAC_ARC_RST_MASK, val);
> > -	/* must clear bit 31 in related status register on programming */
> > -	config->ss_status[idx] &= ~TRCSSCSRn_STATUS;
> > +	/* must clear bit 31 and 30 in related status register on programming */
> > +	drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);
> 
> [Severity: Medium]
> Can this read-modify-write be interrupted by the hardware PMU path?
> 
> If a user writes to the sshot_ctrl or sshot_pe_ctrl sysfs node concurrently
> with a perf session stopping (e.g. during a task context switch),
> etm4_disable_hw() can trigger in interrupt context.
> 
> Because the sysfs path uses raw_spin_lock() rather than an IRQ-safe variant,
> the PMU interrupt can preempt this sequence. Won't this lead to lost updates
> or corrupted hardware status capability bits when the sysfs thread resumes?
> 
> >  	raw_spin_unlock(&drvdata->spinlock);
> >  	return size;
> >  }

that's why the curr_config and sysfs_config is separated. This is not
true.


-- 
Sincerely,
Yeoreum Yun