[PATCH v1 00/11] CoreSight: Refactor CPU PM and hotplug

[PATCH v1 00/11] CoreSight: Refactor CPU PM and hotplug

[Leo Yan]

Besides managing tracers (ETM) in CPU PM and hotplug flows, the
CoreSight framework is found the issues below:

Firstly, on some hardware platforms, CoreSight links (e.g., funnels and
replicators, etc) reside in a cluster power domain.  If the cluster is
powered off, the link components also will lose their context.  In this
case, Arm CoreSight drivers report errors when detect unpaired self-host
claim tags.

Secondly, if a path has been activated from per CPU's tracer (ETM) to
links and a sink in Sysfs mode, then when the CPU is hot-plugged off,
only the associated ETM will be disabled.  Afterwards, the links and the
sink always keep on and no chance to be disabled.

The last issue was reported by Yabin Cui (Google) that the TRBE driver
misses to save and restore context during CPU low power states.  As a
result, it may cause hardware lockup issue on some devices.

To resolve the power management issues, this series extends CPU power
management to cover the entire activated path, including links and
sinks.  It moves CPU PM and hotplug notifiers from the ETMv4 driver to
the CoreSight core layer.  The core layer has sufficient info to
maintain activated paths and can traverse the entire path to manipulate
CoreSight modules accordingly.

Patch 01 is to fix a bug in ETMv4 save and restore callbacks.

Patches 02 ~ 06 move CPU PM code from ETMv4 driver to the core layer, and
extends to maintain activated paths and control links.

Patches 07 and 08 support save and restore context for per-CPU sink
(TRBE).  Note, for avoid long latency, system level's sinks in an
activated path are not touched during CPU suspend and resume.

Patches 09 ~ 11 move CPU hotplug notifier from ETMv4 driver to the core
layer.  The entire path will be controlled if the corresponding CPU is
hot-plugged on or off.

This series has been verified on Hikey960 for CPUIdle and hotplug.  And
it is tested on FVP for verifying TRBE with idle states.


Leo Yan (10):
  coresight: etm4x: Control the trace unit in CPU suspend
  coresight: Set per CPU source pointer
  coresight: Register CPU PM notifier in core layer
  coresight: etm4x: Hook CPU PM callbacks
  coresight: Save activated path into source device
  coresight: Control path during CPU PM
  coresight: Add PM callbacks in sink operation
  coresight: Take hotplug lock in enable_source_store() for Sysfs mode
  coresight: Move CPU hotplug callbacks to core layer
  coresight: Manage activated paths during CPU hotplug

Yabin Cui (1):
  coresight: trbe: Save and restore state across CPU low power state

 drivers/hwtracing/coresight/coresight-core.c       | 252 +++++++++++++++++++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-etm3x-core.c |   2 +
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 133 ++++++----------------
 drivers/hwtracing/coresight/coresight-priv.h       |   1 +
 drivers/hwtracing/coresight/coresight-sysfs.c      |  12 +-
 drivers/hwtracing/coresight/coresight-trbe.c       |  65 +++++++++++
 include/linux/coresight.h                          |  11 ++
 7 files changed, 375 insertions(+), 101 deletions(-)

-- 
2.34.1

[PATCH v1 01/11] coresight: etm4x: Control the trace unit in CPU suspend

[Leo Yan]

If a trace unit has been enabled, a CPU suspend operation misses to
disable the trace unit.  After the CPU resumes, it also does not follow
the general flow for re-enabling the trace unit.  As a result, this may
lead to a wrong state machine for the ETM.

This commit stops the trace unit during the CPU suspend flow and
re-enables the trace unit in the resume flow.

Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states")
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 6a5898355a83..b12d59b89a49 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1820,6 +1820,11 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
 	state = drvdata->save_state;
 
 	state->trcprgctlr = etm4x_read32(csa, TRCPRGCTLR);
+
+	/* Stop the trace unit if it is enabled */
+	if (state->trcprgctlr)
+		etm4_disable_trace_unit(drvdata);
+
 	if (drvdata->nr_pe)
 		state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);
 	state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);
@@ -1951,7 +1956,6 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 	etm4_cs_unlock(drvdata, csa);
 	etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
 
-	etm4x_relaxed_write32(csa, state->trcprgctlr, TRCPRGCTLR);
 	if (drvdata->nr_pe)
 		etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);
 	etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);
@@ -2035,6 +2039,15 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 
 	/* Unlock the OS lock to re-enable trace and external debug access */
 	etm4_os_unlock(drvdata);
+
+	/*
+	 * Re-enable the trace unit if it was enabled before suspend.
+	 * Put it after etm4_os_unlock() as unlocking the OS lock is the
+	 * prerequisite for enabling the trace unit.
+	 */
+	if (state->trcprgctlr)
+		etm4_enable_trace_unit(drvdata);
+
 	etm4_cs_lock(drvdata, csa);
 }
 
-- 
2.34.1

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

[Leo Yan]

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.

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

[PATCH v1 03/11] coresight: Register CPU PM notifier in core layer

[Leo Yan]

The current implementation only saves and restores the context for ETM
sources while ignoring the context of links.  However, if funnels or
replicators on a linked path resides in a CPU or cluster power domain,
the hardware context for the link will be lost after resuming from low
power states.

To support context management for links during CPU low power modes, a
better way is to implement CPU PM callbacks in the Arm CoreSight core
layer.  As the core layer has sufficient information for linked paths,
from tracers to links, which can be used for power management.

As a first step, this patch registers CPU PM notifier in the core layer.
If a source driver provides callbacks for saving and restoring context,
these callbacks will be invoked in CPU suspend and resume.

Further changes will extend for controlling path.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/coresight.h                    |  4 ++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 1503037662f2..7ab7321ca8d5 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/build_bug.h>
+#include <linux/cpu_pm.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/types.h>
@@ -421,6 +422,21 @@ int coresight_resume_source(struct coresight_device *csdev)
 }
 EXPORT_SYMBOL_GPL(coresight_resume_source);
 
+static int coresight_save_source(struct coresight_device *csdev)
+{
+	if (csdev && source_ops(csdev)->save)
+		return source_ops(csdev)->save(csdev);
+
+	/* Return success if callback is not supported */
+	return 0;
+}
+
+static void coresight_restore_source(struct coresight_device *csdev)
+{
+	if (csdev && source_ops(csdev)->restore)
+		source_ops(csdev)->restore(csdev);
+}
+
 /*
  * coresight_disable_path_from : Disable components in the given path beyond
  * @nd in the list. If @nd is NULL, all the components, except the SOURCE are
@@ -1566,6 +1582,45 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict,
 }
 EXPORT_SYMBOL_GPL(coresight_alloc_device_name);
 
+static int coresight_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
+				   void *v)
+{
+	unsigned int cpu = smp_processor_id();
+	struct coresight_device *source = per_cpu(csdev_source, cpu);
+
+	if (!source)
+		return NOTIFY_OK;
+
+	switch (cmd) {
+	case CPU_PM_ENTER:
+		if (coresight_save_source(source))
+			return NOTIFY_BAD;
+		break;
+	case CPU_PM_EXIT:
+	case CPU_PM_ENTER_FAILED:
+		coresight_restore_source(source);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block coresight_cpu_pm_nb = {
+	.notifier_call = coresight_cpu_pm_notify,
+};
+
+static int __init coresight_pm_setup(void)
+{
+	return cpu_pm_register_notifier(&coresight_cpu_pm_nb);
+}
+
+static void coresight_pm_cleanup(void)
+{
+	cpu_pm_unregister_notifier(&coresight_cpu_pm_nb);
+}
+
 const struct bus_type coresight_bustype = {
 	.name	= "coresight",
 };
@@ -1620,9 +1675,15 @@ static int __init coresight_init(void)
 
 	/* initialise the coresight syscfg API */
 	ret = cscfg_init();
+	if (ret)
+		goto exit_notifier;
+
+	ret = coresight_pm_setup();
 	if (!ret)
 		return 0;
 
+	cscfg_exit();
+exit_notifier:
 	atomic_notifier_chain_unregister(&panic_notifier_list,
 					     &coresight_notifier);
 exit_perf:
