Re: [PATCH 1/2] perf/x86/intel: Fix redundant branch type check in intel_pmu_lbr_filter()

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] Meaningful code and commit message mismatch due to a false premise
--

commit 98c2839085a2b609f94d9f3f338217d29525fd08
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Fix redundant branch type check in intel_pmu_lbr_filter()

This commit modifies the branch filter logic to mask out privilege level
bits before checking if a branch type is X86_BR_NONE.

> In intel_pmu_lbr_filter(), the 'type' variable is bitwise ORed with
> 'to_plm' (which contains X86_BR_USER and/or X86_BR_KERNEL bits). Because
> of this, 'type' can never equal X86_BR_NONE (0) after the assignment.
> 
> As a result, the subsequent check 'if (type == X86_BR_NONE)' is dead code
> and the entries with X86_BR_NONE type would not be skipped eventually.

Is this description entirely accurate? 

Looking at intel_pmu_lbr_filter(), the bitwise OR with to_plm only happens
in the X86_FEATURE_ARCH_LBR path:

intel_pmu_lbr_filter() {
    ...
    if (static_cpu_has(X86_FEATURE_ARCH_LBR) &&
        type <= ARCH_LBR_BR_TYPE_KNOWN_MAX) {
        to_plm = kernel_ip(to) ? X86_BR_KERNEL : X86_BR_USER;
        type = arch_lbr_br_type_map[type] | to_plm;
    } else
        type = branch_type(from, to, cpuc->lbr_entries[i].abort);
    ...
}

In the software fallback path, type is assigned the result of branch_type().
If branch_type() cannot decode the branch and returns X86_BR_NONE (0),
wouldn't type evaluate to exactly X86_BR_NONE without any to_plm bits set?

> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> index 72f2adcda7c68..16977e4c6f8a5 100644
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -1245,7 +1245,7 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
>  		}
>  
>  		/* if type does not correspond, then discard */
> -		if (type == X86_BR_NONE || (br_sel & type) != type) {
> +		if ((type & ~X86_BR_PLM) == X86_BR_NONE || (br_sel & type) != type) {

While masking out ~X86_BR_PLM doesn't appear to break the software fallback
behavior since (0 & ~X86_BR_PLM) == 0, the original check for X86_BR_NONE
appears to have been functional and necessary for that path.

Could the commit message be updated to clarify that the check was not actually
dead code for the software fallback path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260414021440.928068-1-dapeng1.mi@linux.intel.com?part=1