| Message ID | 20220629200433.6103-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is supported | expand |
On Wed, Jun 29, 2022 at 10:04 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > commit 72f2ecb7ece7 ("ACPI: bus: Set CPPC _OSC bits for all and > when CPPC_LIB is supported") added support for claiming to > support CPPC in _OSC on non-Intel platforms. > > This unfortunately caused a regression on a vartiety of AMD > platforms in the field because a number of AMD platforms don't set > the `_OSC` bit 5 or 6 to indicate CPPC or CPPC v2 support. > > As these AMD platforms already claim CPPC support via a dedicated > MSR from `X86_FEATURE_CPPC`, use that to enable this feature rather > than requiring the `_OSC` on platforms with a dedicated MSR. > > If there is additional breakage on the shared memory designs also > missing this _OSC, additional follow up changes may be needed. > > Fixes: 72f2ecb7ece7 ("Set CPPC _OSC bits for all and when CPPC_LIB is supported") > Reported-by: Perry Yuan <perry.yuan@amd.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> I'm still concerned about the possible cases in which there is _CPC, but it returns garbage, because the firmware thinks that the OS will not use _CPC due to the _OSC handshake. > --- > v1->v2: > * Make the code easier to follow (suggested by Rafael) > * Update commit message to reflect this is only fixing the MSR case > and that any other breakage from 72f2ecb7ece7 will need additional > follow ups > drivers/acpi/cppc_acpi.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 903528f7e187..cc154519c608 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -629,7 +629,15 @@ static bool is_cppc_supported(int revision, int num_ent) > return false; > } > > - return true; > + if (!osc_sb_cppc_not_supported) > + return true; > + > + pr_debug("Firmware missing _OSC support\n"); > +#ifdef CONFIG_X86 > + return boot_cpu_has(X86_FEATURE_CPPC); So can you please add an additional X86_VENDOR_AMD check to the above? > +#else > + return false; > +#endif > } > > /* > @@ -684,9 +692,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) > acpi_status status; > int ret = -ENODATA; > > - if (osc_sb_cppc_not_supported) > - return -ENODEV; > - > /* Parse the ACPI _CPC table for this CPU. */ > status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output, > ACPI_TYPE_PACKAGE); > -- > 2.25.1 >
[Public] > -----Original Message----- > From: Rafael J. Wysocki <rafael@kernel.org> > Sent: Tuesday, July 5, 2022 13:12 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; > Sudeep Holla <sudeep.holla@arm.com>; Pierre Gondois > <pierre.gondois@arm.com>; Yuan, Perry <Perry.Yuan@amd.com>; ACPI > Devel Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v2] ACPI: CPPC: Don't require _OSC if > X86_FEATURE_CPPC is supported > > On Wed, Jun 29, 2022 at 10:04 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: > > > > commit 72f2ecb7ece7 ("ACPI: bus: Set CPPC _OSC bits for all and > > when CPPC_LIB is supported") added support for claiming to > > support CPPC in _OSC on non-Intel platforms. > > > > This unfortunately caused a regression on a vartiety of AMD > > platforms in the field because a number of AMD platforms don't set > > the `_OSC` bit 5 or 6 to indicate CPPC or CPPC v2 support. > > > > As these AMD platforms already claim CPPC support via a dedicated > > MSR from `X86_FEATURE_CPPC`, use that to enable this feature rather > > than requiring the `_OSC` on platforms with a dedicated MSR. > > > > If there is additional breakage on the shared memory designs also > > missing this _OSC, additional follow up changes may be needed. > > > > Fixes: 72f2ecb7ece7 ("Set CPPC _OSC bits for all and when CPPC_LIB is > supported") > > Reported-by: Perry Yuan <perry.yuan@amd.com> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > I'm still concerned about the possible cases in which there is _CPC, > but it returns garbage, because the firmware thinks that the OS will > not use _CPC due to the _OSC handshake. > > > --- > > v1->v2: > > * Make the code easier to follow (suggested by Rafael) > > * Update commit message to reflect this is only fixing the MSR case > > and that any other breakage from 72f2ecb7ece7 will need additional > > follow ups > > drivers/acpi/cppc_acpi.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > > index 903528f7e187..cc154519c608 100644 > > --- a/drivers/acpi/cppc_acpi.c > > +++ b/drivers/acpi/cppc_acpi.c > > @@ -629,7 +629,15 @@ static bool is_cppc_supported(int revision, int > num_ent) > > return false; > > } > > > > - return true; > > + if (!osc_sb_cppc_not_supported) > > + return true; > > + > > + pr_debug("Firmware missing _OSC support\n"); > > +#ifdef CONFIG_X86 > > + return boot_cpu_has(X86_FEATURE_CPPC); > > So can you please add an additional X86_VENDOR_AMD check to the above? Very well, I'll send out v4 shortly with that modification. > > > +#else > > + return false; > > +#endif > > } > > > > /* > > @@ -684,9 +692,6 @@ int acpi_cppc_processor_probe(struct > acpi_processor *pr) > > acpi_status status; > > int ret = -ENODATA; > > > > - if (osc_sb_cppc_not_supported) > > - return -ENODEV; > > - > > /* Parse the ACPI _CPC table for this CPU. */ > > status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output, > > ACPI_TYPE_PACKAGE); > > -- > > 2.25.1 > >
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 903528f7e187..cc154519c608 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -629,7 +629,15 @@ static bool is_cppc_supported(int revision, int num_ent) return false; } - return true; + if (!osc_sb_cppc_not_supported) + return true; + + pr_debug("Firmware missing _OSC support\n"); +#ifdef CONFIG_X86 + return boot_cpu_has(X86_FEATURE_CPPC); +#else + return false; +#endif } /* @@ -684,9 +692,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) acpi_status status; int ret = -ENODATA; - if (osc_sb_cppc_not_supported) - return -ENODEV; - /* Parse the ACPI _CPC table for this CPU. */ status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output, ACPI_TYPE_PACKAGE);
commit 72f2ecb7ece7 ("ACPI: bus: Set CPPC _OSC bits for all and when CPPC_LIB is supported") added support for claiming to support CPPC in _OSC on non-Intel platforms. This unfortunately caused a regression on a vartiety of AMD platforms in the field because a number of AMD platforms don't set the `_OSC` bit 5 or 6 to indicate CPPC or CPPC v2 support. As these AMD platforms already claim CPPC support via a dedicated MSR from `X86_FEATURE_CPPC`, use that to enable this feature rather than requiring the `_OSC` on platforms with a dedicated MSR. If there is additional breakage on the shared memory designs also missing this _OSC, additional follow up changes may be needed. Fixes: 72f2ecb7ece7 ("Set CPPC _OSC bits for all and when CPPC_LIB is supported") Reported-by: Perry Yuan <perry.yuan@amd.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v1->v2: * Make the code easier to follow (suggested by Rafael) * Update commit message to reflect this is only fixing the MSR case and that any other breakage from 72f2ecb7ece7 will need additional follow ups drivers/acpi/cppc_acpi.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)