@@ -1634,6 +1695,7 @@ static int __init coresight_init(void)
 
 static void __exit coresight_exit(void)
 {
+	coresight_pm_cleanup();
 	cscfg_exit();
 	atomic_notifier_chain_unregister(&panic_notifier_list,
 					     &coresight_notifier);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 4ac65c68bbf4..64cee4040daa 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -400,6 +400,8 @@ struct coresight_ops_link {
  * @disable:	disables tracing for a source.
  * @resume_perf: resumes tracing for a source in perf session.
  * @pause_perf:	pauses tracing for a source in perf session.
+ * @save:	save context for a source.
+ * @restore:	restore context for a source.
  */
 struct coresight_ops_source {
 	int (*cpu_id)(struct coresight_device *csdev);
@@ -409,6 +411,8 @@ struct coresight_ops_source {
 			struct perf_event *event);
 	int (*resume_perf)(struct coresight_device *csdev);
 	void (*pause_perf)(struct coresight_device *csdev);
+	int (*save)(struct coresight_device *csdev);
+	void (*restore)(struct coresight_device *csdev);
 };
 
 /**
-- 
2.34.1

[PATCH v1 04/11] coresight: etm4x: Hook CPU PM callbacks

[Leo Yan]

Since the CoreSight core layer has registered CPU PM notifiers, this
patch hooks CPU save and restore callbacks to be invoked from the core
layer.

The CPU PM notifier in the ETMv4 driver is no longer needed, remove it
along with its registration and unregistration code.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 73 +++++++++++-------------------------------
 1 file changed, 18 insertions(+), 55 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index fede0be5cd43..d5fd9e58a962 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1118,19 +1118,6 @@ static void etm4_pause_perf(struct coresight_device *csdev)
 	drvdata->paused = true;
 }
 
-static const struct coresight_ops_source etm4_source_ops = {
-	.cpu_id		= etm4_cpu_id,
-	.enable		= etm4_enable,
-	.disable	= etm4_disable,
-	.resume_perf	= etm4_resume_perf,
-	.pause_perf	= etm4_pause_perf,
-};
-
-static const struct coresight_ops etm4_cs_ops = {
-	.trace_id	= coresight_etm_get_trace_id,
-	.source_ops	= &etm4_source_ops,
-};
-
 static bool cpu_supports_sysreg_trace(void)
 {
 	u64 dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
@@ -1928,8 +1915,9 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
 	return ret;
 }
 
-static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
+static int etm4_cpu_save(struct coresight_device *csdev)
 {
+	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 	int ret = 0;
 
 	/* Save the TRFCR irrespective of whether the ETM is ON */
@@ -2051,46 +2039,29 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
 	etm4_cs_lock(drvdata, csa);
 }
 
-static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
+static void etm4_cpu_restore(struct coresight_device *csdev)
 {
+	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
 	if (drvdata->trfcr)
 		write_trfcr(drvdata->save_trfcr);
 	if (drvdata->state_needs_restore)
 		__etm4_cpu_restore(drvdata);
 }
 
-static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
-			      void *v)
-{
-	struct etmv4_drvdata *drvdata;
-	unsigned int cpu = smp_processor_id();
-
-	if (!etmdrvdata[cpu])
-		return NOTIFY_OK;
-
-	drvdata = etmdrvdata[cpu];
-
-	if (WARN_ON_ONCE(drvdata->cpu != cpu))
-		return NOTIFY_BAD;
-
-	switch (cmd) {
-	case CPU_PM_ENTER:
-		if (etm4_cpu_save(drvdata))
-			return NOTIFY_BAD;
-		break;
-	case CPU_PM_EXIT:
-	case CPU_PM_ENTER_FAILED:
-		etm4_cpu_restore(drvdata);
-		break;
-	default:
-		return NOTIFY_DONE;
-	}
-
-	return NOTIFY_OK;
-}
+static const struct coresight_ops_source etm4_source_ops = {
+	.cpu_id		= etm4_cpu_id,
+	.enable		= etm4_enable,
+	.disable	= etm4_disable,
+	.resume_perf	= etm4_resume_perf,
+	.pause_perf	= etm4_pause_perf,
+	.save		= etm4_cpu_save,
+	.restore	= etm4_cpu_restore,
+};
 
