Re: [PATCH v2 02/28] coresight: etm4x: Always set tracer's device mode on target CPU

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

LGTM.

Reviewed-by: Yeoreum Yun <yeoreum.yun@arm.com>

> 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")
> Signed-off-by: Leo Yan <leo.yan@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 42e5d37403addc6ec81f2e3184522d67d1677c04..ee405c88ea5faa130819f96b00b8307f8764d58a 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -590,10 +590,23 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  static void etm4_enable_hw_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);
>  }
>
>  /*
> @@ -809,6 +822,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;
> +
>  	if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) {
>  		ret = -EINVAL;
>  		goto out;
> @@ -828,6 +844,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;
>  }
>
> @@ -880,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);
> @@ -896,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;
>  }
>
> @@ -951,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;
> @@ -991,6 +1000,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)
> +{
> +	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)
>  {
> @@ -1020,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.
> @@ -1045,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_hw_smp_call,
> +				 drvdata, 1);
>
>  	raw_spin_unlock(&drvdata->spinlock);
>
> @@ -1085,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)
>
> --
> 2.34.1
>

--
Sincerely,
Yeoreum Yun