Re: [PATCH] ACPI: Pass the same capabilities to the _OSC regardless of the query flag

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.