[PATCH 0/4] small fix for configuaring etm csdev via sysfs

[PATCH 0/4] small fix for configuaring etm csdev via sysfs

[Yeoreum Yun]

when etm csdev's configuration is modified via sysfs while etm is being
enabled via perf, enabled etm could run with different configuration
from perf_event.

Also, there are some redeundant usage of drvdata->spinlock in
etm_starting/dying_cpu() with enable/disable etm device.

This patchset fixes above small problems.

NOTE:
   This patchset based on:
     - https://lore.kernel.org/linux-arm-kernel/20241220104521.809071-1-yeoreum.yun@arm.com/

Yeoreum Yun (4):
  coresight/etm4x: disallow altering config via sysfs while enabled
  coresight/etm4x: remove redundant usage of drvdata->spinlock
  coresight/etm3x: disallow altering config via sysfs while enabled
  coresight/etm3x: remove redundant usage of drvdata->spinlock

 .../coresight/coresight-etm3x-core.c          |  33 ++---
 .../coresight/coresight-etm3x-sysfs.c         | 120 ++++++++++++++++
 .../coresight/coresight-etm4x-core.c          |  20 +--
 .../coresight/coresight-etm4x-sysfs.c         | 132 +++++++++++++++++-
 4 files changed, 272 insertions(+), 33 deletions(-)

--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Yeoreum Yun]

When etm4x configuration is modified via sysfs while etm4x is being
enabled via perf, enabled etm4x could run with different configuration
from perf_event.

To address this, disallow altering config via sysfs while csdev is enabled.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 .../coresight/coresight-etm4x-sysfs.c         | 132 +++++++++++++++++-
 1 file changed, 128 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index 11e865b8e824..cc1f112921d7 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -174,6 +174,9 @@ static ssize_t reset_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	if (val)
 		config->mode = 0x0;
@@ -300,6 +303,9 @@ static ssize_t mode_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	config->mode = val & ETMv4_MODE_ALL;
 
