Re: [PATCH V3 1/7] perf/x86/intel/uncore: Fix discovery unit lookup for multi-die systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message inaccurately claims that a NULL unit can be expected when a CPU die is offline, justifying the removal of a valid WARN_ON_ONCE assertion, but the code path is structurally unreachable for missing units.
--

commit ddb51b3d3e54081e6dbacecec75a18fa773ae364
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Fix discovery unit lookup for multi-die systems

This commit fixes an issue where PMON units with the same unit ID may
retrieve the wrong node on multi-die systems by using box->dieid. It
also removes a WARN_ON_ONCE() under the assumption that a NULL unit
can be expected during CPU die offline/online scenarios.

> Remove WARN_ON_ONCE() because with the above change a NULL unit can be
> expected, e.g. when a CPU die is offline during uncore enumeration and
> the unit is not added to the RB-tree. In this case,
> intel_uncore_find_discovery_unit() returns NULL once the die becomes
> online, and it is expected that the PMU box is not functional for that
> die.

> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index 583cbd06b9b88..60e1200c4691d 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c
> @@ -481,8 +481,8 @@ static u64 intel_generic_uncore_box_ctl(struct intel_uncore_box *box)
>  	struct intel_uncore_discovery_unit *unit;
>  
>  	unit = intel_uncore_find_discovery_unit(box->pmu->type->boxes,
> -						-1, box->pmu->pmu_idx);
> -	if (WARN_ON_ONCE(!unit))
> +						box->dieid, box->pmu->pmu_idx);
> +	if (!unit)
>  		return 0;

[Severity: Medium]
Is the scenario described in the commit message actually possible in the
current codebase?

When a CPU die comes online, uncore_event_cpu_online() invokes
uncore_change_type_ctx(), which relies on uncore_die_has_box() to check
for the unit. If the unit is missing from the RB-tree, uncore_die_has_box()
returns false, and the box cpu assignment is skipped:

arch/x86/events/intel/uncore.c:uncore_change_type_ctx() {
    ...
    if (uncore_die_has_box(type, die, pmu->pmu_idx)) {
        box->cpu = new_cpu;
        cpumask_set_cpu(new_cpu, &pmu->cpu_mask);
    }
    ...
}

Because box->cpu remains -1, uncore_box_ref() bypasses initialization:

arch/x86/events/intel/uncore.c:uncore_box_ref() {
    ...
    if (box && box->cpu >= 0 && atomic_inc_return(&box->refcnt) == 1)
        uncore_box_init(box);
    ...
}

Furthermore, uncore_pmu_event_init() rejects event creations if box->cpu
is negative. Because of this, it appears the PMU operations (init, enable,
disable) are never executed for this box, making the call to
intel_generic_uncore_box_ctl() unreachable.

Does this mean the removed WARN_ON_ONCE(!unit) was actually a valid
assertion, and the !unit check is effectively dead code?

>  
>  	return unit->addr;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602144908.263680-1-zide.chen@intel.com?part=1