Re: [PATCH V3 4/7] perf/x86/intel/uncore: Defer ADL global PMON enable to enable_box()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch defers ADL global PMON enablement to `enable_box()` to save power when no events are in use, but fails to realize that `enable_box()` is unconditionally called by the perf core during event teardown, permanently re-enabling the PMON block after its first use.
--

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

perf/x86/intel/uncore: Defer ADL global PMON enable to enable_box()

This patch removes adl_uncore_msr_init_box() and defers programming the
global control register to enable_box(). The goal is to reduce power
consumption on Raptor Cove by enabling uncore PMON globally only when a
box is actually used.

Cc: stable@vger.kernel.org
Fixes: 772ed05f3c5c ("perf/x86/intel/uncore: Add Alder Lake support")
Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Signed-off-by: Zide Chen <zide.chen@intel.com>

> diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
> index 3dbc6bacbd9de..edddd4f9ab5fc 100644
> --- a/arch/x86/events/intel/uncore_snb.c
> +++ b/arch/x86/events/intel/uncore_snb.c

[ ... ]

> @@ -563,12 +563,6 @@ void tgl_uncore_cpu_init(void)
>  	skl_uncore_msr_ops.init_box = rkl_uncore_msr_init_box;
>  }
>  
> -static void adl_uncore_msr_init_box(struct intel_uncore_box *box)
> -{
> -	if (box->pmu->pmu_idx == 0)
> -		wrmsrq(ADL_UNC_PERF_GLOBAL_CTL, SNB_UNC_GLOBAL_CTL_EN);
> -}
> -
>  static void adl_uncore_msr_enable_box(struct intel_uncore_box *box)
>  {
>  	wrmsrq(ADL_UNC_PERF_GLOBAL_CTL, SNB_UNC_GLOBAL_CTL_EN);
>  }

[Severity: Medium]
Will this leave the PMU permanently enabled after its first use?

During event teardown, the perf core brackets the removal with calls to
perf_pmu_disable() and perf_pmu_enable(). perf_pmu_enable() will ultimately
invoke uncore_pmu_enable(), which unconditionally calls enable_box().

Because adl_uncore_msr_enable_box() unconditionally writes the enable bit
without checking if there are any remaining active events (for example,
checking if box->n_active > 0), the global MSR appears to be immediately
re-enabled at the end of the teardown process.

Does this mean the intended power savings are lost the first time the
PMU is used?

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