From: Leo Yan Date: Thu, 26 Feb 2026 09:23:49 +0000 (+0000) Subject: coresight: cti: Make spinlock usage consistent X-Git-Tag: v7.1-rc1~17^2^2~12 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ef7d4aaf686785b8b0f70c9fe373fb5455d3e5b5;p=thirdparty%2Flinux.git 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 Reviewed-by: Mike Leach Signed-off-by: Leo Yan Signed-off-by: Suzuki K Poulose Link: https://lore.kernel.org/r/20260226-arm_coresight_cti_refactor_v1-v2-1-b30fada3cfec@arm.com --- diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index fddc8f31b91d..e3c98d89c987 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -81,10 +81,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) @@ -93,22 +92,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 */ @@ -116,25 +108,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 */ @@ -142,23 +130,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); @@ -168,13 +153,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) @@ -189,11 +168,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); } /* @@ -360,7 +339,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] : @@ -379,7 +358,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; } @@ -397,7 +376,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: @@ -417,7 +397,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; } @@ -436,7 +416,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: @@ -464,7 +445,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; } @@ -667,7 +647,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: @@ -707,7 +687,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; } @@ -734,11 +713,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 572b80ee96fb..455d08bcccd4 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)