[PATCH v3 02/31] coresight: etm4x: Always set tracer's device mode on target CPU
Suzuki K Poulose
suzuki.poulose at arm.com
Mon Sep 15 03:47:43 PDT 2025
On 15/09/2025 11:33, Leo Yan wrote:
> 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_hw_smp_call() function
> for SMP calls, which disables the tracer and explicitly set the mode to
> DISABLED during SysFS operations.
>
> 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 at arm.com>
> Signed-off-by: Leo Yan <leo.yan at arm.com>
> ---
> drivers/hwtracing/coresight/coresight-etm4x-core.c | 48 +++++++++++++++-------
> 1 file changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 020f070bf17dc0557dfb3ae4f282b0a0c1778bd8..02ad41da7356547a67c53ff0a9146aec844f89da 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -592,10 +592,23 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> static void etm4_enable_hw_smp_call(void *info)
While at this, please could you rename this function to make it explicit
that this is only for sysfs mode ?
e.g., etm4_enable_hw_sysfs_smp_call()
> {
> 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);
> }
>
> /*
> @@ -811,6 +824,9 @@ static int etm4_enable_perf(struct coresight_device *csdev,
> int ret = 0;
> struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> + if (!coresight_take_mode(csdev, CS_MODE_PERF))
> + return -EBUSY;
> +
Should be done after the CPU check below ? Otherwise you are undoing the
fix on sysfs side.
> if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) {
> ret = -EINVAL;
> goto out;
> @@ -830,6 +846,9 @@ static int etm4_enable_perf(struct coresight_device *csdev,
> ret = etm4_enable_hw(drvdata);
>
> out:
> + /* The tracer didn't start */
> + if (ret)
> + coresight_set_mode(csdev, CS_MODE_DISABLED);
> return ret;
> }
>
> @@ -882,11 +901,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 +912,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 +963,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 +1002,15 @@ static void etm4_disable_hw(void *info)
> "cpu: %d disable smp call done\n", drvdata->cpu);
> }
>
> +static void etm4_disable_hw_smp_call(void *info)
sysfs mode again.
Rest looks fine
Suzuki
> +{
> + 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 +1040,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 +1067,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_hw_smp_call,
> + drvdata, 1);
>
> raw_spin_unlock(&drvdata->spinlock);
>
> @@ -1087,9 +1108,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)
>
More information about the linux-arm-kernel
mailing list