| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJ9a7VgUMLR9cnY-QUqUcJj6ECfqxN0dD9QUqM8jmHj7t=Pd4g@mail.gmail.com>
Date: Thu, 19 Dec 2024 12:24:13 +0000
From: Mike Leach <mike.leach@...aro.org>
To: Yeoreum Yun <yeoreum.yun@....com>
Cc: suzuki.poulose@....com, james.clark@...aro.org,
alexander.shishkin@...ux.intel.com, bigeasy@...utronix.de,
clrkwllms@...nel.org, rostedt@...dmis.org, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH v3 4/9] coresight-cti: change cti_drvdata spinlock's type
to raw_spinlock_t
On Mon, 16 Dec 2024 at 11:50, Yeoreum Yun <yeoreum.yun@....com> wrote:
>
> In coresight-cti drivers, cti_drvdata->spinlock can be held during __schedule()
> by perf_event_task_sched_out()/in().
>
> Since cti_drvdata->spinlock type is spinlock_t and
> perf_event_task_sched_out()/in() is called after acquiring rq_lock,
> which is raw_spinlock_t (an unsleepable lock),
> this poses an issue in PREEMPT_RT kernel where spinlock_t is sleepable.
>
> To address this, change type cti_drvdata->spinlock in coresight-cti drivers,
> which can be called by perf_event_task_sched_out()/in(),
> from spinlock_t to raw_spinlock_t.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
> ---
> .../hwtracing/coresight/coresight-cti-core.c | 44 +++++------
> .../hwtracing/coresight/coresight-cti-sysfs.c | 76 +++++++++----------
> drivers/hwtracing/coresight/coresight-cti.h | 2 +-
> 3 files changed, 61 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> index d2b5a5718c29..80f6265e3740 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> @@ -93,7 +93,7 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
> unsigned long flags;
> int rc = 0;
>
> - spin_lock_irqsave(&drvdata->spinlock, flags);
> + raw_spin_lock_irqsave(&drvdata->spinlock, flags);
>
> /* no need to do anything if enabled or unpowered*/
> if (config->hw_enabled || !config->hw_powered)
> @@ -108,7 +108,7 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
>
> config->hw_enabled = true;
> drvdata->config.enable_req_count++;
> - spin_unlock_irqrestore(&drvdata->spinlock, flags);
> + raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
>
> cti_state_unchanged:
> @@ -116,7 +116,7 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
>
> /* cannot enable due to error */
> cti_err_not_enabled:
> - spin_unlock_irqrestore(&drvdata->spinlock, flags);
> + raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -125,7 +125,7 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata)
> {
> struct cti_config *config = &drvdata->config;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> config->hw_powered = true;
>
> /* no need to do anything if no enable request */
> @@ -138,12 +138,12 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata)
>
> cti_write_all_hw_regs(drvdata);
> config->hw_enabled = true;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return;
>
> /* did not re-enable due to no claim / no request */
> cti_hp_not_enabled:
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> }
>
> /* disable hardware */
> @@ -153,7 +153,7 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
> struct coresight_device *csdev = drvdata->csdev;
> int ret = 0;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
>
> /* don't allow negative refcounts, return an error */
> if (!drvdata->config.enable_req_count) {
> @@ -177,12 +177,12 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
>
> coresight_disclaim_device_unlocked(csdev);
> CS_LOCK(drvdata->base);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return ret;
>
> /* not disabled this call */
> cti_not_disabled:
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return ret;
> }
>
> @@ -198,11 +198,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;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> /* write if enabled */
> if (cti_active(config))
> cti_write_single_reg(drvdata, CTIINTACK, ackval);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> }
>
> /*
> @@ -369,7 +369,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));
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
>
> /* read - modify write - the trigger / channel enable value */
> reg_value = direction == CTI_TRIG_IN ? config->ctiinen[trigger_idx] :
> @@ -388,7 +388,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);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return 0;
> }
>
> @@ -406,7 +406,7 @@ int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op,
>
> chan_bitmask = BIT(channel_idx);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> reg_value = config->ctigate;
> switch (op) {
> case CTI_GATE_CHAN_ENABLE:
> @@ -426,7 +426,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);
> }
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return err;
> }
>
> @@ -445,7 +445,7 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,
>
> chan_bitmask = BIT(channel_idx);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> reg_value = config->ctiappset;
> switch (op) {
> case CTI_CHAN_SET:
> @@ -473,7 +473,7 @@ 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);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
>
> return err;
> }
> @@ -676,7 +676,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;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
>
> switch (cmd) {
> case CPU_PM_ENTER:
> @@ -716,7 +716,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> }
>
> cti_notify_exit:
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return notify_res;
> }
>
> @@ -743,11 +743,11 @@ static int cti_dying_cpu(unsigned int cpu)
> if (!drvdata)
> return 0;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> drvdata->config.hw_powered = false;
> if (drvdata->config.hw_enabled)
> coresight_disclaim_device(drvdata->csdev);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return 0;
> }
>
> @@ -888,7 +888,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
> drvdata->ctidev.ctm_id = 0;
> INIT_LIST_HEAD(&drvdata->ctidev.trig_cons);
>
> - spin_lock_init(&drvdata->spinlock);
> + raw_spin_lock_init(&drvdata->spinlock);
>
> /* initialise CTI driver config values */
> cti_set_default_config(dev, drvdata);
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index d25dd2737b49..572b80ee96fb 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);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> enable_req = drvdata->config.enable_req_count;
> powered = drvdata->config.hw_powered;
> enabled = drvdata->config.hw_enabled;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
>
> if (powered)
> return sprintf(buf, "%d\n", enabled);
> @@ -134,9 +134,9 @@ static ssize_t powered_show(struct device *dev,
> bool powered;
> struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> powered = drvdata->config.hw_powered;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
>
> return sprintf(buf, "%d\n", powered);
> }
> @@ -181,10 +181,10 @@ static ssize_t coresight_cti_reg_show(struct device *dev,
> u32 val = 0;
>
> pm_runtime_get_sync(dev->parent);
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> if (drvdata->config.hw_powered)
> val = readl_relaxed(drvdata->base + cti_attr->off);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> pm_runtime_put_sync(dev->parent);
> return sysfs_emit(buf, "0x%x\n", val);
> }
> @@ -202,10 +202,10 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev,
> return -EINVAL;
>
> pm_runtime_get_sync(dev->parent);
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> if (drvdata->config.hw_powered)
> cti_write_single_reg(drvdata, cti_attr->off, val);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> pm_runtime_put_sync(dev->parent);
> return size;
> }
> @@ -264,7 +264,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;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> if ((reg_offset >= 0) && cti_active(config)) {
> CS_UNLOCK(drvdata->base);
> val = readl_relaxed(drvdata->base + reg_offset);
> @@ -274,7 +274,7 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf,
> } else if (pcached_val) {
> val = *pcached_val;
> }
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return sprintf(buf, "%#x\n", val);
> }
>
> @@ -293,7 +293,7 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf,
> if (kstrtoul(buf, 0, &val))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> /* local store */
> if (pcached_val)
> *pcached_val = (u32)val;
> @@ -301,7 +301,7 @@ 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);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
>
> @@ -349,9 +349,9 @@ static ssize_t inout_sel_store(struct device *dev,
> if (val > (CTIINOUTEN_MAX - 1))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> drvdata->config.ctiinout_sel = val;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
> static DEVICE_ATTR_RW(inout_sel);
> @@ -364,10 +364,10 @@ static ssize_t inen_show(struct device *dev,
> int index;
> struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> index = drvdata->config.ctiinout_sel;
> val = drvdata->config.ctiinen[index];
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return sprintf(buf, "%#lx\n", val);
> }
>
> @@ -383,14 +383,14 @@ static ssize_t inen_store(struct device *dev,
> if (kstrtoul(buf, 0, &val))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&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);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
> static DEVICE_ATTR_RW(inen);
> @@ -403,10 +403,10 @@ static ssize_t outen_show(struct device *dev,
> int index;
> struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> index = drvdata->config.ctiinout_sel;
> val = drvdata->config.ctiouten[index];
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return sprintf(buf, "%#lx\n", val);
> }
>
> @@ -422,14 +422,14 @@ static ssize_t outen_store(struct device *dev,
> if (kstrtoul(buf, 0, &val))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&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);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
> static DEVICE_ATTR_RW(outen);
> @@ -463,7 +463,7 @@ static ssize_t appclear_store(struct device *dev,
> if (kstrtoul(buf, 0, &val))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
>
> /* a 1'b1 in appclr clears down the same bit in appset*/
> config->ctiappset &= ~val;
> @@ -471,7 +471,7 @@ static ssize_t appclear_store(struct device *dev,
> /* write through if enabled */
> if (cti_active(config))
> cti_write_single_reg(drvdata, CTIAPPCLEAR, val);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
> static DEVICE_ATTR_WO(appclear);
> @@ -487,12 +487,12 @@ static ssize_t apppulse_store(struct device *dev,
> if (kstrtoul(buf, 0, &val))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
>
> /* write through if enabled */
> if (cti_active(config))
> cti_write_single_reg(drvdata, CTIAPPPULSE, val);
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
> static DEVICE_ATTR_WO(apppulse);
> @@ -681,9 +681,9 @@ static ssize_t trig_filter_enable_show(struct device *dev,
> u32 val;
> struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> val = drvdata->config.trig_filter_enable;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return sprintf(buf, "%d\n", val);
> }
>
> @@ -697,9 +697,9 @@ static ssize_t trig_filter_enable_store(struct device *dev,
> if (kstrtoul(buf, 0, &val))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> drvdata->config.trig_filter_enable = !!val;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
> static DEVICE_ATTR_RW(trig_filter_enable);
> @@ -728,7 +728,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;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
>
> /* clear the CTI trigger / channel programming registers */
> for (i = 0; i < config->nr_trig_max; i++) {
> @@ -747,7 +747,7 @@ static ssize_t chan_xtrigs_reset_store(struct device *dev,
> if (cti_active(config))
> cti_write_all_hw_regs(drvdata);
>
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
> static DEVICE_ATTR_WO(chan_xtrigs_reset);
> @@ -768,9 +768,9 @@ static ssize_t chan_xtrigs_sel_store(struct device *dev,
> if (val > (drvdata->config.nr_ctm_channels - 1))
> return -EINVAL;
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> drvdata->config.xtrig_rchan_sel = val;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
> return size;
> }
>
> @@ -781,9 +781,9 @@ static ssize_t chan_xtrigs_sel_show(struct device *dev,
> unsigned long val;
> struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
>
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> val = drvdata->config.xtrig_rchan_sel;
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
>
> return sprintf(buf, "%ld\n", val);
> }
> @@ -838,12 +838,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. */
> - spin_lock(&drvdata->spinlock);
> + raw_spin_lock(&drvdata->spinlock);
> for (i = 0; i < config->nr_trig_max; i++) {
> inuse_bits |= config->ctiinen[i];
> inuse_bits |= config->ctiouten[i];
> }
> - spin_unlock(&drvdata->spinlock);
> + raw_spin_unlock(&drvdata->spinlock);
>
> /* inverse bits if printing free channels */
> if (!inuse)
> diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
> index cb9ee616d01f..16e310e7e9d4 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.h
> +++ b/drivers/hwtracing/coresight/coresight-cti.h
> @@ -175,7 +175,7 @@ struct cti_drvdata {
> void __iomem *base;
> struct coresight_device *csdev;
> struct cti_device ctidev;
> - spinlock_t spinlock;
> + raw_spinlock_t spinlock;
> struct cti_config config;
> struct list_head node;
> void (*csdev_release)(struct device *dev);
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>
Reviewed-by: Mike Leach <mike.leach@...aro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Powered by blists - more mailing lists