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

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

Hi Leo,

[...]
> @@ -464,17 +477,29 @@ static int etm_enable_perf(struct coresight_device *csdev,
>  			   struct perf_event *event,
>  			   struct coresight_path *path)
>  {
> +	int ret = 0;
>  	struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> -	if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id()))
> -		return -EINVAL;
> +	if (!coresight_take_mode(csdev, CS_MODE_PERF))
> +		return -EBUSY;
> +
> +	if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

Small question: why drvdata->cpu != smp_processor_id() check after
changing mode? Would it better to check before change of it?

>
>  	/* 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);
> +
> +out:
> +	/* The tracer didn't start */
> +	if (ret)
> +		coresight_set_mode(csdev, CS_MODE_DISABLED);
> +	return ret;
>  }
>
>  static int etm_enable_sysfs(struct coresight_device *csdev, struct coresight_path *path)
> @@ -519,11 +544,6 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event,
>  	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:
>  		ret = etm_enable_sysfs(csdev, path);
> @@ -535,17 +555,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 etmv4_drvdata *drvdata)
>  {
>  	int i;
> -	struct etm_drvdata *drvdata = info;
>  	struct etm_config *config = &drvdata->config;
>  	struct coresight_device *csdev = drvdata->csdev;
>
> @@ -567,6 +582,15 @@ static void etm_disable_hw(void *info)
>  		"cpu: %d disable smp call done\n", drvdata->cpu);
>  }
>
> +static void etm_disable_hw_smp_call(void *info)
> +{
> +	struct etmv_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 +612,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 +638,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_hw_smp_call,
> +				 drvdata, 1);
>
>  	spin_unlock(&drvdata->spinlock);
>  	cpus_read_unlock();
> @@ -652,9 +679,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 = {
>
> --
> 2.34.1
>

--
Sincerely,
Yeoreum Yun