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

[sashiko-bot@kernel.org]

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.

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

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

>  	}
>  
[ ... ]
> 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;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-separate_etm_cfg_v2-v12-0-a2b41d2f7c2b@arm.com?part=8