Re: [PATCH V2 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 global PMON control MSR remains permanently enabled after the first perf event is removed, defeating the patch's intended dynamic power-saving goal.
--

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

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

This commit removes adl_uncore_msr_init_box() to stop unconditionally
enabling uncore PMON globally at driver init on Raptor Cove CPUs, which
was increasing power consumption. Instead, it defers programming the
global control register to enable_box() so it is only turned on when a
box is in use.

> 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]
When the last perf event is removed, will the MSR remain permanently enabled?

The perf core framework wraps PMU transactions in perf_pmu_disable() and
perf_pmu_enable(). For example, when removing the last event,
perf_remove_from_context() ends with a call to perf_pmu_enable().

This ultimately invokes uncore_pmu_enable(), which unconditionally calls
the enable_box() callback. Since adl_uncore_msr_enable_box() doesn't
check if there are any active events (like checking box->n_active > 0),
it writes SNB_UNC_GLOBAL_CTL_EN to the MSR.

While this saves power before the first event is created, does it fail to
dynamically power down the hardware after its first use?

>  }

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