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