Re: [PATCH 2/3] perf/x86/amd/uncore: Convert AMD driver to common PMU helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Context migration logic incorrectly alters the designated PMU CPU when a non-designated sibling CPU goes offline, permanently breaking PMU hrtimers and event polling.
- [High] Module initialization failure during CPU hotplug setup leaks previously registered PMUs and their allocated structures, leading to a dangling pointer in the kernel's PMU list.
--

commit 9535e3c8779a5fdb3921bce37f7ad4c941fd34c9
Author: Qi Liu <liuqi@hygon.cn>

perf/x86/amd/uncore: Convert AMD driver to common PMU helpers
    
Use the common uncore PMU helpers for AMD uncore event handling and
per-CPU context management.

> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index dbc00b6dd69ee..9cb8b0d1afe7a 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
[ ... ]
> @@ -1087,41 +654,40 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore, unsigned int cpu)
[ ... ]
> -static struct amd_uncore uncores[UNCORE_TYPE_MAX] = {
> +static struct uncore_common uncores[UNCORE_TYPE_MAX] = {
>  	/* UNCORE_TYPE_DF */
>  	{
>  		.scan = amd_uncore_df_ctx_scan,
>  		.init = amd_uncore_df_ctx_init,
> -		.move = amd_uncore_ctx_move,
> -		.free = amd_uncore_ctx_free,
> +		.move = uncore_common_ctx_move,
> +		.free = uncore_common_ctx_free,
>  	},

[Severity: High]
This isn't a bug introduced by this patch, but while moving the context
migration logic to uncore_common_ctx_move(), I noticed a potential issue
when a sibling CPU is taken offline.

In the migration loop, if the offline CPU is not the currently designated CPU,
will the loop pick the first online sibling CPU and forcibly update next->cpu
to it without checking if the offline CPU was the active one?

If this happens, the hrtimer would never start on the newly designated CPU
because no events were actually migrated. When the existing hrtimer fires
on the originally designated CPU, it evaluates this check in
uncore_common_hrtimer():

    if (!ctx->nr_active || ctx->cpu != smp_processor_id())
        return HRTIMER_NORESTART;

Would this mismatched CPU cause the pinned hrtimer to permanently return
HRTIMER_NORESTART, stopping the PMU event polling?

[ ... ]
> @@ -1138,7 +706,7 @@ static int __init amd_uncore_init(void)
>  		BUG_ON(!uncore->move);
>  		BUG_ON(!uncore->free);
>  
> -		uncore->info = alloc_percpu(union amd_uncore_info);
> +		uncore->info = alloc_percpu(union uncore_common_info);
>  		if (!uncore->info) {
>  			ret = -ENOMEM;
>  			goto fail;
>  		}

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak in the module
initialization error path, such as at this goto fail or if cpuhp_setup_state()
fails later in amd_uncore_init().

If setup fails for CPUHP_AP_PERF_X86_AMD_UNCORE_ONLINE, it jumps to the error
handling labels (e.g., fail_start). However, the error path doesn't seem to
unregister successfully registered PMUs or free uncore->pmus.

Unlike amd_uncore_exit() which correctly cleans up the PMUs:

    perf_pmu_unregister(&pmu->pmu);
    free_percpu(pmu->ctx);
    ...
    kfree(uncore->pmus);

Will failing during initialization leave dangling pointers in the global pmus
list and leak the allocated context memory, potentially causing a crash
during subsequent PMU accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701034413.718872-1-liuqi@hygon.cn?part=2