[AMD Official Use Only]
> -----Original Message-----
> From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> Sent: Monday, March 14, 2022 15:01
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Rafael J . Wysocki
> <rjw@xxxxxxxxxxxxx>; ACPI Devel Maling List <linux-acpi@xxxxxxxxxxxxxxx>;
> Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>; Hou, Xiaomeng
> (Matthew) <Xiaomeng.Hou@xxxxxxx>; Liu, Aaron <Aaron.Liu@xxxxxxx>;
> Huang, Ray <Ray.Huang@xxxxxxx>; Hans de Goede
> <hdegoede@xxxxxxxxxx>
> Subject: Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities
>
> On Mon, Mar 14, 2022 at 12:45 AM Limonciello, Mario
> <Mario.Limonciello@xxxxxxx> 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?
>
> >
> > >
> > > > + */
> > > > +#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
> > > >
