Re: [PATCH v4 15/15] coresight: Move CPU hotplug callbacks to core layer

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

On 04/11/2025 3:21 pm, Leo Yan wrote:
> This commit moves CPU hotplug callbacks from ETMv4 driver to core layer.
> The motivation is the core layer can control all components on an
> activated path rather but not only managing tracer in ETMv4 driver.
> 
> The perf event layer will disable CoreSight PMU event 'cs_etm' when
> hotplug off a CPU.  That means a perf mode will be always converted to
> disabled mode in CPU hotplug.  Arm CoreSight CPU hotplug callbacks only
> need to handle the Sysfs mode and ignore the perf mode.
> 
> The core layer disables a source and the path when hotplug-off a CPU.
> 
> When hotplug-in a CPU, it does not re-enable even the trace was enabled
> before hotplug-off, in this case, users are asked to re-enable the trace
> via Sysfs interface.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c       | 41 +++++++++++++++++++++-
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 37 -------------------
>   2 files changed, 40 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 7095f9befa98c7d638f857466351ef7cd960fde7..9149f028031808dc2b7285f621b97f3631a02b01 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1726,6 +1726,32 @@ static void coresight_pm_restore(struct coresight_device *csdev)
>   	coresight_ops(csdev)->pm_restore_enable(csdev);
>   }
>   
> +static int coresight_dying_cpu(unsigned int cpu)
> +{
> +	struct coresight_device *source = per_cpu(csdev_source, cpu);
> +	struct coresight_path *path;
> +
> +	if (!source || !source->path)
> +		return 0;
> +
> +	/*
> +	 * The perf event layer will disable PMU events in the CPU hotplug.
> +	 * CoreSight driver should never handle the CS_MODE_PERF case.
> +	 */
> +	if (coresight_get_mode(source) != CS_MODE_SYSFS)
> +		return 0;
> +
> +	/*
> +	 * Save 'source->path' here, as it will be cleared in
> +	 * coresight_disable_source().
> +	 */
> +	path = source->path;
> +
> +	coresight_disable_source(source, NULL);
> +	coresight_disable_path(path);
> +	return 0;

If the user is expected to re-enable and this new state is visible, 
don't you need to use the regular coresight_disable_sysfs() function? It 
calls coresight_disable_source_sysfs() which updates a refcount.

Maybe you didn't do it because it has a mutex in it? It would be easier 
to change that to a spinlock or take the mutex in a wrapper function and 
share the core disabling code with the hotplug path.

> +}
> +
>   static int coresight_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
>   				   void *v)
>   {
> @@ -1757,11 +1783,24 @@ static struct notifier_block coresight_cpu_pm_nb = {
>   
>   static int __init coresight_pm_setup(void)
>   {
> -	return cpu_pm_register_notifier(&coresight_cpu_pm_nb);
> +	int ret;
> +
> +	ret = cpu_pm_register_notifier(&coresight_cpu_pm_nb);
> +	if (ret)
> +		return ret;
> +
> +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
> +					"arm/coresight-core:starting",
> +					NULL, coresight_dying_cpu);
> +	if (ret)
> +		cpu_pm_unregister_notifier(&coresight_cpu_pm_nb);
> +
> +	return ret;
>   }
>   
>   static void coresight_pm_cleanup(void)
>   {
> +	cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
>   	cpu_pm_unregister_notifier(&coresight_cpu_pm_nb);
>   }
>   
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 7bea3d2c6f49a2169b8e3de1cb07cc22147aae3a..1730cee3c3e1845efb2be815553085aba1246cca 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1802,33 +1802,6 @@ static int etm4_online_cpu(unsigned int cpu)
>   	return 0;
>   }
>   
> -static int etm4_starting_cpu(unsigned int cpu)
> -{
> -	if (!etmdrvdata[cpu])
> -		return 0;
> -
> -	raw_spin_lock(&etmdrvdata[cpu]->spinlock);
> -	if (!etmdrvdata[cpu]->os_unlock)
> -		etm4_os_unlock(etmdrvdata[cpu]);
> -
> -	if (coresight_get_mode(etmdrvdata[cpu]->csdev))
> -		etm4_enable_hw(etmdrvdata[cpu], true);
> -	raw_spin_unlock(&etmdrvdata[cpu]->spinlock);
> -	return 0;
> -}
> -
> -static int etm4_dying_cpu(unsigned int cpu)
> -{
> -	if (!etmdrvdata[cpu])
> -		return 0;
> -
> -	raw_spin_lock(&etmdrvdata[cpu]->spinlock);
> -	if (coresight_get_mode(etmdrvdata[cpu]->csdev))
> -		etm4_disable_hw(etmdrvdata[cpu]);
> -	raw_spin_unlock(&etmdrvdata[cpu]->spinlock);
> -	return 0;
> -}
> -
>   static bool etm4_need_save_restore_context(struct coresight_device *csdev)
>   {
>   	if (pm_save_enable != PARAM_PM_SAVE_SELF_HOSTED)
> @@ -1915,13 +1888,6 @@ static int __init etm4_pm_setup(void)
>   {
>   	int ret;
>   
> -	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
> -					"arm/coresight4:starting",
> -					etm4_starting_cpu, etm4_dying_cpu);
> -
> -	if (ret)
> -		return ret;
> -
>   	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>   					"arm/coresight4:online",
>   					etm4_online_cpu, NULL);
> @@ -1932,14 +1898,11 @@ static int __init etm4_pm_setup(void)
>   		return 0;
>   	}
>   
> -	/* failed dyn state - remove others */
> -	cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
>   	return ret;
>   }
>   
>   static void etm4_pm_clear(void)
>   {
> -	cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
>   	if (hp_online) {
>   		cpuhp_remove_state_nocalls(hp_online);
>   		hp_online = 0;
>