On Tue, Mar 15, 2022 at 5:30 AM Limonciello, Mario <Mario.Limonciello@xxxxxxx> wrote: > > [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? 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.
