Re: [PATCH v7 09/13] coresight: etm4x: missing cscfg_csdev_disable_active_config() in perf enable

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;
  

  } else {

@@ -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);

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.