| Message ID | 20241102032353.2372544-1-superm1@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ACPI: processor: Move arch_init_invariance_cppc() call later | expand |
On Sat, Nov 2, 2024 at 4:24 AM Mario Limonciello <superm1@kernel.org> wrote: > > From: Mario Limonciello <mario.limonciello@amd.com> > > arch_init_invariance_cppc() is called at the end of > acpi_cppc_processor_probe() in order to configure frequency invariance > based upon the values from _CPC. > > This however doesn't work on AMD CPPC shared memory designs that have > AMD preferred cores enabled because _CPC needs to be analyzed from all > cores to judge if preferred cores are enabled. > > This issue manifests to users as a warning since commit 21fb59ab4b97 > ("ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn"): > ``` > Could not retrieve highest performance (-19) > ``` > > However the warning isn't the cause of this, it was actually > commit 279f838a61f9 ("x86/amd: Detect preferred cores in > amd_get_boost_ratio_numerator()") which exposed the issue. > > To fix this problem, push the call to the arch_init_invariance_cppc() > macro to the end of acpi_processor_driver_init(). > > Fixes: 279f838a61f9 ("x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()") > Reported-by: Ivan Shapovalov <intelfx@intelfx.name> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219431 > Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v1->v2: > * Fix LKP robot issue when CONFIG_ACPI_CPPC_LIB not defined > --- > arch/x86/include/asm/topology.h | 2 ++ > drivers/acpi/cppc_acpi.c | 6 ------ > drivers/acpi/processor_driver.c | 1 + > 3 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h > index abe3a8f22cbd..b04c5db7e945 100644 > --- a/arch/x86/include/asm/topology.h > +++ b/arch/x86/include/asm/topology.h > @@ -295,6 +295,8 @@ extern void arch_scale_freq_tick(void); > #ifdef CONFIG_ACPI_CPPC_LIB > void init_freq_invariance_cppc(void); > #define arch_init_invariance_cppc init_freq_invariance_cppc > +#else > +static inline void arch_init_invariance_cppc(void) { } > #endif > > #endif /* _ASM_X86_TOPOLOGY_H */ > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index ed91dfd4fdca..9d48cd706659 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) > * ) > */ > > -#ifndef arch_init_invariance_cppc > -static inline void arch_init_invariance_cppc(void) { } > -#endif > - > /** > * acpi_cppc_processor_probe - Search for per CPU _CPC objects. > * @pr: Ptr to acpi_processor containing this CPU's logical ID. > @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) > goto out_free; > } > > - arch_init_invariance_cppc(); > - > kfree(output.pointer); > return 0; > > diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c > index cb52dd000b95..59620e7bc664 100644 > --- a/drivers/acpi/processor_driver.c > +++ b/drivers/acpi/processor_driver.c > @@ -270,6 +270,7 @@ static int __init acpi_processor_driver_init(void) > NULL, acpi_soft_cpu_dead); > > acpi_processor_throttling_init(); > + arch_init_invariance_cppc(); > return 0; > err: > driver_unregister(&acpi_processor_driver); > -- Applied as a fix for 6.12-rc7. However, it would be good to add a comment explaining why acpi_processor_driver_init() calls arch_init_invariance_cppc() at the end. The ACPI processor driver and CPPC are not otherwise related I think?
On 11/4/2024 10:55, Rafael J. Wysocki wrote: > On Sat, Nov 2, 2024 at 4:24 AM Mario Limonciello <superm1@kernel.org> wrote: >> >> From: Mario Limonciello <mario.limonciello@amd.com> >> >> arch_init_invariance_cppc() is called at the end of >> acpi_cppc_processor_probe() in order to configure frequency invariance >> based upon the values from _CPC. >> >> This however doesn't work on AMD CPPC shared memory designs that have >> AMD preferred cores enabled because _CPC needs to be analyzed from all >> cores to judge if preferred cores are enabled. >> >> This issue manifests to users as a warning since commit 21fb59ab4b97 >> ("ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn"): >> ``` >> Could not retrieve highest performance (-19) >> ``` >> >> However the warning isn't the cause of this, it was actually >> commit 279f838a61f9 ("x86/amd: Detect preferred cores in >> amd_get_boost_ratio_numerator()") which exposed the issue. >> >> To fix this problem, push the call to the arch_init_invariance_cppc() >> macro to the end of acpi_processor_driver_init(). >> >> Fixes: 279f838a61f9 ("x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()") >> Reported-by: Ivan Shapovalov <intelfx@intelfx.name> >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219431 >> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v1->v2: >> * Fix LKP robot issue when CONFIG_ACPI_CPPC_LIB not defined >> --- >> arch/x86/include/asm/topology.h | 2 ++ >> drivers/acpi/cppc_acpi.c | 6 ------ >> drivers/acpi/processor_driver.c | 1 + >> 3 files changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h >> index abe3a8f22cbd..b04c5db7e945 100644 >> --- a/arch/x86/include/asm/topology.h >> +++ b/arch/x86/include/asm/topology.h >> @@ -295,6 +295,8 @@ extern void arch_scale_freq_tick(void); >> #ifdef CONFIG_ACPI_CPPC_LIB >> void init_freq_invariance_cppc(void); >> #define arch_init_invariance_cppc init_freq_invariance_cppc >> +#else >> +static inline void arch_init_invariance_cppc(void) { } >> #endif >> >> #endif /* _ASM_X86_TOPOLOGY_H */ >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index ed91dfd4fdca..9d48cd706659 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) >> * ) >> */ >> >> -#ifndef arch_init_invariance_cppc >> -static inline void arch_init_invariance_cppc(void) { } >> -#endif >> - >> /** >> * acpi_cppc_processor_probe - Search for per CPU _CPC objects. >> * @pr: Ptr to acpi_processor containing this CPU's logical ID. >> @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) >> goto out_free; >> } >> >> - arch_init_invariance_cppc(); >> - >> kfree(output.pointer); >> return 0; >> >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c >> index cb52dd000b95..59620e7bc664 100644 >> --- a/drivers/acpi/processor_driver.c >> +++ b/drivers/acpi/processor_driver.c >> @@ -270,6 +270,7 @@ static int __init acpi_processor_driver_init(void) >> NULL, acpi_soft_cpu_dead); >> >> acpi_processor_throttling_init(); >> + arch_init_invariance_cppc(); >> return 0; >> err: >> driver_unregister(&acpi_processor_driver); >> -- > > Applied as a fix for 6.12-rc7. > > However, it would be good to add a comment explaining why > acpi_processor_driver_init() calls arch_init_invariance_cppc() at the > end. The ACPI processor driver and CPPC are not otherwise related I > think? Sure, I'm thinking a comment like this. /* * Frequency invariance calculations on AMD platforms can't be run until * _CPC has been evaluated on all processors which will only happen * after probing is complete. */ If that sounds good do you want to squash it in? Or if you would prefer another commit tacked on that's no problem I'll do that.
On Mon, Nov 4, 2024 at 6:17 PM Mario Limonciello <superm1@kernel.org> wrote: > > On 11/4/2024 10:55, Rafael J. Wysocki wrote: > > On Sat, Nov 2, 2024 at 4:24 AM Mario Limonciello <superm1@kernel.org> wrote: > >> > >> From: Mario Limonciello <mario.limonciello@amd.com> > >> > >> arch_init_invariance_cppc() is called at the end of > >> acpi_cppc_processor_probe() in order to configure frequency invariance > >> based upon the values from _CPC. > >> > >> This however doesn't work on AMD CPPC shared memory designs that have > >> AMD preferred cores enabled because _CPC needs to be analyzed from all > >> cores to judge if preferred cores are enabled. > >> > >> This issue manifests to users as a warning since commit 21fb59ab4b97 > >> ("ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn"): > >> ``` > >> Could not retrieve highest performance (-19) > >> ``` > >> > >> However the warning isn't the cause of this, it was actually > >> commit 279f838a61f9 ("x86/amd: Detect preferred cores in > >> amd_get_boost_ratio_numerator()") which exposed the issue. > >> > >> To fix this problem, push the call to the arch_init_invariance_cppc() > >> macro to the end of acpi_processor_driver_init(). > >> > >> Fixes: 279f838a61f9 ("x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()") > >> Reported-by: Ivan Shapovalov <intelfx@intelfx.name> > >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219431 > >> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> --- > >> v1->v2: > >> * Fix LKP robot issue when CONFIG_ACPI_CPPC_LIB not defined > >> --- > >> arch/x86/include/asm/topology.h | 2 ++ > >> drivers/acpi/cppc_acpi.c | 6 ------ > >> drivers/acpi/processor_driver.c | 1 + > >> 3 files changed, 3 insertions(+), 6 deletions(-) > >> > >> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h > >> index abe3a8f22cbd..b04c5db7e945 100644 > >> --- a/arch/x86/include/asm/topology.h > >> +++ b/arch/x86/include/asm/topology.h > >> @@ -295,6 +295,8 @@ extern void arch_scale_freq_tick(void); > >> #ifdef CONFIG_ACPI_CPPC_LIB > >> void init_freq_invariance_cppc(void); > >> #define arch_init_invariance_cppc init_freq_invariance_cppc > >> +#else > >> +static inline void arch_init_invariance_cppc(void) { } > >> #endif > >> > >> #endif /* _ASM_X86_TOPOLOGY_H */ > >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > >> index ed91dfd4fdca..9d48cd706659 100644 > >> --- a/drivers/acpi/cppc_acpi.c > >> +++ b/drivers/acpi/cppc_acpi.c > >> @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) > >> * ) > >> */ > >> > >> -#ifndef arch_init_invariance_cppc > >> -static inline void arch_init_invariance_cppc(void) { } > >> -#endif > >> - > >> /** > >> * acpi_cppc_processor_probe - Search for per CPU _CPC objects. > >> * @pr: Ptr to acpi_processor containing this CPU's logical ID. > >> @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) > >> goto out_free; > >> } > >> > >> - arch_init_invariance_cppc(); > >> - > >> kfree(output.pointer); > >> return 0; > >> > >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c > >> index cb52dd000b95..59620e7bc664 100644 > >> --- a/drivers/acpi/processor_driver.c > >> +++ b/drivers/acpi/processor_driver.c > >> @@ -270,6 +270,7 @@ static int __init acpi_processor_driver_init(void) > >> NULL, acpi_soft_cpu_dead); > >> > >> acpi_processor_throttling_init(); > >> + arch_init_invariance_cppc(); > >> return 0; > >> err: > >> driver_unregister(&acpi_processor_driver); > >> -- > > > > Applied as a fix for 6.12-rc7. > > > > However, it would be good to add a comment explaining why > > acpi_processor_driver_init() calls arch_init_invariance_cppc() at the > > end. The ACPI processor driver and CPPC are not otherwise related I > > think? > > Sure, I'm thinking a comment like this. > > /* > * Frequency invariance calculations on AMD platforms can't be run until > * _CPC has been evaluated on all processors which will only happen > * after probing is complete. > */ It would be more precise to say "after acpi_cppc_processor_probe() has been called for all online CPUs". Which among other things means that the ordering of this call with respect to acpi_processor_throttling_init() does not matter. > If that sounds good do you want to squash it in? Or if you would prefer > another commit tacked on that's no problem I'll do that. IMV a separate patch for 6.13 will be fine.
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h index abe3a8f22cbd..b04c5db7e945 100644 --- a/arch/x86/include/asm/topology.h +++ b/arch/x86/include/asm/topology.h @@ -295,6 +295,8 @@ extern void arch_scale_freq_tick(void); #ifdef CONFIG_ACPI_CPPC_LIB void init_freq_invariance_cppc(void); #define arch_init_invariance_cppc init_freq_invariance_cppc +#else +static inline void arch_init_invariance_cppc(void) { } #endif #endif /* _ASM_X86_TOPOLOGY_H */ diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index ed91dfd4fdca..9d48cd706659 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -671,10 +671,6 @@ static int pcc_data_alloc(int pcc_ss_id) * ) */ -#ifndef arch_init_invariance_cppc -static inline void arch_init_invariance_cppc(void) { } -#endif - /** * acpi_cppc_processor_probe - Search for per CPU _CPC objects. * @pr: Ptr to acpi_processor containing this CPU's logical ID. @@ -905,8 +901,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) goto out_free; } - arch_init_invariance_cppc(); - kfree(output.pointer); return 0; diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index cb52dd000b95..59620e7bc664 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -270,6 +270,7 @@ static int __init acpi_processor_driver_init(void) NULL, acpi_soft_cpu_dead); acpi_processor_throttling_init(); + arch_init_invariance_cppc(); return 0; err: driver_unregister(&acpi_processor_driver);