Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Mario Limonciello <mario.limonciello@amd.com>
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	"Hou, Xiaomeng (Matthew)" <Xiaomeng.Hou@amd.com>,
	"Liu, Aaron" <Aaron.Liu@amd.com>, Huang Rui <Ray.Huang@amd.com>,
	Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities
Date: Fri, 11 Mar 2022 16:37:26 +0100	[thread overview]
Message-ID: <CAJZ5v0ibnaZZu_Gxngjbu5vzdQaJog8XZnJP6_msLqV_gi4Zig@mail.gmail.com> (raw)
In-Reply-To: <20220310212805.3786-1-mario.limonciello@amd.com>

On Thu, Mar 10, 2022 at 10:30 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> According to the ACPI 6.4 spec:
> It is strongly recommended that the OS evaluate _OSC with the Query
> Support Flag set until _OSC returns the Capabilities Masked bit clear,
> to negotiate the set of features to be granted to the OS for native
> support; a platform may require a specific combination of features
> to be supported natively by an OS before granting native control
> of a given feature. After negotiation with the query flag set,
> the OS should evaluate without it so that any negotiated values
> can be made effective to hardware.
>
> Currently the code sends the exact same values in both executions of the
> _OSC and this leads to some problems on some AMD platforms in certain
> configurations.
>
> The following notable capabilities are set by OSPM when query is enabled:
> * OSC_SB_PR3_SUPPORT
> * OSC_SB_PCLPI_SUPPORT
> * OSC_SB_NATIVE_USB4_SUPPORT
>
> The first call to the platform OSC returns back a masked capabilities
> error because the firmware did not acknowledge OSC_SB_PCLPI_SUPPORT but
> it acknolwedged the others.
>
> The second call to the platform _OSC without the query flag set then
> fails because the OSPM still sent the exact same values.  This leads
> to not acknowledging OSC_SB_NATIVE_USB4_SUPPORT and later USB4 PCIe
> tunnels can't be authorized.
>
> This problem was first introduced by commit 159d8c274fd9 ("ACPI: Pass the
> same capabilities to the _OSC regardless of the query flag") which subtly
> adjusted the behavior from 719e1f5 ("ACPI: Execute platform _OSC also
> with query bit clear").
>
> The _OSC was called exactly 2 times:
>  * Once to query and request from firmware
>  * Once to commit to firmware without query
>
> To fix this problem, continue to call the _OSC until the firmware has
> indicated that capabilities are no longer masked.
>
> Furthermore, to avoid the problem that commit 159d8c274fd9 ("ACPI: Pass
> the same capabilities to the _OSC regardless of the query flag")
> introduced, explicitly mark support for CPC and CPPCv2 even if they
> were masked by the series of query calls due to table loading order on
> some systems.
>
> Fixes: 159d8c274fd9 ("ACPI: Pass the same capabilities to the _OSC regardless of the query flag")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>
> changes from v5->v6
>  * drop mika's tag due to changes
>  * negotiate until support result is empty
> ---
>  drivers/acpi/bus.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index ec83c4f6d628..351bac98f70c 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -330,14 +330,29 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>         if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
>                 return;
>
> -       if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> -               return;
>
> -       kfree(context.ret.pointer);
> +       do {
> +               if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> +                       return;
> +               capbuf_ret = context.ret.pointer;
> +               if (capbuf[OSC_SUPPORT_DWORD] == capbuf_ret[OSC_SUPPORT_DWORD])
> +                       capbuf[OSC_QUERY_DWORD] = 0;
> +               capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];

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?

> +               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"?

> +        */
> +#ifdef CONFIG_X86
> +       if (boot_cpu_has(X86_FEATURE_HWP)) {
> +               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
> +               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
> +       }
> +#endif
>
> +       /* Now run _OSC again with query flag clear */
>         if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
>                 return;
>
> --
> 2.34.1
>