-static struct notifier_block etm4_cpu_pm_nb = {
-	.notifier_call = etm4_cpu_pm_notify,
+static const struct coresight_ops etm4_cs_ops = {
+	.trace_id	= coresight_etm_get_trace_id,
+	.source_ops	= &etm4_source_ops,
 };
 
 /* Setup PM. Deals with error conditions and counts */
@@ -2098,16 +2069,12 @@ static int __init etm4_pm_setup(void)
 {
 	int ret;
 
-	ret = cpu_pm_register_notifier(&etm4_cpu_pm_nb);
-	if (ret)
-		return ret;
-
 	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
 					"arm/coresight4:starting",
 					etm4_starting_cpu, etm4_dying_cpu);
 
 	if (ret)
-		goto unregister_notifier;
+		return ret;
 
 	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
 					"arm/coresight4:online",
@@ -2121,15 +2088,11 @@ static int __init etm4_pm_setup(void)
 
 	/* failed dyn state - remove others */
 	cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
-
-unregister_notifier:
-	cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
 	return ret;
 }
 
 static void etm4_pm_clear(void)
 {
-	cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
 	cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
 	if (hp_online) {
 		cpuhp_remove_state_nocalls(hp_online);
-- 
2.34.1

[PATCH v1 05/11] coresight: Save activated path into source device

[Leo Yan]

For a source device, save activated path into the coresight_device
structure.

This will be used by sequential changes for controlling the path during
CPU idle.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 10 ++++++++++
 include/linux/coresight.h                    |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 7ab7321ca8d5..8f565bb3c535 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -495,6 +495,12 @@ static void coresight_disable_path_from(struct coresight_path *path,
 
 void coresight_disable_path(struct coresight_path *path)
 {
+	struct coresight_device *source;
+
+	source = coresight_get_source(path);
+	if (coresight_is_percpu_source(source))
+		source->path = NULL;
+
 	coresight_disable_path_from(path, NULL);
 }
 EXPORT_SYMBOL_GPL(coresight_disable_path);
@@ -577,6 +583,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 		}
 	}
 
+	source = coresight_get_source(path);
+	if (coresight_is_percpu_source(source))
+		source->path = path;
+
 out:
 	return ret;
 err_disable_helpers:
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 64cee4040daa..cd9709a36b9c 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -265,6 +265,7 @@ struct coresight_trace_id_map {
  *		CS_MODE_SYSFS. Otherwise it must be accessed from inside the
  *		spinlock.
  * @orphan:	true if the component has connections that haven't been linked.
+ * @path:	activated path pointer (only used for source device).
  * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
  *		by writing a 1 to the 'enable_sink' file.  A sink can be
  *		activated but not yet enabled.  Enabling for a _sink_ happens
@@ -291,6 +292,8 @@ struct coresight_device {
 	local_t	mode;
 	int refcnt;
 	bool orphan;
+	/* activated path (for source only) */
+	struct coresight_path *path;
 	/* sink specific fields */
 	bool sysfs_sink_activated;
 	struct dev_ext_attribute *ea;
-- 
2.34.1

[PATCH v1 06/11] coresight: Control path during CPU PM

[Leo Yan]

This commit extends control over CoreSight components on an activated path
during CPU PM.

An activated path is recorded in the 'path' field of coresight_device
structure.  The patch pointer is cleared when the path is disabled.
Traverses the links on the path to disable and re-enable components during
CPU low-power modes.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 8f565bb3c535..075d485dcea5 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -596,6 +596,94 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
 	goto out;
 }
 
+static int coresight_save_path(struct coresight_path *path)
+{
+	u32 type;
+	struct coresight_node *nd;
+	struct coresight_device *source, *sink;
+
+	if (!path)
+		return 0;
+
+	source = coresight_get_source(path);
+
+	/*
+	 * Do a sanity check as the source device must have been enabled when
+	 * run at here.
+	 */
+	if (coresight_get_mode(source) == CS_MODE_DISABLED)
+		return -EINVAL;
+
+	sink = coresight_get_sink(path);
+
+	/*
+	 * Disable links. Deliberately to skip disabling the sink to avoid
+	 * latency.
+	 */
+	nd = list_first_entry(&path->path_list, struct coresight_node, link);
+	list_for_each_entry_continue(nd, &path->path_list, link) {
+		struct coresight_device *csdev, *parent, *child;
+
+		csdev = nd->csdev;
+		type = csdev->type;
+
+		/* Adjust type for LINKSINK */
+		if (type == CORESIGHT_DEV_TYPE_LINKSINK)
+			type = (csdev == sink) ? CORESIGHT_DEV_TYPE_SINK :
+						 CORESIGHT_DEV_TYPE_LINK;
+
+		if (type != CORESIGHT_DEV_TYPE_LINK)
+			continue;
+
+		parent = list_prev_entry(nd, link)->csdev;
+		child = list_next_entry(nd, link)->csdev;
+		coresight_disable_link(csdev, parent, child, source);
+	}
+
+	return 0;
+}
+
+static void coresight_restore_path(struct coresight_path *path)
+{
+	int ret;
+	u32 type;
+	struct coresight_device *source, *sink;
+	struct coresight_node *nd;
+
+	if (!path)
+		return;
+
+	source = coresight_get_source(path);
+	if (coresight_get_mode(source) == CS_MODE_DISABLED)
+		return;
+
+	sink = coresight_get_sink(path);
+
+	list_for_each_entry_reverse(nd, &path->path_list, link) {
+		struct coresight_device *csdev, *parent, *child;
+
+		csdev = nd->csdev;
+		type = csdev->type;
+
+		if (type == CORESIGHT_DEV_TYPE_LINKSINK)
+			type = (csdev == sink) ? CORESIGHT_DEV_TYPE_SINK :
+						 CORESIGHT_DEV_TYPE_LINK;
+
+		if (type != CORESIGHT_DEV_TYPE_LINK)
+			continue;
+
+		parent = list_prev_entry(nd, link)->csdev;
+		child = list_next_entry(nd, link)->csdev;
+		ret = coresight_enable_link(csdev, parent, child,
+					    coresight_get_source(path));
+		if (ret) {
+			dev_err(&csdev->dev, "Failed to restore\n");
+			coresight_disable_path_from(path, nd);
+			return;
+		}
+	}
+}
+
 struct coresight_device *coresight_get_sink(struct coresight_path *path)
 {
 	struct coresight_device *csdev;
@@ -1605,9 +1693,15 @@ static int coresight_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
 	case CPU_PM_ENTER:
 		if (coresight_save_source(source))
 			return NOTIFY_BAD;
+
+		if (coresight_save_path(source->path)) {
+			coresight_restore_source(source);
+			return NOTIFY_BAD;
+		}
 		break;
 	case CPU_PM_EXIT:
 	case CPU_PM_ENTER_FAILED:
+		coresight_restore_path(source->path);
 		coresight_restore_source(source);
 		break;
 	default:
-- 
2.34.1

[PATCH v1 07/11] coresight: Add PM callbacks in sink operation

[Leo Yan]

Unlike a system level's sink, the per-CPU sink may lose power during CPU
idle states.  So far, this refers to the TRBE as a sink.  This commit
adds save and restore callbacks for the sink, and these callbacks are
via the PM notifier.

For not per-CPU sinks (e.g., ETR, ETB, etc), which are system level
components and should not be affected by CPU low power states.  The PM
notifier does not interact with them to avoid additional latency.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 23 +++++++++++++++++++++++
 include/linux/coresight.h                    |  4 ++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 075d485dcea5..02b80db1da3d 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -437,6 +437,21 @@ static void coresight_restore_source(struct coresight_device *csdev)
 		source_ops(csdev)->restore(csdev);
 }
 
+static int coresight_save_sink(struct coresight_device *csdev)
+{
+	if (csdev && sink_ops(csdev)->save)
+		return sink_ops(csdev)->save(csdev);
+
+	/* Return success if callback is not supported */
+	return 0;
+}
+
+static void coresight_restore_sink(struct coresight_device *csdev)
+{
+	if (csdev && sink_ops(csdev)->restore)
+		sink_ops(csdev)->restore(csdev);
+}
+
 /*
  * coresight_disable_path_from : Disable components in the given path beyond
  * @nd in the list. If @nd is NULL, all the components, except the SOURCE are
@@ -616,6 +631,10 @@ static int coresight_save_path(struct coresight_path *path)
 
 	sink = coresight_get_sink(path);
 
+	/* Save per CPU sink and directly bail out */
+	if (coresight_is_percpu_sink(sink))
+		return coresight_save_sink(sink);
+
 	/*
 	 * Disable links. Deliberately to skip disabling the sink to avoid
 	 * latency.
@@ -659,6 +678,10 @@ static void coresight_restore_path(struct coresight_path *path)
 
 	sink = coresight_get_sink(path);
 
+	/* Restore per CPU sink and directly bail out */
+	if (coresight_is_percpu_sink(sink))
+		return coresight_restore_sink(sink);
+
 	list_for_each_entry_reverse(nd, &path->path_list, link) {
 		struct coresight_device *csdev, *parent, *child;
 
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index cd9709a36b9c..bffcb2238102 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -365,6 +365,8 @@ enum cs_mode {
  * @alloc_buffer:	initialises perf's ring buffer for trace collection.
  * @free_buffer:	release memory allocated in @get_config.
  * @update_buffer:	update buffer pointers after a trace session.
+ * @save:		save context for a sink.
+ * @restore:		restore context for a sink.
  */
 struct coresight_ops_sink {
 	int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
@@ -377,6 +379,8 @@ struct coresight_ops_sink {
 	unsigned long (*update_buffer)(struct coresight_device *csdev,
 			      struct perf_output_handle *handle,
 			      void *sink_config);
+	int (*save)(struct coresight_device *csdev);
+	void (*restore)(struct coresight_device *csdev);
 };
 
 /**
-- 
2.34.1

[PATCH v1 08/11] coresight: trbe: Save and restore state across CPU low power state

[Leo Yan]

From: Yabin Cui <yabinc@google.com>

Similar to ETE, TRBE may lose its context when a CPU enters low power
state. To make things worse, if ETE is restored without TRBE being
restored, an enabled source device with no enabled sink devices can
cause CPU hang on some devices (e.g., Pixel 9).

This commit adds save and restore callbacks to the TRBE driver.  During
the suspend, it stops the trace buffer unit and saves the context, and
restores the context in the resume flow.

Signed-off-by: Yabin Cui <yabinc@google.com>
Co-developed-by: Leo Yan <leo.yan@arm.com>
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 8267dd1a2130..07597cb95b74 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -115,6 +115,20 @@ static int trbe_errata_cpucaps[] = {
  */
 #define TRBE_WORKAROUND_OVERWRITE_FILL_MODE_SKIP_BYTES	256
 
+/*
+ * struct trbe_save_state: Register values representing TRBE state
+ * @trblimitr		- Trace Buffer Limit Address Register value
+ * @trbbaser		- Trace Buffer Base Register value
+ * @trbptr		- Trace Buffer Write Pointer Register value
+ * @trbsr		- Trace Buffer Status Register value
+ */
+struct trbe_save_state {
+	u64 trblimitr;
+	u64 trbbaser;
+	u64 trbptr;
+	u64 trbsr;
+};
+
 /*
  * struct trbe_cpudata: TRBE instance specific data
  * @trbe_flag		- TRBE dirty/access flag support
@@ -133,6 +147,7 @@ struct trbe_cpudata {
 	enum cs_mode mode;
 	struct trbe_buf *buf;
 	struct trbe_drvdata *drvdata;
+	struct trbe_save_state save_state;
 	DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
 };
 
@@ -1187,12 +1202,62 @@ static irqreturn_t arm_trbe_irq_handler(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static int arm_trbe_save(struct coresight_device *csdev)
+{
+	struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
+	struct trbe_save_state *state = &cpudata->save_state;
+	u64 trblimitr;
+
+	if (cpudata->mode == CS_MODE_DISABLED)
+		return 0;
+
+	state->trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
+
+	trbe_drain_buffer();
+
+	/* Disable trace buffer unit */
+	trblimitr = read_sysreg_s(SYS_TRBLIMITR_EL1);
+	trblimitr &= ~TRBLIMITR_EL1_E;
+	write_sysreg_s(trblimitr, SYS_TRBLIMITR_EL1);
+
+	if (trbe_needs_drain_after_disable(cpudata))
+		trbe_drain_buffer();
+	isb();
+
+	state->trbbaser = read_sysreg_s(SYS_TRBBASER_EL1);
+	state->trbptr = read_sysreg_s(SYS_TRBPTR_EL1);
+	state->trbsr = read_sysreg_s(SYS_TRBSR_EL1);
+	return 0;
+}
+
+static void arm_trbe_restore(struct coresight_device *csdev)
+{
+	struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev);
+	struct trbe_save_state *state = &cpudata->save_state;
+
+	if (cpudata->mode == CS_MODE_DISABLED)
+		return;
+
+	write_sysreg_s(state->trbbaser, SYS_TRBBASER_EL1);
+	write_sysreg_s(state->trbptr, SYS_TRBPTR_EL1);
+	write_sysreg_s(state->trbsr, SYS_TRBSR_EL1);
+	write_sysreg_s(state->trblimitr, SYS_TRBLIMITR_EL1);
+
+	/* Synchronize the TRBE enable event */
+	isb();
+
+	if (trbe_needs_ctxt_sync_after_enable(cpudata))
+		isb();
+}
+
 static const struct coresight_ops_sink arm_trbe_sink_ops = {
 	.enable		= arm_trbe_enable,
 	.disable	= arm_trbe_disable,
 	.alloc_buffer	= arm_trbe_alloc_buffer,
 	.free_buffer	= arm_trbe_free_buffer,
 	.update_buffer	= arm_trbe_update_buffer,
+	.save		= arm_trbe_save,
+	.restore	= arm_trbe_restore,
 };
 
 static const struct coresight_ops arm_trbe_cs_ops = {
-- 
2.34.1

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

[Leo Yan]

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();
+
 	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);
-- 
2.34.1

[PATCH v1 10/11] coresight: Move CPU hotplug callbacks to core layer

[Leo Yan]

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 invokes a high level API coresight_disable_source() to
disable a source when hotplug-off a CPU.  It disables a tracer and
changes the tracer's mode to CS_MODE_DISABLED.

When hotplug-in a CPU, if a activated path is detected - when the
activated path pointer is not NULL - in this case, the tracer will be
re-enabled.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c       | 50 +++++++++++++++++++++++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 37 -------------------------------
 2 files changed, 49 insertions(+), 38 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 02b80db1da3d..a1fb00dbdb55 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1703,6 +1703,41 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict,
 }
 EXPORT_SYMBOL_GPL(coresight_alloc_device_name);
 
+static int coresight_starting_cpu(unsigned int cpu)
+{
+	struct coresight_device *csdev = per_cpu(csdev_source, cpu);
+
+	/*
+	 * After a CPU is hotpluged-off, source will be disabled and set as
+	 * CS_MODE_DISABLED mode.  Here, if a path is activated, we need to
+	 * re-enable components on the path.
+	 */
+	if (!csdev || !csdev->path)
+		return 0;
+
+	/* Only support SYSFS mode */
+	source_ops(csdev)->enable(csdev, NULL, CS_MODE_SYSFS, csdev->path);
+	return 0;
+}
+
+static int coresight_dying_cpu(unsigned int cpu)
+{
+	struct coresight_device *csdev = per_cpu(csdev_source, cpu);
+
+	/* If a path is activated, we need to disable components on the path. */
+	if (!csdev || !csdev->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.
+	 */
+	WARN_ON(coresight_get_mode(csdev) != CS_MODE_SYSFS);
+
+	coresight_disable_source(csdev, NULL);
+	return 0;
+}
+
 static int coresight_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
 				   void *v)
 {
@@ -1740,11 +1775,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",
+					coresight_starting_cpu, 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 e5a7c0dd7f8e..cc20b63f2d35 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1735,33 +1735,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]);
-	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 int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
 {
 	int i, ret = 0;
@@ -2061,13 +2034,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);
@@ -2078,14 +2044,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;
-- 
2.34.1

[PATCH v1 11/11] coresight: Manage activated paths during CPU hotplug

[Leo Yan]

This commit handles activated paths during the CPU hotplug process.

When a CPU is hot-unplugged, and if an activated path is associated with
it, the CPU PM notifier disables the path.  This helps saving power,
especially when the CPU is expected to remain offline for long time.

Note, when disabling a path, then sink's disable() callback
automatically updates its buffer SYSFS mode.  Therefore, there is no
need to manually update the buffer.

During CPU hotplug-in, the previously active path is re-enabled
accordingly.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index a1fb00dbdb55..445908e61d8a 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1716,6 +1716,7 @@ static int coresight_starting_cpu(unsigned int cpu)
 		return 0;
 
 	/* Only support SYSFS mode */
+	coresight_enable_path(csdev->path, CS_MODE_SYSFS, NULL);
 	source_ops(csdev)->enable(csdev, NULL, CS_MODE_SYSFS, csdev->path);
 	return 0;
 }
