[PATCH v2 1/3] ACPI: APEI: Adjust for acpi_run_osc logic changes

[PATCH v2 1/3] ACPI: APEI: Adjust for acpi_run_osc logic changes

[Mario Limonciello]

As this function calls the OSC with the OSC_QUERY_ENABLE set in
OSC_QUERY_DWORD, ensure that it continues to operate the same if
the function has returned AE_SUPPORT.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/apei/apei-base.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index c7fdb12c3310..f7d1aa687fd9 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -780,6 +780,7 @@ int apei_osc_setup(void)
 {
 	static u8 whea_uuid_str[] = "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
 	acpi_handle handle;
+	acpi_status status;
 	u32 capbuf[3];
 	struct acpi_osc_context context = {
 		.uuid_str	= whea_uuid_str,
@@ -792,12 +793,12 @@ int apei_osc_setup(void)
 	capbuf[OSC_SUPPORT_DWORD] = 1;
 	capbuf[OSC_CONTROL_DWORD] = 0;
 
-	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle))
-	    || ACPI_FAILURE(acpi_run_osc(handle, &context)))
+	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
 		return -EIO;
-	else {
-		kfree(context.ret.pointer);
-		return 0;
-	}
+	status = acpi_run_osc(handle, &context);
+	if (status != AE_SUPPORT && status != AE_OK)
+		return -EIO;
+	kfree(context.ret.pointer);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(apei_osc_setup);
-- 
2.34.1

[PATCH v2 2/3] ACPI: bus: Allow negotiating OSC capabilities

[Mario Limonciello]

Currently when capabilities have been masked by firmware during a
negotiation with OSC_QUERY_ENABLE set they're silently ignored
by the caller.  If the caller calls `acpi_run_osc` again without
query set and the same capabilities, then they instead get a failure
possibly leading to downstream problems.

So instead when query is set return AE_SUPPORT which callers can then
use for determining that capabilities were masked.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/bus.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 07f604832fd6..f0f9e0934c10 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -243,16 +243,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 			acpi_print_osc_error(handle, context,
 				"_OSC invalid revision");
 		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
+			acpi_print_osc_error(handle, context, "_OSC capabilities masked");
 			if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD]
-			    & OSC_QUERY_ENABLE)
-				goto out_success;
-			status = AE_SUPPORT;
-			goto out_kfree;
+			    & OSC_QUERY_ENABLE) {
+				status = AE_SUPPORT;
+				goto out_masked;
+			}
 		}
 		status = AE_ERROR;
 		goto out_kfree;
 	}
-out_success:
+
+	status =  AE_OK;
+out_masked:
 	context->ret.length = out_obj->buffer.length;
 	context->ret.pointer = kmemdup(out_obj->buffer.pointer,
 				       context->ret.length, GFP_KERNEL);
-- 
2.34.1

Re: [PATCH v2 2/3] ACPI: bus: Allow negotiating OSC capabilities

[Mika Westerberg]

On Thu, Feb 24, 2022 at 11:52:23PM -0600, Mario Limonciello wrote:
> Currently when capabilities have been masked by firmware during a
> negotiation with OSC_QUERY_ENABLE set they're silently ignored
> by the caller.  If the caller calls `acpi_run_osc` again without
> query set and the same capabilities, then they instead get a failure
> possibly leading to downstream problems.
> 
> So instead when query is set return AE_SUPPORT which callers can then
> use for determining that capabilities were masked.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v2 3/3] ACPI: bus: For platform OSC negotiate capabilities

[Mario Limonciello]

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 acknolwedge 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 or after an arbitrary
number of negotiation attempts.

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 v1->v2:
* Fix a NULL pointer dereference caught by 0day CI
 drivers/acpi/bus.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index f0f9e0934c10..489cc4f6b6e6 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -297,6 +297,8 @@ static void acpi_bus_osc_negotiate_platform_control(void)
 		.cap.pointer = capbuf,
 	};
 	acpi_handle handle;
