Commit ef7d4aaf authored by Leo Yan's avatar Leo Yan Committed by Suzuki K Poulose
Browse files

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: 984f37ef ("coresight: cti: Write regsiters directly in cti_enable_hw()")
Tested-by: default avatarJames Clark <james.clark@linaro.org>
Reviewed-by: default avatarMike Leach <mike.leach@arm.com>
Signed-off-by: default avatarLeo Yan <leo.yan@arm.com>
Signed-off-by: default avatarSuzuki K Poulose <suzuki.poulose@arm.com>
Link: https://lore.kernel.org/r/20260226-arm_coresight_cti_refactor_v1-v2-1-b30fada3cfec@arm.com
parent 061c39a1

drivers/hwtracing/coresight/coresight-cti-core.c

+27 −47
Original line number Diff line number Diff line
@@ -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;

}

drivers/hwtracing/coresight/coresight-cti-sysfs.c

+76 −69
Original line number Diff line number Diff line
@@ -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);

	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

		enable_req = drvdata->config.enable_req_count;

		powered = drvdata->config.hw_powered;

		enabled = drvdata->config.hw_enabled;

	raw_spin_unlock(&drvdata->spinlock);

	}



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

	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock)

		powered = drvdata->config.hw_powered;

	raw_spin_unlock(&drvdata->spinlock);



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



	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

		if (drvdata->config.hw_powered)

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

	raw_spin_unlock(&drvdata->spinlock);

	}



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



	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

		if (drvdata->config.hw_powered)

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

	raw_spin_unlock(&drvdata->spinlock);

	}



	pm_runtime_put_sync(dev->parent);

	return size;

}
@@ -264,7 +267,7 @@ 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);

	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);
@@ -274,7 +277,8 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf, 
		} else if (pcached_val) {

			val = *pcached_val;

		}

	raw_spin_unlock(&drvdata->spinlock);

	}



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

}
@@ -293,7 +297,7 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, 
	if (kstrtoul(buf, 0, &val))

		return -EINVAL;



	raw_spin_lock(&drvdata->spinlock);

	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

		/* local store */

		if (pcached_val)

			*pcached_val = (u32)val;
@@ -301,7 +305,8 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, 
		/* 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);

	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

		index = drvdata->config.ctiinout_sel;

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

	raw_spin_unlock(&drvdata->spinlock);

	}



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

	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {

		index = drvdata->config.ctiinout_sel;

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

	raw_spin_unlock(&drvdata->spinlock);

	}



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

	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock)

		val = drvdata->config.trig_filter_enable;

	raw_spin_unlock(&drvdata->spinlock);



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

	scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock)

		val = drvdata->config.xtrig_rchan_sel;

	raw_spin_unlock(&drvdata->spinlock);



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

	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)