@@ -1735,6 +1736,13 @@ static int coresight_dying_cpu(unsigned int cpu)
 	WARN_ON(coresight_get_mode(csdev) != CS_MODE_SYSFS);
 
 	coresight_disable_source(csdev, NULL);
+
+	/*
+	 * Here, it calls coresight_disable_path_from() instead of invoking
+	 * coresight_disable_path() to avoid cleaning up the path pointer
+	 * in the coresight_device structure.
+	 */
+	coresight_disable_path_from(csdev->path, NULL);
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v1 00/11] CoreSight: Refactor CPU PM and hotplug

[Yabin Cui]

Hi Leo,

Thanks for the patchset!

I have tested the patches by backporting them to Android 6.1 kernel and running
system wide ETM data collection on Pixel 9, using both ETR and TRBE.
I didn't see any cpu idle problems.

Tested-by: Yabin Cui <yabinc@google.com>




On Fri, May 16, 2025 at 9:07 AM Leo Yan <leo.yan@arm.com> wrote:
>
> Besides managing tracers (ETM) in CPU PM and hotplug flows, the
> CoreSight framework is found the issues below:
>
> Firstly, on some hardware platforms, CoreSight links (e.g., funnels and
> replicators, etc) reside in a cluster power domain.  If the cluster is
> powered off, the link components also will lose their context.  In this
> case, Arm CoreSight drivers report errors when detect unpaired self-host
> claim tags.
>
> Secondly, if a path has been activated from per CPU's tracer (ETM) to
> links and a sink in Sysfs mode, then when the CPU is hot-plugged off,
> only the associated ETM will be disabled.  Afterwards, the links and the
> sink always keep on and no chance to be disabled.
>
> The last issue was reported by Yabin Cui (Google) that the TRBE driver
> misses to save and restore context during CPU low power states.  As a
> result, it may cause hardware lockup issue on some devices.
>
> To resolve the power management issues, this series extends CPU power
> management to cover the entire activated path, including links and
> sinks.  It moves CPU PM and hotplug notifiers from the ETMv4 driver to
> the CoreSight core layer.  The core layer has sufficient info to
> maintain activated paths and can traverse the entire path to manipulate
> CoreSight modules accordingly.
>
> Patch 01 is to fix a bug in ETMv4 save and restore callbacks.
>
> Patches 02 ~ 06 move CPU PM code from ETMv4 driver to the core layer, and
> extends to maintain activated paths and control links.
>
> Patches 07 and 08 support save and restore context for per-CPU sink
> (TRBE).  Note, for avoid long latency, system level's sinks in an
> activated path are not touched during CPU suspend and resume.
>
> Patches 09 ~ 11 move CPU hotplug notifier from ETMv4 driver to the core
> layer.  The entire path will be controlled if the corresponding CPU is
> hot-plugged on or off.
>
> This series has been verified on Hikey960 for CPUIdle and hotplug.  And
> it is tested on FVP for verifying TRBE with idle states.
>
>
> Leo Yan (10):
>   coresight: etm4x: Control the trace unit in CPU suspend
>   coresight: Set per CPU source pointer
>   coresight: Register CPU PM notifier in core layer
>   coresight: etm4x: Hook CPU PM callbacks
>   coresight: Save activated path into source device
>   coresight: Control path during CPU PM
>   coresight: Add PM callbacks in sink operation
>   coresight: Take hotplug lock in enable_source_store() for Sysfs mode
>   coresight: Move CPU hotplug callbacks to core layer
>   coresight: Manage activated paths during CPU hotplug
>
> Yabin Cui (1):
>   coresight: trbe: Save and restore state across CPU low power state
>
>  drivers/hwtracing/coresight/coresight-core.c       | 252 +++++++++++++++++++++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-etm3x-core.c |   2 +
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 133 ++++++----------------
>  drivers/hwtracing/coresight/coresight-priv.h       |   1 +
>  drivers/hwtracing/coresight/coresight-sysfs.c      |  12 +-
>  drivers/hwtracing/coresight/coresight-trbe.c       |  65 +++++++++++
>  include/linux/coresight.h                          |  11 ++
>  7 files changed, 375 insertions(+), 101 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH v1 01/11] coresight: etm4x: Control the trace unit in CPU suspend

[James Clark]

On 16/05/2025 5:07 pm, Leo Yan wrote:
> If a trace unit has been enabled, a CPU suspend operation misses to
> disable the trace unit.  After the CPU resumes, it also does not follow
> the general flow for re-enabling the trace unit.  As a result, this may
> lead to a wrong state machine for the ETM.
> 
> This commit stops the trace unit during the CPU suspend flow and
> re-enables the trace unit in the resume flow.
> 
> Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states")
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 6a5898355a83..b12d59b89a49 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1820,6 +1820,11 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata)
>   	state = drvdata->save_state;
>   
>   	state->trcprgctlr = etm4x_read32(csa, TRCPRGCTLR);
> +
> +	/* Stop the trace unit if it is enabled */
> +	if (state->trcprgctlr)

etm4_cpu_save() looks at coresight_get_mode() to determine enabled 
state. Seems a bit inconsistent to look directly at the enable bit right 
after checking coresight_get_mode().

> +		etm4_disable_trace_unit(drvdata);
> +

Section 3.4.1 in ARM IHI 0064H.b doesn't say that anything extra needs 
to be done here, and it implies that restoring to an enabled state 
should work. It might make sense to mention why it diverges from the 
published sequence. Or get the sequence updated? I suppose that document 
doesn't include ETE and I see there is one extra register missing from 
__etm4_cpu_save() which is written to in etm4_disable_trace_unit().

Plus we end up doing extra things like two waits on TRCSTATR.PMSTABLE.

>   	if (drvdata->nr_pe)
>   		state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);
>   	state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);
> @@ -1951,7 +1956,6 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>   	etm4_cs_unlock(drvdata, csa);
>   	etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
>   
> -	etm4x_relaxed_write32(csa, state->trcprgctlr, TRCPRGCTLR);
>   	if (drvdata->nr_pe)
>   		etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);
>   	etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);
> @@ -2035,6 +2039,15 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>   
>   	/* Unlock the OS lock to re-enable trace and external debug access */
>   	etm4_os_unlock(drvdata);
> +
> +	/*
> +	 * Re-enable the trace unit if it was enabled before suspend.
> +	 * Put it after etm4_os_unlock() as unlocking the OS lock is the
> +	 * prerequisite for enabling the trace unit.
> +	 */
> +	if (state->trcprgctlr)
> +		etm4_enable_trace_unit(drvdata);
> +
>   	etm4_cs_lock(drvdata, csa);
>   }
>   

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

[James Clark]

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

