RE: [PATCH 1/8] coresight: cti: Make spinlock usage consistent

Hi Leo

-----Original Message----- From: Leo Yan leo.yan@arm.com Sent: Monday, February 9, 2026 6:01 PM To: Suzuki Poulose Suzuki.Poulose@arm.com; Mike Leach Mike.Leach@arm.com; James Clark james.clark@linaro.org; Alexander Shishkin alexander.shishkin@linux.intel.com; Greg Kroah-Hartman gregkh@linuxfoundation.org; Mathieu Poirier mathieu.poirier@linaro.org; Tingwei Zhang quic_tingwei@quicinc.com; Yingchao Deng yingchao.deng@oss.qualcomm.com; Jie Gan jie.gan@oss.qualcomm.com Cc: coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org; Leo Yan Leo.Yan@arm.com Subject: [PATCH 1/8] coresight: cti: Make spinlock usage consistent

The spinlock is acquired sometimes with IRQs disabled and sometimes without. This leads to inconsistent semantics: the lock can be either HARDIRQ-safe or HARDIRQ-unsafe, which may trigger lockdep complaints.

Make spinlock usage consistent by acquiring it with disabling IRQs. It is possible for sysfs knobs to acquire the spinlock for accessing a CTI device, while at the same time a perf session sends an IPI to enable the same CTI device. In this case, the spinlock must be IRQ-safe, which is why all lock acquisitions are changed to disable IRQs.

Use guard() and scoped_guard() for spinlock to tidy up the code.

Fixes: 984f37efa385 ("coresight: cti: Write regsiters directly in cti_enable_hw()") Tested-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com

---

drivers/hwtracing/coresight/coresight-cti-core.c | 74 ++++------- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 145 ++++++++++++-------

---

2 files changed, 103 insertions(+), 116 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index bfbc365bb2ef2744efab11c056b8450472957005..b75c4ec2c8d5d58da8761 334643dba34001b48c7 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -90,10 +90,9 @@ void cti_write_all_hw_regs(struct cti_drvdata *drvdata) static int cti_enable_hw(struct cti_drvdata *drvdata) { struct cti_config *config = &drvdata->config;

• unsigned long flags;
• int rc = 0;

• int rc;

• raw_spin_lock_irqsave(&drvdata->spinlock, flags);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

  /* no need to do anything if enabled or unpowered*/ if (config->hw_enabled || !config->hw_powered)

@@ -102,22 +101,15 @@ static int cti_enable_hw(struct cti_drvdata *drvdata) /* claim the device */ rc = coresight_claim_device(drvdata->csdev); if (rc)

• goto cti_err_not_enabled;
  

• return rc;
  
  

  cti_write_all_hw_regs(drvdata);

  config->hw_enabled = true;

• drvdata->config.enable_req_count++;
• raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return rc;

cti_state_unchanged: drvdata->config.enable_req_count++;

• /* cannot enable due to error */

-cti_err_not_enabled:

• raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
• return rc;

• return 0;

}

/* re-enable CTI on CPU when using CPU hotplug */ @@ -125,25 +117,21 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata) { struct cti_config *config = &drvdata->config;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

• config->hw_powered = true;

  /* no need to do anything if no enable request */ if (!drvdata->config.enable_req_count)

• goto cti_hp_not_enabled;
  

• return;
  
  

  /* try to claim the device */ if (coresight_claim_device(drvdata->csdev))

• goto cti_hp_not_enabled;
  

• return;
  
  

  cti_write_all_hw_regs(drvdata); config->hw_enabled = true;

• raw_spin_unlock(&drvdata->spinlock); return;
• /* did not re-enable due to no claim / no request */

-cti_hp_not_enabled:

• raw_spin_unlock(&drvdata->spinlock);

}

