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

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

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.