Re: [PATCH v1 06/11] coresight: Control path during CPU PM

[James Clark]

On 16/05/2025 5:07 pm, Leo Yan wrote:
> This commit extends control over CoreSight components on an activated path
> during CPU PM.
> 
> An activated path is recorded in the 'path' field of coresight_device
> structure.  The patch pointer is cleared when the path is disabled.

patch -> path

> Traverses the links on the path to disable and re-enable components during
> CPU low-power modes.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 94 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 8f565bb3c535..075d485dcea5 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -596,6 +596,94 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>   	goto out;
>   }
>   
> +static int coresight_save_path(struct coresight_path *path)
 > +{> +	u32 type;
> +	struct coresight_node *nd;
> +	struct coresight_device *source, *sink;
> +
> +	if (!path)
> +		return 0;
> +
> +	source = coresight_get_source(path);
> +
> +	/*
> +	 * Do a sanity check as the source device must have been enabled when
> +	 * run at here.
> +	 */
> +	if (coresight_get_mode(source) == CS_MODE_DISABLED)
> +		return -EINVAL;
> +
> +	sink = coresight_get_sink(path);
> +
> +	/*
> +	 * Disable links. Deliberately to skip disabling the sink to avoid
> +	 * latency.
> +	 */
> +	nd = list_first_entry(&path->path_list, struct coresight_node, link);
> +	list_for_each_entry_continue(nd, &path->path_list, link) {
> +		struct coresight_device *csdev, *parent, *child;
> +
> +		csdev = nd->csdev;
> +		type = csdev->type;
> +
> +		/* Adjust type for LINKSINK */
> +		if (type == CORESIGHT_DEV_TYPE_LINKSINK)
> +			type = (csdev == sink) ? CORESIGHT_DEV_TYPE_SINK :
> +						 CORESIGHT_DEV_TYPE_LINK;
> +

Not sure if we want to disable helper devices too?

> +		if (type != CORESIGHT_DEV_TYPE_LINK)
> +			continue;
> +
> +		parent = list_prev_entry(nd, link)->csdev;
> +		child = list_next_entry(nd, link)->csdev;
> +		coresight_disable_link(csdev, parent, child, source);
> +	}
> +
> +	return 0;
> +}
> +
> +static void coresight_restore_path(struct coresight_path *path)
> +{
> +	int ret;
> +	u32 type;
> +	struct coresight_device *source, *sink;
> +	struct coresight_node *nd;
> +
> +	if (!path)
> +		return;
> +
> +	source = coresight_get_source(path);
> +	if (coresight_get_mode(source) == CS_MODE_DISABLED)
> +		return;
> +
> +	sink = coresight_get_sink(path);
> +
> +	list_for_each_entry_reverse(nd, &path->path_list, link) {
> +		struct coresight_device *csdev, *parent, *child;
> +
> +		csdev = nd->csdev;
> +		type = csdev->type;
> +
> +		if (type == CORESIGHT_DEV_TYPE_LINKSINK)
> +			type = (csdev == sink) ? CORESIGHT_DEV_TYPE_SINK :
> +						 CORESIGHT_DEV_TYPE_LINK;
> +
> +		if (type != CORESIGHT_DEV_TYPE_LINK)
> +			continue;
> +
> +		parent = list_prev_entry(nd, link)->csdev;
> +		child = list_next_entry(nd, link)->csdev;
> +		ret = coresight_enable_link(csdev, parent, child,
> +					    coresight_get_source(path));
> +		if (ret) {
> +			dev_err(&csdev->dev, "Failed to restore\n");
> +			coresight_disable_path_from(path, nd);
> +			return;
> +		}
> +	}
> +}
> +
>   struct coresight_device *coresight_get_sink(struct coresight_path *path)
>   {
>   	struct coresight_device *csdev;
> @@ -1605,9 +1693,15 @@ static int coresight_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
>   	case CPU_PM_ENTER:
>   		if (coresight_save_source(source))
>   			return NOTIFY_BAD;
> +
> +		if (coresight_save_path(source->path)) {
> +			coresight_restore_source(source);
> +			return NOTIFY_BAD;
> +		}
>   		break;
>   	case CPU_PM_EXIT:
>   	case CPU_PM_ENTER_FAILED:
> +		coresight_restore_path(source->path);
>   		coresight_restore_source(source);
>   		break;
>   	default:

Re: [PATCH v1 05/11] coresight: Save activated path into source device

[James Clark]

On 16/05/2025 5:07 pm, Leo Yan wrote:
> For a source device, save activated path into the coresight_device
> structure.
> 
> This will be used by sequential changes for controlling the path during
> CPU idle.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 10 ++++++++++
>   include/linux/coresight.h                    |  3 +++
>   2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 7ab7321ca8d5..8f565bb3c535 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -495,6 +495,12 @@ static void coresight_disable_path_from(struct coresight_path *path,
>   
>   void coresight_disable_path(struct coresight_path *path)
>   {
> +	struct coresight_device *source;
> +
> +	source = coresight_get_source(path);
> +	if (coresight_is_percpu_source(source))
> +		source->path = NULL;
> +
>   	coresight_disable_path_from(path, NULL);
>   }
>   EXPORT_SYMBOL_GPL(coresight_disable_path);
> @@ -577,6 +583,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>   		}
>   	}
>   
> +	source = coresight_get_source(path);
> +	if (coresight_is_percpu_source(source))
> +		source->path = path;
> +
>   out:
>   	return ret;
>   err_disable_helpers:
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 64cee4040daa..cd9709a36b9c 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -265,6 +265,7 @@ struct coresight_trace_id_map {
>    *		CS_MODE_SYSFS. Otherwise it must be accessed from inside the
>    *		spinlock.
>    * @orphan:	true if the component has connections that haven't been linked.
> + * @path:	activated path pointer (only used for source device).
>    * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
>    *		by writing a 1 to the 'enable_sink' file.  A sink can be
>    *		activated but not yet enabled.  Enabling for a _sink_ happens
> @@ -291,6 +292,8 @@ struct coresight_device {
>   	local_t	mode;
>   	int refcnt;
>   	bool orphan;
> +	/* activated path (for source only) */
> +	struct coresight_path *path;

This is probably cleaner if it's saved in the new global per-cpu you 
added in patch 2. It's even more specific than "for source only", it's 
actually only for per-cpu sources so it's not worth having it on every 
device.

>   	/* sink specific fields */
>   	bool sysfs_sink_activated;
>   	struct dev_ext_attribute *ea;

Re: [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@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);

Re: [PATCH v1 01/11] coresight: etm4x: Control the trace unit in CPU suspend

[Suzuki K Poulose]

On 03/06/2025 14:40, James Clark wrote:
> 
> 
> On 16/05/2025 5:07 pm, Leo Yan wrote:
>> If a trace unit has been enabled, a CPU suspend operation misses to
>> disable the trace unit.  After the CPU resumes, it also does not follow
>> the general flow for re-enabling the trace unit.  As a result, this may
>> lead to a wrong state machine for the ETM.
>>
>> This commit stops the trace unit during the CPU suspend flow and
>> re-enables the trace unit in the resume flow.
>>
>> Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU 
>> low power states")
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/ 
>> drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 6a5898355a83..b12d59b89a49 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -1820,6 +1820,11 @@ static int __etm4_cpu_save(struct etmv4_drvdata 
>> *drvdata)
>>       state = drvdata->save_state;
>>       state->trcprgctlr = etm4x_read32(csa, TRCPRGCTLR);
>> +
>> +    /* Stop the trace unit if it is enabled */
>> +    if (state->trcprgctlr)
> 
> etm4_cpu_save() looks at coresight_get_mode() to determine enabled 
> state. Seems a bit inconsistent to look directly at the enable bit right 
> after checking coresight_get_mode().

May be this is due to AUX pause case ? The mode could still be PERF, but
  the ETM/ETE is disabled ?

> 
>> +        etm4_disable_trace_unit(drvdata);
>> +
> 
> Section 3.4.1 in ARM IHI 0064H.b doesn't say that anything extra needs 
> to be done here, and it implies that restoring to an enabled state 
> should work. It might make sense to mention why it diverges from the 
> published sequence. Or get the sequence updated? I suppose that document 
> doesn't include ETE and I see there is one extra register missing from 
> __etm4_cpu_save() which is written to in etm4_disable_trace_unit().
> 
> Plus we end up doing extra things like two waits on TRCSTATR.PMSTABLE.

I guess, we need to stop the tracing before we reliably capture the
register states (similar to the disable/pause sequence).

Suzuki

> 
>>       if (drvdata->nr_pe)
>>           state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);
>>       state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);
>> @@ -1951,7 +1956,6 @@ static void __etm4_cpu_restore(struct 
>> etmv4_drvdata *drvdata)
>>       etm4_cs_unlock(drvdata, csa);
>>       etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
>> -    etm4x_relaxed_write32(csa, state->trcprgctlr, TRCPRGCTLR);
>>       if (drvdata->nr_pe)
>>           etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);
>>       etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);
>> @@ -2035,6 +2039,15 @@ static void __etm4_cpu_restore(struct 
>> etmv4_drvdata *drvdata)
>>       /* Unlock the OS lock to re-enable trace and external debug 
>> access */
>>       etm4_os_unlock(drvdata);
>> +
>> +    /*
>> +     * Re-enable the trace unit if it was enabled before suspend.
>> +     * Put it after etm4_os_unlock() as unlocking the OS lock is the
>> +     * prerequisite for enabling the trace unit.
>> +     */
>> +    if (state->trcprgctlr)
>> +        etm4_enable_trace_unit(drvdata);
>> +
>>       etm4_cs_lock(drvdata, csa);
>>   }
> 

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