/* disable hardware */ @@ -151,23 +139,20 @@ static int cti_disable_hw(struct cti_drvdata *drvdata) { struct cti_config *config = &drvdata->config; struct coresight_device *csdev = drvdata->csdev;

• int ret = 0;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

  /* don't allow negative refcounts, return an error */

• if (!drvdata->config.enable_req_count) {

• ret = -EINVAL;
  

• goto cti_not_disabled;
  

• }

• if (!drvdata->config.enable_req_count)

• return -EINVAL;
  
  

  /* check refcount - disable on 0 */ if (--drvdata->config.enable_req_count > 0)

• goto cti_not_disabled;
  

• return 0;
  
  

  /* no need to do anything if disabled or cpu unpowered */ if (!config->hw_enabled || !config->hw_powered)

• goto cti_not_disabled;
  

• return 0;
  
  

  CS_UNLOCK(drvdata->base);

@@ -177,13 +162,7 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)

coresight_disclaim_device_unlocked(csdev); CS_LOCK(drvdata->base);

• raw_spin_unlock(&drvdata->spinlock);
• return ret;
• /* not disabled this call */

-cti_not_disabled:

• raw_spin_unlock(&drvdata->spinlock);
• return ret;

• return 0;

}

void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value) @@ -198,11 +177,11 @@ void cti_write_intack(struct device *dev, u32 ackval) struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);
• /* write if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIINTACK, ackval);

• raw_spin_unlock(&drvdata->spinlock);

}

/* @@ -369,7 +348,7 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, reg_offset = (direction == CTI_TRIG_IN ? CTIINEN(trigger_idx) : CTIOUTEN(trigger_idx));

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

  /* read - modify write - the trigger / channel enable value */ reg_value = direction == CTI_TRIG_IN ? config->ctiinen[trigger_idx] :

@@ -388,7 +367,7 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, /* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, reg_offset, reg_value);

• raw_spin_unlock(&drvdata->spinlock);

• return 0;

}

@@ -406,7 +385,8 @@ int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op,

chan_bitmask = BIT(channel_idx);

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);
• reg_value = config->ctigate; switch (op) { case CTI_GATE_CHAN_ENABLE:

@@ -426,7 +406,7 @@ int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op, if (cti_active(config)) cti_write_single_reg(drvdata, CTIGATE, reg_value); }

• raw_spin_unlock(&drvdata->spinlock);

• return err;

}

@@ -445,7 +425,8 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,

chan_bitmask = BIT(channel_idx);

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);
• reg_value = config->ctiappset; switch (op) { case CTI_CHAN_SET:

@@ -473,7 +454,6 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,

if ((err == 0) && cti_active(config)) cti_write_single_reg(drvdata, reg_offset, reg_value);

• raw_spin_unlock(&drvdata->spinlock);

  return err;

} @@ -676,7 +656,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, if (WARN_ON_ONCE(drvdata->ctidev.cpu != cpu)) return NOTIFY_BAD;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

  switch (cmd) { case CPU_PM_ENTER:

@@ -716,7 +696,6 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, }

cti_notify_exit:

• raw_spin_unlock(&drvdata->spinlock); return notify_res;

}

@@ -743,11 +722,12 @@ static int cti_dying_cpu(unsigned int cpu) if (!drvdata) return 0;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);
• drvdata->config.hw_powered = false; if (drvdata->config.hw_enabled) coresight_disclaim_device(drvdata->csdev);

• raw_spin_unlock(&drvdata->spinlock);

• return 0;

}

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 572b80ee96fbf18ec8cf9abc30d109a676dfbc5d..455d08bcccd49a3f1eac8ab d8246806ef73a9ab6 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -84,11 +84,11 @@ static ssize_t enable_show(struct device *dev, bool enabled, powered; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• enable_req = drvdata->config.enable_req_count;
• powered = drvdata->config.hw_powered;
• enabled = drvdata->config.hw_enabled;
• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• enable_req = drvdata->config.enable_req_count;
  

• powered = drvdata->config.hw_powered;
  

• enabled = drvdata->config.hw_enabled;
  

• }

  if (powered) return sprintf(buf, "%d\n", enabled);

@@ -134,9 +134,8 @@ static ssize_t powered_show(struct device *dev, bool powered; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• powered = drvdata->config.hw_powered;
• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock)

• powered = drvdata->config.hw_powered;
  
  

  return sprintf(buf, "%d\n", powered);

} @@ -181,10 +180,12 @@ static ssize_t coresight_cti_reg_show(struct device *dev, u32 val = 0;

pm_runtime_get_sync(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• if (drvdata->config.hw_powered)

• val = readl_relaxed(drvdata->base + cti_attr->off);
  

• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• if (drvdata->config.hw_powered)
  

• 	val = readl_relaxed(drvdata->base + cti_attr->off);
  

• }
• pm_runtime_put_sync(dev->parent); return sysfs_emit(buf, "0x%x\n", val);

} @@ -202,10 +203,12 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, return -EINVAL;

pm_runtime_get_sync(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• if (drvdata->config.hw_powered)

• cti_write_single_reg(drvdata, cti_attr->off, val);
  

• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• if (drvdata->config.hw_powered)
  

• 	cti_write_single_reg(drvdata, cti_attr->off, val);
  

• }
• pm_runtime_put_sync(dev->parent); return size;

} @@ -264,17 +267,18 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf, struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;

• raw_spin_lock(&drvdata->spinlock);
• if ((reg_offset >= 0) && cti_active(config)) {

• CS_UNLOCK(drvdata->base);
  

• val = readl_relaxed(drvdata->base + reg_offset);
  

• if (pcached_val)
  

• 	*pcached_val = val;
  

• CS_LOCK(drvdata->base);
  

• } else if (pcached_val) {

• val = *pcached_val;
  

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• if ((reg_offset >= 0) && cti_active(config)) {
  

• 	CS_UNLOCK(drvdata->base);
  

• 	val = readl_relaxed(drvdata->base + reg_offset);
  

• 	if (pcached_val)
  

• 		*pcached_val = val;
  

• 	CS_LOCK(drvdata->base);
  

• } else if (pcached_val) {
  

• 	val = *pcached_val;
  

• }
  

  }

• raw_spin_unlock(&drvdata->spinlock);

• return sprintf(buf, "%#x\n", val);

}

@@ -293,15 +297,16 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, if (kstrtoul(buf, 0, &val)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);
• /* local store */
• if (pcached_val)

