On 25/11/2024 9:48 am, Yeoreum Yun wrote:
From: Levi Yun yeoreum.yun@arm.com
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@arm.com
[...]
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.