Re: [PATCH v8 08/13] coresight: etm4x: fix inconsistencies with sysfs configuration

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 06/07/2026 18:39, Leo Yan wrote:
> On Mon, Jun 29, 2026 at 10:00:01AM +0100, Yeoreum Yun wrote:
> 
> [...]
> 
>> @@ -922,25 +950,7 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
>>   {
>>   	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>   	struct etm4_enable_arg arg = { };
>> -	unsigned long cfg_hash;
>> -	int ret, preset;
>> -
>> -	/* enable any config activated by configfs */
>> -	cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
>> -	if (cfg_hash) {
>> -		ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
>> -		if (ret) {
>> -			etm4_release_trace_id(drvdata);
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	raw_spin_lock(&drvdata->spinlock);
>> -
>> -	drvdata->trcid = path->trace_id;
>> -
>> -	/* Tracer will never be paused in sysfs mode */
>> -	drvdata->paused = false;
>> +	int ret;
>>   
>>   	/*
>>   	 * Executing etm4_enable_hw on the cpu whose ETM is being enabled
>> @@ -948,20 +958,20 @@ static int etm4_enable_sysfs(struct coresight_device *csdev, struct coresight_pa
>>   	 */
>>   	arg.drvdata = drvdata;
>>   	arg.path = path;
>> +
>> +	raw_spin_lock(&drvdata->spinlock);
>> +	arg.config = drvdata->config;
>> +	raw_spin_unlock(&drvdata->spinlock);
>> +
>>   	ret = smp_call_function_single(drvdata->cpu,
>>   				       etm4_enable_sysfs_smp_call, &arg, 1);
>>   	if (!ret)
>>   		ret = arg.rc;
>>   	if (!ret)
>> -		drvdata->sticky_enable = true;
>> -
>> -	if (ret)
>> +		dev_dbg(&csdev->dev, "ETM tracing enabled\n");
>> +	else
>>   		etm4_release_trace_id(drvdata);
>>   
>> -	raw_spin_unlock(&drvdata->spinlock);
>> -
>> -	if (!ret)
>> -		dev_dbg(&csdev->dev, "ETM tracing enabled\n");
>>   	return ret;
>>   }
> 
> This is most valuable change for me, as now we will have much clear
> scope for what is protected ("drvdata->config").
> 
> However, a corner case was mentioned by Sashiko:
> 
>   | It appears etm4_enable_hw() modifies drvdata->ss_status while executing
>   | via IPI, but sshot_ctrl_store() can modify the same array concurrently

Could we stop modifying the drvdata->ss_status in enable_hw() ? We moved
it outside the config claiming that the status is not used(or useful). #
Then why not remove the updating from enable_hw()

Suzuki


>   | from process context since the lock is no longer held across the
>   | smp_call_function_single() call.
> 
> "drvdata->ss_status" is a race condition between SMP call and sysfs
> knobs. Should we change to spinlock_irqsave/irqrestore when access
> drvdata->ss_status?
> 
> Thanks,
> Leo