[PATCH v1 09/11] coresight: Take hotplug lock in enable_source_store() for Sysfs mode

[James Clark]

On 16/05/2025 5:07 pm, Leo Yan wrote:
> The hotplug lock is acquired and released in the etm4_disable_sysfs()
> function, which is a low-level function located in the ETM4 driver.
> This prevents us from a new solution for hotplug.
> 
> Firstly, hotplug callbacks cannot invoke etm4_disable_sysfs() to disable
> the source; otherwise, a deadlock issue occurs.  The reason is that, in
> the hotplug flow, the kernel acquires the hotplug lock before calling
> callbacks.  Subsequently, if coresight_disable_source() is invoked and
> it calls etm4_disable_sysfs(), the hotplug lock will be acquired twice,
> leading to a double lock issue.
> 
> Secondly, when hotplugging a CPU on or off, if we want to manipulate all
> components on a path attached to the CPU, we need to maintain atomicity
> for the entire path.  Otherwise, a race condition may occur with users
> setting the same path via the Sysfs knobs, ultimately causing mess
> states in CoreSight components.
> 
> This patch moves the hotplug locking from etm4_disable_sysfs() into
> enable_source_store().  As a result, when users control the Sysfs knobs,
> the whole flow is protected by hotplug locking, ensuring it is mutual
> exclusive with hotplug callbacks.
> 
> Note, the paired function etm4_enable_sysfs() does not use hotplug
> locking, which is why this patch does not modify it.
> 
> Signed-off-by: Leo Yan <leo.yan at arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-etm4x-core.c |  8 --------
>   drivers/hwtracing/coresight/coresight-sysfs.c      | 12 +++++++++++-
>   2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d5fd9e58a962..e5a7c0dd7f8e 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1032,13 +1032,6 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
>   {
>   	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>   
> -	/*
> -	 * Taking hotplug lock here protects from clocks getting disabled
> -	 * with tracing being left on (crash scenario) if user disable occurs
> -	 * after cpu online mask indicates the cpu is offline but before the
> -	 * DYING hotplug callback is serviced by the ETM driver.
> -	 */
> -	cpus_read_lock();
>   	raw_spin_lock(&drvdata->spinlock);
>   
>   	/*
> @@ -1048,7 +1041,6 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
>   	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
>   
>   	raw_spin_unlock(&drvdata->spinlock);
> -	cpus_read_unlock();
>   
>   	/*
>   	 * we only release trace IDs when resetting sysfs.
> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
> index feadaf065b53..ea839a5e601b 100644
> --- a/drivers/hwtracing/coresight/coresight-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
> @@ -359,14 +359,24 @@ static ssize_t enable_source_store(struct device *dev,
>   	if (ret)
>   		return ret;
>   
> +	/*
> +	 * CoreSight hotplug callbacks in core layer control a activated path
> +	 * from its source to sink. Taking hotplug lock here protects a race
> +	 * condition with hotplug callbacks.
> +	 */
> +	cpus_read_lock();
> +

Looks like you can do guard(cpus_read_lock)(); and avoid the multiple 
unlocks below.

>   	if (val) {
>   		ret = coresight_enable_sysfs(csdev);
> -		if (ret)
> +		if (ret) {
> +			cpus_read_unlock();
>   			return ret;
> +		}
>   	} else {
>   		coresight_disable_sysfs(csdev);
>   	}
>   
> +	cpus_read_unlock();
>   	return size;
>   }
>   static DEVICE_ATTR_RW(enable_source);