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

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

[Mi, Dapeng]

On 4/14/2026 10:53 AM, sashiko-bot@kernel.org wrote:
> 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?

No, the branch_type() would bitwise ored the to_plm at last as below code
shows.

```

/*
     * branch priv level determined by target as
     * is done by HW when LBR_SELECT is implemented
     */
    if (ret != X86_BR_NONE)
        ret |= to_plm;

```

Thanks.


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