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

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@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);