+	acpi_status status;
+	int i;
 
 	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
 	capbuf[OSC_SUPPORT_DWORD] = OSC_SB_PR3_SUPPORT; /* _PR3 is in use */
@@ -332,10 +334,34 @@ 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)))
+	/*
+	 * check if bits were masked, we need to negotiate
+	 * prevent potential endless loop by limited number of
+	 * negotiation cycles
+	 */
+	for (i = 0; i < 5; i++) {
+		status = acpi_run_osc(handle, &context);
+		if (ACPI_SUCCESS(status) || status == AE_SUPPORT) {
+			capbuf_ret = context.ret.pointer;
+			capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
+			kfree(context.ret.pointer);
+		}
+		if (status != AE_SUPPORT)
+			break;
+	}
+	if (ACPI_FAILURE(status))
 		return;
 
-	kfree(context.ret.pointer);
+	/*
+	 * avoid problems with BIOS dynamically loading tables by indicating
+	 * support for CPPC even if it was masked
+	 */
+#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 */
 	capbuf[OSC_QUERY_DWORD] = 0;
-- 
2.34.1

Re: [PATCH v2 3/3] ACPI: bus: For platform OSC negotiate capabilities

[Mika Westerberg]

On Thu, Feb 24, 2022 at 11:52:24PM -0600, Mario Limonciello 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

_OSC

> 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 acknolwedge OSC_SB_PCLPI_SUPPORT but

acknowledge

> it acknolwedged the others.
> 
> The second call to the platform OSC without the query flag set then

_OSC

> 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

_OSC

> indicated that capabilities are no longer masked or after an arbitrary
> number of negotiation attempts.
> 
> 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 v1->v2:
> * Fix a NULL pointer dereference caught by 0day CI
>  drivers/acpi/bus.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index f0f9e0934c10..489cc4f6b6e6 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -297,6 +297,8 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>  		.cap.pointer = capbuf,
>  	};
>  	acpi_handle handle;
> +	acpi_status status;
> +	int i;
>  
>  	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
>  	capbuf[OSC_SUPPORT_DWORD] = OSC_SB_PR3_SUPPORT; /* _PR3 is in use */
> @@ -332,10 +334,34 @@ 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)))
> +	/*
> +	 * check if bits were masked, we need to negotiate
> +	 * prevent potential endless loop by limited number of
> +	 * negotiation cycles

Start with capital letter and end with '.' a multiline comment.

/*
 * Check if ...
 * ...
 * negotiation cycles.
 */

> +	 */
> +	for (i = 0; i < 5; i++) {
> +		status = acpi_run_osc(handle, &context);
> +		if (ACPI_SUCCESS(status) || status == AE_SUPPORT) {
> +			capbuf_ret = context.ret.pointer;
> +			capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
> +			kfree(context.ret.pointer);

I wonder if it makes sense to document the acpi_run_osc() to tell in
which return codes you actually need to kfree() the result. Here it is
hard to tell IMHO.

> +		}
> +		if (status != AE_SUPPORT)
> +			break;
> +	}
> +	if (ACPI_FAILURE(status))
>  		return;
>  
> -	kfree(context.ret.pointer);
> +	/*
> +	 * avoid problems with BIOS dynamically loading tables by indicating
> +	 * support for CPPC even if it was masked

Ditto for the comment.

> +	 */
> +#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 */
>  	capbuf[OSC_QUERY_DWORD] = 0;
> -- 
> 2.34.1

Re: [PATCH v2 1/3] ACPI: APEI: Adjust for acpi_run_osc logic changes

[Mika Westerberg]

On Thu, Feb 24, 2022 at 11:52:22PM -0600, Mario Limonciello wrote:
> As this function calls the OSC with the OSC_QUERY_ENABLE set in
> OSC_QUERY_DWORD, ensure that it continues to operate the same if
> the function has returned AE_SUPPORT.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>