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

[Suzuki K Poulose]

On 28/05/2026 17:01, Yeoreum Yun wrote:
> > 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?

I prefer the end result with Leo's patch applied. That kind of indicates
clearly that we need to cleanup something from the event config parsing
and keeps the disabling only once, rather than spreading it.

Cheers
Suzuki



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