[Suzuki K Poulose]

On 03/06/2025 15:12, James Clark wrote:
> 
> 
> 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.

+1, add it to the coresight_desc and you could use the dev.subtype to 
detect if this is per-cpu source.

Suzuki

Re: [PATCH v1 01/11] coresight: etm4x: Control the trace unit in CPU suspend

[James Clark]

On 04/06/2025 10:48 am, Suzuki K Poulose wrote:
> On 03/06/2025 14:40, James Clark wrote:
>>
>>
>> On 16/05/2025 5:07 pm, Leo Yan wrote:
>>> If a trace unit has been enabled, a CPU suspend operation misses to
>>> disable the trace unit.  After the CPU resumes, it also does not follow
>>> the general flow for re-enabling the trace unit.  As a result, this may
>>> lead to a wrong state machine for the ETM.
>>>
>>> This commit stops the trace unit during the CPU suspend flow and
>>> re-enables the trace unit in the resume flow.
>>>
>>> Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU 
>>> low power states")
>>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 15 +++++++++++ 
>>> +++-
>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/ 
>>> drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index 6a5898355a83..b12d59b89a49 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -1820,6 +1820,11 @@ static int __etm4_cpu_save(struct 
>>> etmv4_drvdata *drvdata)
>>>       state = drvdata->save_state;
>>>       state->trcprgctlr = etm4x_read32(csa, TRCPRGCTLR);
>>> +
>>> +    /* Stop the trace unit if it is enabled */
>>> +    if (state->trcprgctlr)
>>
>> etm4_cpu_save() looks at coresight_get_mode() to determine enabled 
>> state. Seems a bit inconsistent to look directly at the enable bit 
>> right after checking coresight_get_mode().
> 
> May be this is due to AUX pause case ? The mode could still be PERF, but
>   the ETM/ETE is disabled ?
> 

Disabling unconditionally would be harmless, and that's quite a bit of 
an edge case so it might be worth the simplification.

Then __etm4_cpu_save() could remove the duplicate wait on 
TRCSTATR_PMSTABLE_BIT and multiple reads of TRCPRGCTLR. There are other 
inconsistencies like one does etm4x_relaxed_read32(csa, TRCPRGCTLR) and 
the other does etm4x_read32(csa, TRCPRGCTLR).


>>
>>> +        etm4_disable_trace_unit(drvdata);
>>> +
>>
>> Section 3.4.1 in ARM IHI 0064H.b doesn't say that anything extra needs 
>> to be done here, and it implies that restoring to an enabled state 
>> should work. It might make sense to mention why it diverges from the 
>> published sequence. Or get the sequence updated? I suppose that 
>> document doesn't include ETE and I see there is one extra register 
>> missing from __etm4_cpu_save() which is written to in 
>> etm4_disable_trace_unit().
>>
>> Plus we end up doing extra things like two waits on TRCSTATR.PMSTABLE.
> 
> I guess, we need to stop the tracing before we reliably capture the
> register states (similar to the disable/pause sequence).
> 

Is that because the counters and comparators are changing? I'm not sure 
why we need to read everything anyway, etm4_enable_hw() is done entirely 
from drvdata, implying that everything we need is already saved.

If there are only two things that might be different to drvdata 
(TRCSSCSRn and TRCCNTVRn) we could only save those in __etm4_cpu_save(). 
But then the function looks an awful lot like etm4_disable_hw() and I 
wonder if there isn't a way to share the code to show we're doing 
basically the same thing.

There are a few other small inconsistencies between disabling and saving 
like etm4_disable_hw() removes power first and __etm4_cpu_save() does it 
last. And __etm4_cpu_restore() doesn't follow the correct sequence to 
write to TRCCLAIMSET.

Could it look something like this:

void __etm4_cpu_save()
{
   etm4_disable_hw(drvdata);
   drvdata->state_needs_restore = true;
}


void __etm4_cpu_restore()
{
   etm4_enable_hw(drvdata, false);
   drvdata->state_needs_restore = false;
}

Where the extra argument for enable stops clearing the comparator status:

void etm4_enable_hw(struct etmv4_drvdata *drvdata, bool clear_cmp)
{
   /* clear status bit on restart if using single-shot */
   if (clear_cmp && (config->ss_ctrl[i] || config->ss_pe_cmp[i]))
      config->ss_status[i] &= ~TRCSSCSRn_STATUS;


> Suzuki
> 
>>
>>>       if (drvdata->nr_pe)
>>>           state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);
>>>       state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);
>>> @@ -1951,7 +1956,6 @@ static void __etm4_cpu_restore(struct 
>>> etmv4_drvdata *drvdata)
>>>       etm4_cs_unlock(drvdata, csa);
>>>       etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
>>> -    etm4x_relaxed_write32(csa, state->trcprgctlr, TRCPRGCTLR);
>>>       if (drvdata->nr_pe)
>>>           etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);
>>>       etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);
>>> @@ -2035,6 +2039,15 @@ static void __etm4_cpu_restore(struct 
>>> etmv4_drvdata *drvdata)
>>>       /* Unlock the OS lock to re-enable trace and external debug 
>>> access */
>>>       etm4_os_unlock(drvdata);
>>> +
>>> +    /*
>>> +     * Re-enable the trace unit if it was enabled before suspend.
>>> +     * Put it after etm4_os_unlock() as unlocking the OS lock is the
>>> +     * prerequisite for enabling the trace unit.
>>> +     */
>>> +    if (state->trcprgctlr)
>>> +        etm4_enable_trace_unit(drvdata);
>>> +
>>>       etm4_cs_lock(drvdata, csa);
>>>   }
>>
> 

Re: [PATCH v1 00/11] CoreSight: Refactor CPU PM and hotplug

[James Clark]

On 16/05/2025 5:07 pm, Leo Yan wrote:
> Besides managing tracers (ETM) in CPU PM and hotplug flows, the
> CoreSight framework is found the issues below:
> 
> Firstly, on some hardware platforms, CoreSight links (e.g., funnels and
> replicators, etc) reside in a cluster power domain.  If the cluster is
> powered off, the link components also will lose their context.  In this
> case, Arm CoreSight drivers report errors when detect unpaired self-host
> claim tags.
> 
> Secondly, if a path has been activated from per CPU's tracer (ETM) to
> links and a sink in Sysfs mode, then when the CPU is hot-plugged off,
> only the associated ETM will be disabled.  Afterwards, the links and the
> sink always keep on and no chance to be disabled.
> 
> The last issue was reported by Yabin Cui (Google) that the TRBE driver
> misses to save and restore context during CPU low power states.  As a
> result, it may cause hardware lockup issue on some devices.
> 
> To resolve the power management issues, this series extends CPU power
> management to cover the entire activated path, including links and
> sinks.  It moves CPU PM and hotplug notifiers from the ETMv4 driver to
> the CoreSight core layer.  The core layer has sufficient info to
> maintain activated paths and can traverse the entire path to manipulate
> CoreSight modules accordingly.
> 
> Patch 01 is to fix a bug in ETMv4 save and restore callbacks.
> 
> Patches 02 ~ 06 move CPU PM code from ETMv4 driver to the core layer, and
> extends to maintain activated paths and control links.
> 
> Patches 07 and 08 support save and restore context for per-CPU sink
> (TRBE).  Note, for avoid long latency, system level's sinks in an
> activated path are not touched during CPU suspend and resume.
> 
> Patches 09 ~ 11 move CPU hotplug notifier from ETMv4 driver to the core
> layer.  The entire path will be controlled if the corresponding CPU is
> hot-plugged on or off.
> 
> This series has been verified on Hikey960 for CPUIdle and hotplug.  And
> it is tested on FVP for verifying TRBE with idle states.
> 
> 

Hi Leo,

I ran this stress test on Juno by enabling and disabling concurrently 
with no sleeps:

   # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
   # while true; do \
         echo 0 > /sys/devices/system/cpu/cpu2/online; \
         echo 1 > /sys/devices/system/cpu/cpu2/online; \
         done &

   # while true; do \
         echo 1 > /sys/bus/coresight/devices/etm2/enable_source; \
         echo 0 > /sys/bus/coresight/devices/etm2/enable_source; \
         done &

I get lots of these:

   coresight tmc_etr0: timeout while waiting for TMC to be Ready
   coresight tmc_etr0: Failed to enable : TMC not ready

