Hi,
On 6/9/21 12:25 PM, Mika Westerberg wrote:
> Commit 719e1f561afb ("ACPI: Execute platform _OSC also with query bit
> clear") makes acpi_bus_osc_negotiate_platform_control() not only query
> the platforms capabilities but it also commits the result back to the
> firmware to report which capabilities are supported by the OS back to
> the firmware
>
> 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)
>
> In addition the ACPI 6.4 spec says following [1]:
>
> If the OS declares support of a feature in the Support Field in one
> call to _OSC, then it must preserve the set state of that bit
> (declaring support for that feature) in all subsequent calls.
>
> Based on the above we can fix the issue by passing the same set of
> capabilities to the platform wide _OSC in both calls regardless of the
> query flag.
>
> While there drop the context.ret.length checks which were wrong to begin
> with (as the length is number of bytes not elements). This is already
> checked in acpi_run_osc() that also returns an error in that case.
>
> Includes fixes by Hans de Goede.
>
> [1] https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#sequence-of-osc-calls
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=213023
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1963717
> Fixes: 719e1f561afb ("ACPI: Execute platform _OSC also with query bit clear")
> Cc: Mario Limonciello <mario.limonciello@xxxxxxxxxxx>
> cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Regards,
Hans
> ---
> Changes from v1:
>
> - Include fixup suggested by Hans
>
> Was not sure how to attribute that so I just mention it in the commit log
> for now. Please let me know if there is more standard way :)
>
> drivers/acpi/bus.c | 27 ++++++++-------------------
> 1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index be7da23fad76..a4bd673934c0 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -330,32 +330,21 @@ static void acpi_bus_osc_negotiate_platform_control(void)
> if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> return;
>
> - capbuf_ret = context.ret.pointer;
> - if (context.ret.length <= OSC_SUPPORT_DWORD) {
> - kfree(context.ret.pointer);
> - return;
> - }
> + kfree(context.ret.pointer);
>
> - /*
> - * Now run _OSC again with query flag clear and with the caps
> - * supported by both the OS and the platform.
> - */
> + /* Now run _OSC again with query flag clear */
> capbuf[OSC_QUERY_DWORD] = 0;
> - capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
> - kfree(context.ret.pointer);
>
> if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> return;
>
> capbuf_ret = context.ret.pointer;
> - if (context.ret.length > OSC_SUPPORT_DWORD) {
> - osc_sb_apei_support_acked =
> - 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_native_usb4_support_confirmed =
> - capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
> - }
> + osc_sb_apei_support_acked =
> + 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_native_usb4_support_confirmed =
> + capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
>
> kfree(context.ret.pointer);
> }
>
