This series is extracted from the CoreSight power management fixes and refactoring [1], focusing on ETMv3/4 power management.
This series has been verified on Juno-r2 platform.
[1] https://lore.kernel.org/linux-arm-kernel/20250915-arm_coresight_power_manage...
--- Changes in v6: - Added a 'retain_ss_status' flag to indicate if need to restain single-shot status (Suzuki). - Introduced __etm4_disable_hw() without calling etm4_cs_{unlock|lock}() pairs, which is convenient for the save callback (Suzuki). - Link to v5: https://lore.kernel.org/r/20251103-arm_coresight_power_management_fix-v5-0-f...
Changes in v5: - Added more info for removing redundant DSB in etm4_{enable|disable}_trace_unit() (Suzuki/Levi). - Dropped moving isb() out from etm4_{enable|disable}_trace_unit() (Suzuki). - Reordered patches to move fixes earlier. - Link to v4: https://lore.kernel.org/r/20251024-arm_coresight_power_management_fix-v4-0-6...
Changes in v4: - Added patch 10 for retaining sequencer state in ETMv4 driver (Mike). - Added Mike's review tags. - Added James' test tags. - Link to v3: https://lore.kernel.org/r/20250915-arm_coresight_power_management_fix-v3-0-e...
Changes in v3: - Fixed building failure in ETMv3 driver (kernel test robot). - Refactoring ETMv3 change for checking CPU ID (Levi). - Fixed NULL pointer issue during CPU idle (James). - Fixed lockdep complaint for HARDIRQ-safe and HARDIRA-unsafe (James). - Fixed acquiring mutex in atomic context (James). - Rebased on the latest coresight-next branch. - Link to v2: https://lore.kernel.org/r/20250701-arm_cs_pm_fix_v3-v2-0-23ebb864fcc1@arm.co...
Changes in v2: - Refactored ETMv4 suspend and resume for reusing the normal enabling and disabling flows (James). - Used a per-CPU structure to maintain path pointers (James). - Supported helpers in CPU PM flows (James). - Fixed the SMP-safe access to device mode. - Fixed the context synchronization in ETMv4x driver. - Link to v1: https://lore.kernel.org/linux-arm-kernel/20250516160742.1200904-1-leo.yan@ar...
Signed-off-by: Leo Yan leo.yan@arm.com
--- Leo Yan (11): coresight: Change device mode to atomic type coresight: etm4x: Always set tracer's device mode on target CPU coresight: etm3x: Always set tracer's device mode on target CPU coresight: etm4x: Correct polling IDLE bit coresight: etm4x: Add context synchronization before enabling trace coresight: etm4x: Properly control filter in CPU idle with FEAT_TRF coresight: etm4x: Remove the redundant DSB coresight: etm4x: Remove the state_needs_restore flag coresight: etm4x: Add flag to retain single-shot status coresight: etm4x: Retain sequencer state coresight: etm4x: Reuse normal enable and disable logic in CPU idle
drivers/hwtracing/coresight/coresight-etm3x-core.c | 59 ++-- drivers/hwtracing/coresight/coresight-etm4x-core.c | 358 +++++++-------------- drivers/hwtracing/coresight/coresight-etm4x.h | 64 +--- include/linux/coresight.h | 25 +- 4 files changed, 172 insertions(+), 334 deletions(-) --- base-commit: b139702a889692ec30702534ebb1ae2b11ed1cbf change-id: 20250909-arm_coresight_power_management_fix-139873f942e8
Best regards,
The device mode is defined as local type. This type cannot promise SMP-safe access.
Change to atomic type and impose relax ordering, which ensures the SMP-safe synchronisation and the ordering between the mode setting and relevant operations.
Fixes: 22fd532eaa0c ("coresight: etm3x: adding operation mode for etm_enable()") Reviewed-by: Mike Leach mike.leach@linaro.org Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com --- include/linux/coresight.h | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 56d0108658db34b8f2d184cd3214ca172a3ce808..2b48be97fcd0d7ea2692206692bd33f35ba4ec79 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -251,15 +251,11 @@ struct coresight_trace_id_map { * by @coresight_ops. * @access: Device i/o access abstraction for this device. * @dev: The device entity associated to this component. - * @mode: This tracer's mode, i.e sysFS, Perf or disabled. This is - * actually an 'enum cs_mode', but is stored in an atomic type. - * This is always accessed through local_read() and local_set(), - * but wherever it's done from within the Coresight device's lock, - * a non-atomic read would also work. This is the main point of - * synchronisation between code happening inside the sysfs mode's - * coresight_mutex and outside when running in Perf mode. A compare - * and exchange swap is done to atomically claim one mode or the - * other. + * @mode: The device mode, i.e sysFS, Perf or disabled. This is actually + * an 'enum cs_mode' but stored in an atomic type. Access is always + * through atomic APIs, ensuring SMP-safe synchronisation between + * racing from sysFS and Perf mode. A compare-and-exchange + * operation is done to atomically claim one mode or the other. * @refcnt: keep track of what is in use. Only access this outside of the * device's spinlock when the coresight_mutex held and mode == * CS_MODE_SYSFS. Otherwise it must be accessed from inside the @@ -288,7 +284,7 @@ struct coresight_device { const struct coresight_ops *ops; struct csdev_access access; struct device dev; - local_t mode; + atomic_t mode; int refcnt; bool orphan; /* sink specific fields */ @@ -624,13 +620,14 @@ static inline bool coresight_is_percpu_sink(struct coresight_device *csdev) static inline bool coresight_take_mode(struct coresight_device *csdev, enum cs_mode new_mode) { - return local_cmpxchg(&csdev->mode, CS_MODE_DISABLED, new_mode) == - CS_MODE_DISABLED; + int curr = CS_MODE_DISABLED; + + return atomic_try_cmpxchg_acquire(&csdev->mode, &curr, new_mode); }
static inline enum cs_mode coresight_get_mode(struct coresight_device *csdev) { - return local_read(&csdev->mode); + return atomic_read_acquire(&csdev->mode); }
static inline void coresight_set_mode(struct coresight_device *csdev, @@ -646,7 +643,7 @@ static inline void coresight_set_mode(struct coresight_device *csdev, WARN(new_mode != CS_MODE_DISABLED && current_mode != CS_MODE_DISABLED && current_mode != new_mode, "Device already in use\n");
- local_set(&csdev->mode, new_mode); + atomic_set_release(&csdev->mode, new_mode); }
struct coresight_device *coresight_register(struct coresight_desc *desc);
When enabling a tracer via SysFS interface, the device mode may be set by any CPU - not necessarily the target CPU. This can lead to race condition in SMP, and may result in incorrect mode values being read.
Consider the following example, where CPU0 attempts to enable the tracer on CPU1 (the target CPU):
CPU0 CPU1 etm4_enable() ` coresight_take_mode(SYSFS) ` etm4_enable_sysfs() ` smp_call_function_single() ----> etm4_enable_hw_smp_call() / / CPU idle: / etm4_cpu_save() / ` coresight_get_mode() Failed to enable h/w / ^^^ ` coresight_set_mode(DISABLED) <-' Read the intermediate SYSFS mode
In this case, CPU0 initiates the operation by taking the SYSFS mode to avoid conflicts with the Perf mode. It then sends an IPI to CPU1 to configure the tracer registers. If any error occurs during this process, CPU0 rolls back by setting the mode to DISABLED.
However, if CPU1 enters an idle state during this time, it might read the intermediate SYSFS mode. As a result, the CPU PM flow could wrongly save and restore tracer context that is actually disabled.
To resolve the issue, this commit moves the device mode setting logic on the target CPU. This ensures that the device mode is only modified by the target CPU, eliminating race condition between mode writes and reads across CPUs.
An additional change introduces the etm4_disable_sysfs_smp_call() function for SMP calls, which disables the tracer and explicitly set the mode to DISABLED during SysFS operations. Rename etm4_disable_hw_smp_call() to etm4_disable_sysfs_smp_call() for naming consistency.
The flow is updated with this change:
CPU0 CPU1 etm4_enable() ` etm4_enable_sysfs() ` smp_call_function_single() ----> etm4_enable_hw_smp_call() ` coresight_take_mode(SYSFS) Failed, set back to DISABLED ` coresight_set_mode(DISABLED)
CPU idle: etm4_cpu_save() ` coresight_get_mode() ^^^ Read out the DISABLED mode
Fixes: c38a9ec2b2c1 ("coresight: etm4x: moving etm_drvdata::enable to atomic field") Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Reviewed-by: mike Leach mike.leach@linaro.org Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 60 ++++++++++++++-------- 1 file changed, 38 insertions(+), 22 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 020f070bf17dc0557dfb3ae4f282b0a0c1778bd8..1324b40d54210078b70a760ac1b084a4b20e8770 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -589,13 +589,26 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) return rc; }
-static void etm4_enable_hw_smp_call(void *info) +static void etm4_enable_sysfs_smp_call(void *info) { struct etm4_enable_arg *arg = info; + struct coresight_device *csdev;
if (WARN_ON(!arg)) return; + + csdev = arg->drvdata->csdev; + if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) { + /* Someone is already using the tracer */ + arg->rc = -EBUSY; + return; + } + arg->rc = etm4_enable_hw(arg->drvdata); + + /* The tracer didn't start */ + if (arg->rc) + coresight_set_mode(csdev, CS_MODE_DISABLED); }
/* @@ -808,13 +821,14 @@ static int etm4_enable_perf(struct coresight_device *csdev, struct perf_event *event, struct coresight_path *path) { - int ret = 0; struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + int ret;
- if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) { - ret = -EINVAL; - goto out; - } + if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) + return -EINVAL; + + if (!coresight_take_mode(csdev, CS_MODE_PERF)) + return -EBUSY;
/* Configure the tracer based on the session's specifics */ ret = etm4_parse_event_config(csdev, event); @@ -830,6 +844,9 @@ static int etm4_enable_perf(struct coresight_device *csdev, ret = etm4_enable_hw(drvdata);
out: + /* Failed to start tracer; roll back to DISABLED mode */ + if (ret) + coresight_set_mode(csdev, CS_MODE_DISABLED); return ret; }
@@ -861,7 +878,7 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa */ arg.drvdata = drvdata; ret = smp_call_function_single(drvdata->cpu, - etm4_enable_hw_smp_call, &arg, 1); + etm4_enable_sysfs_smp_call, &arg, 1); if (!ret) ret = arg.rc; if (!ret) @@ -882,11 +899,6 @@ static int etm4_enable(struct coresight_device *csdev, struct perf_event *event, { int ret;
- if (!coresight_take_mode(csdev, mode)) { - /* Someone is already using the tracer */ - return -EBUSY; - } - switch (mode) { case CS_MODE_SYSFS: ret = etm4_enable_sysfs(csdev, path); @@ -898,10 +910,6 @@ static int etm4_enable(struct coresight_device *csdev, struct perf_event *event, ret = -EINVAL; }
- /* The tracer didn't start */ - if (ret) - coresight_set_mode(csdev, CS_MODE_DISABLED); - return ret; }
@@ -953,10 +961,9 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata) isb(); }
-static void etm4_disable_hw(void *info) +static void etm4_disable_hw(struct etmv4_drvdata *drvdata) { u32 control; - struct etmv4_drvdata *drvdata = info; struct etmv4_config *config = &drvdata->config; struct coresight_device *csdev = drvdata->csdev; struct csdev_access *csa = &csdev->access; @@ -993,6 +1000,15 @@ static void etm4_disable_hw(void *info) "cpu: %d disable smp call done\n", drvdata->cpu); }
+static void etm4_disable_sysfs_smp_call(void *info) +{ + struct etmv4_drvdata *drvdata = info; + + etm4_disable_hw(drvdata); + + coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED); +} + static int etm4_disable_perf(struct coresight_device *csdev, struct perf_event *event) { @@ -1022,6 +1038,8 @@ static int etm4_disable_perf(struct coresight_device *csdev, /* TRCVICTLR::SSSTATUS, bit[9] */ filters->ssstatus = (control & BIT(9));
+ coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED); + /* * perf will release trace ids when _free_aux() is * called at the end of the session. @@ -1047,7 +1065,8 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) * Executing etm4_disable_hw on the cpu whose ETM is being disabled * ensures that register writes occur when cpu is powered. */ - smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1); + smp_call_function_single(drvdata->cpu, etm4_disable_sysfs_smp_call, + drvdata, 1);
raw_spin_unlock(&drvdata->spinlock);
@@ -1087,9 +1106,6 @@ static void etm4_disable(struct coresight_device *csdev, etm4_disable_perf(csdev, event); break; } - - if (mode) - coresight_set_mode(csdev, CS_MODE_DISABLED); }
static int etm4_resume_perf(struct coresight_device *csdev)
The ETMv3 driver shares the same issue as ETMv4 regarding race conditions when accessing the device mode.
This commit applies the same fix: ensuring that the device mode is modified only by the target CPU to eliminate race conditions across CPUs.
Fixes: 22fd532eaa0c ("coresight: etm3x: adding operation mode for etm_enable()") Reviewed-by: Mike Leach mike.leach@linaro.org Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm3x-core.c | 59 +++++++++++++++------- 1 file changed, 40 insertions(+), 19 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c index 45630a1cd32fbd05ec8b2a6979f0174cacce365e..a5e809589d3e382acde24ee457e94e5dcb18ea35 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c @@ -439,13 +439,26 @@ struct etm_enable_arg { int rc; };
-static void etm_enable_hw_smp_call(void *info) +static void etm_enable_sysfs_smp_call(void *info) { struct etm_enable_arg *arg = info; + struct coresight_device *csdev;
if (WARN_ON(!arg)) return; + + csdev = arg->drvdata->csdev; + if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) { + /* Someone is already using the tracer */ + arg->rc = -EBUSY; + return; + } + arg->rc = etm_enable_hw(arg->drvdata); + + /* The tracer didn't start */ + if (arg->rc) + coresight_set_mode(csdev, CS_MODE_DISABLED); }
static int etm_cpu_id(struct coresight_device *csdev) @@ -465,16 +478,26 @@ static int etm_enable_perf(struct coresight_device *csdev, struct coresight_path *path) { struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); + int ret;
if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) return -EINVAL;
+ if (!coresight_take_mode(csdev, CS_MODE_PERF)) + return -EBUSY; + /* Configure the tracer based on the session's specifics */ etm_parse_event_config(drvdata, event); drvdata->traceid = path->trace_id;
/* And enable it */ - return etm_enable_hw(drvdata); + ret = etm_enable_hw(drvdata); + + /* Failed to start tracer; roll back to DISABLED mode */ + if (ret) + coresight_set_mode(csdev, CS_MODE_DISABLED); + + return ret; }
static int etm_enable_sysfs(struct coresight_device *csdev, struct coresight_path *path) @@ -494,7 +517,7 @@ static int etm_enable_sysfs(struct coresight_device *csdev, struct coresight_pat if (cpu_online(drvdata->cpu)) { arg.drvdata = drvdata; ret = smp_call_function_single(drvdata->cpu, - etm_enable_hw_smp_call, &arg, 1); + etm_enable_sysfs_smp_call, &arg, 1); if (!ret) ret = arg.rc; if (!ret) @@ -517,12 +540,6 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event, enum cs_mode mode, struct coresight_path *path) { int ret; - struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); - - if (!coresight_take_mode(csdev, mode)) { - /* Someone is already using the tracer */ - return -EBUSY; - }
switch (mode) { case CS_MODE_SYSFS: @@ -535,17 +552,12 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event, ret = -EINVAL; }
- /* The tracer didn't start */ - if (ret) - coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED); - return ret; }
-static void etm_disable_hw(void *info) +static void etm_disable_hw(struct etm_drvdata *drvdata) { int i; - struct etm_drvdata *drvdata = info; struct etm_config *config = &drvdata->config; struct coresight_device *csdev = drvdata->csdev;
@@ -567,6 +579,15 @@ static void etm_disable_hw(void *info) "cpu: %d disable smp call done\n", drvdata->cpu); }
+static void etm_disable_sysfs_smp_call(void *info) +{ + struct etm_drvdata *drvdata = info; + + etm_disable_hw(drvdata); + + coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED); +} + static void etm_disable_perf(struct coresight_device *csdev) { struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -588,6 +609,8 @@ static void etm_disable_perf(struct coresight_device *csdev)
CS_LOCK(drvdata->csa.base);
+ coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED); + /* * perf will release trace ids when _free_aux() * is called at the end of the session @@ -612,7 +635,8 @@ static void etm_disable_sysfs(struct coresight_device *csdev) * Executing etm_disable_hw on the cpu whose ETM is being disabled * ensures that register writes occur when cpu is powered. */ - smp_call_function_single(drvdata->cpu, etm_disable_hw, drvdata, 1); + smp_call_function_single(drvdata->cpu, etm_disable_sysfs_smp_call, + drvdata, 1);
spin_unlock(&drvdata->spinlock); cpus_read_unlock(); @@ -652,9 +676,6 @@ static void etm_disable(struct coresight_device *csdev, WARN_ON_ONCE(mode); return; } - - if (mode) - coresight_set_mode(csdev, CS_MODE_DISABLED); }
static const struct coresight_ops_source etm_source_ops = {
Since commit 4ff6039ffb79 ("coresight-etm4x: add isb() before reading the TRCSTATR"), the code has incorrectly been polling the PMSTABLE bit instead of the IDLE bit.
This commit corrects the typo.
Fixes: 4ff6039ffb79 ("coresight-etm4x: add isb() before reading the TRCSTATR") Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Reviewed-by: Mike Leach mike.leach@linaro.org Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 1324b40d54210078b70a760ac1b084a4b20e8770..c562f82985192ad71e181be1b570c9a2f334f29f 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1924,7 +1924,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) state->trcpdcr = etm4x_read32(csa, TRCPDCR);
/* wait for TRCSTATR.IDLE to go up */ - if (etm4x_wait_status(csa, TRCSTATR_PMSTABLE_BIT, 1)) { + if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1)) { dev_err(etm_dev, "timeout while waiting for Idle Trace Status\n"); etm4_os_unlock(drvdata);
According to the software usage PKLXF in Arm ARM (ARM DDI 0487 L.a), a Context synchronization event is required before enabling the trace unit.
An ISB is added to meet this requirement, particularly for guarding the operations in the flow:
etm4x_allow_trace() `> kvm_tracing_set_el1_configuration() `> write_sysreg_s(trfcr_while_in_guest, SYS_TRFCR_EL12)
Improved the barrier comments to provide more accurate information.
Fixes: 1ab3bb9df5e3 ("coresight: etm4x: Add necessary synchronization for sysreg access") Reviewed-by: Mike Leach mike.leach@linaro.org Reviewed-by: Yeoreun Yun yeoreum.yun@arm.com Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 27 ++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index c562f82985192ad71e181be1b570c9a2f334f29f..5e707d082537a405e86fc30e28957ef8beec250f 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -446,10 +446,24 @@ static int etm4_enable_trace_unit(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, TRCRSR_TA, TRCRSR);
etm4x_allow_trace(drvdata); + + /* + * According to software usage PKLXF in Arm ARM (ARM DDI 0487 L.a), + * execute a Context synchronization event to guarantee the trace unit + * will observe the new values of the System registers. + */ + if (!csa->io_mem) + isb(); + /* Enable the trace unit */ etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
- /* Synchronize the register updates for sysreg access */ + /* + * As recommended by section 4.3.7 ("Synchronization when using system + * instructions to progrom the trace unit") of ARM IHI 0064H.b, the + * self-hosted trace analyzer must perform a Context synchronization + * event between writing to the TRCPRGCTLR and reading the TRCSTATR. + */ if (!csa->io_mem) isb();
@@ -931,11 +945,16 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata) */ etm4x_prohibit_trace(drvdata); /* - * Make sure everything completes before disabling, as recommended - * by section 7.3.77 ("TRCVICTLR, ViewInst Main Control Register, - * SSTATUS") of ARM IHI 0064D + * Prevent being speculative at the point of disabling the trace unit, + * as recommended by section 7.3.77 ("TRCVICTLR, ViewInst Main Control + * Register, SSTATUS") of ARM IHI 0064D */ dsb(sy); + /* + * According to software usage VKHHY in Arm ARM (ARM DDI 0487 L.a), + * execute a Context synchronization event to guarantee no new + * program-flow trace is generated. + */ isb(); /* Trace synchronization barrier, is a nop if not supported */ tsb_csync();
If a CPU supports FEAT_TRF, as described in the section K5.5 "Context switching", Arm ARM (ARM DDI 0487 L.a), it defines a flow to prohibit program-flow trace, execute a TSB CSYNC instruction for flushing, followed by clearing TRCPRGCTLR.EN bit.
To restore the state, the reverse sequence is required.
This differs from the procedure described in the section 3.4.1 "The procedure when powering down the PE" of ARM IHI0064H.b, which involves the OS Lock to prevent external debugger accesses and implicitly disables trace.
To be compatible with different ETM versions, explicitly control trace unit using etm4_disable_trace_unit() and etm4_enable_trace_unit() during CPU idle to comply with FEAT_TRF.
As a result, the save states for TRFCR_ELx and trcprgctlr are redundant, remove them.
Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Reviewed-by: Mike Leach mike.leach@linaro.org Tested-by: James Clark james.clark@linaro.org Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 14 +++++++------- drivers/hwtracing/coresight/coresight-etm4x.h | 3 --- 2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 5e707d082537a405e86fc30e28957ef8beec250f..fdda924a2c7117a6828e5325d78586b09e75febd 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1858,9 +1858,11 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) goto out; }
+ if (!drvdata->paused) + etm4_disable_trace_unit(drvdata); + state = drvdata->save_state;
- state->trcprgctlr = etm4x_read32(csa, TRCPRGCTLR); if (drvdata->nr_pe) state->trcprocselr = etm4x_read32(csa, TRCPROCSELR); state->trcconfigr = etm4x_read32(csa, TRCCONFIGR); @@ -1970,9 +1972,6 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata) { int ret = 0;
- /* Save the TRFCR irrespective of whether the ETM is ON */ - if (drvdata->trfcr) - drvdata->save_trfcr = read_trfcr(); /* * Save and restore the ETM Trace registers only if * the ETM is active. @@ -1994,7 +1993,6 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) etm4_cs_unlock(drvdata, csa); etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
- etm4x_relaxed_write32(csa, state->trcprgctlr, TRCPRGCTLR); if (drvdata->nr_pe) etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR); etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR); @@ -2079,13 +2077,15 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
/* Unlock the OS lock to re-enable trace and external debug access */ etm4_os_unlock(drvdata); + + if (!drvdata->paused) + etm4_enable_trace_unit(drvdata); + etm4_cs_lock(drvdata, csa); }
static void etm4_cpu_restore(struct etmv4_drvdata *drvdata) { - if (drvdata->trfcr) - write_trfcr(drvdata->save_trfcr); if (drvdata->state_needs_restore) __etm4_cpu_restore(drvdata); } diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 13ec9ecef46f5b60a38c464e8ebde3ef72e7d50b..b8796b4271025f6aba79fe3cd6e4d17d0b56952d 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -866,7 +866,6 @@ struct etmv4_config { * struct etm4_save_state - state to be preserved when ETM is without power */ struct etmv4_save_state { - u32 trcprgctlr; u32 trcprocselr; u32 trcconfigr; u32 trcauxctlr; @@ -980,7 +979,6 @@ struct etmv4_save_state { * at runtime, due to the additional setting of TRFCR_CX when * in EL2. Otherwise, 0. * @config: structure holding configuration parameters. - * @save_trfcr: Saved TRFCR_EL1 register during a CPU PM event. * @save_state: State to be preserved across power loss * @state_needs_restore: True when there is context to restore after PM exit * @skip_power_up: Indicates if an implementation can skip powering up @@ -1037,7 +1035,6 @@ struct etmv4_drvdata { bool lpoverride; u64 trfcr; struct etmv4_config config; - u64 save_trfcr; struct etmv4_save_state *save_state; bool state_needs_restore; bool skip_power_up;
As recommended in section 4.3.7 "Synchronization when using the memory-mapped interface" of ARM IHI0064H.b:
When using the memory-mapped interface to program the trace unit, the trace analyzer must ensure that writes have completed, to ensure that the trace unit is fully programmed and either enabled or disabled.
To ensure writes have completed, the trace analyzer can do ...
If the memory marked is as Device-nGnRE or stronger, read back the value of any register in the trace unit. This relies on peripheral coherence order defined in the Arm architecture.
Polling TRCSTATR ensures the previous write has completed. Therefore, removes the redundant DSB barrier in the enabling flow.
Update the comment in the disable flow for consistency.
Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 23 +++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index fdda924a2c7117a6828e5325d78586b09e75febd..bdf5ab85b2213dfd601391fdcf50960667fa7cea 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -475,10 +475,16 @@ static int etm4_enable_trace_unit(struct etmv4_drvdata *drvdata) }
/* - * As recommended by section 4.3.7 ("Synchronization when using the - * memory-mapped interface") of ARM IHI 0064D + * As recommended in section 4.3.7 (Synchronization of register updates) + * of ARM IHI 0064H.b, the self-hosted trace analyzer always executes an + * ISB instruction after programming the trace unit registers. + * + * For the memory-mapped interface, the registers are mapped as Device + * type (Device-nGnRE). Reading back the value of any register in the + * trace unit ensures that all writes have completed. Therefore, polling + * on TRCSTATR guarantees that the writing TRCPRGCTLR is complete, and + * no explicit dsb() is required at here. */ - dsb(sy); isb();
return 0; @@ -974,8 +980,15 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata) dev_err(etm_dev, "timeout while waiting for PM stable Trace Status\n"); /* - * As recommended by section 4.3.7 (Synchronization of register updates) - * of ARM IHI 0064H.b. + * As recommended in section 4.3.7 (Synchronization of register updates) + * of ARM IHI 0064H.b, the self-hosted trace analyzer always executes an + * ISB instruction after programming the trace unit registers. + * + * For the memory-mapped interface, the registers are mapped as Device + * type (Device-nGnRE). Reading back the value of any register in the + * trace unit ensures that all writes have completed. Therefore, polling + * on TRCSTATR guarantees that the writing TRCPRGCTLR is complete, and + * no explicit dsb() is required at here. */ isb(); }
On Tue, 11 Nov 2025 at 18:59, Leo Yan leo.yan@arm.com wrote:
As recommended in section 4.3.7 "Synchronization when using the memory-mapped interface" of ARM IHI0064H.b:
When using the memory-mapped interface to program the trace unit, the trace analyzer must ensure that writes have completed, to ensure that the trace unit is fully programmed and either enabled or disabled.
To ensure writes have completed, the trace analyzer can do ...
If the memory marked is as Device-nGnRE or stronger, read back the value of any register in the trace unit. This relies on peripheral coherence order defined in the Arm architecture.
Polling TRCSTATR ensures the previous write has completed. Therefore, removes the redundant DSB barrier in the enabling flow.
Update the comment in the disable flow for consistency.
Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 23 +++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index fdda924a2c7117a6828e5325d78586b09e75febd..bdf5ab85b2213dfd601391fdcf50960667fa7cea 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -475,10 +475,16 @@ static int etm4_enable_trace_unit(struct etmv4_drvdata *drvdata) }
/*
* As recommended by section 4.3.7 ("Synchronization when using the* memory-mapped interface") of ARM IHI 0064D
* As recommended in section 4.3.7 (Synchronization of register updates)* of ARM IHI 0064H.b, the self-hosted trace analyzer always executes an* ISB instruction after programming the trace unit registers.** For the memory-mapped interface, the registers are mapped as Device* type (Device-nGnRE). Reading back the value of any register in the* trace unit ensures that all writes have completed. Therefore, polling* on TRCSTATR guarantees that the writing TRCPRGCTLR is complete, and* no explicit dsb() is required at here. */
dsb(sy); isb(); return 0;@@ -974,8 +980,15 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata) dev_err(etm_dev, "timeout while waiting for PM stable Trace Status\n"); /*
* As recommended by section 4.3.7 (Synchronization of register updates)* of ARM IHI 0064H.b.
* As recommended in section 4.3.7 (Synchronization of register updates)* of ARM IHI 0064H.b, the self-hosted trace analyzer always executes an* ISB instruction after programming the trace unit registers.** For the memory-mapped interface, the registers are mapped as Device* type (Device-nGnRE). Reading back the value of any register in the* trace unit ensures that all writes have completed. Therefore, polling* on TRCSTATR guarantees that the writing TRCPRGCTLR is complete, and* no explicit dsb() is required at here. */ isb();}
-- 2.34.1
Reviewed-by: Mike Leach mike.leach@linaro.org
When the restore flow is invoked, it means no error occurred during the save phase. Otherwise, if any errors happened while saving the context, the function would return an error and abort the suspend sequence.
Therefore, the state_needs_restore flag is unnecessary. The save and restore functions are changed to check two conditions:
1) The global flag pm_save_enable is SELF_HOSTED mode; 2) The device is in active mode (non DISABLED).
Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Reviewed-by: Mike Leach mike.leach@linaro.org Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 14 ++++++++------ drivers/hwtracing/coresight/coresight-etm4x.h | 2 -- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index bdf5ab85b2213dfd601391fdcf50960667fa7cea..560975b70474c3449aa6799fc1a8e05d5ea93495 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1966,8 +1966,6 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) goto out; }
- drvdata->state_needs_restore = true; - /* * Power can be removed from the trace unit now. We do this to * potentially save power on systems that respect the TRCPDCR_PU @@ -1985,11 +1983,14 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata) { int ret = 0;
+ if (pm_save_enable != PARAM_PM_SAVE_SELF_HOSTED) + return 0; + /* * Save and restore the ETM Trace registers only if * the ETM is active. */ - if (coresight_get_mode(drvdata->csdev) && drvdata->save_state) + if (coresight_get_mode(drvdata->csdev)) ret = __etm4_cpu_save(drvdata); return ret; } @@ -2079,8 +2080,6 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) if (!drvdata->skip_power_up) etm4x_relaxed_write32(csa, state->trcpdcr, TRCPDCR);
- drvdata->state_needs_restore = false; - /* * As recommended by section 4.3.7 ("Synchronization when using the * memory-mapped interface") of ARM IHI 0064D @@ -2099,7 +2098,10 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
static void etm4_cpu_restore(struct etmv4_drvdata *drvdata) { - if (drvdata->state_needs_restore) + if (pm_save_enable != PARAM_PM_SAVE_SELF_HOSTED) + return; + + if (coresight_get_mode(drvdata->csdev)) __etm4_cpu_restore(drvdata); }
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index b8796b4271025f6aba79fe3cd6e4d17d0b56952d..012c52fd19338133129752d35523dc102df24604 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -980,7 +980,6 @@ struct etmv4_save_state { * in EL2. Otherwise, 0. * @config: structure holding configuration parameters. * @save_state: State to be preserved across power loss - * @state_needs_restore: True when there is context to restore after PM exit * @skip_power_up: Indicates if an implementation can skip powering up * the trace unit. * @paused: Indicates if the trace unit is paused. @@ -1036,7 +1035,6 @@ struct etmv4_drvdata { u64 trfcr; struct etmv4_config config; struct etmv4_save_state *save_state; - bool state_needs_restore; bool skip_power_up; bool paused; DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
Add a 'retain_ss_status' flag to the config structure.
By default, the flag is set to zero so that the TRCSSCSRn.STATUS bit is cleared to restart the single-shot mode.
If the flag is set, the single-shot status will be retained, thus avoiding restart the single-shot. The retain flag is automatically cleared to restore the default behavior on the next call.
Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 15 ++++++++++++--- drivers/hwtracing/coresight/coresight-etm4x.h | 2 ++ 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 560975b70474c3449aa6799fc1a8e05d5ea93495..8b85a73e29f407a8d5e26a25f4dceaadaeed05ed 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -565,9 +565,18 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
for (i = 0; i < drvdata->nr_ss_cmp; i++) { - /* always clear status bit on restart if using single-shot */ - if (config->ss_ctrl[i] || config->ss_pe_cmp[i]) - config->ss_status[i] &= ~TRCSSCSRn_STATUS; + if (!config->retain_ss_status) { + /* always clear status bit on restart if using single-shot */ + if (config->ss_ctrl[i] || config->ss_pe_cmp[i]) + config->ss_status[i] &= ~TRCSSCSRn_STATUS; + } else { + /* + * Leave the single-shot status unchanged; clear the + * retain flag to restore the default behavior + * (restart single-shot) for the next call. + */ + config->retain_ss_status = 0; + } etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i)); etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i)); if (etm4x_sspcicrn_present(drvdata, i)) diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 012c52fd19338133129752d35523dc102df24604..d82ec1559b34787dffc7debf4643d948e3850b2a 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -802,6 +802,7 @@ enum etm_impdef_type { * @ss_ctrl: Controls the corresponding single-shot comparator resource. * @ss_status: The status of the corresponding single-shot comparator. * @ss_pe_cmp: Selects the PE comparator inputs for Single-shot control. + * @retain_ss_status: Retains single-shot status. * @addr_idx: Address comparator index selector. * @addr_val: Value for address comparator. * @addr_acc: Address comparator access type. @@ -846,6 +847,7 @@ struct etmv4_config { u32 ss_ctrl[ETM_MAX_SS_CMP]; u32 ss_status[ETM_MAX_SS_CMP]; u32 ss_pe_cmp[ETM_MAX_SS_CMP]; + u8 retain_ss_status; u8 addr_idx; u64 addr_val[ETM_MAX_SINGLE_ADDR_CMP]; u64 addr_acc[ETM_MAX_SINGLE_ADDR_CMP];
On Tue, 11 Nov 2025 at 18:59, Leo Yan leo.yan@arm.com wrote:
Add a 'retain_ss_status' flag to the config structure.
By default, the flag is set to zero so that the TRCSSCSRn.STATUS bit is cleared to restart the single-shot mode.
If the flag is set, the single-shot status will be retained, thus avoiding restart the single-shot. The retain flag is automatically cleared to restore the default behavior on the next call.
Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 15 ++++++++++++--- drivers/hwtracing/coresight/coresight-etm4x.h | 2 ++ 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 560975b70474c3449aa6799fc1a8e05d5ea93495..8b85a73e29f407a8d5e26a25f4dceaadaeed05ed 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -565,9 +565,18 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
for (i = 0; i < drvdata->nr_ss_cmp; i++) {
/* always clear status bit on restart if using single-shot */if (config->ss_ctrl[i] || config->ss_pe_cmp[i])config->ss_status[i] &= ~TRCSSCSRn_STATUS;
if (!config->retain_ss_status) {/* always clear status bit on restart if using single-shot */if (config->ss_ctrl[i] || config->ss_pe_cmp[i])config->ss_status[i] &= ~TRCSSCSRn_STATUS;} else {/** Leave the single-shot status unchanged; clear the* retain flag to restore the default behavior* (restart single-shot) for the next call.*/config->retain_ss_status = 0;} etm4x_relaxed_write32(csa, config->ss_ctrl[i], TRCSSCCRn(i)); etm4x_relaxed_write32(csa, config->ss_status[i], TRCSSCSRn(i)); if (etm4x_sspcicrn_present(drvdata, i))diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 012c52fd19338133129752d35523dc102df24604..d82ec1559b34787dffc7debf4643d948e3850b2a 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -802,6 +802,7 @@ enum etm_impdef_type {
- @ss_ctrl: Controls the corresponding single-shot comparator resource.
- @ss_status: The status of the corresponding single-shot comparator.
- @ss_pe_cmp: Selects the PE comparator inputs for Single-shot control.
- @retain_ss_status: Retains single-shot status.
- @addr_idx: Address comparator index selector.
- @addr_val: Value for address comparator.
- @addr_acc: Address comparator access type.
@@ -846,6 +847,7 @@ struct etmv4_config { u32 ss_ctrl[ETM_MAX_SS_CMP]; u32 ss_status[ETM_MAX_SS_CMP]; u32 ss_pe_cmp[ETM_MAX_SS_CMP];
u8 retain_ss_status; u8 addr_idx; u64 addr_val[ETM_MAX_SINGLE_ADDR_CMP]; u64 addr_acc[ETM_MAX_SINGLE_ADDR_CMP];-- 2.34.1
Reviewed-by: Mike Leach mike.leach@linaro.org
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Retain sequencer state in the disable flow and will be restored in the enable flow.
As a result, the flow will be reused by CPU suspend and resume in sequential change.
Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 8b85a73e29f407a8d5e26a25f4dceaadaeed05ed..afee54ac0aec0c0e521fa1885a95f48555a369de 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1034,6 +1034,10 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) etm4x_relaxed_read32(csa, TRCCNTVRn(i)); }
+ /* read back the sequencer state */ + if (drvdata->nrseqstate) + config->seq_state = etm4x_relaxed_read32(csa, TRCSEQSTR); + coresight_disclaim_device_unlocked(csdev); etm4_cs_lock(drvdata, csa);
On Tue, 11 Nov 2025 at 18:59, Leo Yan leo.yan@arm.com wrote:
Retain sequencer state in the disable flow and will be restored in the enable flow.
As a result, the flow will be reused by CPU suspend and resume in sequential change.
Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 8b85a73e29f407a8d5e26a25f4dceaadaeed05ed..afee54ac0aec0c0e521fa1885a95f48555a369de 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1034,6 +1034,10 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) etm4x_relaxed_read32(csa, TRCCNTVRn(i)); }
/* read back the sequencer state */if (drvdata->nrseqstate)config->seq_state = etm4x_relaxed_read32(csa, TRCSEQSTR);coresight_disclaim_device_unlocked(csdev); etm4_cs_lock(drvdata, csa);-- 2.34.1
Reviewed-by: Mike Leach mike.leach@linaro.org
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
The etm4_enable_trace_unit() function is almost identical to the restore flow, with a few differences listed below:
1) TRCQCTLR register
The TRCQCTLR register is saved and restored during CPU idle, but it is never touched in the normal flow. Given the Q element is not enabled (TRCCONFIGR.QE bits), it is acceptable to omit saving and restoring this register during idle.
2) TRCSSCSRn.STATUS bit
The restore flow does not explicitly clear the TRCSSCSRn.STATUS bit but instead directly loads the saved value. In contrast, the normal flow clears this bit to 0 when re-enabling single-shot control.
Therefore, the restore function passes the restart_ss argument as false to etm4_enable_hw() to avoid re-enabling single-shot mode.
3) Claim Tag Handling
The claim tag is acquired and released in normal flow, on the other hand, the claim protocol is not applied in CPU idle flow - it simply restores the saved value.
The claim bits serve two purpose:
* Exclusive access between the kernel driver and an external debugger. During CPU idle, the kernel driver locks the OS Lock, ensuring that the external debugger cannot access the trace unit. Therefore, it is safe to release the claim tag during idle.
* Notification to supervisory software to save/restore context for external debuggers. The kernel driver does not touch the external debugger's claim bit (ETMCLAIM[0]).
Based on this, it is safe to acquire and release claim tag in the idle sequence.
4) OS Lock Behavior
The OS Lock should be locked during CPU idle states. This differs from the normal flow, which unlock it. This special handling remains in the CPU save path.
This commit reuses the normal enable and disable logic in the CPU idle path. The common disable logic is moved into a new function __etm4_disable_hw(), which omits the etm4_cs_{unlock|lock}() pairs and is convenient to call from the save callback.
The resume callback sets the 'retain_ss_status' flag to avoid restarting the single-shot mode, and then directly calls etm4_enable_hw().
The save context in the driver data is no longer needed and has been removed.
Tested-by: James Clark james.clark@linaro.org Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm4x-core.c | 215 ++------------------- drivers/hwtracing/coresight/coresight-etm4x.h | 57 ------ 2 files changed, 16 insertions(+), 256 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index afee54ac0aec0c0e521fa1885a95f48555a369de..1c17d54729200671ec6339f43755ea4267dd61dc 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1002,7 +1002,7 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata) isb(); }
-static void etm4_disable_hw(struct etmv4_drvdata *drvdata) +static void __etm4_disable_hw(struct etmv4_drvdata *drvdata) { u32 control; struct etmv4_config *config = &drvdata->config; @@ -1010,7 +1010,6 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) struct csdev_access *csa = &csdev->access; int i;
- etm4_cs_unlock(drvdata, csa); etm4_disable_arch_specific(drvdata);
if (!drvdata->skip_power_up) { @@ -1039,12 +1038,21 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) config->seq_state = etm4x_relaxed_read32(csa, TRCSEQSTR);
coresight_disclaim_device_unlocked(csdev); - etm4_cs_lock(drvdata, csa);
dev_dbg(&drvdata->csdev->dev, "cpu: %d disable smp call done\n", drvdata->cpu); }
+static void etm4_disable_hw(struct etmv4_drvdata *drvdata) +{ + struct coresight_device *csdev = drvdata->csdev; + struct csdev_access *csa = &csdev->access; + + etm4_cs_unlock(drvdata, csa); + __etm4_disable_hw(drvdata); + etm4_cs_lock(drvdata, csa); +} + static void etm4_disable_sysfs_smp_call(void *info) { struct etmv4_drvdata *drvdata = info; @@ -1852,8 +1860,7 @@ static int etm4_dying_cpu(unsigned int cpu)
static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) { - int i, ret = 0; - struct etmv4_save_state *state; + int ret = 0; struct coresight_device *csdev = drvdata->csdev; struct csdev_access *csa; struct device *etm_dev; @@ -1884,91 +1891,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) goto out; }
- if (!drvdata->paused) - etm4_disable_trace_unit(drvdata); - - state = drvdata->save_state; - - if (drvdata->nr_pe) - state->trcprocselr = etm4x_read32(csa, TRCPROCSELR); - state->trcconfigr = etm4x_read32(csa, TRCCONFIGR); - state->trcauxctlr = etm4x_read32(csa, TRCAUXCTLR); - state->trceventctl0r = etm4x_read32(csa, TRCEVENTCTL0R); - state->trceventctl1r = etm4x_read32(csa, TRCEVENTCTL1R); - if (drvdata->stallctl) - state->trcstallctlr = etm4x_read32(csa, TRCSTALLCTLR); - state->trctsctlr = etm4x_read32(csa, TRCTSCTLR); - state->trcsyncpr = etm4x_read32(csa, TRCSYNCPR); - state->trcccctlr = etm4x_read32(csa, TRCCCCTLR); - state->trcbbctlr = etm4x_read32(csa, TRCBBCTLR); - state->trctraceidr = etm4x_read32(csa, TRCTRACEIDR); - if (drvdata->q_filt) - state->trcqctlr = etm4x_read32(csa, TRCQCTLR); - - state->trcvictlr = etm4x_read32(csa, TRCVICTLR); - state->trcviiectlr = etm4x_read32(csa, TRCVIIECTLR); - state->trcvissctlr = etm4x_read32(csa, TRCVISSCTLR); - if (drvdata->nr_pe_cmp) - state->trcvipcssctlr = etm4x_read32(csa, TRCVIPCSSCTLR); - - for (i = 0; i < drvdata->nrseqstate - 1; i++) - state->trcseqevr[i] = etm4x_read32(csa, TRCSEQEVRn(i)); - - if (drvdata->nrseqstate) { - state->trcseqrstevr = etm4x_read32(csa, TRCSEQRSTEVR); - state->trcseqstr = etm4x_read32(csa, TRCSEQSTR); - } - - if (drvdata->numextinsel) - state->trcextinselr = etm4x_read32(csa, TRCEXTINSELR); - - for (i = 0; i < drvdata->nr_cntr; i++) { - state->trccntrldvr[i] = etm4x_read32(csa, TRCCNTRLDVRn(i)); - state->trccntctlr[i] = etm4x_read32(csa, TRCCNTCTLRn(i)); - state->trccntvr[i] = etm4x_read32(csa, TRCCNTVRn(i)); - } - - /* Resource selector pair 0 is reserved */ - for (i = 2; i < drvdata->nr_resource * 2; i++) - state->trcrsctlr[i] = etm4x_read32(csa, TRCRSCTLRn(i)); - - for (i = 0; i < drvdata->nr_ss_cmp; i++) { - state->trcssccr[i] = etm4x_read32(csa, TRCSSCCRn(i)); - state->trcsscsr[i] = etm4x_read32(csa, TRCSSCSRn(i)); - if (etm4x_sspcicrn_present(drvdata, i)) - state->trcsspcicr[i] = etm4x_read32(csa, TRCSSPCICRn(i)); - } - - for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) { - state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i)); - state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i)); - } - - /* - * Data trace stream is architecturally prohibited for A profile cores - * so we don't save (or later restore) trcdvcvr and trcdvcmr - As per - * section 1.3.4 ("Possible functional configurations of an ETMv4 trace - * unit") of ARM IHI 0064D. - */ - - for (i = 0; i < drvdata->numcidc; i++) - state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i)); - - for (i = 0; i < drvdata->numvmidc; i++) - state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i)); - - state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0); - if (drvdata->numcidc > 4) - state->trccidcctlr1 = etm4x_read32(csa, TRCCIDCCTLR1); - - state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR0); - if (drvdata->numvmidc > 4) - state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR1); - - state->trcclaimset = etm4x_read32(csa, TRCCLAIMCLR); - - if (!drvdata->skip_power_up) - state->trcpdcr = etm4x_read32(csa, TRCPDCR); + __etm4_disable_hw(drvdata);
/* wait for TRCSTATR.IDLE to go up */ if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1)) { @@ -1976,17 +1899,8 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) "timeout while waiting for Idle Trace Status\n"); etm4_os_unlock(drvdata); ret = -EBUSY; - goto out; }
- /* - * Power can be removed from the trace unit now. We do this to - * potentially save power on systems that respect the TRCPDCR_PU - * despite requesting software to save/restore state. - */ - if (!drvdata->skip_power_up) - etm4x_relaxed_write32(csa, (state->trcpdcr & ~TRCPDCR_PU), - TRCPDCR); out: etm4_cs_lock(drvdata, csa); return ret; @@ -2010,103 +1924,13 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) { - int i; - struct etmv4_save_state *state = drvdata->save_state; - struct csdev_access *csa = &drvdata->csdev->access; - if (WARN_ON(!drvdata->csdev)) return;
- etm4_cs_unlock(drvdata, csa); - etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET); - - if (drvdata->nr_pe) - etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR); - etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR); - etm4x_relaxed_write32(csa, state->trcauxctlr, TRCAUXCTLR); - etm4x_relaxed_write32(csa, state->trceventctl0r, TRCEVENTCTL0R); - etm4x_relaxed_write32(csa, state->trceventctl1r, TRCEVENTCTL1R); - if (drvdata->stallctl) - etm4x_relaxed_write32(csa, state->trcstallctlr, TRCSTALLCTLR); - etm4x_relaxed_write32(csa, state->trctsctlr, TRCTSCTLR); - etm4x_relaxed_write32(csa, state->trcsyncpr, TRCSYNCPR); - etm4x_relaxed_write32(csa, state->trcccctlr, TRCCCCTLR); - etm4x_relaxed_write32(csa, state->trcbbctlr, TRCBBCTLR); - etm4x_relaxed_write32(csa, state->trctraceidr, TRCTRACEIDR); - if (drvdata->q_filt) - etm4x_relaxed_write32(csa, state->trcqctlr, TRCQCTLR); - - etm4x_relaxed_write32(csa, state->trcvictlr, TRCVICTLR); - etm4x_relaxed_write32(csa, state->trcviiectlr, TRCVIIECTLR); - etm4x_relaxed_write32(csa, state->trcvissctlr, TRCVISSCTLR); - if (drvdata->nr_pe_cmp) - etm4x_relaxed_write32(csa, state->trcvipcssctlr, TRCVIPCSSCTLR); - - for (i = 0; i < drvdata->nrseqstate - 1; i++) - etm4x_relaxed_write32(csa, state->trcseqevr[i], TRCSEQEVRn(i)); - - if (drvdata->nrseqstate) { - etm4x_relaxed_write32(csa, state->trcseqrstevr, TRCSEQRSTEVR); - etm4x_relaxed_write32(csa, state->trcseqstr, TRCSEQSTR); - } - if (drvdata->numextinsel) - etm4x_relaxed_write32(csa, state->trcextinselr, TRCEXTINSELR); + /* Instruct etm4_enable_hw() not to restart the single-shot mode */ + drvdata->config.retain_ss_status = 1;
- for (i = 0; i < drvdata->nr_cntr; i++) { - etm4x_relaxed_write32(csa, state->trccntrldvr[i], TRCCNTRLDVRn(i)); - etm4x_relaxed_write32(csa, state->trccntctlr[i], TRCCNTCTLRn(i)); - etm4x_relaxed_write32(csa, state->trccntvr[i], TRCCNTVRn(i)); - } - - /* Resource selector pair 0 is reserved */ - for (i = 2; i < drvdata->nr_resource * 2; i++) - etm4x_relaxed_write32(csa, state->trcrsctlr[i], TRCRSCTLRn(i)); - - for (i = 0; i < drvdata->nr_ss_cmp; i++) { - etm4x_relaxed_write32(csa, state->trcssccr[i], TRCSSCCRn(i)); - etm4x_relaxed_write32(csa, state->trcsscsr[i], TRCSSCSRn(i)); - if (etm4x_sspcicrn_present(drvdata, i)) - etm4x_relaxed_write32(csa, state->trcsspcicr[i], TRCSSPCICRn(i)); - } - - for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) { - etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i)); - etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i)); - } - - for (i = 0; i < drvdata->numcidc; i++) - etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i)); - - for (i = 0; i < drvdata->numvmidc; i++) - etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i)); - - etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0); - if (drvdata->numcidc > 4) - etm4x_relaxed_write32(csa, state->trccidcctlr1, TRCCIDCCTLR1); - - etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR0); - if (drvdata->numvmidc > 4) - etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR1); - - etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET); - - if (!drvdata->skip_power_up) - etm4x_relaxed_write32(csa, state->trcpdcr, TRCPDCR); - - /* - * As recommended by section 4.3.7 ("Synchronization when using the - * memory-mapped interface") of ARM IHI 0064D - */ - dsb(sy); - isb(); - - /* Unlock the OS lock to re-enable trace and external debug access */ - etm4_os_unlock(drvdata); - - if (!drvdata->paused) - etm4_enable_trace_unit(drvdata); - - etm4_cs_lock(drvdata, csa); + etm4_enable_hw(drvdata); }
static void etm4_cpu_restore(struct etmv4_drvdata *drvdata) @@ -2293,13 +2117,6 @@ static int etm4_probe(struct device *dev) pm_save_enable = coresight_loses_context_with_cpu(dev) ? PARAM_PM_SAVE_SELF_HOSTED : PARAM_PM_SAVE_NEVER;
- if (pm_save_enable != PARAM_PM_SAVE_NEVER) { - drvdata->save_state = devm_kmalloc(dev, - sizeof(struct etmv4_save_state), GFP_KERNEL); - if (!drvdata->save_state) - return -ENOMEM; - } - raw_spin_lock_init(&drvdata->spinlock);
drvdata->cpu = coresight_get_cpu(dev); diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index d82ec1559b34787dffc7debf4643d948e3850b2a..8d4a1f0f1e52a38a280b7fbe978129287a8ba60c 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -864,61 +864,6 @@ struct etmv4_config { u8 s_ex_level; };
-/** - * struct etm4_save_state - state to be preserved when ETM is without power - */ -struct etmv4_save_state { - u32 trcprocselr; - u32 trcconfigr; - u32 trcauxctlr; - u32 trceventctl0r; - u32 trceventctl1r; - u32 trcstallctlr; - u32 trctsctlr; - u32 trcsyncpr; - u32 trcccctlr; - u32 trcbbctlr; - u32 trctraceidr; - u32 trcqctlr; - - u32 trcvictlr; - u32 trcviiectlr; - u32 trcvissctlr; - u32 trcvipcssctlr; - - u32 trcseqevr[ETM_MAX_SEQ_STATES]; - u32 trcseqrstevr; - u32 trcseqstr; - u32 trcextinselr; - u32 trccntrldvr[ETMv4_MAX_CNTR]; - u32 trccntctlr[ETMv4_MAX_CNTR]; - u32 trccntvr[ETMv4_MAX_CNTR]; - - u32 trcrsctlr[ETM_MAX_RES_SEL]; - - u32 trcssccr[ETM_MAX_SS_CMP]; - u32 trcsscsr[ETM_MAX_SS_CMP]; - u32 trcsspcicr[ETM_MAX_SS_CMP]; - - u64 trcacvr[ETM_MAX_SINGLE_ADDR_CMP]; - u64 trcacatr[ETM_MAX_SINGLE_ADDR_CMP]; - u64 trccidcvr[ETMv4_MAX_CTXID_CMP]; - u64 trcvmidcvr[ETM_MAX_VMID_CMP]; - u32 trccidcctlr0; - u32 trccidcctlr1; - u32 trcvmidcctlr0; - u32 trcvmidcctlr1; - - u32 trcclaimset; - - u32 cntr_val[ETMv4_MAX_CNTR]; - u32 seq_state; - u32 vinst_ctrl; - u32 ss_status[ETM_MAX_SS_CMP]; - - u32 trcpdcr; -}; - /** * struct etm4_drvdata - specifics associated to an ETM component * @pclk: APB clock if present, otherwise NULL @@ -981,7 +926,6 @@ struct etmv4_save_state { * at runtime, due to the additional setting of TRFCR_CX when * in EL2. Otherwise, 0. * @config: structure holding configuration parameters. - * @save_state: State to be preserved across power loss * @skip_power_up: Indicates if an implementation can skip powering up * the trace unit. * @paused: Indicates if the trace unit is paused. @@ -1036,7 +980,6 @@ struct etmv4_drvdata { bool lpoverride; u64 trfcr; struct etmv4_config config; - struct etmv4_save_state *save_state; bool skip_power_up; bool paused; DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
Hi Leo,
The save/restore sequence is only used if the ETM is active at the time of power down.
However, by removing the specific save/restore structure, and using the existing drvdata->config structure, the actual state can be changed by sysfs from another PE, as the sysfs accesses only write to the drvdata->config structure and not the actual hardware. Thus this change permits the corruption of the saved state, which will result in restore != saved..
While using the same code is a good idea avoiding lots of duplication, it may be necessary to have a copy of the drvdata->config passed in to the relevant functions.
We have considered in the past if it might be worthwhile isolating the sysfs and perf config settings in the etm - it may be worth re-visiting this again with a save restore copy in mind to.
On Tue, 11 Nov 2025 at 18:59, Leo Yan leo.yan@arm.com wrote:
The etm4_enable_trace_unit() function is almost identical to the restore flow, with a few differences listed below:
TRCQCTLR register
The TRCQCTLR register is saved and restored during CPU idle, but it is never touched in the normal flow. Given the Q element is not enabled (TRCCONFIGR.QE bits), it is acceptable to omit saving and restoring this register during idle.
TRCSSCSRn.STATUS bit
The restore flow does not explicitly clear the TRCSSCSRn.STATUS bit but instead directly loads the saved value. In contrast, the normal flow clears this bit to 0 when re-enabling single-shot control.
Therefore, the restore function passes the restart_ss argument as false to etm4_enable_hw() to avoid re-enabling single-shot mode.
Claim Tag Handling
The claim tag is acquired and released in normal flow, on the other hand, the claim protocol is not applied in CPU idle flow - it simply restores the saved value.
The claim bits serve two purpose:
Exclusive access between the kernel driver and an external debugger. During CPU idle, the kernel driver locks the OS Lock, ensuring that the external debugger cannot access the trace unit. Therefore, it is safe to release the claim tag during idle.
Notification to supervisory software to save/restore context for external debuggers. The kernel driver does not touch the external debugger's claim bit (ETMCLAIM[0]).
Based on this, it is safe to acquire and release claim tag in the idle sequence.
OS Lock Behavior
The OS Lock should be locked during CPU idle states. This differs from the normal flow, which unlock it. This special handling remains in the CPU save path.
This commit reuses the normal enable and disable logic in the CPU idle path. The common disable logic is moved into a new function __etm4_disable_hw(), which omits the etm4_cs_{unlock|lock}() pairs and is convenient to call from the save callback.
Why omit cs lock/unlock? Not unlocking the software lock prevents the claim tag registers from being written, which contradicts 3) above, and prevents other writes during the disable sequence.
Regards
Mike.
The resume callback sets the 'retain_ss_status' flag to avoid restarting the single-shot mode, and then directly calls etm4_enable_hw().
The save context in the driver data is no longer needed and has been removed.
Tested-by: James Clark james.clark@linaro.org Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 215 ++------------------- drivers/hwtracing/coresight/coresight-etm4x.h | 57 ------ 2 files changed, 16 insertions(+), 256 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index afee54ac0aec0c0e521fa1885a95f48555a369de..1c17d54729200671ec6339f43755ea4267dd61dc 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1002,7 +1002,7 @@ static void etm4_disable_trace_unit(struct etmv4_drvdata *drvdata) isb(); }
-static void etm4_disable_hw(struct etmv4_drvdata *drvdata) +static void __etm4_disable_hw(struct etmv4_drvdata *drvdata) { u32 control; struct etmv4_config *config = &drvdata->config; @@ -1010,7 +1010,6 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) struct csdev_access *csa = &csdev->access; int i;
etm4_cs_unlock(drvdata, csa); etm4_disable_arch_specific(drvdata); if (!drvdata->skip_power_up) {@@ -1039,12 +1038,21 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata) config->seq_state = etm4x_relaxed_read32(csa, TRCSEQSTR);
coresight_disclaim_device_unlocked(csdev);
etm4_cs_lock(drvdata, csa); dev_dbg(&drvdata->csdev->dev, "cpu: %d disable smp call done\n", drvdata->cpu);}
+static void etm4_disable_hw(struct etmv4_drvdata *drvdata) +{
struct coresight_device *csdev = drvdata->csdev;struct csdev_access *csa = &csdev->access;etm4_cs_unlock(drvdata, csa);__etm4_disable_hw(drvdata);etm4_cs_lock(drvdata, csa);+}
static void etm4_disable_sysfs_smp_call(void *info) { struct etmv4_drvdata *drvdata = info; @@ -1852,8 +1860,7 @@ static int etm4_dying_cpu(unsigned int cpu)
static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) {
int i, ret = 0;struct etmv4_save_state *state;
int ret = 0; struct coresight_device *csdev = drvdata->csdev; struct csdev_access *csa; struct device *etm_dev;@@ -1884,91 +1891,7 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) goto out; }
if (!drvdata->paused)etm4_disable_trace_unit(drvdata);state = drvdata->save_state;if (drvdata->nr_pe)state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);state->trcauxctlr = etm4x_read32(csa, TRCAUXCTLR);state->trceventctl0r = etm4x_read32(csa, TRCEVENTCTL0R);state->trceventctl1r = etm4x_read32(csa, TRCEVENTCTL1R);if (drvdata->stallctl)state->trcstallctlr = etm4x_read32(csa, TRCSTALLCTLR);state->trctsctlr = etm4x_read32(csa, TRCTSCTLR);state->trcsyncpr = etm4x_read32(csa, TRCSYNCPR);state->trcccctlr = etm4x_read32(csa, TRCCCCTLR);state->trcbbctlr = etm4x_read32(csa, TRCBBCTLR);state->trctraceidr = etm4x_read32(csa, TRCTRACEIDR);if (drvdata->q_filt)state->trcqctlr = etm4x_read32(csa, TRCQCTLR);state->trcvictlr = etm4x_read32(csa, TRCVICTLR);state->trcviiectlr = etm4x_read32(csa, TRCVIIECTLR);state->trcvissctlr = etm4x_read32(csa, TRCVISSCTLR);if (drvdata->nr_pe_cmp)state->trcvipcssctlr = etm4x_read32(csa, TRCVIPCSSCTLR);for (i = 0; i < drvdata->nrseqstate - 1; i++)state->trcseqevr[i] = etm4x_read32(csa, TRCSEQEVRn(i));if (drvdata->nrseqstate) {state->trcseqrstevr = etm4x_read32(csa, TRCSEQRSTEVR);state->trcseqstr = etm4x_read32(csa, TRCSEQSTR);}if (drvdata->numextinsel)state->trcextinselr = etm4x_read32(csa, TRCEXTINSELR);for (i = 0; i < drvdata->nr_cntr; i++) {state->trccntrldvr[i] = etm4x_read32(csa, TRCCNTRLDVRn(i));state->trccntctlr[i] = etm4x_read32(csa, TRCCNTCTLRn(i));state->trccntvr[i] = etm4x_read32(csa, TRCCNTVRn(i));}/* Resource selector pair 0 is reserved */for (i = 2; i < drvdata->nr_resource * 2; i++)state->trcrsctlr[i] = etm4x_read32(csa, TRCRSCTLRn(i));for (i = 0; i < drvdata->nr_ss_cmp; i++) {state->trcssccr[i] = etm4x_read32(csa, TRCSSCCRn(i));state->trcsscsr[i] = etm4x_read32(csa, TRCSSCSRn(i));if (etm4x_sspcicrn_present(drvdata, i))state->trcsspcicr[i] = etm4x_read32(csa, TRCSSPCICRn(i));}for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {state->trcacvr[i] = etm4x_read64(csa, TRCACVRn(i));state->trcacatr[i] = etm4x_read64(csa, TRCACATRn(i));}/** Data trace stream is architecturally prohibited for A profile cores* so we don't save (or later restore) trcdvcvr and trcdvcmr - As per* section 1.3.4 ("Possible functional configurations of an ETMv4 trace* unit") of ARM IHI 0064D.*/for (i = 0; i < drvdata->numcidc; i++)state->trccidcvr[i] = etm4x_read64(csa, TRCCIDCVRn(i));for (i = 0; i < drvdata->numvmidc; i++)state->trcvmidcvr[i] = etm4x_read64(csa, TRCVMIDCVRn(i));state->trccidcctlr0 = etm4x_read32(csa, TRCCIDCCTLR0);if (drvdata->numcidc > 4)state->trccidcctlr1 = etm4x_read32(csa, TRCCIDCCTLR1);state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR0);if (drvdata->numvmidc > 4)state->trcvmidcctlr0 = etm4x_read32(csa, TRCVMIDCCTLR1);state->trcclaimset = etm4x_read32(csa, TRCCLAIMCLR);if (!drvdata->skip_power_up)state->trcpdcr = etm4x_read32(csa, TRCPDCR);
__etm4_disable_hw(drvdata); /* wait for TRCSTATR.IDLE to go up */ if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1)) {@@ -1976,17 +1899,8 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) "timeout while waiting for Idle Trace Status\n"); etm4_os_unlock(drvdata); ret = -EBUSY;
goto out; }/** Power can be removed from the trace unit now. We do this to* potentially save power on systems that respect the TRCPDCR_PU* despite requesting software to save/restore state.*/if (!drvdata->skip_power_up)etm4x_relaxed_write32(csa, (state->trcpdcr & ~TRCPDCR_PU),TRCPDCR);out: etm4_cs_lock(drvdata, csa); return ret; @@ -2010,103 +1924,13 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) {
int i;struct etmv4_save_state *state = drvdata->save_state;struct csdev_access *csa = &drvdata->csdev->access;if (WARN_ON(!drvdata->csdev)) return;etm4_cs_unlock(drvdata, csa);etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);if (drvdata->nr_pe)etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);etm4x_relaxed_write32(csa, state->trcauxctlr, TRCAUXCTLR);etm4x_relaxed_write32(csa, state->trceventctl0r, TRCEVENTCTL0R);etm4x_relaxed_write32(csa, state->trceventctl1r, TRCEVENTCTL1R);if (drvdata->stallctl)etm4x_relaxed_write32(csa, state->trcstallctlr, TRCSTALLCTLR);etm4x_relaxed_write32(csa, state->trctsctlr, TRCTSCTLR);etm4x_relaxed_write32(csa, state->trcsyncpr, TRCSYNCPR);etm4x_relaxed_write32(csa, state->trcccctlr, TRCCCCTLR);etm4x_relaxed_write32(csa, state->trcbbctlr, TRCBBCTLR);etm4x_relaxed_write32(csa, state->trctraceidr, TRCTRACEIDR);if (drvdata->q_filt)etm4x_relaxed_write32(csa, state->trcqctlr, TRCQCTLR);etm4x_relaxed_write32(csa, state->trcvictlr, TRCVICTLR);etm4x_relaxed_write32(csa, state->trcviiectlr, TRCVIIECTLR);etm4x_relaxed_write32(csa, state->trcvissctlr, TRCVISSCTLR);if (drvdata->nr_pe_cmp)etm4x_relaxed_write32(csa, state->trcvipcssctlr, TRCVIPCSSCTLR);for (i = 0; i < drvdata->nrseqstate - 1; i++)etm4x_relaxed_write32(csa, state->trcseqevr[i], TRCSEQEVRn(i));if (drvdata->nrseqstate) {etm4x_relaxed_write32(csa, state->trcseqrstevr, TRCSEQRSTEVR);etm4x_relaxed_write32(csa, state->trcseqstr, TRCSEQSTR);}if (drvdata->numextinsel)etm4x_relaxed_write32(csa, state->trcextinselr, TRCEXTINSELR);
/* Instruct etm4_enable_hw() not to restart the single-shot mode */drvdata->config.retain_ss_status = 1;
for (i = 0; i < drvdata->nr_cntr; i++) {etm4x_relaxed_write32(csa, state->trccntrldvr[i], TRCCNTRLDVRn(i));etm4x_relaxed_write32(csa, state->trccntctlr[i], TRCCNTCTLRn(i));etm4x_relaxed_write32(csa, state->trccntvr[i], TRCCNTVRn(i));}/* Resource selector pair 0 is reserved */for (i = 2; i < drvdata->nr_resource * 2; i++)etm4x_relaxed_write32(csa, state->trcrsctlr[i], TRCRSCTLRn(i));for (i = 0; i < drvdata->nr_ss_cmp; i++) {etm4x_relaxed_write32(csa, state->trcssccr[i], TRCSSCCRn(i));etm4x_relaxed_write32(csa, state->trcsscsr[i], TRCSSCSRn(i));if (etm4x_sspcicrn_present(drvdata, i))etm4x_relaxed_write32(csa, state->trcsspcicr[i], TRCSSPCICRn(i));}for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {etm4x_relaxed_write64(csa, state->trcacvr[i], TRCACVRn(i));etm4x_relaxed_write64(csa, state->trcacatr[i], TRCACATRn(i));}for (i = 0; i < drvdata->numcidc; i++)etm4x_relaxed_write64(csa, state->trccidcvr[i], TRCCIDCVRn(i));for (i = 0; i < drvdata->numvmidc; i++)etm4x_relaxed_write64(csa, state->trcvmidcvr[i], TRCVMIDCVRn(i));etm4x_relaxed_write32(csa, state->trccidcctlr0, TRCCIDCCTLR0);if (drvdata->numcidc > 4)etm4x_relaxed_write32(csa, state->trccidcctlr1, TRCCIDCCTLR1);etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR0);if (drvdata->numvmidc > 4)etm4x_relaxed_write32(csa, state->trcvmidcctlr0, TRCVMIDCCTLR1);etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);if (!drvdata->skip_power_up)etm4x_relaxed_write32(csa, state->trcpdcr, TRCPDCR);/** As recommended by section 4.3.7 ("Synchronization when using the* memory-mapped interface") of ARM IHI 0064D*/dsb(sy);isb();/* Unlock the OS lock to re-enable trace and external debug access */etm4_os_unlock(drvdata);if (!drvdata->paused)etm4_enable_trace_unit(drvdata);etm4_cs_lock(drvdata, csa);
etm4_enable_hw(drvdata);}
static void etm4_cpu_restore(struct etmv4_drvdata *drvdata) @@ -2293,13 +2117,6 @@ static int etm4_probe(struct device *dev) pm_save_enable = coresight_loses_context_with_cpu(dev) ? PARAM_PM_SAVE_SELF_HOSTED : PARAM_PM_SAVE_NEVER;
if (pm_save_enable != PARAM_PM_SAVE_NEVER) {drvdata->save_state = devm_kmalloc(dev,sizeof(struct etmv4_save_state), GFP_KERNEL);if (!drvdata->save_state)return -ENOMEM;}raw_spin_lock_init(&drvdata->spinlock); drvdata->cpu = coresight_get_cpu(dev);diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index d82ec1559b34787dffc7debf4643d948e3850b2a..8d4a1f0f1e52a38a280b7fbe978129287a8ba60c 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -864,61 +864,6 @@ struct etmv4_config { u8 s_ex_level; };
-/**
- struct etm4_save_state - state to be preserved when ETM is without power
- */
-struct etmv4_save_state {
u32 trcprocselr;u32 trcconfigr;u32 trcauxctlr;u32 trceventctl0r;u32 trceventctl1r;u32 trcstallctlr;u32 trctsctlr;u32 trcsyncpr;u32 trcccctlr;u32 trcbbctlr;u32 trctraceidr;u32 trcqctlr;u32 trcvictlr;u32 trcviiectlr;u32 trcvissctlr;u32 trcvipcssctlr;u32 trcseqevr[ETM_MAX_SEQ_STATES];u32 trcseqrstevr;u32 trcseqstr;u32 trcextinselr;u32 trccntrldvr[ETMv4_MAX_CNTR];u32 trccntctlr[ETMv4_MAX_CNTR];u32 trccntvr[ETMv4_MAX_CNTR];u32 trcrsctlr[ETM_MAX_RES_SEL];u32 trcssccr[ETM_MAX_SS_CMP];u32 trcsscsr[ETM_MAX_SS_CMP];u32 trcsspcicr[ETM_MAX_SS_CMP];u64 trcacvr[ETM_MAX_SINGLE_ADDR_CMP];u64 trcacatr[ETM_MAX_SINGLE_ADDR_CMP];u64 trccidcvr[ETMv4_MAX_CTXID_CMP];u64 trcvmidcvr[ETM_MAX_VMID_CMP];u32 trccidcctlr0;u32 trccidcctlr1;u32 trcvmidcctlr0;u32 trcvmidcctlr1;u32 trcclaimset;u32 cntr_val[ETMv4_MAX_CNTR];u32 seq_state;u32 vinst_ctrl;u32 ss_status[ETM_MAX_SS_CMP];u32 trcpdcr;-};
/**
- struct etm4_drvdata - specifics associated to an ETM component
- @pclk: APB clock if present, otherwise NULL
@@ -981,7 +926,6 @@ struct etmv4_save_state {
at runtime, due to the additional setting of TRFCR_CX whenin EL2. Otherwise, 0.- @config: structure holding configuration parameters.
- @save_state: State to be preserved across power loss
- @skip_power_up: Indicates if an implementation can skip powering up
the trace unit.- @paused: Indicates if the trace unit is paused.
@@ -1036,7 +980,6 @@ struct etmv4_drvdata { bool lpoverride; u64 trfcr; struct etmv4_config config;
struct etmv4_save_state *save_state; bool skip_power_up; bool paused; DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);-- 2.34.1
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Mike,
On Wed, Nov 12, 2025 at 09:16:46AM +0000, Mike Leach wrote:
Hi Leo,
The save/restore sequence is only used if the ETM is active at the time of power down.
However, by removing the specific save/restore structure, and using the existing drvdata->config structure, the actual state can be changed by sysfs from another PE, as the sysfs accesses only write to the drvdata->config structure and not the actual hardware. Thus this change permits the corruption of the saved state, which will result in restore != saved..
While using the same code is a good idea avoiding lots of duplication, it may be necessary to have a copy of the drvdata->config passed in to the relevant functions.
We have considered in the past if it might be worthwhile isolating the sysfs and perf config settings in the etm - it may be worth re-visiting this again with a save restore copy in mind to.
Yes, Levi reminded me he already has some patches [1] to address the issue. Basically, the series returns -EBUSY if trying to write sysfs knobs but the device has been enabled.
Actually, we have another solution (credits to Levi who has worked on it with some internal discussion). Let me elaberate at here:
Similiar to you suggested "isolating sysfs and perf config setting", but isolate in a different way:
User's config: it is a set parameters stored for setting from user space. It can come from sysfs knobs or from cscfs parameters.
Hardware's config: it is a set parameters for setting hardware registers (we can simply use drvdata->config for this purpose).
As a result, every time when a session is enabled (it can be a sysfs session or perf session), we apply user's config for updating hardware's config (e.g., in etm4_enable_sysfs() / etm4_enable_perf()), afterwards, when invoke etm4_disable_hw() / etm4_enable_hw(), we only use hardware's config for register setting.
This can naturally resolve the mismatched issue you mentioned in CPU PM save and restore, and this can benefit us for later consolidating cscfg parameters.
I prefer we can use a separate patch series to address this issue (I should review Levi's patch series so can make things proceed). At the meantime, I would say this patch is still valid.
As a side topic, actually we have found issues for the config is reset when start a session so the user's config does not take affect at all (see etm4_set_default_config()).
[1] https://lore.kernel.org/linux-arm-kernel/20241221165934.1161856-2-yeoreum.yu...
On Tue, 11 Nov 2025 at 18:59, Leo Yan leo.yan@arm.com wrote:
The etm4_enable_trace_unit() function is almost identical to the restore flow, with a few differences listed below:
TRCQCTLR register
The TRCQCTLR register is saved and restored during CPU idle, but it is never touched in the normal flow. Given the Q element is not enabled (TRCCONFIGR.QE bits), it is acceptable to omit saving and restoring this register during idle.
TRCSSCSRn.STATUS bit
The restore flow does not explicitly clear the TRCSSCSRn.STATUS bit but instead directly loads the saved value. In contrast, the normal flow clears this bit to 0 when re-enabling single-shot control.
Therefore, the restore function passes the restart_ss argument as false to etm4_enable_hw() to avoid re-enabling single-shot mode.
Claim Tag Handling
The claim tag is acquired and released in normal flow, on the other hand, the claim protocol is not applied in CPU idle flow - it simply restores the saved value.
The claim bits serve two purpose:
Exclusive access between the kernel driver and an external debugger. During CPU idle, the kernel driver locks the OS Lock, ensuring that the external debugger cannot access the trace unit. Therefore, it is safe to release the claim tag during idle.
Notification to supervisory software to save/restore context for external debuggers. The kernel driver does not touch the external debugger's claim bit (ETMCLAIM[0]).
Based on this, it is safe to acquire and release claim tag in the idle sequence.
OS Lock Behavior
The OS Lock should be locked during CPU idle states. This differs from the normal flow, which unlock it. This special handling remains in the CPU save path.
This commit reuses the normal enable and disable logic in the CPU idle path. The common disable logic is moved into a new function __etm4_disable_hw(), which omits the etm4_cs_{unlock|lock}() pairs and is convenient to call from the save callback.
Why omit cs lock/unlock? Not unlocking the software lock prevents the claim tag registers from being written, which contradicts 3) above, and prevents other writes during the disable sequence.
Sorry for confusion.
I will update the commit log:
Split the hardware disable sequence into a new function __etm4_disable_hw(), which performs the common disable logic without the CoreSight lock.
__etm4_cpu_save() has its own CoreSight lock handling, it calls __etm4_disable_hw() so without duplicate lock issues.
Thanks, Leo
Hi Leo
On Wed, 12 Nov 2025 at 11:19, Leo Yan leo.yan@arm.com wrote:
Hi Mike,
On Wed, Nov 12, 2025 at 09:16:46AM +0000, Mike Leach wrote:
Hi Leo,
The save/restore sequence is only used if the ETM is active at the time of power down.
However, by removing the specific save/restore structure, and using the existing drvdata->config structure, the actual state can be changed by sysfs from another PE, as the sysfs accesses only write to the drvdata->config structure and not the actual hardware. Thus this change permits the corruption of the saved state, which will result in restore != saved..
While using the same code is a good idea avoiding lots of duplication, it may be necessary to have a copy of the drvdata->config passed in to the relevant functions.
We have considered in the past if it might be worthwhile isolating the sysfs and perf config settings in the etm - it may be worth re-visiting this again with a save restore copy in mind to.
Yes, Levi reminded me he already has some patches [1] to address the issue. Basically, the series returns -EBUSY if trying to write sysfs knobs but the device has been enabled.
This does solve the problem and as such should become part of this set rather than have it as a separate set, or must precede this set so no potential issue is created using this set.
Actually, we have another solution (credits to Levi who has worked on it with some internal discussion). Let me elaberate at here:
Similiar to you suggested "isolating sysfs and perf config setting", but isolate in a different way:
User's config: it is a set parameters stored for setting from user space. It can come from sysfs knobs or from cscfs parameters.
Hardware's config: it is a set parameters for setting hardware registers (we can simply use drvdata->config for this purpose).
As a result, every time when a session is enabled (it can be a sysfs session or perf session), we apply user's config for updating hardware's config (e.g., in etm4_enable_sysfs() / etm4_enable_perf()), afterwards, when invoke etm4_disable_hw() / etm4_enable_hw(), we only use hardware's config for register setting.
Has this set been sent to the coresight mailing list - I do not believe I have seen it?
This can naturally resolve the mismatched issue you mentioned in CPU PM save and restore, and this can benefit us for later consolidating cscfg parameters.
What consolidation of cscfg parameters? Not seen anything on this either?
I prefer we can use a separate patch series to address this issue (I should review Levi's patch series so can make things proceed). At the meantime, I would say this patch is still valid.
As a side topic, actually we have found issues for the config is reset when start a session so the user's config does not take affect at all (see etm4_set_default_config()).
This code is used by the perf session - not the sysfs session. This is deliberate to start from a stable state and add any configuration from the perf command line. The perf session deliberately ignores parameters set by sysfs - in fact the first act in parsing the perf event structure in etm4_parse_event_config(), is to completely zero out the entire config structure. Then the default config is set - which guarantees trace is captured, then any perf command line options are set in the registers (e.g. timestamps, address filtering etc). After this if we have s cscfg configuration active, this too is added in.
This ensures that the configuration run on all the ETMs used during the perf session is identical - which cannot be guaranteed if we used sysfs set parameters as part of the perf session, as each ETM might have different parameters set by sysfs.This gives us predictable, stable and repeatable perf configurations.
For sysfs sessions the config for a given etm is whatever has been set using sysfs + any active cscfg config settings.Here it is adbantageous to use a cscfg as this is applied to all etms being enabled by sysfs - without the need to use the syfs files for each individual etm.
Regards
Mike.
[1] https://lore.kernel.org/linux-arm-kernel/20241221165934.1161856-2-yeoreum.yu...
On Tue, 11 Nov 2025 at 18:59, Leo Yan leo.yan@arm.com wrote:
The etm4_enable_trace_unit() function is almost identical to the restore flow, with a few differences listed below:
TRCQCTLR register
The TRCQCTLR register is saved and restored during CPU idle, but it is never touched in the normal flow. Given the Q element is not enabled (TRCCONFIGR.QE bits), it is acceptable to omit saving and restoring this register during idle.
TRCSSCSRn.STATUS bit
The restore flow does not explicitly clear the TRCSSCSRn.STATUS bit but instead directly loads the saved value. In contrast, the normal flow clears this bit to 0 when re-enabling single-shot control.
Therefore, the restore function passes the restart_ss argument as false to etm4_enable_hw() to avoid re-enabling single-shot mode.
Claim Tag Handling
The claim tag is acquired and released in normal flow, on the other hand, the claim protocol is not applied in CPU idle flow - it simply restores the saved value.
The claim bits serve two purpose:
Exclusive access between the kernel driver and an external debugger. During CPU idle, the kernel driver locks the OS Lock, ensuring that the external debugger cannot access the trace unit. Therefore, it is safe to release the claim tag during idle.
Notification to supervisory software to save/restore context for external debuggers. The kernel driver does not touch the external debugger's claim bit (ETMCLAIM[0]).
Based on this, it is safe to acquire and release claim tag in the idle sequence.
OS Lock Behavior
The OS Lock should be locked during CPU idle states. This differs from the normal flow, which unlock it. This special handling remains in the CPU save path.
This commit reuses the normal enable and disable logic in the CPU idle path. The common disable logic is moved into a new function __etm4_disable_hw(), which omits the etm4_cs_{unlock|lock}() pairs and is convenient to call from the save callback.
Why omit cs lock/unlock? Not unlocking the software lock prevents the claim tag registers from being written, which contradicts 3) above, and prevents other writes during the disable sequence.
Sorry for confusion.
I will update the commit log:
Split the hardware disable sequence into a new function __etm4_disable_hw(), which performs the common disable logic without the CoreSight lock.
__etm4_cpu_save() has its own CoreSight lock handling, it calls __etm4_disable_hw() so without duplicate lock issues.
Thanks, Leo
Hi Mike,
On Wed, Nov 12, 2025 at 11:58:26AM +0000, Mike Leach wrote:
[...]
I prefer we can use a separate patch series to address this issue (I should review Levi's patch series so can make things proceed). At the meantime, I would say this patch is still valid.
As a side topic, actually we have found issues for the config is reset when start a session so the user's config does not take affect at all (see etm4_set_default_config()).
This code is used by the perf session - not the sysfs session. This is deliberate to start from a stable state and add any configuration from the perf command line.
My feeling is now the discussion mixes up two things:
- Should we apply user's parameters (via sysfs knobs) on a perf session?
- Can a perf session overwrite user's parameters?
For me, there is no reason to prevent a perf session from using the user's configs. We might disagree on this, but regarding the second question, a perf session definitely should not overwrite the user's parameters — it can ignore the user's configuration, but it must not erase these parameters.
The perf session deliberately ignores parameters set by sysfs - in fact the first act in parsing the perf event structure in etm4_parse_event_config(), is to completely zero out the entire config structure. Then the default config is set - which guarantees trace is captured, then any perf command line options are set in the registers (e.g. timestamps, address filtering etc). After this if we have s cscfg configuration active, this too is added in.
Seems to me, a correct sequence is:
1) Clean up hw_config; 2) Apply user's config on hw_config; 3) Apply cscfg on hw_config. 4) Apply perf's format/config on hw_config;
In this sequence, the last applying config has highest priority (as the perf format is the latest setting from user).
Here I am not quite sure for step 2) and 3). Should we apply both user's config and cscfg, or we apply only one and ignore another (e.g., if cscfg has been set, then ignore user's config, vice versa).
This ensures that the configuration run on all the ETMs used during the perf session is identical - which cannot be guaranteed if we used sysfs set parameters as part of the perf session, as each ETM might have different parameters set by sysfs.This gives us predictable, stable and repeatable perf configurations.
Why can't we leave this to users?
For sanity, drivers should reject invalid parameters when a user is setting it, once the parameters are accepted, afterwards drivers should simply follow the user's settings in a runtime session.
Thanks, Leo
Hi Leo
On Wed, 12 Nov 2025 at 13:56, Leo Yan leo.yan@arm.com wrote:
Hi Mike,
On Wed, Nov 12, 2025 at 11:58:26AM +0000, Mike Leach wrote:
[...]
I prefer we can use a separate patch series to address this issue (I should review Levi's patch series so can make things proceed). At the meantime, I would say this patch is still valid.
As a side topic, actually we have found issues for the config is reset when start a session so the user's config does not take affect at all (see etm4_set_default_config()).
This code is used by the perf session - not the sysfs session. This is deliberate to start from a stable state and add any configuration from the perf command line.
My feeling is now the discussion mixes up two things:
Should we apply user's parameters (via sysfs knobs) on a perf session?
Can a perf session overwrite user's parameters?
For me, there is no reason to prevent a perf session from using the user's configs. We might disagree on this, but regarding the second question, a perf session definitely should not overwrite the user's parameters — it can ignore the user's configuration, but it must not erase these parameters.
The perf session deliberately ignores parameters set by sysfs - in fact the first act in parsing the perf event structure in etm4_parse_event_config(), is to completely zero out the entire config structure. Then the default config is set - which guarantees trace is captured, then any perf command line options are set in the registers (e.g. timestamps, address filtering etc). After this if we have s cscfg configuration active, this too is added in.
Seems to me, a correct sequence is:
- Clean up hw_config;
- Apply user's config on hw_config;
The configuration system was designed precisely to implement the concept of a "users config" without the unnecessary burden of having to program up all the ETMs via sysfs in a multicore system.
The users config you describe is simply repeating work already done.
The danger in using sysfs configuration is that the perf session cannot possibly know if it was set by the current user of the resources or left over from a previous session.
The only elements that should affect a perf session should be those that appear on the perf command line - these being the traditional commands + a named configuration if present. This means that the owner of the perf session is 100% sure of what is being requested.
Changing the configuration of a perf session via some backdoor sysfs additions is unacceptable.
Regards
Mike
- Apply cscfg on hw_config.
- Apply perf's format/config on hw_config;
In this sequence, the last applying config has highest priority (as the perf format is the latest setting from user).
Here I am not quite sure for step 2) and 3). Should we apply both user's config and cscfg, or we apply only one and ignore another (e.g., if cscfg has been set, then ignore user's config, vice versa).
This ensures that the configuration run on all the ETMs used during the perf session is identical - which cannot be guaranteed if we used sysfs set parameters as part of the perf session, as each ETM might have different parameters set by sysfs.This gives us predictable, stable and repeatable perf configurations.
Why can't we leave this to users?
For sanity, drivers should reject invalid parameters when a user is setting it, once the parameters are accepted, afterwards drivers should simply follow the user's settings in a runtime session.
Thanks, Leo
On Tue, 11 Nov 2025 18:58:34 +0000, Leo Yan wrote:
This series is extracted from the CoreSight power management fixes and refactoring [1], focusing on ETMv3/4 power management.
This series has been verified on Juno-r2 platform.
[1] https://lore.kernel.org/linux-arm-kernel/20250915-arm_coresight_power_manage...
[...]
Applied, thanks!
[01/11] coresight: Change device mode to atomic type https://git.kernel.org/coresight/c/693d1eaca940 [02/11] coresight: etm4x: Always set tracer's device mode on target CPU https://git.kernel.org/coresight/c/28eee2158575 [03/11] coresight: etm3x: Always set tracer's device mode on target CPU https://git.kernel.org/coresight/c/ab3fde32afe6 [04/11] coresight: etm4x: Correct polling IDLE bit https://git.kernel.org/coresight/c/4dc4e22f9536 [05/11] coresight: etm4x: Add context synchronization before enabling trace https://git.kernel.org/coresight/c/64eb04ae5452 [06/11] coresight: etm4x: Properly control filter in CPU idle with FEAT_TRF https://git.kernel.org/coresight/c/1fdc2cd347a7 [07/11] coresight: etm4x: Remove the redundant DSB https://git.kernel.org/coresight/c/a5e6f584dab0 [08/11] coresight: etm4x: Remove the state_needs_restore flag https://git.kernel.org/coresight/c/d2c051f7a5ed [09/11] coresight: etm4x: Add flag to retain single-shot status https://git.kernel.org/coresight/c/c3d84848bd70 [10/11] coresight: etm4x: Retain sequencer state https://git.kernel.org/coresight/c/1b4e911af51c [11/11] coresight: etm4x: Reuse normal enable and disable logic in CPU idle https://git.kernel.org/coresight/c/efdccf6a5118
Best regards,
On 12/11/2025 12:10, Suzuki K Poulose wrote:
On Tue, 11 Nov 2025 18:58:34 +0000, Leo Yan wrote:
This series is extracted from the CoreSight power management fixes and refactoring [1], focusing on ETMv3/4 power management.
This series has been verified on Juno-r2 platform.
[1] https://lore.kernel.org/linux-arm-kernel/20250915-arm_coresight_power_manage...
[...]
Applied, thanks!
[01/11] coresight: Change device mode to atomic type https://git.kernel.org/coresight/c/693d1eaca940 [02/11] coresight: etm4x: Always set tracer's device mode on target CPU https://git.kernel.org/coresight/c/28eee2158575 [03/11] coresight: etm3x: Always set tracer's device mode on target CPU https://git.kernel.org/coresight/c/ab3fde32afe6 [04/11] coresight: etm4x: Correct polling IDLE bit https://git.kernel.org/coresight/c/4dc4e22f9536 [05/11] coresight: etm4x: Add context synchronization before enabling trace https://git.kernel.org/coresight/c/64eb04ae5452 [06/11] coresight: etm4x: Properly control filter in CPU idle with FEAT_TRF https://git.kernel.org/coresight/c/1fdc2cd347a7 [07/11] coresight: etm4x: Remove the redundant DSB https://git.kernel.org/coresight/c/a5e6f584dab0
Based on the recent discussions, I have dropped the patches below. We cannot let the corruption creep in with cpu-idle, which wasn't possible otherwise.
Suzuki
[08/11] coresight: etm4x: Remove the state_needs_restore flag https://git.kernel.org/coresight/c/d2c051f7a5ed [09/11] coresight: etm4x: Add flag to retain single-shot status https://git.kernel.org/coresight/c/c3d84848bd70 [10/11] coresight: etm4x: Retain sequencer state https://git.kernel.org/coresight/c/1b4e911af51c [11/11] coresight: etm4x: Reuse normal enable and disable logic in CPU idle https://git.kernel.org/coresight/c/efdccf6a5118
Best regards,
-
Leo Yan -
Mike Leach -
Suzuki K Poulose