Re: [PATCH] perf/arm-cmn: Fix multi-filter encoding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Broken Multi-Filter Detection / Dead Code
- [High] Error Handling / Reference Count Corruption
--

commit fec40d747e8bd7f98185ffd85e0e2e426200c368
Author: Robin Murphy <robin.murphy@arm.com>
Date:   Mon Sep 14 17:50:53 2026 +0000

    perf/arm-cmn: Fix multi-filter encoding

This patch reorders the programming of multi-filters for EVICT_STATE_SEL
events so that they match the logical order based on their bitfield
positions, updating the UABI to align with the CMN S3 r2 TRM.

> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 33ee2be9b3864..b1eabd1ac0d61 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c

[ ... ]

> @@ -1014,13 +1018,9 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
>  	_CMN_EVENT_HNS(CMNS3R2, _name##_ccglcn, _event, SEL_ENHANCED_HBT_LBT_SEL, 5), \
>  	_CMN_EVENT_HNS(CMNS3R2, _name##_ccgrn, _event, SEL_ENHANCED_HBT_LBT_SEL, 6)
>  #define CMN_EVENT_HNS_EVICT(_model, _name, _event)			\
> -	_CMN_EVENT_HNS_HBT2(_model, _name##_all, _event, SEL_EVICT_STATE_SEL, 0), \
> -	_CMN_EVENT_HNS_HBT2(_model, _name##_eu, _event, SEL_EVICT_STATE_SEL, 1), \
> -	_CMN_EVENT_HNS_HBT2(_model, _name##_en, _event, SEL_EVICT_STATE_SEL, 2), \
> -	_CMN_EVENT_HNS_HBT2(_model, _name##_su, _event, SEL_EVICT_STATE_SEL, 3), \
> -	_CMN_EVENT_HNS_HBT2(_model, _name##_sn, _event, SEL_EVICT_STATE_SEL, 4), \
> -	_CMN_EVENT_HNS_HBT2(_model, _name##_mu, _event, SEL_EVICT_STATE_SEL, 5), \
> -	_CMN_EVENT_HNS_HBT2(_model, _name##_mn, _event, SEL_EVICT_STATE_SEL, 6)
> +	_CMN_EVENT_HNS_EVICT(_model, _name##_all, _event, SEL_HBT_LBT_SEL, 0), \
> +	_CMN_EVENT_HNS_EVICT(_model, _name##_hbt, _event, SEL_HBT_LBT_SEL, 1), \
> +	_CMN_EVENT_HNS_EVICT(_model, _name##_lbt, _event, SEL_HBT_LBT_SEL, 2)

[Severity: High]
Does this change effectively make the dual-filter logic unreachable?

By passing SEL_HBT_LBT_SEL as the primary filter selector here, it becomes
assigned to hw->filter_sel during event initialization.

The internal validation and configuration logic across arm_cmn_val_add_event(),
arm_cmn_validate_group(), and arm_cmn_set_event_filter() all check if the
selector is SEL_EVICT_STATE_SEL to configure the secondary filter:

    if (sel == SEL_EVICT_STATE_SEL) {
        val->filter[dtm][sel] = CMN_EVENT_FILTER2(event) + 1;
        sel = SEL_HBT_LBT_SEL;
    }

Since the primary selector is now SEL_HBT_LBT_SEL, these checks will always
evaluate to false, meaning the driver will treat these as standard
single-filter events and the secondary filter will never be validated or
programmed to the hardware.

[ ... ]

> @@ -2081,16 +2086,19 @@ static void arm_cmn_event_clear(struct arm_cmn *cmn, struct perf_event *event,
>  static int arm_cmn_set_event_filter(struct arm_cmn_node *dn, struct perf_event *event)
>  {
>  	enum cmn_filter_select fsel = to_cmn_hw(event)->filter_sel;
> +	bool evict_state = fsel == SEL_EVICT_STATE_SEL;
>  	int ret = 0;
>  
> +	if (evict_state) {
> +		ret = arm_cmn_set_event_sel_hi(dn, fsel, CMN_EVENT_FILTER2(event));
> +		fsel = SEL_HBT_LBT_SEL;
> +	}
>  	if (fsel)
>  		ret = arm_cmn_set_event_sel_hi(dn, fsel, CMN_EVENT_FILTER(event));
>  
> -	if (fsel == SEL_EVICT_STATE_SEL && !ret) {
> -		ret = arm_cmn_set_event_sel_hi(dn, SEL_HBT_LBT_SEL, CMN_EVENT_FILTER2(event));
> -		if (ret)
> -			dn->filter[fsel].count--;
> -	}
> +	if (ret && evict_state)
> +		dn->filter[SEL_EVICT_STATE_SEL].count--;
> +
>  	return ret;
>  }

[Severity: High]
If we assume the macro bug above is fixed and evict_state can be true,
does the error handling here mask failures or corrupt the reference count?

If the first arm_cmn_set_event_sel_hi() call fails (e.g. returning -EBUSY),
it does not increment the count for SEL_EVICT_STATE_SEL.

The code then unconditionally proceeds to the second call of
arm_cmn_set_event_sel_hi() since fsel is now SEL_HBT_LBT_SEL. If this
second call succeeds, it will overwrite the error in ret with 0, silently
ignoring the first failure.

If the second call also fails, ret remains non-zero. The rollback block then
decrements dn->filter[SEL_EVICT_STATE_SEL].count. Since the first call failed
and never incremented this count, could this decrement an unrelated active
event's reference or cause this unsigned bitfield to underflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/03f15158e4c0a036f41cb4ab34c138a265c0730c.1789408253.git.robin.murphy@arm.com?part=1