On Mon, Nov 4, 2024 at 10:14 PM Mario Limonciello <superm1@xxxxxxxxxx> wrote:
>
> On 11/4/2024 15:10, Rafael J. Wysocki wrote:
> > On Mon, Nov 4, 2024 at 9:54 PM Mario Limonciello <superm1@xxxxxxxxxx> wrote:
> >>
> >> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>
> >> 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, change arch_init_invariance_cppc() into a new weak
> >> symbol that is called at the end of acpi_processor_driver_init().
> >> Each architecture that supports it can declare the symbol to override
> >> the weak one.
> >>
> >> Fixes: 279f838a61f9 ("x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()")
> >> Reported-by: Ivan Shapovalov <intelfx@xxxxxxxxxxxx>
> >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219431
> >> Tested-by: Oleksandr Natalenko <oleksandr@xxxxxxxxxxxxxx>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >> ---
> >> v3:
> >> * Weak symbol instead of macro to help riscv build failure
> >> * Update commit message
> >> * Add comment
> >> ---
> >> arch/arm64/include/asm/topology.h | 2 +-
> >> arch/x86/include/asm/topology.h | 2 +-
> >> drivers/acpi/cppc_acpi.c | 6 ------
> >> drivers/acpi/processor_driver.c | 9 +++++++++
> >> include/acpi/processor.h | 2 ++
> >> 5 files changed, 13 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> >> index 5fc3af9f8f29b..8a1860877967e 100644
> >> --- a/arch/arm64/include/asm/topology.h
> >> +++ b/arch/arm64/include/asm/topology.h
> >> @@ -27,7 +27,7 @@ void update_freq_counters_refs(void);
> >> #define arch_scale_freq_ref topology_get_freq_ref
> >>
> >> #ifdef CONFIG_ACPI_CPPC_LIB
> >> -#define arch_init_invariance_cppc topology_init_cpu_capacity_cppc
> >> +#define acpi_processor_init_invariance_cppc topology_init_cpu_capacity_cppc
> >> #endif
> >>
> >> /* Replace task scheduler's default cpu-invariant accounting */
> >> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
> >> index aef70336d6247..0fb705524aeaa 100644
> >> --- a/arch/x86/include/asm/topology.h
> >> +++ b/arch/x86/include/asm/topology.h
> >> @@ -307,7 +307,7 @@ 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
> >> +#define acpi_processor_init_invariance_cppc init_freq_invariance_cppc
> >> #endif
> >>
> >> #endif /* _ASM_X86_TOPOLOGY_H */
> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> >> index 1a40f0514eaa3..5c0cc7aae8726 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 cb52dd000b958..3b281bc1e73c3 100644
> >> --- a/drivers/acpi/processor_driver.c
> >> +++ b/drivers/acpi/processor_driver.c
> >> @@ -237,6 +237,9 @@ static struct notifier_block acpi_processor_notifier_block = {
> >> .notifier_call = acpi_processor_notifier,
> >> };
> >>
> >> +void __weak acpi_processor_init_invariance_cppc(void)
> >> +{ }
> >
> > Does this actually work if acpi_processor_init_invariance_cppc is a
> > macro? How does the compiler know that it needs to use
> > init_freq_invariance_cppc() instead of this?
> >
> > It would work if a __weak definition of init_freq_invariance_cppc() was present.
>
> I also wasn't sure, so I explicitly added some tracing in
> init_freq_invariance_cppc() to make sure it got called and checked it
> (GCC 13.2.0).
>
> But I'll admit it's a confusing behavior. If you think it's too
> confusing I'll swap it around to just axe the macros.
Yes, please.
I'm kind of worried that different compilers may do different things
here (say clang vs gcc) and it really is confusing.