And then even after disabling the source and sink the Perf tests don't 
pass anymore:

   # perf test -F -vvv "arm coresight trace"
   --- start ---
   Recording trace (only user mode) with path: CPU0 => tmc_etf0
   Looking at perf.data file for dumping branch samples:
   CoreSight path testing (CPU0 -> tmc_etf0): FAIL

I suppose it's possible this isn't entirely related to your changes, and 
I know we've seen some of those timeout issues before. But it's probably 
worth investigating.

Thanks
James

Re: [PATCH v1 00/11] CoreSight: Refactor CPU PM and hotplug

[Leo Yan]

On Thu, Jun 05, 2025 at 02:50:41PM +0100, James Clark wrote:

[...]

> Hi Leo,
> 
> I ran this stress test on Juno by enabling and disabling concurrently with
> no sleeps:
> 
>   # echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
>   # while true; do \
>         echo 0 > /sys/devices/system/cpu/cpu2/online; \
>         echo 1 > /sys/devices/system/cpu/cpu2/online; \
>         done &
> 
>   # while true; do \
>         echo 1 > /sys/bus/coresight/devices/etm2/enable_source; \
>         echo 0 > /sys/bus/coresight/devices/etm2/enable_source; \
>         done &
> 
> I get lots of these:
> 
>   coresight tmc_etr0: timeout while waiting for TMC to be Ready
>   coresight tmc_etr0: Failed to enable : TMC not ready
> 
> And then even after disabling the source and sink the Perf tests don't pass
> anymore:
> 
>   # perf test -F -vvv "arm coresight trace"
>   --- start ---
>   Recording trace (only user mode) with path: CPU0 => tmc_etf0
>   Looking at perf.data file for dumping branch samples:
>   CoreSight path testing (CPU0 -> tmc_etf0): FAIL
> 
> I suppose it's possible this isn't entirely related to your changes, and I
> know we've seen some of those timeout issues before. But it's probably worth
> investigating.

Thanks a lot for testing!  I will look into the issue.

Leo

Re: [PATCH v1 01/11] coresight: etm4x: Control the trace unit in CPU suspend

[Leo Yan]

On Wed, Jun 04, 2025 at 11:50:44AM +0100, James Clark wrote:
> On 04/06/2025 10:48 am, Suzuki K Poulose wrote:

[...]

> > > > @@ -1820,6 +1820,11 @@ static int __etm4_cpu_save(struct
> > > > etmv4_drvdata *drvdata)
> > > >       state = drvdata->save_state;
> > > >       state->trcprgctlr = etm4x_read32(csa, TRCPRGCTLR);
> > > > +
> > > > +    /* Stop the trace unit if it is enabled */
> > > > +    if (state->trcprgctlr)
> > > 
> > > etm4_cpu_save() looks at coresight_get_mode() to determine enabled
> > > state. Seems a bit inconsistent to look directly at the enable bit
> > > right after checking coresight_get_mode().
> > 
> > May be this is due to AUX pause case ? The mode could still be PERF, but
> >   the ETM/ETE is disabled ?

Yes. It can be a case that device mode = PERF but the trace unit is
disabled.

> Disabling unconditionally would be harmless, and that's quite a bit of an
> edge case so it might be worth the simplification.

In new version, I will encapsulate most memory barriers and polling into
the trace unit enabling and disable functions.

So if the trace unit is disabled during the CPU idle flow, we can skip
to call the trace unit disabling and enabling, which can avoid the
latency caused by barriers and polling.

> Then __etm4_cpu_save() could remove the duplicate wait on
> TRCSTATR_PMSTABLE_BIT and multiple reads of TRCPRGCTLR. There are other
> inconsistencies like one does etm4x_relaxed_read32(csa, TRCPRGCTLR) and the
> other does etm4x_read32(csa, TRCPRGCTLR).

In __etm4_cpu_save(), tt is a typo for the second polling on
TRCSTATR_PMSTABLE_BIT. Actually it should be IDLE bit. This bug will
be fixed in the new series.

Regarding etm4x_read32() vs etm4x_relaxed_read32(), given the
registers are mapped as Device-nGnRnE, all readings are in order, and
the critical synchronization should have been covered in
etm4_disable_trace_unit(), so I think __etm4_cpu_save() should use
relaxed APIs for all readings.

> > > > +        etm4_disable_trace_unit(drvdata);
> > > > +
> > > 
> > > Section 3.4.1 in ARM IHI 0064H.b doesn't say that anything extra
> > > needs to be done here, and it implies that restoring to an enabled
> > > state should work. It might make sense to mention why it diverges
> > > from the published sequence. Or get the sequence updated?

For example, for ETE and TRF, the driver should firstly disable filter,
and then disable trace unit.

On old ETM IPs, the document suggests to acquire OS Lock to implicitly
disable the trace unit.

We need to consolidate the flow to support all these cases. Simply to
say, in the new patch, I will firstly disable filter and trace unit,
and grab the OS Lock. This can be compatible for all ETM versions. And
I assume anyway we need OS Lock to prevent external debugger's
connection.

> > > I suppose
> > > that document doesn't include ETE and I see there is one extra
> > > register missing from __etm4_cpu_save() which is written to in
> > > etm4_disable_trace_unit().

The section K5.5 Context switching, Arm ARM (ARM DDI 0487 L.a) gives
details for context switching for ETE and TRBE.

> > > Plus we end up doing extra things like two waits on TRCSTATR.PMSTABLE.

This follows section 3.4.1 "The procedure when powering down the PE" of
ARM IHI0064H.b. As said, it is a typo for second polling on
TRCSTATR.PMSTABLE, it should be IDLE bit.

> > I guess, we need to stop the tracing before we reliably capture the
> > register states (similar to the disable/pause sequence).
> > 
> 
> Is that because the counters and comparators are changing? I'm not sure why
> we need to read everything anyway, etm4_enable_hw() is done entirely from
> drvdata, implying that everything we need is already saved.

The register context is defined in section 3.4.4 "Guidelines for trace
unit registers to be saved and restored", ARM IHI0064H.b. We can see
the driver exactly follows up the guidance for saving and restoring
registers.

> If there are only two things that might be different to drvdata (TRCSSCSRn
> and TRCCNTVRn) we could only save those in __etm4_cpu_save(). But then the
> function looks an awful lot like etm4_disable_hw() and I wonder if there
> isn't a way to share the code to show we're doing basically the same thing.
> 
> There are a few other small inconsistencies between disabling and saving
> like etm4_disable_hw() removes power first and __etm4_cpu_save() does it
> last. And __etm4_cpu_restore() doesn't follow the correct sequence to write
> to TRCCLAIMSET.

__etm4_cpu_save() and __etm4_cpu_restore() follow up the document for
operating CLAIM bit. See section 3.4.4 of ARM IHI0064H.b.

I am not sure if there have hole that if driver clears the CLAIM bit
before it is powered down. Seems to me, it is more safe to leave the
hardware to clear the claim bit when it power on the CPU.

Moreover, we can see the OS Lock is locked during CPU idle, this
differs from the enabling / disable flows.

> Could it look something like this:
> 
> void __etm4_cpu_save()
> {
>   etm4_disable_hw(drvdata);
>   drvdata->state_needs_restore = true;
> }
> 
> 
> void __etm4_cpu_restore()
> {
>   etm4_enable_hw(drvdata, false);
>   drvdata->state_needs_restore = false;
> }

Good point. This is a refactoring, except we need to take care each
configuration setting, we also need to consolidate:

- OS Lock;
- Claim bit;
- PowerUp request (TRCPDCR setting).

I have another patch series for refactoring PowerUp request and
pm_save_enable. So I would defer the refactoring in that series.

Thanks,
Leo

> Where the extra argument for enable stops clearing the comparator status:
> 
> void etm4_enable_hw(struct etmv4_drvdata *drvdata, bool clear_cmp)
> {
>   /* clear status bit on restart if using single-shot */
>   if (clear_cmp && (config->ss_ctrl[i] || config->ss_pe_cmp[i]))
>      config->ss_status[i] &= ~TRCSSCSRn_STATUS;

> 
> 
> > Suzuki
> > 
> > > 
> > > >       if (drvdata->nr_pe)
> > > >           state->trcprocselr = etm4x_read32(csa, TRCPROCSELR);
> > > >       state->trcconfigr = etm4x_read32(csa, TRCCONFIGR);
> > > > @@ -1951,7 +1956,6 @@ static void __etm4_cpu_restore(struct
> > > > etmv4_drvdata *drvdata)
> > > >       etm4_cs_unlock(drvdata, csa);
> > > >       etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET);
> > > > -    etm4x_relaxed_write32(csa, state->trcprgctlr, TRCPRGCTLR);
> > > >       if (drvdata->nr_pe)
> > > >           etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR);
> > > >       etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR);
> > > > @@ -2035,6 +2039,15 @@ static void __etm4_cpu_restore(struct
> > > > etmv4_drvdata *drvdata)
> > > >       /* Unlock the OS lock to re-enable trace and external
> > > > debug access */
> > > >       etm4_os_unlock(drvdata);
> > > > +
> > > > +    /*
> > > > +     * Re-enable the trace unit if it was enabled before suspend.
> > > > +     * Put it after etm4_os_unlock() as unlocking the OS lock is the
> > > > +     * prerequisite for enabling the trace unit.
> > > > +     */
> > > > +    if (state->trcprgctlr)
> > > > +        etm4_enable_trace_unit(drvdata);
> > > > +
> > > >       etm4_cs_lock(drvdata, csa);
> > > >   }
> > > 
> > 
> 

