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

[Yeoreum Yun]

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@xxxxxxx>
> > ---
> >  .../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.

-- 
Sincerely,
Yeoreum Yun