• *pcached_val = (u32)val;
  

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• /* local store */
  

• if (pcached_val)
  

• 	*pcached_val = (u32)val;
  

• /* write through if offset and enabled */
  

• if ((reg_offset >= 0) && cti_active(config))
  

• 	cti_write_single_reg(drvdata, reg_offset, val);
  

• }

• /* write through if offset and enabled */
• if ((reg_offset >= 0) && cti_active(config))

• cti_write_single_reg(drvdata, reg_offset, val);
  

• raw_spin_unlock(&drvdata->spinlock); return size;

}

@@ -349,9 +354,9 @@ static ssize_t inout_sel_store(struct device *dev, if (val > (CTIINOUTEN_MAX - 1)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);
• drvdata->config.ctiinout_sel = val;

• raw_spin_unlock(&drvdata->spinlock); return size;

} static DEVICE_ATTR_RW(inout_sel); @@ -364,10 +369,11 @@ static ssize_t inen_show(struct device *dev, int index; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• index = drvdata->config.ctiinout_sel;
• val = drvdata->config.ctiinen[index];
• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• index = drvdata->config.ctiinout_sel;
  

• val = drvdata->config.ctiinen[index];
  

• }
• return sprintf(buf, "%#lx\n", val);

}

@@ -383,14 +389,15 @@ static ssize_t inen_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

