Re: [PATCH 2/9] coresight-etm4x: change etmv4_drvdata spinlock type to raw_spinlock_t

[James Clark <james.clark@linaro.org>]

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.