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

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;
  

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