Re: [PATCH v7 09/13] coresight: etm4x: missing cscfg_csdev_disable_active_config() in perf enable
On Tue, May 19, 2026 at 04:48:08PM +0100, Yeoreum Yun wrote:
In the perf enable path, there are missing cases where cscfg_csdev_disable_active_config() is not called:
- Branch broadcast is selected but not supported by the hardware
- etm4_enable_hw() fails
This can lead to a leak of config_desc->active_cnt. Fix this by properly calling cscfg_csdev_disable_active_config() in these error paths.
Fixes: 810ac401db1f ("coresight: etm4x: Add complex configuration handlers to etmv4") Signed-off-by: Yeoreum Yun yeoreum.yun@arm.com
.../hwtracing/coresight/coresight-etm4x-core.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index f0a9f2ef4b27..0889937811cb 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -899,6 +899,8 @@ 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;@@ -915,6 +917,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, struct coresight_path *path) { struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- struct perf_event_attr *attr = &event->attr; int ret;
if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) @@ -926,7 +929,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, /* Configure the tracer based on the session's specifics */ ret = etm4_parse_event_config(csdev, event); if (ret)
goto out;
goto err;drvdata->trcid = path->trace_id; @@ -935,16 +938,19 @@ static int etm4_enable_perf(struct coresight_device *csdev, /* And enable it */ ret = etm4_enable_hw(drvdata);
-out:
- /* Failed to start tracer; roll back to DISABLED mode */ if (ret) {
coresight_set_mode(csdev, CS_MODE_DISABLED);return ret;
if (ATTR_CFG_GET_FLD(attr, configid))cscfg_csdev_disable_active_config(csdev);goto err;csdev->path = path; return 0;
+err:
- /* Failed to start tracer; roll back to DISABLED mode */
- coresight_set_mode(csdev, CS_MODE_DISABLED);
- return ret;
}
Seems to me, it is not readable if spread error handling into both child and parent functions. I would like we can stick to use the same function for resource allocation and rollback for failures.
How about below change (I did not verify it yet):
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 0889937811cb..8347efb2069b 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -794,8 +794,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, .ATTR_CFG_FLD_timestamp_CFG = U64_MAX, }; struct perf_event_attr *attr = &event->attr; - unsigned long cfg_hash; - int preset, cc_threshold; + int cc_threshold; u8 ts_level;
/* Clear configuration from previous run */ @@ -882,16 +881,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 +888,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 { @@ -918,7 +905,8 @@ static int etm4_enable_perf(struct coresight_device *csdev, { struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct perf_event_attr *attr = &event->attr; - int ret; + unsigned long cfg_hash; + int preset, ret;
if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) return -EINVAL; @@ -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; + } + drvdata->trcid = path->trace_id;
/* Populate pause state */ @@ -938,15 +938,15 @@ 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_enable_hw;
csdev->path = path; return 0;
+err_enable_hw: + if (cfg_hash) + cscfg_csdev_disable_active_config(csdev); err: /* Failed to start tracer; roll back to DISABLED mode */ coresight_set_mode(csdev, CS_MODE_DISABLED);
Hi Leo,
On Tue, May 19, 2026 at 04:48:08PM +0100, Yeoreum Yun wrote:
In the perf enable path, there are missing cases where cscfg_csdev_disable_active_config() is not called:
- Branch broadcast is selected but not supported by the hardware
- etm4_enable_hw() fails
This can lead to a leak of config_desc->active_cnt. Fix this by properly calling cscfg_csdev_disable_active_config() in these error paths.
Fixes: 810ac401db1f ("coresight: etm4x: Add complex configuration handlers to etmv4") Signed-off-by: Yeoreum Yun yeoreum.yun@arm.com
.../hwtracing/coresight/coresight-etm4x-core.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index f0a9f2ef4b27..0889937811cb 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -899,6 +899,8 @@ 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;@@ -915,6 +917,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, struct coresight_path *path) { struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- struct perf_event_attr *attr = &event->attr; int ret;
if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) @@ -926,7 +929,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, /* Configure the tracer based on the session's specifics */ ret = etm4_parse_event_config(csdev, event); if (ret)
goto out;
goto err;drvdata->trcid = path->trace_id; @@ -935,16 +938,19 @@ static int etm4_enable_perf(struct coresight_device *csdev, /* And enable it */ ret = etm4_enable_hw(drvdata);
-out:
- /* Failed to start tracer; roll back to DISABLED mode */ if (ret) {
coresight_set_mode(csdev, CS_MODE_DISABLED);return ret;
if (ATTR_CFG_GET_FLD(attr, configid))cscfg_csdev_disable_active_config(csdev);goto err;csdev->path = path; return 0;
+err:
- /* Failed to start tracer; roll back to DISABLED mode */
- coresight_set_mode(csdev, CS_MODE_DISABLED);
- return ret;
}
Seems to me, it is not readable if spread error handling into both child and parent functions. I would like we can stick to use the same function for resource allocation and rollback for failures.
How about below change (I did not verify it yet):
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 0889937811cb..8347efb2069b 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -794,8 +794,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, .ATTR_CFG_FLD_timestamp_CFG = U64_MAX, }; struct perf_event_attr *attr = &event->attr;
- unsigned long cfg_hash;
- int preset, cc_threshold;
- int cc_threshold; u8 ts_level;
/* Clear configuration from previous run */ @@ -882,16 +881,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 +888,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;@@ -918,7 +905,8 @@ static int etm4_enable_perf(struct coresight_device *csdev, { struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); struct perf_event_attr *attr = &event->attr;
- int ret;
- unsigned long cfg_hash;
- int preset, ret;
if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id())) return -EINVAL; @@ -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;- }
- drvdata->trcid = path->trace_id;
/* Populate pause state */ @@ -938,15 +938,15 @@ 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_enable_hw;csdev->path = path; return 0; +err_enable_hw:
- if (cfg_hash)
cscfg_csdev_disable_active_config(csdev);err: /* Failed to start tracer; roll back to DISABLED mode */ coresight_set_mode(csdev, CS_MODE_DISABLED);
No. since preset overrides the "perf configuratoin" formerly but this code makes it vice versa. 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.
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);
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?
Ah sorry. I've misread the code location that was my bad.
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.
Funcionally, Code works. However, To be honest, the pairing between etm4_parse_event_config() and etm4_clean_event_config() feels a bit artificial to me.
So here I have simply followed the principle that, if etm4_parse_event_config() fails, the configuration it touched should be cleaned up within that function; and if a failure happens after etm4_parse_event_config() has succeeded, the caller should perform the cleanup.
Renaming etm4_parse_event_config() and splitting out the CSCFG-related handling as suggested would be possible, although I still feel it may not be strictly necessary.
My preference would be to keep this as-is, but Suzuki, what do you think?
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;@@ -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);
On 28/05/2026 17:01, Yeoreum Yun wrote:
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?
Ah sorry. I've misread the code location that was my bad.
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.
Funcionally, Code works. However, To be honest, the pairing between etm4_parse_event_config() and etm4_clean_event_config() feels a bit artificial to me.
So here I have simply followed the principle that, if etm4_parse_event_config() fails, the configuration it touched should be cleaned up within that function; and if a failure happens after etm4_parse_event_config() has succeeded, the caller should perform the cleanup.
Renaming etm4_parse_event_config() and splitting out the CSCFG-related handling as suggested would be possible, although I still feel it may not be strictly necessary.
My preference would be to keep this as-is, but Suzuki, what do you think?
I prefer the end result with Leo's patch applied. That kind of indicates clearly that we need to cleanup something from the event config parsing and keeps the disabling only once, rather than spreading it.
Cheers Suzuki
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;@@ -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);
On 28/05/2026 17:01, Yeoreum Yun wrote:
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?
Ah sorry. I've misread the code location that was my bad.
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.
Funcionally, Code works. However, To be honest, the pairing between etm4_parse_event_config() and etm4_clean_event_config() feels a bit artificial to me.
So here I have simply followed the principle that, if etm4_parse_event_config() fails, the configuration it touched should be cleaned up within that function; and if a failure happens after etm4_parse_event_config() has succeeded, the caller should perform the cleanup.
Renaming etm4_parse_event_config() and splitting out the CSCFG-related handling as suggested would be possible, although I still feel it may not be strictly necessary.
My preference would be to keep this as-is, but Suzuki, what do you think?
I prefer the end result with Leo's patch applied. That kind of indicates clearly that we need to cleanup something from the event config parsing and keeps the disabling only once, rather than spreading it.
Okay. Then I'll apply with a little bit of modification. Thanks
-
Leo Yan -
Suzuki K Poulose -
Yeoreum Yun