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

[Leo Yan <leo.yan@arm.com>]

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