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

[Yeoreum Yun <yeoreum.yun@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?

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

-- 
Sincerely,
Yeoreum Yun