On Thu, May 28, 2026 at 03:43:40PM +0100, Yeoreum Yun wrote:
[...]
@@ -931,6 +919,18 @@ static int etm4_enable_perf(struct coresight_device *csdev, if (ret) goto err;
- /*
* Set any selected configuration and preset. A zero configid means no* configuration active, preset = 0 means no preset selected.*/- cfg_hash = ATTR_CFG_GET_FLD(attr, configid);
- if (cfg_hash) {
preset = ATTR_CFG_GET_FLD(attr, preset);ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);if (ret)goto err;- }
No. since preset overrides the "perf configuratoin" formerly but this code makes it vice versa.
The above proposed change applies cfgfs after calling etm4_parse_event_config(). This is just use preset to override the config. Do I miss anything?
Also, cfg_hash and prest is also part of etm4_parse_event_config(), and it doesn't seem to good to separate cfgfs handling from that function.
IMHO, It would be better to keep this as it is.
I have another version to give a try. I'd leave to you and maintainers to choose which is better.
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 0889937811cb..471824234800 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -882,16 +882,6 @@ static int etm4_parse_event_config(struct coresight_device *csdev, /* bit[12], Return stack enable bit */ config->cfg |= TRCCONFIGR_RS;
- /* - * Set any selected configuration and preset. A zero configid means no - * configuration active, preset = 0 means no preset selected. - */ - cfg_hash = ATTR_CFG_GET_FLD(attr, configid); - if (cfg_hash) { - preset = ATTR_CFG_GET_FLD(attr, preset); - ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); - } - /* branch broadcast - enable if selected and supported */ if (ATTR_CFG_GET_FLD(attr, branch_broadcast)) { if (!caps->trcbb) { @@ -899,8 +889,6 @@ static int etm4_parse_event_config(struct coresight_device *csdev, * Missing BB support could cause silent decode errors * so fail to open if it's not supported. */ - if (cfg_hash) - cscfg_csdev_disable_active_config(csdev); ret = -EINVAL; goto out; } else { @@ -908,10 +896,31 @@ static int etm4_parse_event_config(struct coresight_device *csdev, } }
+ /* + * Set any selected configuration and preset. A zero configid means no + * configuration active, preset = 0 means no preset selected. + */ + cfg_hash = ATTR_CFG_GET_FLD(attr, configid); + if (cfg_hash) { + preset = ATTR_CFG_GET_FLD(attr, preset); + ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset); + } + out: return ret; }
+static void etm4_clean_event_config(struct coresight_device *csdev, + struct perf_event *event) +{ + struct perf_event_attr *attr = &event->attr; + unsigned long cfg_hash; + + cfg_hash = ATTR_CFG_GET_FLD(attr, configid); + if (cfg_hash) + cscfg_csdev_disable_active_config(csdev); +} + static int etm4_enable_perf(struct coresight_device *csdev, struct perf_event *event, struct coresight_path *path) @@ -938,15 +947,14 @@ static int etm4_enable_perf(struct coresight_device *csdev,
/* And enable it */ ret = etm4_enable_hw(drvdata); - if (ret) { - if (ATTR_CFG_GET_FLD(attr, configid)) - cscfg_csdev_disable_active_config(csdev); - goto err; - } + if (ret) + goto err_hw;
csdev->path = path; return 0;
+err_hw: + etm4_clean_event_config(csdev, event); err: /* Failed to start tracer; roll back to DISABLED mode */ coresight_set_mode(csdev, CS_MODE_DISABLED);