Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities

["Rafael J. Wysocki" <rafael@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4471 bytes --]

On Tue, Mar 15, 2022 at 11:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Mar 15, 2022 at 5:30 AM Limonciello, Mario
> <Mario.Limonciello@amd.com> wrote:
> >
> > [AMD Official Use Only]
> >
> >
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <rafael@kernel.org>
> > > Sent: Monday, March 14, 2022 15:01
> > > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Rafael J . Wysocki
> > > <rjw@rjwysocki.net>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>;
> > > Mika Westerberg <mika.westerberg@linux.intel.com>; Hou, Xiaomeng
> > > (Matthew) <Xiaomeng.Hou@amd.com>; Liu, Aaron <Aaron.Liu@amd.com>;
> > > Huang, Ray <Ray.Huang@amd.com>; Hans de Goede
> > > <hdegoede@redhat.com>
> > > Subject: Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities
> > >
> > > On Mon, Mar 14, 2022 at 12:45 AM Limonciello, Mario
> > > <Mario.Limonciello@amd.com> wrote:
> > > >
> > > > [Public]
> > > >
> > > > > I would do
> > > > >
> > > > > if (capbuf[OSC_SUPPORT_DWORD] ==
> > > capbuf_ret[OSC_SUPPORT_DWORD])
> > > > >         capbuf[OSC_QUERY_DWORD] = 0;
> > > > > else
> > > > >         capbuf[OSC_SUPPORT_DWORD] &=
> > > capbuf_ret[OSC_SUPPORT_DWORD];
> > > > >
> > > > > so that the loop terminates even if the firmware does strange things
> > > > > and then it would only be necessary to check
> > > capbuf[OSC_QUERY_DWORD]
> > > > > in the loop termination condition.
> > > > >
> > > > > Would that work?
> > > > >
> > > >
> > > > I think it will.  I'll try it and send up a v7 if so.
> > > >
> > > > > > +               kfree(context.ret.pointer);
> > > > > > +       } while (capbuf[OSC_QUERY_DWORD] &&
> > > > > capbuf[OSC_SUPPORT_DWORD]);
> > > > > >
> > > > > > -       /* Now run _OSC again with query flag clear */
> > > > > > -       capbuf[OSC_QUERY_DWORD] = 0;
> > > > > > +       /*
> > > > > > +        * Avoid problems with BIOS dynamically loading tables by
> > > indicating
> > > > > > +        * support for CPPC even if it was masked.
> > > > >
> > > > > What exactly do you mean by "BIOS dynamically loading tables"?
> > > >
> > > > As mentioned in commit 159d8c274fd9:
> > > >
> > > >     On certain systems the BIOS loads SSDT tables dynamically based on the
> > > >     capabilities the OS claims to support. However, on these systems the
> > > >     _OSC actually clears some of the bits (under certain conditions) so what
> > > >     happens is that now when we call the _OSC twice the second time we
> > > pass
> > > >     the cleared values and that results errors like below to appear on the
> > > >     system log:
> > > >
> > > >       ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC],
> > > AE_NOT_FOUND (20210105/psargs-330)
> > > >       ACPI Error: Aborting method \_PR.PR01._CPC due to previous error
> > > (AE_NOT_FOUND) (20210105/psparse-529)
> > > >
> > > > This block  is to avoid regressing that again by forcing it on these systems.
> > >
> > > Well, this means that the code before and after the patch is not
> > > actually following the spec.
> > >
> > > First off, as mentioned in the changelog of commit 159d8c274fd9 (in
> > > the part that has not been quoted above), the OS is required to pass
> > > the same set of capabilities every time _OSC is evaluated.  This
> > > doesn't happen after the change.
> > >
> > > Second, acpi_bus_osc_negotiate_platform_control() should take the
> > > capabilities mask returned by the platform which it doesn't do without
> > > the patch.
> >
> > Right on both points.
> >
> > >
> > > That latter piece appears to be the bug in question here and IMO
> > > fixing it doesn't even require calling acpi_run_osc() with the query
> > > flag set for multiple times.
> >
> > I think just taking the results will re-introduce the CPC bug though
> > won't it?  So how to avoid it but also to take the results?
>
> I think that the OS should not ask for the control of the CPPC bits if
> they are masked by the firmware and it should avoid invoking _CPC
> then.
>
> Otherwise we risk breaking legitimate cases in which the firmware
> actually doesn't want the OS to control those bits.

I'm basically talking about reverting commit 159d8c274fd9, as the part
of the _OSC definition in the spec it is based on appears to be bogus
(that will be addressed separately via the ACPI spec process), and
applying the attached change on top of that.

If this looks good to you, I'll take care of it.

[-- Attachment #2: acpi-osc-fix-cppc-fallout.patch --]
[-- Type: text/x-patch, Size: 1922 bytes --]

---
 drivers/acpi/bus.c       |    4 ++++
 drivers/acpi/cppc_acpi.c |    3 +++
 include/linux/acpi.h     |    1 +
 3 files changed, 8 insertions(+)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -278,6 +278,8 @@ bool osc_sb_apei_support_acked;
 bool osc_pc_lpi_support_confirmed;
 EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
 
+bool osc_sb_cppc_control_granted;
+
 /*
  * ACPI 6.4 Operating System Capabilities for USB.
  */
@@ -346,6 +348,8 @@ static void acpi_bus_osc_negotiate_platf
 		capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
 	osc_pc_lpi_support_confirmed =
 		capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
+	osc_sb_cppc_control_granted = capbuf_ret[OSC_SUPPORT_DWORD] &
+		(OSC_SB_CPC_SUPPORT | OSC_SB_CPCV2_SUPPORT);
 	osc_sb_native_usb4_support_confirmed =
 		capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
 
Index: linux-pm/drivers/acpi/cppc_acpi.c
===================================================================
--- linux-pm.orig/drivers/acpi/cppc_acpi.c
+++ linux-pm/drivers/acpi/cppc_acpi.c
@@ -656,6 +656,9 @@ int acpi_cppc_processor_probe(struct acp
 	acpi_status status;
 	int ret = -EFAULT;
 
+	if (!osc_sb_cppc_control_granted)
+		return -ENODEV;
+
 	/* Parse the ACPI _CPC table for this CPU. */
 	status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output,
 			ACPI_TYPE_PACKAGE);
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -579,6 +579,7 @@ acpi_status acpi_run_osc(acpi_handle han
 
 extern bool osc_sb_apei_support_acked;
 extern bool osc_pc_lpi_support_confirmed;
+extern bool osc_sb_cppc_control_granted;
 extern bool osc_sb_native_usb4_support_confirmed;
 
 /* USB4 Capabilities */