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

From: "Rafael J. Wysocki" <rafael@kernel.org>
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
Date: Mon, 14 Mar 2022 21:01:23 +0100	[thread overview]
Message-ID: <CAJZ5v0g_TH+OeQkBwXELKZiRcxjLvstr8G_bXOBChJ94sTkvHw@mail.gmail.com> (raw)
In-Reply-To: <BL1PR12MB5157839B59321A1A6AD9F73FE20E9@BL1PR12MB5157.namprd12.prod.outlook.com>

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.

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.

>
> >
> > > +        */
> > > +#ifdef CONFIG_X86
> > > +       if (boot_cpu_has(X86_FEATURE_HWP)) {
> > > +               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
> > > +               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
> > > +       }
> > > +#endif
> > >
> > > +       /* Now run _OSC again with query flag clear */
> > >         if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > >                 return;
> > >
> > > --
> > > 2.34.1
> > >