Re: [PATCH v1 05/11] coresight: Save activated path into source device

[Leo Yan]

On Tue, Jun 03, 2025 at 03:49:26PM +0100, James Clark wrote:

[...]

> > @@ -577,6 +583,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >   		}
> >   	}
> > +	source = coresight_get_source(path);
> > +	if (coresight_is_percpu_source(source))
> > +		source->path = path;
> > +
> >   out:
> >   	return ret;
> >   err_disable_helpers:
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index 64cee4040daa..cd9709a36b9c 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -265,6 +265,7 @@ struct coresight_trace_id_map {
> >    *		CS_MODE_SYSFS. Otherwise it must be accessed from inside the
> >    *		spinlock.
> >    * @orphan:	true if the component has connections that haven't been linked.
> > + * @path:	activated path pointer (only used for source device).
> >    * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
> >    *		by writing a 1 to the 'enable_sink' file.  A sink can be
> >    *		activated but not yet enabled.  Enabling for a _sink_ happens
> > @@ -291,6 +292,8 @@ struct coresight_device {
> >   	local_t	mode;
> >   	int refcnt;
> >   	bool orphan;
> > +	/* activated path (for source only) */
> > +	struct coresight_path *path;
> 
> This is probably cleaner if it's saved in the new global per-cpu you added
> in patch 2. It's even more specific than "for source only", it's actually
> only for per-cpu sources so it's not worth having it on every device.

We can maintain Coresight path pointers in a global per-CPU data
structure. These path pointers are set and cleared during path enable
and disable operations, respectively. Idle threads can then access
these pointers for power management purposes.

However, the challenge is how we can safely access the path pointer from
an idle thread - particularly because idle threads cannot acquire
coresight_mutex, which is a sleepable lock. Consider a race condition
scenario where SysFS knobs frequently enable and disable paths, while
the corresponding CPUs enter and exit idle states. In such cases, if
an idle thread attempts to access the path pointers, we would need to
use raw spinlocks to ensure safety. This is problematic, as it could
result in the CPU suspend/resume flow waiting on SysFS operations.

This is why it turns out to use lockless approach. Two key background
info:

- Path pointers are not guaranteed to be safe to access in CPU power
  management (PM) notifiers. In contrast, the coresight_device pointer
  of a source device (e.g., an ETM) is safe to access.

  A path's lifecycle begins and ends with SysFS enable and disable
  operations. OTOH, an ETM’s coresight_device pointer is initialized
  during the probe phase and cleared during module removal. Module
  loading and unloading trigger SMP calls for initialization and
  cleanup. These synchronized SMP calls ensure that it is safe for CPU
  PM notifiers to access the source device structure.

- Another factor is that the device mode is used to determine whether
  it is safe to access the path pointer. When a source device is
  enabled, we must ensure that its state transitions from DISABLED to
  an enabled state on the target CPU. This guarantees that the mode,
  and any subsequent operations dependent on it, follow a strictly
  sequential order.

  CPU0                                    CPU1
  etm4_enable()
   ` etm4_enable_sysfs()
      ` smp_call_function_single() ---->  etm4_enable_hw_smp_call()
                                           ` coresight_take_mode(SYSFS)

                                           CPU idle:
                                           etm4_cpu_save()
                                            ` coresight_get_mode()
                                               ^^^
                                               Read out the SYSFS mode
                                               Safely access coresight_device::path

Therefore, safe access to the Coresight path heavily relies on the
mode (as part of the state machine). This is why the patch adds the
path pointer to the same structure as the mode.

Thanks,
Leo

> >   	/* sink specific fields */
> >   	bool sysfs_sink_activated;
> >   	struct dev_ext_attribute *ea;
> 

Re: [PATCH v1 05/11] coresight: Save activated path into source device

[James Clark]

On 20/06/2025 3:24 pm, Leo Yan wrote:
> On Tue, Jun 03, 2025 at 03:49:26PM +0100, James Clark wrote:
> 
> [...]
> 
>>> @@ -577,6 +583,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>>>    		}
>>>    	}
>>> +	source = coresight_get_source(path);
>>> +	if (coresight_is_percpu_source(source))
>>> +		source->path = path;
>>> +
>>>    out:
>>>    	return ret;
>>>    err_disable_helpers:
>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>> index 64cee4040daa..cd9709a36b9c 100644
>>> --- a/include/linux/coresight.h
>>> +++ b/include/linux/coresight.h
>>> @@ -265,6 +265,7 @@ struct coresight_trace_id_map {
>>>     *		CS_MODE_SYSFS. Otherwise it must be accessed from inside the
>>>     *		spinlock.
>>>     * @orphan:	true if the component has connections that haven't been linked.
>>> + * @path:	activated path pointer (only used for source device).
>>>     * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
>>>     *		by writing a 1 to the 'enable_sink' file.  A sink can be
>>>     *		activated but not yet enabled.  Enabling for a _sink_ happens
>>> @@ -291,6 +292,8 @@ struct coresight_device {
>>>    	local_t	mode;
>>>    	int refcnt;
>>>    	bool orphan;
>>> +	/* activated path (for source only) */
>>> +	struct coresight_path *path;
>>
>> This is probably cleaner if it's saved in the new global per-cpu you added
>> in patch 2. It's even more specific than "for source only", it's actually
>> only for per-cpu sources so it's not worth having it on every device.
> 
> We can maintain Coresight path pointers in a global per-CPU data
> structure. These path pointers are set and cleared during path enable
> and disable operations, respectively. Idle threads can then access
> these pointers for power management purposes.
> 
> However, the challenge is how we can safely access the path pointer from
> an idle thread - particularly because idle threads cannot acquire
> coresight_mutex, which is a sleepable lock. Consider a race condition
> scenario where SysFS knobs frequently enable and disable paths, while
> the corresponding CPUs enter and exit idle states. In such cases, if
> an idle thread attempts to access the path pointers, we would need to
> use raw spinlocks to ensure safety. This is problematic, as it could
> result in the CPU suspend/resume flow waiting on SysFS operations.
> 
> This is why it turns out to use lockless approach. Two key background
> info:
> 
> - Path pointers are not guaranteed to be safe to access in CPU power
>    management (PM) notifiers. In contrast, the coresight_device pointer
>    of a source device (e.g., an ETM) is safe to access.
> 
>    A path's lifecycle begins and ends with SysFS enable and disable
>    operations. OTOH, an ETM’s coresight_device pointer is initialized
>    during the probe phase and cleared during module removal. Module
>    loading and unloading trigger SMP calls for initialization and
>    cleanup. These synchronized SMP calls ensure that it is safe for CPU
>    PM notifiers to access the source device structure.
> 
> - Another factor is that the device mode is used to determine whether
>    it is safe to access the path pointer. When a source device is
>    enabled, we must ensure that its state transitions from DISABLED to
>    an enabled state on the target CPU. This guarantees that the mode,
>    and any subsequent operations dependent on it, follow a strictly
>    sequential order.
> 
>    CPU0                                    CPU1
>    etm4_enable()
>     ` etm4_enable_sysfs()
>        ` smp_call_function_single() ---->  etm4_enable_hw_smp_call()
>                                             ` coresight_take_mode(SYSFS)
> 
>                                             CPU idle:
>                                             etm4_cpu_save()
>                                              ` coresight_get_mode()
>                                                 ^^^
>                                                 Read out the SYSFS mode
>                                                 Safely access coresight_device::path
> 
> Therefore, safe access to the Coresight path heavily relies on the
> mode (as part of the state machine). This is why the patch adds the
> path pointer to the same structure as the mode.
> 
> Thanks,
> Leo
> 

I'm not suggesting to change the flow for accessing it, only where it's 
stored to not waste space storing it for devices that never need it. It 
also makes the code a bit more readable because you know that variable 
is never accessed for other device types by the fact that it doesn't exist.

Because we only have 1 per-CPU source for each CPU, there is no 
functional difference between storing it in the device or in a global 
per-cpu area.

If we imagine that the path pointer saved in the device is actually a 
pointer to a pointer, and that pointer happens to point to the per-CPU area,

  struct coresight_device {
     struct coresight_path **path;  ---------------------
  }                                                      |
                                                         |
                                                        \ /
  static DEFINE_PER_CPU(struct coresight_device *, csdev_cpu_path);

then the argument for safely accessing doesn't really make sense, it's 
just a stored somewhere else. Moving it doesn't add or remove any safety 
or concurrency issues, it just has a different address than the one in 
the struct coresight_device and looks a bit different in the code.


>>>    	/* sink specific fields */
>>>    	bool sysfs_sink_activated;
>>>    	struct dev_ext_attribute *ea;
>>