Re: [PATCH v1 02/11] coresight: Set per CPU source pointer

[James Clark <james.clark@linaro.org>]

On 16/05/2025 5:07 pm, Leo Yan wrote:
> Introduce coresight_set_percpu_source() for setting CPU source device. The
> sources are maintained in a per CPU structure 'csdev_source'.
> 
> The coresight_register() function cannot be used for setting CPU source
> device as it is absent info for CPU ID.  So this commit uses the ETM3 and
> ETM4 drivers to set and clear CPU sources in probing and removal,
> respectively.
> 

I agree that doing it in coresight_register() would be better and 
slightly less fragile.

You could add 'cpu' to 'struct coresight_desc' and then do the 
assignment to 'csdev_source' in coresight_register() if 
coresight_is_percpu_source() == true.

> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c       | 7 +++++++
>   drivers/hwtracing/coresight/coresight-etm3x-core.c | 2 ++
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 ++
>   drivers/hwtracing/coresight/coresight-priv.h       | 1 +
>   4 files changed, 12 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fa758cc21827..1503037662f2 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -32,6 +32,7 @@
>    */
>   DEFINE_MUTEX(coresight_mutex);
>   static DEFINE_PER_CPU(struct coresight_device *, csdev_sink);
> +static DEFINE_PER_CPU(struct coresight_device *, csdev_source);
>   
>   /**
>    * struct coresight_node - elements of a path, from source to sink
> @@ -77,6 +78,12 @@ struct coresight_device *coresight_get_percpu_sink(int cpu)
>   }
>   EXPORT_SYMBOL_GPL(coresight_get_percpu_sink);
>   
> +void coresight_set_percpu_source(int cpu, struct coresight_device *csdev)
> +{
> +	per_cpu(csdev_source, cpu) = csdev;
> +}
> +EXPORT_SYMBOL_GPL(coresight_set_percpu_source);
> +
>   static struct coresight_device *coresight_get_source(struct coresight_path *path)
>   {
>   	struct coresight_device *csdev;
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index 1c6204e14422..de62a0bb7faf 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> @@ -879,6 +879,7 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)
>   	}
>   
>   	etmdrvdata[drvdata->cpu] = drvdata;
> +	coresight_set_percpu_source(drvdata->cpu, drvdata->csdev);

Doesn't this need to be done before coresight_register()? Once 
coresight_mutex is unlocked the device can be used. So you could end up 
with an enabled device hitting the PM notifier with the per-cpu variable 
still NULL and not disabling the device. Then the state would be messed up.

Probably another reason to put the assignment in coresight_register().

>   
>   	pm_runtime_put(&adev->dev);
>   	dev_info(&drvdata->csdev->dev,
> @@ -902,6 +903,7 @@ static void etm_remove(struct amba_device *adev)
>   {
>   	struct etm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
>   
> +	coresight_set_percpu_source(drvdata->cpu, NULL);
>   	etm_perf_symlink(drvdata->csdev, false);
>   
>   	/*
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index b12d59b89a49..fede0be5cd43 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -2203,6 +2203,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
>   	}
>   
>   	etmdrvdata[drvdata->cpu] = drvdata;
> +	coresight_set_percpu_source(drvdata->cpu, drvdata->csdev);
>   
>   	dev_info(&drvdata->csdev->dev, "CPU%d: %s v%d.%d initialized\n",
>   		 drvdata->cpu, type_name, major, minor);
> @@ -2402,6 +2403,7 @@ static void etm4_remove_dev(struct etmv4_drvdata *drvdata)
>   	cpus_read_unlock();
>   
>   	if (!had_delayed_probe) {
> +		coresight_set_percpu_source(drvdata->cpu, NULL);
>   		etm_perf_symlink(drvdata->csdev, false);
>   		cscfg_unregister_csdev(drvdata->csdev);
>   		coresight_unregister(drvdata->csdev);
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 33e22b1ba043..59f9ec9cc260 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -250,6 +250,7 @@ void coresight_add_helper(struct coresight_device *csdev,
>   
>   void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev);
>   struct coresight_device *coresight_get_percpu_sink(int cpu);
> +void coresight_set_percpu_source(int cpu, struct coresight_device *csdev);
>   void coresight_disable_source(struct coresight_device *csdev, void *data);
>   void coresight_pause_source(struct coresight_device *csdev);
>   int coresight_resume_source(struct coresight_device *csdev);