Re: [PATCH 2/9] coresight-etm4x: change etmv4_drvdata spinlock type to raw_spinlock_t
From: James Clark
Date: Wed Nov 27 2024 - 11:30:58 EST
On 25/11/2024 9:48 am, Yeoreum Yun wrote:
From: Levi Yun <yeoreum.yun@xxxxxxx>
In coresight-etm4x drivers, etmv4_drvdata->spinlock can be held during
__schedule() by perf_event_task_sched_out()/in().
Since etmv4_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 etmv4_drvdata->spinlock
in coresight-etm4x 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@xxxxxxx>
---
[...]
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
index a9f19629f3f8..2e6b79c37f87 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
@@ -174,99 +174,99 @@ static ssize_t reset_store(struct device *dev,
if (kstrtoul(buf, 16, &val))
return -EINVAL;
- spin_lock(&drvdata->spinlock);
- if (val)
- config->mode = 0x0;
+ scoped_guard(raw_spinlock, &drvdata->spinlock) {
+ if (val)
+ config->mode = 0x0;
- /* Disable data tracing: do not trace load and store data transfers */
- config->mode &= ~(ETM_MODE_LOAD | ETM_MODE_STORE);
- config->cfg &= ~(TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE);
+ /* Disable data tracing: do not trace load and store data transfers */
+ config->mode &= ~(ETM_MODE_LOAD | ETM_MODE_STORE);
+ config->cfg &= ~(TRCCONFIGR_INSTP0_LOAD | TRCCONFIGR_INSTP0_STORE);
- /* Disable data value and data address tracing */
- config->mode &= ~(ETM_MODE_DATA_TRACE_ADDR |
- ETM_MODE_DATA_TRACE_VAL);
- config->cfg &= ~(TRCCONFIGR_DA | TRCCONFIGR_DV);
+ /* Disable data value and data address tracing */
+ config->mode &= ~(ETM_MODE_DATA_TRACE_ADDR |
+ ETM_MODE_DATA_TRACE_VAL);
+ config->cfg &= ~(TRCCONFIGR_DA | TRCCONFIGR_DV);
- /* Disable all events tracing */
- config->eventctrl0 = 0x0;
- config->eventctrl1 = 0x0;
+ /* Disable all events tracing */
+ config->eventctrl0 = 0x0;
+ config->eventctrl1 = 0x0;
- /* Disable timestamp event */
- config->ts_ctrl = 0x0;
+ /* Disable timestamp event */
+ config->ts_ctrl = 0x0;
- /* Disable stalling */
- config->stall_ctrl = 0x0;
+ /* Disable stalling */
+ config->stall_ctrl = 0x0;
- /* Reset trace synchronization period to 2^8 = 256 bytes*/
- if (drvdata->syncpr == false)
- config->syncfreq = 0x8;
+ /* Reset trace synchronization period to 2^8 = 256 bytes*/
+ if (drvdata->syncpr == false)
+ config->syncfreq = 0x8;
- /*
- * Enable ViewInst to trace everything with start-stop logic in
- * started state. ARM recommends start-stop logic is set before
- * each trace run.
- */
- config->vinst_ctrl = FIELD_PREP(TRCVICTLR_EVENT_MASK, 0x01);
- if (drvdata->nr_addr_cmp > 0) {
- config->mode |= ETM_MODE_VIEWINST_STARTSTOP;
- /* SSSTATUS, bit[9] */
- config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
- }
+ /*
+ * Enable ViewInst to trace everything with start-stop logic in
+ * started state. ARM recommends start-stop logic is set before
+ * each trace run.
+ */
+ config->vinst_ctrl = FIELD_PREP(TRCVICTLR_EVENT_MASK, 0x01);
+ if (drvdata->nr_addr_cmp > 0) {
+ config->mode |= ETM_MODE_VIEWINST_STARTSTOP;
+ /* SSSTATUS, bit[9] */
+ config->vinst_ctrl |= TRCVICTLR_SSSTATUS;
+ }
- /* No address range filtering for ViewInst */
- config->viiectlr = 0x0;
+ /* No address range filtering for ViewInst */
+ config->viiectlr = 0x0;
- /* No start-stop filtering for ViewInst */
- config->vissctlr = 0x0;
- config->vipcssctlr = 0x0;
+ /* No start-stop filtering for ViewInst */
+ config->vissctlr = 0x0;
+ config->vipcssctlr = 0x0;
- /* Disable seq events */
- for (i = 0; i < drvdata->nrseqstate-1; i++)
- config->seq_ctrl[i] = 0x0;
- config->seq_rst = 0x0;
- config->seq_state = 0x0;
+ /* Disable seq events */
+ for (i = 0; i < drvdata->nrseqstate-1; i++)
+ config->seq_ctrl[i] = 0x0;
+ config->seq_rst = 0x0;
+ config->seq_state = 0x0;
- /* Disable external input events */
- config->ext_inp = 0x0;
+ /* Disable external input events */
+ config->ext_inp = 0x0;
- config->cntr_idx = 0x0;
- for (i = 0; i < drvdata->nr_cntr; i++) {
- config->cntrldvr[i] = 0x0;
- config->cntr_ctrl[i] = 0x0;
- config->cntr_val[i] = 0x0;
- }
+ config->cntr_idx = 0x0;
+ for (i = 0; i < drvdata->nr_cntr; i++) {
+ config->cntrldvr[i] = 0x0;
+ config->cntr_ctrl[i] = 0x0;
+ config->cntr_val[i] = 0x0;
+ }
- config->res_idx = 0x0;
- for (i = 2; i < 2 * drvdata->nr_resource; i++)
- config->res_ctrl[i] = 0x0;
+ config->res_idx = 0x0;
+ for (i = 2; i < 2 * drvdata->nr_resource; i++)
+ config->res_ctrl[i] = 0x0;
- config->ss_idx = 0x0;
- for (i = 0; i < drvdata->nr_ss_cmp; i++) {
- config->ss_ctrl[i] = 0x0;
- config->ss_pe_cmp[i] = 0x0;
- }
+ config->ss_idx = 0x0;
+ for (i = 0; i < drvdata->nr_ss_cmp; i++) {
+ config->ss_ctrl[i] = 0x0;
+ config->ss_pe_cmp[i] = 0x0;
+ }
- config->addr_idx = 0x0;
- for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
- config->addr_val[i] = 0x0;
- config->addr_acc[i] = 0x0;
- config->addr_type[i] = ETM_ADDR_TYPE_NONE;
- }
+ config->addr_idx = 0x0;
+ for (i = 0; i < drvdata->nr_addr_cmp * 2; i++) {
+ config->addr_val[i] = 0x0;
+ config->addr_acc[i] = 0x0;
+ config->addr_type[i] = ETM_ADDR_TYPE_NONE;
+ }
- config->ctxid_idx = 0x0;
- for (i = 0; i < drvdata->numcidc; i++)
- config->ctxid_pid[i] = 0x0;
+ config->ctxid_idx = 0x0;
+ for (i = 0; i < drvdata->numcidc; i++)
+ config->ctxid_pid[i] = 0x0;
- config->ctxid_mask0 = 0x0;
- config->ctxid_mask1 = 0x0;
+ config->ctxid_mask0 = 0x0;
+ config->ctxid_mask1 = 0x0;
- config->vmid_idx = 0x0;
- for (i = 0; i < drvdata->numvmidc; i++)
- config->vmid_val[i] = 0x0;
- config->vmid_mask0 = 0x0;
- config->vmid_mask1 = 0x0;
+ config->vmid_idx = 0x0;
+ for (i = 0; i < drvdata->numvmidc; i++)
+ config->vmid_val[i] = 0x0;
- spin_unlock(&drvdata->spinlock);
+ config->vmid_mask0 = 0x0;
+ config->vmid_mask1 = 0x0;
+ }
There's a lot of churn in this function just to use the new scoped
guard, but there was only one lock and unlock anyway. There are a few
other cases of this in the whole set.
The scoped guards make it easier to write correct code, but I'm not sure
we gain anything here other than a bigger diff and more to review by
changing already working code. Having said that I did review all the
changes and I'm pretty sure they're all ok, so I'm on the fence about
whether it's worth going back and undoing all of them.
Surely updating to raw spinlocks is a search and replace to fix a
specific problem, and the scoped guard stuff can be done on a case by
case basis when anything is re-written in the future?
I don't know if we're considering a fixes tag, if PREEMPT_RT is 6.12?
It's probably not necessary but if so we definitely want to minimise the
change.