| Message ID | 20210608163810.18071-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: Pass the same capabilities to the _OSC regardless of the query flag | expand |
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. 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); > } >
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); >> } >>
On Wed, Jun 09, 2021 at 09:58:31AM +0200, Hans de Goede wrote: > 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? Thanks Hans for the fixup and checking with the users! I will send the v2 with your fixup soon.
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); 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); }
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(-)