• index = config->ctiinout_sel; config->ctiinen[index] = val;

  /* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIINEN(index), val);

• raw_spin_unlock(&drvdata->spinlock);

• return size;

} static DEVICE_ATTR_RW(inen); @@ -403,10 +410,11 @@ static ssize_t outen_show(struct device *dev, int index; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• index = drvdata->config.ctiinout_sel;
• val = drvdata->config.ctiouten[index];
• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• index = drvdata->config.ctiinout_sel;
  

• val = drvdata->config.ctiouten[index];
  

• }
• return sprintf(buf, "%#lx\n", val);

}

@@ -422,14 +430,15 @@ static ssize_t outen_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

• index = config->ctiinout_sel; config->ctiouten[index] = val;

  /* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIOUTEN(index), val);

• raw_spin_unlock(&drvdata->spinlock);

• return size;

} static DEVICE_ATTR_RW(outen); @@ -463,7 +472,7 @@ static ssize_t appclear_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

  /* a 1'b1 in appclr clears down the same bit in appset*/ config->ctiappset &= ~val;

@@ -471,7 +480,7 @@ static ssize_t appclear_store(struct device *dev, /* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIAPPCLEAR, val);

• raw_spin_unlock(&drvdata->spinlock);

• return size;

} static DEVICE_ATTR_WO(appclear); @@ -487,12 +496,12 @@ static ssize_t apppulse_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

  /* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIAPPPULSE, val);

• raw_spin_unlock(&drvdata->spinlock);

• return size;

} static DEVICE_ATTR_WO(apppulse); @@ -681,9 +690,9 @@ static ssize_t trig_filter_enable_show(struct device *dev, u32 val; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• val = drvdata->config.trig_filter_enable;
• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock)

• val = drvdata->config.trig_filter_enable;
  

• return sprintf(buf, "%d\n", val);

}

@@ -697,9 +706,9 @@ static ssize_t trig_filter_enable_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);
• drvdata->config.trig_filter_enable = !!val;

• raw_spin_unlock(&drvdata->spinlock); return size;

} static DEVICE_ATTR_RW(trig_filter_enable); @@ -728,7 +737,7 @@ static ssize_t chan_xtrigs_reset_store(struct device *dev, struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);

  /* clear the CTI trigger / channel programming registers */ for (i = 0; i < config->nr_trig_max; i++) {

@@ -747,7 +756,6 @@ static ssize_t chan_xtrigs_reset_store(struct device *dev, if (cti_active(config)) cti_write_all_hw_regs(drvdata);

• raw_spin_unlock(&drvdata->spinlock); return size;

} static DEVICE_ATTR_WO(chan_xtrigs_reset); @@ -768,9 +776,9 @@ static ssize_t chan_xtrigs_sel_store(struct device *dev, if (val > (drvdata->config.nr_ctm_channels - 1)) return -EINVAL;

• raw_spin_lock(&drvdata->spinlock);

• guard(raw_spinlock_irqsave)(&drvdata->spinlock);
• drvdata->config.xtrig_rchan_sel = val;

• raw_spin_unlock(&drvdata->spinlock); return size;

}

@@ -781,9 +789,8 @@ static ssize_t chan_xtrigs_sel_show(struct device *dev, unsigned long val; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);

• raw_spin_lock(&drvdata->spinlock);
• val = drvdata->config.xtrig_rchan_sel;
• raw_spin_unlock(&drvdata->spinlock);

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock)

• val = drvdata->config.xtrig_rchan_sel;
  
  

  return sprintf(buf, "%ld\n", val);

} @@ -838,12 +845,12 @@ static ssize_t print_chan_list(struct device *dev, unsigned long inuse_bits = 0, chan_mask;

/* scan regs to get bitmap of channels in use. */

• raw_spin_lock(&drvdata->spinlock);
• for (i = 0; i < config->nr_trig_max; i++) {

• inuse_bits |= config->ctiinen[i];
  

• inuse_bits |= config->ctiouten[i];
  

• scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

• for (i = 0; i < config->nr_trig_max; i++) {
  

• 	inuse_bits |= config->ctiinen[i];
  

• 	inuse_bits |= config->ctiouten[i];
  

• }
  

  }

• raw_spin_unlock(&drvdata->spinlock);

  /* inverse bits if printing free channels */ if (!inuse)

-- 2.34.1

Reviewed-by: Mike Leach mike.leach@arm.com