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

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

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