From: Hans de Goede <hdegoede@redhat.com>
To: Mika Westerberg <mika.westerberg@linux.intel.com>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Mario Limonciello <mario.limonciello@outlook.com>,
linux-acpi@vger.kernel.org
Subject: Re: [PATCH] ACPI: Pass the same capabilities to the _OSC regardless of the query flag
Date: Wed, 9 Jun 2021 09:58:31 +0200 [thread overview]
Message-ID: <3b58902f-fda3-d957-2e9c-204e82b2f570@redhat.com> (raw)
In-Reply-To: <d0bdb868-95c9-8908-06e1-01f1364f12a0@redhat.com>
Hi,
On 6/8/21 7:10 PM, Hans de Goede wrote:
> Hi,
>
> On 6/8/21 6:38 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 check which was 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.
>>
>> [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@outlook.com>
>> cc: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> ---
>> drivers/acpi/bus.c | 21 +++++++--------------
>> 1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index be7da23fad76..61e8c02595ac 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -336,26 +336,19 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>> return;
>> }
>>
>> - /*
>> - * 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);
>
> This kfree needs to be moved up, rather then be completely removed
> and you are still leaving 1 of the unnecessary length checks in place.
>
> I've added this fixup on top, to fix both these issues:
>
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -330,11 +330,7 @@ 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 */
> capbuf[OSC_QUERY_DWORD] = 0;
>
> I'll ask the reporters of:
> https://bugzilla.kernel.org/show_bug.cgi?id=213023
> https://bugzilla.redhat.com/show_bug.cgi?id=1963717
>
> To test the (fixed-up) patch, so that they can confirm if this indeed
> fixes things.
I've received confirmation from 2 users that this patch (with the fixup)
fixes this. Can send a v2 with the fixup squashed in for Rafael to pick up?
Regards,
Hans
>>
>> 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);
>> }
>>
next prev parent reply other threads:[~2021-06-09 7:58 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-03 17:26 Some SSDT tables are not loading with kernel >= 5.12 Hans de Goede
2021-06-07 9:43 ` Hans de Goede
2021-06-07 10:05 ` Hans de Goede
2021-06-07 11:13 ` Rafael J. Wysocki
[not found] ` <FR1PR80MB5051E91269FD36681BB357A7E1389@FR1PR80MB5051.lamprd80.prod.outlook.com>
2021-06-07 16:08 ` Mika Westerberg
2021-06-07 19:18 ` Hans de Goede
2021-06-08 9:50 ` Hans de Goede
2021-06-08 11:44 ` Mika Westerberg
2021-06-08 13:01 ` Mika Westerberg
2021-06-08 13:24 ` Hans de Goede
2021-06-08 15:03 ` Mika Westerberg
2021-06-08 16:38 ` [PATCH] ACPI: Pass the same capabilities to the _OSC regardless of the query flag Mika Westerberg
2021-06-08 17:10 ` Hans de Goede
2021-06-09 7:58 ` Hans de Goede [this message]
2021-06-09 9:24 ` Mika Westerberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b58902f-fda3-d957-2e9c-204e82b2f570@redhat.com \
--to=hdegoede@redhat.com \
--cc=linux-acpi@vger.kernel.org \
--cc=mario.limonciello@outlook.com \
--cc=mika.westerberg@linux.intel.com \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).