Re: [PATCH v7 08/13] coresight: etm4x: fix inconsistencies with sysfs configuration
From: Suzuki K Poulose
Date: Mon Jun 01 2026 - 12:01:32 EST
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