@@ -466,6 +472,9 @@ static ssize_t pe_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	if (val > drvdata->nr_pe) {
 		raw_spin_unlock(&drvdata->spinlock);
@@ -501,6 +510,9 @@ static ssize_t event_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	switch (drvdata->nr_event) {
 	case 0x0:
@@ -550,6 +562,9 @@ static ssize_t event_instren_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	/* start by clearing all instruction event enable bits */
 	config->eventctrl1 &= ~TRCEVENTCTL1R_INSTEN_MASK;
@@ -608,6 +623,9 @@ static ssize_t event_ts_store(struct device *dev,
 	if (!drvdata->ts_size)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	config->ts_ctrl = val & ETMv4_EVENT_MASK;
 	return size;
 }
@@ -638,6 +656,9 @@ static ssize_t syncfreq_store(struct device *dev,
 	if (drvdata->syncpr == true)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	config->syncfreq = val & ETMv4_SYNC_MASK;
 	return size;
 }
@@ -666,6 +687,9 @@ static ssize_t cyc_threshold_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/* mask off max threshold before checking min value */
 	val &= ETM_CYC_THRESHOLD_MASK;
 	if (val < drvdata->ccitmin)
@@ -703,6 +727,9 @@ static ssize_t bb_ctrl_store(struct device *dev,
 	if (!drvdata->nr_addr_cmp)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Bit[8] controls include(1) / exclude(0), bits[0-7] select
 	 * individual range comparators. If include then at least 1
@@ -739,6 +766,9 @@ static ssize_t event_vinst_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	val &= TRCVICTLR_EVENT_MASK >> __bf_shf(TRCVICTLR_EVENT_MASK);
 	config->vinst_ctrl &= ~TRCVICTLR_EVENT_MASK;
@@ -771,6 +801,9 @@ static ssize_t s_exlevel_vinst_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	/* clear all EXLEVEL_S bits  */
 	config->vinst_ctrl &= ~TRCVICTLR_EXLEVEL_S_MASK;
@@ -806,6 +839,9 @@ static ssize_t ns_exlevel_vinst_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	/* clear EXLEVEL_NS bits  */
 	config->vinst_ctrl &= ~TRCVICTLR_EXLEVEL_NS_MASK;
@@ -842,6 +878,9 @@ static ssize_t addr_idx_store(struct device *dev,
 	if (val >= drvdata->nr_addr_cmp * 2)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -888,6 +927,9 @@ static ssize_t addr_instdatatype_store(struct device *dev,
 	if (sscanf(buf, "%s", str) != 1)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!strcmp(str, "instr"))
@@ -913,7 +955,7 @@ static ssize_t addr_single_show(struct device *dev,
 	if (!(config->addr_type[idx] == ETM_ADDR_TYPE_NONE ||
 	      config->addr_type[idx] == ETM_ADDR_TYPE_SINGLE)) {
 		raw_spin_unlock(&drvdata->spinlock);
-		return -EPERM;
+		return -EBUSY;
 	}
 	val = (unsigned long)config->addr_val[idx];
 	raw_spin_unlock(&drvdata->spinlock);
@@ -932,12 +974,15 @@ static ssize_t addr_single_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!(config->addr_type[idx] == ETM_ADDR_TYPE_NONE ||
 	      config->addr_type[idx] == ETM_ADDR_TYPE_SINGLE)) {
 		raw_spin_unlock(&drvdata->spinlock);
-		return -EPERM;
+		return -EBUSY;
 	}
 
 	config->addr_val[idx] = (u64)val;
@@ -960,14 +1005,14 @@ static ssize_t addr_range_show(struct device *dev,
 	idx = config->addr_idx;
 	if (idx % 2 != 0) {
 		raw_spin_unlock(&drvdata->spinlock);
-		return -EPERM;
+		return -EBUSY;
 	}
 	if (!((config->addr_type[idx] == ETM_ADDR_TYPE_NONE &&
 	       config->addr_type[idx + 1] == ETM_ADDR_TYPE_NONE) ||
 	      (config->addr_type[idx] == ETM_ADDR_TYPE_RANGE &&
 	       config->addr_type[idx + 1] == ETM_ADDR_TYPE_RANGE))) {
 		raw_spin_unlock(&drvdata->spinlock);
-		return -EPERM;
+		return -EBUSY;
 	}
 
 	val1 = (unsigned long)config->addr_val[idx];
@@ -995,6 +1040,9 @@ static ssize_t addr_range_store(struct device *dev,
 	if (val1 > val2)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (idx % 2 != 0) {
@@ -1063,6 +1111,9 @@ static ssize_t addr_start_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!drvdata->nr_addr_cmp) {
@@ -1118,6 +1169,9 @@ static ssize_t addr_stop_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!drvdata->nr_addr_cmp) {
@@ -1172,6 +1226,9 @@ static ssize_t addr_ctxtype_store(struct device *dev,
 	if (sscanf(buf, "%s", str) != 1)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!strcmp(str, "none"))
@@ -1238,6 +1295,9 @@ static ssize_t addr_context_store(struct device *dev,
 		     drvdata->numcidc : drvdata->numvmidc))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	/* clear context ID comparator bits[6:4] */
@@ -1276,6 +1336,9 @@ static ssize_t addr_exlevel_s_ns_store(struct device *dev,
 	if (kstrtoul(buf, 0, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	if (val & ~(TRCACATRn_EXLEVEL_MASK >> __bf_shf(TRCACATRn_EXLEVEL_MASK)))
 		return -EINVAL;
 
@@ -1366,6 +1429,9 @@ static ssize_t vinst_pe_cmp_start_stop_store(struct device *dev,
 	if (!drvdata->nr_pe_cmp)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	config->vipcssctlr = val;
 	raw_spin_unlock(&drvdata->spinlock);
@@ -1398,6 +1464,9 @@ static ssize_t seq_idx_store(struct device *dev,
 	if (val >= drvdata->nrseqstate - 1)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -1434,6 +1503,9 @@ static ssize_t seq_state_store(struct device *dev,
 	if (val >= drvdata->nrseqstate)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	config->seq_state = val;
 	return size;
 }
@@ -1467,6 +1539,9 @@ static ssize_t seq_event_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->seq_idx;
 	/* Seq control has two masks B[15:8] F[7:0] */
@@ -1501,6 +1576,9 @@ static ssize_t seq_reset_event_store(struct device *dev,
 	if (!(drvdata->nrseqstate))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	config->seq_rst = val & ETMv4_EVENT_MASK;
 	return size;
 }
@@ -1531,6 +1609,9 @@ static ssize_t cntr_idx_store(struct device *dev,
 	if (val >= drvdata->nr_cntr)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -1572,6 +1653,9 @@ static ssize_t cntrldvr_store(struct device *dev,
 	if (val > ETM_CNTR_MAX_VAL)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->cntr_idx;
 	config->cntrldvr[idx] = val;
@@ -1610,6 +1694,9 @@ static ssize_t cntr_val_store(struct device *dev,
 	if (val > ETM_CNTR_MAX_VAL)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->cntr_idx;
 	config->cntr_val[idx] = val;
@@ -1646,6 +1733,9 @@ static ssize_t cntr_ctrl_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->cntr_idx;
 	config->cntr_ctrl[idx] = val;
@@ -1676,6 +1766,10 @@ static ssize_t res_idx_store(struct device *dev,
 
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
+
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Resource selector pair 0 is always implemented and reserved,
 	 * namely an idx with 0 and 1 is illegal.
@@ -1722,6 +1816,9 @@ static ssize_t res_ctrl_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->res_idx;
 	/* For odd idx pair inversal bit is RES0 */
@@ -1761,6 +1858,9 @@ static ssize_t sshot_idx_store(struct device *dev,
 	if (val >= drvdata->nr_ss_cmp)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	config->ss_idx = val;
 	raw_spin_unlock(&drvdata->spinlock);
@@ -1794,6 +1894,9 @@ static ssize_t sshot_ctrl_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->ss_idx;
 	config->ss_ctrl[idx] = FIELD_PREP(TRCSSCCRn_SAC_ARC_RST_MASK, val);
@@ -1844,6 +1947,9 @@ static ssize_t sshot_pe_ctrl_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->ss_idx;
 	config->ss_pe_cmp[idx] = FIELD_PREP(TRCSSPCICRn_PC_MASK, val);
@@ -1879,6 +1985,9 @@ static ssize_t ctxid_idx_store(struct device *dev,
 	if (val >= drvdata->numcidc)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -1944,6 +2053,9 @@ static ssize_t ctxid_pid_store(struct device *dev,
 	if (kstrtoul(buf, 16, &pid))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	idx = config->ctxid_idx;
 	config->ctxid_pid[idx] = (u64)pid;
@@ -2003,6 +2115,9 @@ static ssize_t ctxid_masks_store(struct device *dev,
 	if ((drvdata->numcidc > 4) && (nr_inputs != 2))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	/*
 	 * each byte[0..3] controls mask value applied to ctxid
@@ -2105,6 +2220,9 @@ static ssize_t vmid_idx_store(struct device *dev,
 	if (val >= drvdata->numvmidc)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -2161,6 +2279,9 @@ static ssize_t vmid_val_store(struct device *dev,
 	if (kstrtoul(buf, 16, &val))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 	config->vmid_val[config->vmid_idx] = (u64)val;
 	raw_spin_unlock(&drvdata->spinlock);
@@ -2217,6 +2338,9 @@ static ssize_t vmid_masks_store(struct device *dev,
 	if ((drvdata->numvmidc > 4) && (nr_inputs != 2))
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	raw_spin_lock(&drvdata->spinlock);
 
 	/*
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH 2/4] coresight/etm4x: remove redundant usage of drvdata->spinlock

[Yeoreum Yun]

Remove redundant usage of drvdata->spinlock in  etm4_starting/dying_cpu()
by preventing cpu hotplug while enabling etm4x via sysfs since
 - perf and sysfs enable method are serialized by csdev->mode
 - etm4_starting/dying_cpu() aren't called concurrently with
   etm4_enable_perf/sysfs() because they're called in cpu offline status.
 - while etm4x_enable_sysfs(), config isn't altered since csdev->mode
   isn't DISABLED.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 .../coresight/coresight-etm4x-core.c          | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 86893115df17..5c9475b44194 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -799,16 +799,21 @@ static int etm4_enable_sysfs(struct coresight_device *csdev)
 	unsigned long cfg_hash;
 	int ret, preset;
 
+	cpus_read_lock();
+
+	if (cpu_is_offline(drvdata->cpu)) {
+		ret = -EPERM;
+		goto unlock_sysfs_enable;
+	}
+
 	/* enable any config activated by configfs */
 	cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
 	if (cfg_hash) {
 		ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
 		if (ret)
-			return ret;
+			goto unlock_sysfs_enable;
 	}
 
-	raw_spin_lock(&drvdata->spinlock);
-
 	/* sysfs needs to read and allocate a trace ID */
 	ret = etm4_read_alloc_trace_id(drvdata);
 	if (ret < 0)
@@ -830,10 +835,11 @@ static int etm4_enable_sysfs(struct coresight_device *csdev)
 		etm4_release_trace_id(drvdata);
 
 unlock_sysfs_enable:
-	raw_spin_unlock(&drvdata->spinlock);
+	cpus_read_unlock();
 
 	if (!ret)
 		dev_dbg(&csdev->dev, "ETM tracing enabled\n");
+
 	return ret;
 }
 
@@ -977,7 +983,6 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
 	 * DYING hotplug callback is serviced by the ETM driver.
 	 */
 	cpus_read_lock();
-	raw_spin_lock(&drvdata->spinlock);
 
 	/*
 	 * Executing etm4_disable_hw on the cpu whose ETM is being disabled
@@ -985,7 +990,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();
 
 	/*
@@ -1663,13 +1667,11 @@ 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;
 }
 
@@ -1678,10 +1680,8 @@ 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;
 }
 
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH 3/4] coresight/etm3x: disallow altering config via sysfs while enabled

[Yeoreum Yun]

When etm3x configuration is modified via sysfs while etm3x is being
enabled via perf, enabled etm3x could run with different configuration
from perf_event.

To address this, disallow altering config via sysfs while csdev is enabled.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 .../coresight/coresight-etm3x-sysfs.c         | 120 ++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index 68c644be9813..b3ae9aba7490 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -75,6 +75,9 @@ static ssize_t reset_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	if (val) {
 		spin_lock(&drvdata->spinlock);
 		memset(config, 0, sizeof(struct etm_config));
@@ -117,6 +120,9 @@ static ssize_t mode_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	config->mode = val & ETM_MODE_ALL;
 
@@ -202,7 +208,12 @@ static ssize_t trigger_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->trigger_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
 
 	return size;
 }
@@ -232,7 +243,12 @@ static ssize_t enable_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->enable_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
 
 	return size;
 }
@@ -262,7 +278,12 @@ static ssize_t fifofull_level_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->fifofull_level = val;
+	spin_unlock(&drvdata->spinlock);
 
 	return size;
 }
@@ -295,6 +316,9 @@ static ssize_t addr_idx_store(struct device *dev,
 	if (val >= drvdata->nr_addr_cmp)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -343,6 +367,9 @@ static ssize_t addr_single_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!(config->addr_type[idx] == ETM_ADDR_TYPE_NONE ||
@@ -403,6 +430,9 @@ static ssize_t addr_range_store(struct device *dev,
 	if (val1 > val2)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (idx % 2 != 0) {
@@ -464,6 +494,9 @@ static ssize_t addr_start_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!(config->addr_type[idx] == ETM_ADDR_TYPE_NONE ||
@@ -518,6 +551,9 @@ static ssize_t addr_stop_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	idx = config->addr_idx;
 	if (!(config->addr_type[idx] == ETM_ADDR_TYPE_NONE ||
@@ -563,6 +599,9 @@ static ssize_t addr_acctype_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	config->addr_acctype[config->addr_idx] = val;
 	spin_unlock(&drvdata->spinlock);
@@ -597,6 +636,10 @@ static ssize_t cntr_idx_store(struct device *dev,
 
 	if (val >= drvdata->nr_cntr)
 		return -EINVAL;
+
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -636,6 +679,9 @@ static ssize_t cntr_rld_val_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	config->cntr_rld_val[config->cntr_idx] = val;
 	spin_unlock(&drvdata->spinlock);
@@ -671,6 +717,9 @@ static ssize_t cntr_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	config->cntr_event[config->cntr_idx] = val & ETM_EVENT_MASK;
 	spin_unlock(&drvdata->spinlock);
@@ -706,6 +755,9 @@ static ssize_t cntr_rld_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	config->cntr_rld_event[config->cntr_idx] = val & ETM_EVENT_MASK;
 	spin_unlock(&drvdata->spinlock);
@@ -752,6 +804,9 @@ static ssize_t cntr_val_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	config->cntr_val[config->cntr_idx] = val;
 	spin_unlock(&drvdata->spinlock);
@@ -784,7 +839,13 @@ static ssize_t seq_12_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->seq_12_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(seq_12_event);
@@ -813,7 +874,13 @@ static ssize_t seq_21_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->seq_21_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(seq_21_event);
@@ -842,7 +909,13 @@ static ssize_t seq_23_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->seq_23_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(seq_23_event);
@@ -871,7 +944,13 @@ static ssize_t seq_31_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->seq_31_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(seq_31_event);
@@ -900,7 +979,13 @@ static ssize_t seq_32_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->seq_32_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(seq_32_event);
@@ -929,7 +1014,13 @@ static ssize_t seq_13_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->seq_13_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(seq_13_event);
@@ -975,7 +1066,12 @@ static ssize_t seq_curr_state_store(struct device *dev,
 	if (val > ETM_SEQ_STATE_MAX_VAL)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->seq_curr_state = val;
+	spin_unlock(&drvdata->spinlock);
 
 	return size;
 }
@@ -1008,6 +1104,9 @@ static ssize_t ctxid_idx_store(struct device *dev,
 	if (val >= drvdata->nr_ctxid_cmp)
 		return -EINVAL;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	/*
 	 * Use spinlock to ensure index doesn't change while it gets
 	 * dereferenced multiple times within a spinlock block elsewhere.
@@ -1066,6 +1165,9 @@ static ssize_t ctxid_pid_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
 	spin_lock(&drvdata->spinlock);
 	config->ctxid_pid[config->ctxid_idx] = pid;
 	spin_unlock(&drvdata->spinlock);
@@ -1112,7 +1214,13 @@ static ssize_t ctxid_mask_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->ctxid_mask = val;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(ctxid_mask);
@@ -1141,7 +1249,13 @@ static ssize_t sync_freq_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->sync_freq = val & ETM_SYNC_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(sync_freq);
@@ -1170,7 +1284,13 @@ static ssize_t timestamp_event_store(struct device *dev,
 	if (ret)
 		return ret;
 
+	if (coresight_get_mode(drvdata->csdev))
+		return -EBUSY;
+
+	spin_lock(&drvdata->spinlock);
 	config->timestamp_event = val & ETM_EVENT_MASK;
+	spin_unlock(&drvdata->spinlock);
+
 	return size;
 }
 static DEVICE_ATTR_RW(timestamp_event);
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH 4/4] coresight/etm3x: remove redundant usage of drvdata->spinlock

[Yeoreum Yun]

Remove redundant usage of drvdata->spinlock in etm_starting/dying_cpu()
by preventing cpu hotplug while enabling etm3x via sysfs since
 - perf and sysfs enable method are serialized by csdev->mode
 - etm_starting/dying_cpu() aren't called concurrently with
   etm_enable_perf/sysfs() because they're called in cpu offline status.
 - while etm_enable_sysfs(), config isn't changed since csdev->mode is
   not DISABLED.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 .../coresight/coresight-etm3x-core.c          | 33 ++++++++-----------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index c103f4c70f5d..5ec871979ef7 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -519,7 +519,12 @@ static int etm_enable_sysfs(struct coresight_device *csdev)
 	struct etm_enable_arg arg = { };
 	int ret;
 
-	spin_lock(&drvdata->spinlock);
+	cpus_read_lock();
+
+	if (cpu_is_offline(drvdata->cpu)) {
+		ret = -ENODEV;
+		goto unlock_sysfs_enable;
+	}
 
 	/* sysfs needs to allocate and set a trace ID */
 	ret = etm_read_alloc_trace_id(drvdata);
@@ -530,23 +535,19 @@ static int etm_enable_sysfs(struct coresight_device *csdev)
 	 * Configure the ETM only if the CPU is online.  If it isn't online
 	 * hw configuration will take place on the local CPU during bring up.
 	 */
-	if (cpu_online(drvdata->cpu)) {
-		arg.drvdata = drvdata;
-		ret = smp_call_function_single(drvdata->cpu,
-					       etm_enable_hw_smp_call, &arg, 1);
-		if (!ret)
-			ret = arg.rc;
-		if (!ret)
-			drvdata->sticky_enable = true;
-	} else {
-		ret = -ENODEV;
-	}
+	arg.drvdata = drvdata;
+	ret = smp_call_function_single(drvdata->cpu,
+				       etm_enable_hw_smp_call, &arg, 1);
+	if (!ret)
+		ret = arg.rc;
+	if (!ret)
+		drvdata->sticky_enable = true;
 
 	if (ret)
 		etm_release_trace_id(drvdata);
 
 unlock_enable_sysfs:
-	spin_unlock(&drvdata->spinlock);
+	cpus_read_unlock();
 
 	if (!ret)
 		dev_dbg(&csdev->dev, "ETM tracing enabled\n");
@@ -646,7 +647,6 @@ static void etm_disable_sysfs(struct coresight_device *csdev)
 	 * DYING hotplug callback is serviced by the ETM driver.
 	 */
 	cpus_read_lock();
-	spin_lock(&drvdata->spinlock);
 
 	/*
 	 * Executing etm_disable_hw on the cpu whose ETM is being disabled
@@ -654,7 +654,6 @@ static void etm_disable_sysfs(struct coresight_device *csdev)
 	 */
 	smp_call_function_single(drvdata->cpu, etm_disable_hw, drvdata, 1);
 
-	spin_unlock(&drvdata->spinlock);
 	cpus_read_unlock();
 
 	/*
@@ -722,7 +721,6 @@ static int etm_starting_cpu(unsigned int cpu)
 	if (!etmdrvdata[cpu])
 		return 0;
 
-	spin_lock(&etmdrvdata[cpu]->spinlock);
 	if (!etmdrvdata[cpu]->os_unlock) {
 		etm_os_unlock(etmdrvdata[cpu]);
 		etmdrvdata[cpu]->os_unlock = true;
@@ -730,7 +728,6 @@ static int etm_starting_cpu(unsigned int cpu)
 
 	if (coresight_get_mode(etmdrvdata[cpu]->csdev))
 		etm_enable_hw(etmdrvdata[cpu]);
-	spin_unlock(&etmdrvdata[cpu]->spinlock);
 	return 0;
 }
 
@@ -739,10 +736,8 @@ static int etm_dying_cpu(unsigned int cpu)
 	if (!etmdrvdata[cpu])
 		return 0;
 
-	spin_lock(&etmdrvdata[cpu]->spinlock);
 	if (coresight_get_mode(etmdrvdata[cpu]->csdev))
 		etm_disable_hw(etmdrvdata[cpu]);
-	spin_unlock(&etmdrvdata[cpu]->spinlock);
 	return 0;
 }
 
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH 0/4] small fix for configuaring etm csdev via sysfs

[Yeo Reum Yun]

Gentle ping in case of forgotten.

________________________________________
From: Yeoreum Yun <yeoreum.yun@arm.com>
Sent: 21 December 2024 16:59
To: Suzuki Poulose; mike.leach@linaro.org; james.clark@linaro.org; alexander.shishkin@linux.intel.com
Cc: coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Yeo Reum Yun
Subject: [PATCH 0/4] small fix for configuaring etm csdev via sysfs

when etm csdev's configuration is modified via sysfs while etm is being
enabled via perf, enabled etm could run with different configuration
from perf_event.

Also, there are some redeundant usage of drvdata->spinlock in
etm_starting/dying_cpu() with enable/disable etm device.

This patchset fixes above small problems.

NOTE:
   This patchset based on:
     - https://lore.kernel.org/linux-arm-kernel/20241220104521.809071-1-yeoreum.yun@arm.com/

Yeoreum Yun (4):
  coresight/etm4x: disallow altering config via sysfs while enabled
  coresight/etm4x: remove redundant usage of drvdata->spinlock
  coresight/etm3x: disallow altering config via sysfs while enabled
  coresight/etm3x: remove redundant usage of drvdata->spinlock

 .../coresight/coresight-etm3x-core.c          |  33 ++---
 .../coresight/coresight-etm3x-sysfs.c         | 120 ++++++++++++++++
 .../coresight/coresight-etm4x-core.c          |  20 +--
 .../coresight/coresight-etm4x-sysfs.c         | 132 +++++++++++++++++-
 4 files changed, 272 insertions(+), 33 deletions(-)

--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Suzuki K Poulose]

Hi Levi,

On 21/12/2024 16:59, Yeoreum Yun wrote:
> When etm4x configuration is modified via sysfs while etm4x is being
> enabled via perf, enabled etm4x could run with different configuration
> from perf_event.
> 
> To address this, disallow altering config via sysfs while csdev is enabled.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
>   .../coresight/coresight-etm4x-sysfs.c         | 132 +++++++++++++++++-
>   1 file changed, 128 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index 11e865b8e824..cc1f112921d7 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> @@ -174,6 +174,9 @@ static ssize_t reset_store(struct device *dev,
>   	if (kstrtoul(buf, 16, &val))
>   		return -EINVAL;
>   
> +	if (coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;
> +

Is it not better to have separate "configs" for perf and sysfs ?
And etmX driver can populate the "running" config, based on the
mode specific config. That way, the configs can be updated
independently without affecting the running config or the perf one.

Suzuki

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Yeoreum Yun]

Hi Suzuki,

> Is it not better to have separate "configs" for perf and sysfs ?
> And etmX driver can populate the "running" config, based on the
> mode specific config. That way, the configs can be updated
> independently without affecting the running config or the perf one.
>

That was i've tried but I've accepted Mike's opinion that
it's enough to check whether CS_MODE_DISABLED via coresight_get_mode()
in *_store().

"the .._store functions in sysfs should use coresight_get_mode() to ensure
this is set to CS_MODE_DISABLED before altering the config,
which ensures that the trace system is inactive.
We don't' really care about reading the config if trace is running."

Thanks.

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Suzuki K Poulose]

On 09/01/2025 12:01, Yeoreum Yun wrote:
> Hi Suzuki,
> 
>> Is it not better to have separate "configs" for perf and sysfs ?
>> And etmX driver can populate the "running" config, based on the
>> mode specific config. That way, the configs can be updated
>> independently without affecting the running config or the perf one.
>>
> 
> That was i've tried but I've accepted Mike's opinion that
> it's enough to check whether CS_MODE_DISABLED via coresight_get_mode()
> in *_store().
> 
> "the .._store functions in sysfs should use coresight_get_mode() to ensure
> this is set to CS_MODE_DISABLED before altering the config,
> which ensures that the trace system is inactive.
> We don't' really care about reading the config if trace is running."

There are two issues with that :

1. Sprinkling the get_mode call in each sysfs stor function doesn't look 
good to me.

2. Someone preparing for a sysfs session must not be prevented from 
doing so just because there is a perf session running.


Suzuki


> Thanks.
> 

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Yeoreum Yun]

Hi Suzuki,

> On 09/01/2025 12:01, Yeoreum Yun wrote:
> > Hi Suzuki,
> >
> > > Is it not better to have separate "configs" for perf and sysfs ?
> > > And etmX driver can populate the "running" config, based on the
> > > mode specific config. That way, the configs can be updated
> > > independently without affecting the running config or the perf one.
> > >
> >
> > That was i've tried but I've accepted Mike's opinion that
> > it's enough to check whether CS_MODE_DISABLED via coresight_get_mode()
> > in *_store().
> >
> > "the .._store functions in sysfs should use coresight_get_mode() to ensure
> > this is set to CS_MODE_DISABLED before altering the config,
> > which ensures that the trace system is inactive.
> > We don't' really care about reading the config if trace is running."
>
> There are two issues with that :
>
> 1. Sprinkling the get_mode call in each sysfs stor function doesn't look
> good to me.
>
> 2. Someone preparing for a sysfs session must not be prevented from doing so
> just because there is a perf session running.
>

I agree. okay. I've posted with separate configuration.
Thanks!

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Yeoreum Yun]

Hi Suzuki,

> On 09/01/2025 12:01, Yeoreum Yun wrote:
> > Hi Suzuki,
> >
> > > Is it not better to have separate "configs" for perf and sysfs ?
> > > And etmX driver can populate the "running" config, based on the
> > > mode specific config. That way, the configs can be updated
> > > independently without affecting the running config or the perf one.
> > >
> >
> > That was i've tried but I've accepted Mike's opinion that
> > it's enough to check whether CS_MODE_DISABLED via coresight_get_mode()
> > in *_store().
> >
> > "the .._store functions in sysfs should use coresight_get_mode() to ensure
> > this is set to CS_MODE_DISABLED before altering the config,
> > which ensures that the trace system is inactive.
> > We don't' really care about reading the config if trace is running."
>
> There are two issues with that :
>
> 1. Sprinkling the get_mode call in each sysfs stor function doesn't look
> good to me.
>
> 2. Someone preparing for a sysfs session must not be prevented from doing so
> just because there is a perf session running.
>
>
> Suzuki
>

But, when separate the config, it doesn't show anymore the current
configuration set by perf.
I'm not sure this is okay.
IMHO, If perf is enabled, since the configuration show the "perf",
I think prohabit to modify config via sysfs while PERF_ENABLE seems valid.

and about lossing session, I think this is up to user.
That means to use sysfs, user shouldn't use perf to prevent loss
its configuration.

Am I missing something?

Thanks

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Yeo Reum Yun]

> > On 09/01/2025 12:01, Yeoreum Yun wrote:
> > > Hi Suzuki,
> > >
> > > > Is it not better to have separate "configs" for perf and sysfs ?
> > > > And etmX driver can populate the "running" config, based on the
> > > > mode specific config. That way, the configs can be updated
> > > > independently without affecting the running config or the perf one.
> > > >
> > >
> > > That was i've tried but I've accepted Mike's opinion that
> > > it's enough to check whether CS_MODE_DISABLED via coresight_get_mode()
> > > in *_store().
> > >
> > > "the .._store functions in sysfs should use coresight_get_mode() to ensure
> > > this is set to CS_MODE_DISABLED before altering the config,
> > > which ensures that the trace system is inactive.
> > > We don't' really care about reading the config if trace is running."
> >
> > There are two issues with that :
> >
> > 1. Sprinkling the get_mode call in each sysfs stor function doesn't look
> > good to me.
> >
> > 2. Someone preparing for a sysfs session must not be prevented from doing so
> > just because there is a perf session running.
> >
> >
> > Suzuki
> >
>
> But, when separate the config, it doesn't show anymore the current
> configuration set by perf.
> I'm not sure this is okay.
> IMHO, If perf is enabled, since the configuration show the "perf",
> I think prohabit to modify config via sysfs while PERF_ENABLE seems valid.
> 
> and about lossing session, I think this is up to user.
> That means to use sysfs, user shouldn't use perf to prevent loss
> its configuration.
>
> Am I missing something?
>
> Thanks

And one more question.
When CS_SYSFS_ENABLE,  Is it valid to allow modification of "config"?
I think It seems valid to show the "current working configration".
If It allows to modificaiton the config value showed via sysfs and current applied be different.

So, IMHO, It seems to valid to modify "config" only in CS_DISABLED.

Am I missing something?

Thanks.

Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

[Yeo Reum Yun]

Gentle ping in case of forgotten.

________________________________________
From: Yeo Reum Yun <YeoReum.Yun@arm.com>
Sent: 09 January 2025 17:48
To: Suzuki Poulose
Cc: mike.leach@linaro.org; james.clark@linaro.org; alexander.shishkin@linux.intel.com; coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] coresight/etm4x: disallow altering config via sysfs while enabled

> > On 09/01/2025 12:01, Yeoreum Yun wrote:
> > > Hi Suzuki,
> > >
> > > > Is it not better to have separate "configs" for perf and sysfs ?
> > > > And etmX driver can populate the "running" config, based on the
> > > > mode specific config. That way, the configs can be updated
> > > > independently without affecting the running config or the perf one.
> > > >
> > >
> > > That was i've tried but I've accepted Mike's opinion that
> > > it's enough to check whether CS_MODE_DISABLED via coresight_get_mode()
> > > in *_store().
> > >
> > > "the .._store functions in sysfs should use coresight_get_mode() to ensure
> > > this is set to CS_MODE_DISABLED before altering the config,
> > > which ensures that the trace system is inactive.
> > > We don't' really care about reading the config if trace is running."
> >
> > There are two issues with that :
> >
> > 1. Sprinkling the get_mode call in each sysfs stor function doesn't look
> > good to me.
> >
> > 2. Someone preparing for a sysfs session must not be prevented from doing so
> > just because there is a perf session running.
> >
> >
> > Suzuki
> >
>
> But, when separate the config, it doesn't show anymore the current
> configuration set by perf.
> I'm not sure this is okay.
> IMHO, If perf is enabled, since the configuration show the "perf",
> I think prohabit to modify config via sysfs while PERF_ENABLE seems valid.
>
> and about lossing session, I think this is up to user.
> That means to use sysfs, user shouldn't use perf to prevent loss
> its configuration.
>
> Am I missing something?
>
> Thanks

And one more question.
When CS_SYSFS_ENABLE,  Is it valid to allow modification of "config"?
I think It seems valid to show the "current working configration".
If It allows to modificaiton the config value showed via sysfs and current applied be different.

So, IMHO, It seems to valid to modify "config" only in CS_DISABLED.

Am I missing something?

Thanks.

Re: [PATCH 0/4] small fix for configuaring etm csdev via sysfs

[Yeoreum Yun]

Gentle ping in case of forgotten.

> when etm csdev's configuration is modified via sysfs while etm is being
> enabled via perf, enabled etm could run with different configuration
> from perf_event.
>
> Also, there are some redeundant usage of drvdata->spinlock in
> etm_starting/dying_cpu() with enable/disable etm device.
>
> This patchset fixes above small problems.
>
> NOTE:
>    This patchset based on:
>      - https://lore.kernel.org/linux-arm-kernel/20241220104521.809071-1-yeoreum.yun@arm.com/
>
> Yeoreum Yun (4):
>   coresight/etm4x: disallow altering config via sysfs while enabled
>   coresight/etm4x: remove redundant usage of drvdata->spinlock
>   coresight/etm3x: disallow altering config via sysfs while enabled
>   coresight/etm3x: remove redundant usage of drvdata->spinlock
>
>  .../coresight/coresight-etm3x-core.c          |  33 ++---
>  .../coresight/coresight-etm3x-sysfs.c         | 120 ++++++++++++++++
>  .../coresight/coresight-etm4x-core.c          |  20 +--
>  .../coresight/coresight-etm4x-sysfs.c         | 132 +++++++++++++++++-
>  4 files changed, 272 insertions(+), 33 deletions(-)
>
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>

--
Sincerely,
Yeoreum Yun