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

[Yeoreum Yun <yeoreum.yun@arm.com>]

> 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.
> 

Okay. Then I'll apply with a little bit of modification.
Thanks


-- 
Sincerely,
Yeoreum Yun