Re: [PATCH v10 3/8] coresight: etm3x: change drvdata->spinlock type to raw_spin_lock_t

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Medium] Unwarranted conversion to raw_spinlock_t degrades PREEMPT_RT latency based on a false premise.

Pre-existing issues:
- [High] Ignored return value from pm_runtime_get_sync() in sysfs handlers can lead to synchronous external aborts.
- [High] Concurrent sysfs read can lock CoreSight registers during hardware enablement, causing silent configuration failures.
- [High] Trace ID use-after-free and stream corruption via sysfs reset while tracing.
- [High] TOCTOU race in `cntr_val_show` leads to reading from powered-off hardware.
--

commit 7ed6c9cd3b008064253e2672ab61ffedfc528361
Author: Yeoreum Yun <yeoreum.yun@arm.com>
Date:   Fri Sep 11 10:56:02 2026 +0000

coresight: etm3x: change drvdata->spinlock type to raw_spin_lock_t

This commit converts the etm3x driver's spinlock to a raw_spinlock_t to
avoid issues with sleepable contexts in PREEMPT_RT. It updates the spinlock
initialization and all usages across the core and sysfs code.

> coresight: etm3x: change drvdata->spinlock type to raw_spin_lock_t
> 
> etm_starting_cpu()/etm_dying_cpu() are called in not sleepable context.
> This poses an issue in PREEMPT_RT kernel where spinlock_t is sleepable.

[Severity: Medium]
Does the etm3x driver actually use these hotplug callbacks?

The etm3x driver does not appear to implement etm_starting_cpu() or
etm_dying_cpu(). Its only CPU hotplug callback is etm_online_cpu, which is
registered at CPUHP_AP_ONLINE_DYN and runs in the sleepable cpuhp/%u kthread.

Since drvdata->spinlock is primarily used to protect sysfs configurations
that run in process context, converting to raw_spinlock_t forces sysfs
operations to run with preemption disabled. Can this degrade PREEMPT_RT
latency unnecessarily?

> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index b3c67e96a82a4..0701edc094581 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> @@ -49,13 +49,13 @@ static ssize_t etmsr_show(struct device *dev,
>  	struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>  
>  	pm_runtime_get_sync(dev->parent);
> -	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
>  	CS_UNLOCK(drvdata->csa.base);
>  
>  	val = etm_readl(drvdata, ETMSR);

[Severity: High]
This is a pre-existing issue, but could this code cause a synchronous
external abort if the device fails to resume?

If pm_runtime_get_sync() returns a negative error code (e.g., if the device
is suspended and its clocks or power domains remain gated), the code proceeds
unconditionally to dereference drvdata->csa.base in etm_readl(). 

Accessing unclocked or unpowered CoreSight registers on ARM can result in a
kernel panic. Should the return value of pm_runtime_get_sync() be checked
before accessing the hardware?

[Severity: High]
This is another pre-existing issue, but can this sysfs read silently break
concurrent hardware enablement?

Concurrent sysfs readers like etmsr_show() use drvdata->spinlock to serialize
their own hardware accesses, issuing CS_UNLOCK and CS_LOCK. However,
etm_enable_hw() executes via smp_call_function_single() and does not hold
drvdata->spinlock while issuing its own register writes and CS_LOCK.

If a sysfs read occurs concurrently with etm_enable_hw():

Sysfs thread                           etm_enable_hw()
etmsr_show()
  raw_spin_lock_irqsave()
  CS_UNLOCK()
                                       CS_UNLOCK()
                                       write to registers...
  CS_LOCK()                            
                                       write to more registers... (ignored)
                                       CS_LOCK()

Will the sysfs read's CS_LOCK lock the CoreSight Lock Access Register (LAR)
in the middle of etm_enable_hw()'s configuration sequence, causing
subsequent register writes in etm_enable_hw() to be silently ignored?

[ ... ]

> @@ -76,7 +76,7 @@ static ssize_t reset_store(struct device *dev,
>  		return ret;
>  
>  	if (val) {
> -		spin_lock(&drvdata->spinlock);
> +		raw_spin_lock(&drvdata->spinlock);
>  		memset(config, 0, sizeof(struct etm_config));
>  		config->mode = ETM_MODE_EXCLUDE;
>  		config->trigger_event = ETM_DEFAULT_EVENT_VAL;
> @@ -86,7 +86,7 @@ static ssize_t reset_store(struct device *dev,
>  
>  		etm_set_default(config);
>  		etm_release_trace_id(drvdata);
> -		spin_unlock(&drvdata->spinlock);
> +		raw_spin_unlock(&drvdata->spinlock);

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a trace ID
use-after-free and corrupt the trace stream?

This resets the staging configuration and unconditionally calls
etm_release_trace_id() without checking if the tracer is currently active
(e.g., via coresight_get_mode()). 

If writing to this reset attribute happens while a tracing session is running,
the trace ID is returned to the pool and can be reallocated to another
CoreSight component while this device continues to emit hardware packets using
the freed ID.

[ ... ]

> @@ -722,9 +722,9 @@ static ssize_t cntr_val_show(struct device *dev,
>  	struct etm_config *config = &drvdata->config;
>  
>  	if (!coresight_get_mode(drvdata->csdev)) {
> -		spin_lock(&drvdata->spinlock);
> +		raw_spin_lock(&drvdata->spinlock);
>  		val = config->cntr_val[config->cntr_idx];
> -		spin_unlock(&drvdata->spinlock);
> +		raw_spin_unlock(&drvdata->spinlock);
>  	} else {
>  		val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));

[Severity: High]
This is a pre-existing issue, but can this race with a concurrent power down
and cause a synchronous external abort?

The check for coresight_get_mode() bypasses both the spinlock and
pm_runtime_get_sync(). If the device is active during the check, it jumps
to the else branch to read the hardware register. 

If another thread disables the device (which calls pm_runtime_put() and
powers down the hardware) concurrently, this will read from powered-off
hardware.

Additionally, is it safe to read config->cntr_idx here without a lock?

[ ... ]

> @@ -940,13 +940,13 @@ static ssize_t seq_curr_state_show(struct device *dev,
>  	}
>  
>  	pm_runtime_get_sync(dev->parent);
> -	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
>  
>  	CS_UNLOCK(drvdata->csa.base);
>  	val = (etm_readl(drvdata, ETMSQR) & ETM_SQR_MASK);

[Severity: High]
This is a pre-existing issue, but as with etmsr_show() earlier, could this
cause a synchronous external abort if the device fails to resume?

If pm_runtime_get_sync() returns a negative error, accessing the unclocked
hardware register will cause a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-separate_etm_cfg_v2-v10-0-1b715d95927a@arm.com?part=3