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

From: Suzuki K Poulose <[email-masked]>
Date: Mon, 1 Jun 2026 16:38:07 +0100

On 28/05/2026 16:22, Yeoreum Yun wrote:
>> On Thu, May 28, 2026 at 03:26:57PM +0100, Yeoreum Yun wrote:
>>
>> [...]
>>
>>>>> @@ -47,7 +47,7 @@ static int etm4_cfg_map_reg_offset(struct etmv4_drvdata *drvdata,
>>>>>   				   struct cscfg_regval_csdev *reg_csdev, u32 offset)
>>>>>   {
>>>>>   	int err = -EINVAL, idx;
>>>>> -	struct etmv4_config *drvcfg = &drvdata->config;
>>>>> +	struct etmv4_config *drvcfg = &drvdata->active_config;
>>>>
>>>> Wouldn't it make more sense to keep using drvdata->config for cfgfs?
>>>> This would avoid cfgfs updating the active configuration while a session
>>>> is enabled.
>>>>
>>>> My understanding is after refactoring cfgfs, we will never expose
>>>> active_config or config anymore so this should not an issue. Before
>>>> that, maybe we just keep to use config so can avoid mess.
>>>
>>> I don't think so. since the cfgfs's config is updated right before
>>> enable and in this patchset, It's applied after "take the mode".
>>>
>>> If we do it into drvdta->config, then contention would be increased
>>> with the access of sysfs and that nullifies almost this patch purpose
>>> and because of we use "one config" this makes a corruption with perf
>>> and sysfs.
>>
>> Here have two different contention: the contention between sysfs and
>> cfgfs, and the contention between cfgfs and a runtime config (active
>> config). My suggestion is to avoid the later contention, which is the
>> main idea in this series that avoid the runtime config is modified in
>> the middle of a session.
> 
> What I mean the *latter* contention is also pre-existed even in the
> former (Just think about the access via configfs to modify parameter
> while enabling the cfgfs configuration) and this is known issue
> and should be resolved with another patchset (might we can set a flag
> to disable store operation via conifgfs while "activation") and
> make drvdata->config for cscfg just increase the "contention" for lock.

We don't need to resolve it. We consume what is available at the time of
enable. If the user changes this it doesn't matter, we don't have to
honor the request. (Similar to what we do for the size of the ETR
buffer for sysfs mode)

Suzuki

  > And this patch series main idea is not for contention between
> *cfgfs* and *runtime configuration* but the corruption for racebetween
> *sysfs and perf configuration* when it applies for the sharing one
> configuration.
> 
> That's why this is different problem what you said and that is pre-exist
> one. except above case (configfs vs activate cfgfs config),
> I don't believe there is no contention to be modified *runtime* while
> applying the cscfg config into active config and above case might be
> reolved simply by some flag to prevent current value of each feat.
> 
> What contention scenario do you mean?
> 
>>>>>   static void etm4_enable_sysfs_smp_call(void *info)
>>>>>   {
>>>>>   	struct etm4_enable_arg *arg = info;
>>>>> +	struct etmv4_drvdata *drvdata;
>>>>>   	struct coresight_device *csdev;
>>>>> +	unsigned long cfg_hash;
>>>>> +	int preset;
>>>>>   
>>>>>   	if (WARN_ON(!arg))
>>>>>   		return;
>>>>>   
>>>>> -	csdev = arg->drvdata->csdev;
>>>>> +	drvdata = arg->drvdata;
>>>>> +	csdev = drvdata->csdev;
>>>>>   	if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) {
>>>>>   		/* Someone is already using the tracer */
>>>>>   		arg->rc = -EBUSY;
>>>>>   		return;
>>>>>   	}
>>>>>   
>>>>> -	arg->rc = etm4_enable_hw(arg->drvdata);
>>>>> +	/* enable any config activated by configfs */
>>>>> +	cscfg_config_sysfs_get_active_cfg(&cfg_hash, &preset);
>>>>>   
>>>>> -	/* The tracer didn't start */
>>>>> +	drvdata->active_config = arg->config;
>>>>
>>>> Can move this down to just before drvdata->trcid assignment so cscfg
>>>> operations can be put together?
>>>
>>> No. Copy the arg->config must be before cscfg_csdev_enable_active_config().
>>> If that's after, it overrides the "preset" and this is different from
>>> the former behavior.
>>
>> Please copy config first, then do cscfg stuffs.
> 
> It's already did. the chuck of config is:
>    - cscfg_config_sysfs_get_active_cfg()
>    - drvdata->active_config copy
>    - cscfg_csdev_enable_active_config()
> 
> Why do I need to seperate the copying the config though it's a part of
> "setting" configuration chunk?
> 
>>
>>>>> +	arg->rc = etm4_enable_hw(drvdata);
>>>>>   	if (arg->rc) {
>>>>> -		coresight_set_mode(csdev, CS_MODE_DISABLED);
>>>>> -		return;
>>>>> +		cscfg_csdev_disable_active_config(csdev);
>>>>> +		goto err;
>>>>
>>>> Add a new goto tag (like err_enable_hw) for the disable_active_config
>>>> rollback?
>>>
>>> This is only place where cscfg_csdev_disable_active_config().
>>> Why do we need to goto for cleanup where there is no duplication?
>>
>> It is about to use a central place for handling errors.
> 
> I don't think so. cscfg_csdev_disable_active_config() should be called
> when cscfg_csdev_enable_active_config() is succssed seem not to be
> called freely when cscfg_csdev_enable_active_config() isn't call.
> IOW, for the central place, it makes a another variable to differenciate
> each case and this seems more ugly.
> 
> What the benefit for this?
> 
>>
>>>>> @@ -1449,7 +1458,7 @@ static void etm4_init_arch_data(void *info)
>>>>>   
>>>>>   	/* EXLEVEL_S, bits[19:16] Secure state instruction tracing */
>>>>>   	caps->s_ex_level = FIELD_GET(TRCIDR3_EXLEVEL_S_MASK, etmidr3);
>>>>> -	drvdata->config.s_ex_level = caps->s_ex_level;
>>>>> +	config->s_ex_level = caps->s_ex_level;
>>>>
>>>> config->s_ex_level is redundant and not really a configuration item.
>>>> We should use caps->s_ex_level instead of config->s_ex_level wherever
>>>> it is used.
>>>
>>> Agree, but this includes it should change the omse function
>>> declations to pass "caps" argument:
>>>    - etm4_set_comparator_filter()
>>>    - etm4_set_start_stop_filter()
>>>
>>> If that's okay, I'll respin with it.
>>
>> This is fine for me.
> 
> Okay.
> 
>>
>> Thanks,
>> Leo
>