[PATCH v1 0/8] ACPI: bus: Rework of the \_SB._OSC handling

[PATCH v1 0/8] ACPI: bus: Rework of the \_SB._OSC handling

[Rafael J. Wysocki]

Hi All,

While this is a replacement for

https://lore.kernel.org/linux-acpi/12803663.O9o76ZdvQC@rafael.j.wysocki/

it goes much farther than the simple workaround, so I've decided to start
over version numbering from 1.

The motivation is (again) to make the _OSC evaluation more robust against
platform firmware deficiencies related to setting error bits in _OSC
return buffers by mistake (which apparently don't affect alternative OSes),
but the approach is more in-depth now.

The first 5 patches are preliminary.  The first one fixes the current
inconsistent handling of _OSC error bits.  The second one reworks the
printing of debug messages from acpi_run_osc() for clarity.  Patch
[3/8] splits the _OSC evaluation code out of acpi_run_osc() (so it
can be used in other functions), and patch [4/8] splits the handling
of _OSC error bits out of it (for the same purpose).  Patch [5/8] is
just a by-the-way simple cleanup.

Patch [6/8] introduces a new function for handling _OSC handshakes in
a way that should be less susceptible to failing in certain cases due
to platform firmware mistakes that should not be fatal and makes the
\_SB._OSC platform features handling code use it.

Patch [7/8] is a cleanup on top of the previous one, and patch [8/8]
updates the USB4 \_SB._OSC features handling to use the new function
introduced in patch [8/8].

Thanks!

[PATCH v1 1/8] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The handling of _OSC errors in acpi_run_osc() is inconsistent.

For example, if OSC_CAPABILITIES_MASK_ERROR is set in the return buffer
and OSC_QUERY_ENABLE is set in the capabilities buffer, all of the
_OSC errors will be ignored even though some of them indicate that
_OSC evaluation has failed.  However, if OSC_QUERY_ENABLE is not set in
the capabilities buffer, all _OSC errors will be reported, but the error
value returned by acpi_run_osc() depends on whether or not
OSC_CAPABILITIES_MASK_ERROR is set in the return buffer.

Adress this by making acpi_run_osc() clear OSC_CAPABILITIES_MASK_ERROR
in the return buffer if OSC_QUERY_ENABLE is set in the capabilities
buffer and then check if any other _OSC errors have been returned.  Also
make it use the same error return value for all _OSC errors and print
a message for OSC_CAPABILITIES_MASK_ERROR like for the other error
types.

Moreover, make acpi_run_osc() only take the defined _OSC error bits into
account when checking _OSC errors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -194,6 +194,10 @@ static void acpi_print_osc_error(acpi_ha
 	pr_debug("\n");
 }
 
+#define OSC_ERROR_MASK 	(OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
+			 OSC_INVALID_REVISION_ERROR | \
+			 OSC_CAPABILITIES_MASK_ERROR)
+
 acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 {
 	acpi_status status;
@@ -240,8 +244,12 @@ acpi_status acpi_run_osc(acpi_handle han
 		status = AE_TYPE;
 		goto out_kfree;
 	}
-	/* Need to ignore the bit0 in result code */
-	errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
+	/* Only take defined error bits into account. */
+	errors = *((u32 *)out_obj->buffer.pointer) & OSC_ERROR_MASK;
+	/* If OSC_QUERY_ENABLE is set, ignore the "capabilities masked" bit. */
+	if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)
+		errors &= ~OSC_CAPABILITIES_MASK_ERROR;
+
 	if (errors) {
 		if (errors & OSC_REQUEST_ERROR)
 			acpi_print_osc_error(handle, context,
@@ -252,17 +260,14 @@ acpi_status acpi_run_osc(acpi_handle han
 		if (errors & OSC_INVALID_REVISION_ERROR)
 			acpi_print_osc_error(handle, context,
 				"_OSC invalid revision");
-		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
-			if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD]
-			    & OSC_QUERY_ENABLE)
-				goto out_success;
-			status = AE_SUPPORT;
-			goto out_kfree;
-		}
+		if (errors & OSC_CAPABILITIES_MASK_ERROR)
+			acpi_print_osc_error(handle, context,
+				"_OSC capability bits masked");
+
 		status = AE_ERROR;
 		goto out_kfree;
 	}
-out_success:
+
 	context->ret.length = out_obj->buffer.length;
 	context->ret.pointer = kmemdup(out_obj->buffer.pointer,
 				       context->ret.length, GFP_KERNEL);

[PATCH v1 2/8] ACPI: bus: Rework printing debug messages on _OSC errors

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Instead of using one function, acpi_print_osc_error(), for printing a
debug message and dumping the _OSC request data in one go, use
acpi_handle_debug() directly for printing messages and a separate
function called acpi_dump_osc_data() for dumping the _OSC request data
after printing one or more of them.

This allows the message printing in the _OSC handling code to be
organized so that the messages printed by it are easier to parse.

Also, use %pUL for UUID printing instead of printing UUIDs as strings
and include the revision number into the dumped _OSC request data.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -180,18 +180,15 @@ void acpi_bus_detach_private_data(acpi_h
 }
 EXPORT_SYMBOL_GPL(acpi_bus_detach_private_data);
 
-static void acpi_print_osc_error(acpi_handle handle,
-				 struct acpi_osc_context *context, char *error)
+static void acpi_dump_osc_data(acpi_handle handle, const guid_t *guid, int rev,
+			       struct acpi_buffer *cap)
 {
+	u32 *capbuf = cap->pointer;
 	int i;
 
-	acpi_handle_debug(handle, "(%s): %s\n", context->uuid_str, error);
-
-	pr_debug("_OSC request data:");
-	for (i = 0; i < context->cap.length; i += sizeof(u32))
-		pr_debug(" %x", *((u32 *)(context->cap.pointer + i)));
-
-	pr_debug("\n");
+	for (i = 0; i < cap->length / sizeof(u32); i++)
+		acpi_handle_debug(handle, "(%pUL, %d): capabilities DWORD %i: [%08x]\n",
+				  guid, rev, i, capbuf[i]);
 }
 
 #define OSC_ERROR_MASK 	(OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
@@ -239,8 +236,8 @@ acpi_status acpi_run_osc(acpi_handle han
 	out_obj = output.pointer;
 	if (out_obj->type != ACPI_TYPE_BUFFER
 		|| out_obj->buffer.length != context->cap.length) {
-		acpi_print_osc_error(handle, context,
-			"_OSC evaluation returned wrong type");
+		acpi_handle_debug(handle, "_OSC evaluation returned wrong type");
+		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
 		status = AE_TYPE;
 		goto out_kfree;
 	}
@@ -252,18 +249,18 @@ acpi_status acpi_run_osc(acpi_handle han
 
 	if (errors) {
 		if (errors & OSC_REQUEST_ERROR)
-			acpi_print_osc_error(handle, context,
-				"_OSC request failed");
+			acpi_handle_debug(handle, "_OSC request failed");
+
 		if (errors & OSC_INVALID_UUID_ERROR)
-			acpi_print_osc_error(handle, context,
-				"_OSC invalid UUID");
+			acpi_handle_debug(handle, "_OSC invalid UUID");
+
 		if (errors & OSC_INVALID_REVISION_ERROR)
-			acpi_print_osc_error(handle, context,
-				"_OSC invalid revision");
+			acpi_handle_debug(handle, "_OSC invalid revision");
+
 		if (errors & OSC_CAPABILITIES_MASK_ERROR)
-			acpi_print_osc_error(handle, context,
-				"_OSC capability bits masked");
+			acpi_handle_debug(handle, "_OSC capability bits masked");
 
+		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
 		status = AE_ERROR;
 		goto out_kfree;
 	}

[PATCH v1 3/8] ACPI: bus: Split _OSC evaluation out of acpi_run_osc()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Split a function for evaluating _OSL called acpi_eval_osc() out of
acpi_run_osc() to facilitate subsequent changes and add some more
parameters sanity checks to the latter.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   89 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 52 insertions(+), 37 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -195,52 +195,67 @@ static void acpi_dump_osc_data(acpi_hand
 			 OSC_INVALID_REVISION_ERROR | \
 			 OSC_CAPABILITIES_MASK_ERROR)
 
-acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
+static int acpi_eval_osc(acpi_handle handle, guid_t *guid, int rev,
+			 struct acpi_buffer *cap,
+			 union acpi_object in_params[static 4],
+			 struct acpi_buffer *output)
 {
-	acpi_status status;
 	struct acpi_object_list input;
-	union acpi_object in_params[4];
 	union acpi_object *out_obj;
+	acpi_status status;
+
+	in_params[0].type = ACPI_TYPE_BUFFER;
+	in_params[0].buffer.length = sizeof(*guid);
+	in_params[0].buffer.pointer = (u8 *)guid;
+	in_params[1].type = ACPI_TYPE_INTEGER;
+	in_params[1].integer.value = rev;
+	in_params[2].type = ACPI_TYPE_INTEGER;
+	in_params[2].integer.value = cap->length / sizeof(u32);
+	in_params[3].type = ACPI_TYPE_BUFFER;
+	in_params[3].buffer.length = cap->length;
+	in_params[3].buffer.pointer = cap->pointer;
+	input.pointer = in_params;
+	input.count = 4;
+
+	output->length = ACPI_ALLOCATE_BUFFER;
+	output->pointer = NULL;
+
+	status = acpi_evaluate_object(handle, "_OSC", &input, output);
+	if (ACPI_FAILURE(status) || !output->length)
+		return -ENODATA;
+
+	out_obj = output->pointer;
+	if (out_obj->type != ACPI_TYPE_BUFFER ||
+	    out_obj->buffer.length != cap->length) {
+		acpi_handle_debug(handle, "Invalid _OSC return buffer\n");
+		acpi_dump_osc_data(handle, guid, rev, cap);
+		ACPI_FREE(out_obj);
+		return -ENODATA;
+	}
+
+	return 0;
+}
+
+acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
+{
+	union acpi_object in_params[4], *out_obj;
+	struct acpi_buffer output;
+	acpi_status status = AE_OK;
 	guid_t guid;
 	u32 errors;
-	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	int ret;
 
-	if (!context)
+	if (!context || !context->cap.pointer ||
+	    context->cap.length < 2 * sizeof(32) ||
+	    guid_parse(context->uuid_str, &guid))
+		return AE_BAD_PARAMETER;
+
+	ret = acpi_eval_osc(handle, &guid, context->rev, &context->cap,
+			    in_params, &output);
+	if (ret)
 		return AE_ERROR;
-	if (guid_parse(context->uuid_str, &guid))
-		return AE_ERROR;
-	context->ret.length = ACPI_ALLOCATE_BUFFER;
-	context->ret.pointer = NULL;
-
-	/* Setting up input parameters */
-	input.count = 4;
-	input.pointer = in_params;
-	in_params[0].type 		= ACPI_TYPE_BUFFER;
-	in_params[0].buffer.length 	= 16;
-	in_params[0].buffer.pointer	= (u8 *)&guid;
-	in_params[1].type 		= ACPI_TYPE_INTEGER;
-	in_params[1].integer.value 	= context->rev;
-	in_params[2].type 		= ACPI_TYPE_INTEGER;
-	in_params[2].integer.value	= context->cap.length/sizeof(u32);
-	in_params[3].type		= ACPI_TYPE_BUFFER;
-	in_params[3].buffer.length 	= context->cap.length;
-	in_params[3].buffer.pointer 	= context->cap.pointer;
-
-	status = acpi_evaluate_object(handle, "_OSC", &input, &output);
-	if (ACPI_FAILURE(status))
-		return status;
-
-	if (!output.length)
-		return AE_NULL_OBJECT;
 
 	out_obj = output.pointer;
-	if (out_obj->type != ACPI_TYPE_BUFFER
-		|| out_obj->buffer.length != context->cap.length) {
-		acpi_handle_debug(handle, "_OSC evaluation returned wrong type");
-		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
-		status = AE_TYPE;
-		goto out_kfree;
-	}
 	/* Only take defined error bits into account. */
 	errors = *((u32 *)out_obj->buffer.pointer) & OSC_ERROR_MASK;
 	/* If OSC_QUERY_ENABLE is set, ignore the "capabilities masked" bit. */

[PATCH v1 4/8] ACPI: bus: Split _OSC error processing out of acpi_run_osc()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Split a function for processing _OSL errors called acpi_osc_error_check()
out of acpi_run_osc() to facilitate subsequent changes.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   56 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -236,13 +236,46 @@ static int acpi_eval_osc(acpi_handle han
 	return 0;
 }
 
+static bool acpi_osc_error_check(acpi_handle handle, guid_t *guid, int rev,
+				 struct acpi_buffer *cap, u32 *retbuf)
+{
+	/* Bit 0 in the query DWORD of the return buffer is reserved. */
+	u32 errors = retbuf[OSC_QUERY_DWORD] & OSC_ERROR_MASK;
+	u32 *capbuf = cap->pointer;
+
+	/*
+	 * If OSC_QUERY_ENABLE is set in the capabilities buffer, ignore bit 4.
+	 */
+	if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)
+		errors &= ~OSC_CAPABILITIES_MASK_ERROR;
+
+	if (!errors)
+		return false;
+
+	if (errors & OSC_REQUEST_ERROR)
+		acpi_handle_debug(handle, "_OSC: request failed\n");
+
+	if (errors & OSC_INVALID_UUID_ERROR)
+		acpi_handle_debug(handle, "_OSC: invalid UUID\n");
+
+	if (errors & OSC_INVALID_REVISION_ERROR)
+		acpi_handle_debug(handle, "_OSC: invalid revision\n");
+
+	if (errors & OSC_CAPABILITIES_MASK_ERROR)
+		acpi_handle_debug(handle, "_OSC: capability bits masked\n");
+
+	acpi_dump_osc_data(handle, guid, rev, cap);
+
+	return true;
+}
+
 acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 {
 	union acpi_object in_params[4], *out_obj;
 	struct acpi_buffer output;
 	acpi_status status = AE_OK;
 	guid_t guid;
-	u32 errors;
+	u32 *retbuf;
 	int ret;
 
 	if (!context || !context->cap.pointer ||
@@ -256,26 +289,9 @@ acpi_status acpi_run_osc(acpi_handle han
 		return AE_ERROR;
 
 	out_obj = output.pointer;
-	/* Only take defined error bits into account. */
-	errors = *((u32 *)out_obj->buffer.pointer) & OSC_ERROR_MASK;
-	/* If OSC_QUERY_ENABLE is set, ignore the "capabilities masked" bit. */
-	if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)
-		errors &= ~OSC_CAPABILITIES_MASK_ERROR;
-
-	if (errors) {
-		if (errors & OSC_REQUEST_ERROR)
-			acpi_handle_debug(handle, "_OSC request failed");
-
-		if (errors & OSC_INVALID_UUID_ERROR)
-			acpi_handle_debug(handle, "_OSC invalid UUID");
-
-		if (errors & OSC_INVALID_REVISION_ERROR)
-			acpi_handle_debug(handle, "_OSC invalid revision");
-
-		if (errors & OSC_CAPABILITIES_MASK_ERROR)
-			acpi_handle_debug(handle, "_OSC capability bits masked");
+	retbuf = (u32 *)out_obj->buffer.pointer;
 
-		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
+	if (acpi_osc_error_check(handle, &guid, context->rev, &context->cap, retbuf)) {
 		status = AE_ERROR;
 		goto out_kfree;
 	}

[PATCH v1 5/8] ACPI: bus: Rename label and use ACPI_FREE() in acpi_run_osc()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Use ACPI_FREE() for freeing an object coming from acpi_eval_osc()
and rename the "out_free" to "out" because it does not involve
kfree() any more.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -293,7 +293,7 @@ acpi_status acpi_run_osc(acpi_handle han
 
 	if (acpi_osc_error_check(handle, &guid, context->rev, &context->cap, retbuf)) {
 		status = AE_ERROR;
-		goto out_kfree;
+		goto out;
 	}
 
 	context->ret.length = out_obj->buffer.length;
@@ -301,12 +301,12 @@ acpi_status acpi_run_osc(acpi_handle han
 				       context->ret.length, GFP_KERNEL);
 	if (!context->ret.pointer) {
 		status =  AE_NO_MEMORY;
-		goto out_kfree;
+		goto out;
 	}
 	status =  AE_OK;
 
-out_kfree:
-	kfree(output.pointer);
+out:
+	ACPI_FREE(out_obj);
 	return status;
 }
 EXPORT_SYMBOL(acpi_run_osc);

[PATCH v1 6/8] ACPI: bus: Rework the handling of \_SB._OSC platform features

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The current handling of \_SB._OSC is susceptible to problems with
setting error bits in the output buffer by mistake if the platform
firmware is supplied with a feature mask previously acknowledged
by the analogous _OSC call with OSC_QUERY_ENABLE set.  If that
happens, acpi_run_osc() will return an error and the kernel will
assume that it cannot control any of the features it has asked
for.  If an error bit has been set by mistake, however, the platform
firmware may expect the kernel to actually take over the control of
those features and nobody will take care of them going forward.

If the given feature mask has been already acknowledged once though,
the kernel may reasonably expect the _OSC evaluation to succeed and
acknowledge all of the features in the current mask again, but that
is not generally guaranteed to happen, so it is actually good to
verify the return buffer.  Still, it is sufficient to check the
feature bits in the return buffer for this purpose.

Namely, the OSC_INVALID_UUID_ERROR and OSC_INVALID_REVISION_ERROR bits
should not be set then because they were not set during the previous
_OSC evaluation that has acknowledged the feature mask.  Moreover,
if all of the feature bits that are set in the capabilities buffer
are also set in the return buffer, the OSC_CAPABILITIES_MASK_ERROR
should not be set either and the OSC_REQUEST_ERROR bit doesn't matter
even if set.  Thus if that is the case, the kernel may regard the
entire feature mask as acknowledged and take over the control of the
given features as per Section 6.2.12 of ACPI 6.6 [1].

If the feature masks in the capabilities buffer and in the return
buffer are different, the bits that are set in both masks may still
be regarded as acknowledged and the corresponding features may be
controlled by the kernel.

Introduce a new function carrying out an _OSC handshake along the
lines of the above description and make the \_SB._OSC handling code
use it to avoid failing in some cases in which it may succeed
regardless of platform firmware deficiencies.

Link: https://uefi.org/specs/ACPI/6.6/06_Device_Configuration.html#osc-operating-system-capabilities
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |  128 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 88 insertions(+), 40 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -311,6 +311,79 @@ out:
 }
 EXPORT_SYMBOL(acpi_run_osc);
 
+static int acpi_osc_handshake(acpi_handle handle, const char *uuid_str,
+			      int rev, struct acpi_buffer *cap)
+{
+	union acpi_object in_params[4], *out_obj;
+	size_t bufsize = cap->length / sizeof(u32);
+	struct acpi_object_list input;
+	struct acpi_buffer output;
+	u32 *capbuf, *retbuf, test;
+	guid_t guid;
+	int ret, i;
+
+	if (!cap || cap->length < 2 * sizeof(32) || guid_parse(uuid_str, &guid))
+		return -EINVAL;
+
+	/* First evaluate _OSC with OSC_QUERY_ENABLE set. */
+	capbuf = cap->pointer;
+	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
+
+	ret = acpi_eval_osc(handle, &guid, rev, cap, in_params, &output);
+	if (ret)
+		return ret;
+
+	out_obj = output.pointer;
+	retbuf = (u32 *)out_obj->buffer.pointer;
+
+	if (acpi_osc_error_check(handle, &guid, rev, cap, retbuf)) {
+		ret = -ENODATA;
+		goto out;
+	}
+
+	/*
+	 * Clear the feature bits in the capabilities buffer that have not been
+	 * acknowledged and clear the return buffer.
+	 */
+	for (i = OSC_QUERY_DWORD + 1, test = 0; i < bufsize; i++) {
+		capbuf[i] &= retbuf[i];
+		test |= capbuf[i];
+		retbuf[i] = 0;
+	}
+	/*
+	 * If none of the feature bits have been acknowledged, there's nothing
+	 * more to do.
+	 */
+	if (!test)
+		goto out;
+
+	retbuf[OSC_QUERY_DWORD] = 0;
+	/*
+	 * Now evaluate _OSC again (directly) with OSC_QUERY_ENABLE clear and
+	 * the updated input and output buffers used before.
+	 */
+	capbuf[OSC_QUERY_DWORD] = 0;
+	/* Reuse in_params[] populated by acpi_eval_osc(). */
+	input.pointer = in_params;
+	input.count = 4;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_OSC", &input, &output))) {
+		ret = -ENODATA;
+		goto out;
+	}
+
+	/* Clear the feature bits that have not been acknowledged in capbuf[]. */
+	for (i = OSC_QUERY_DWORD + 1; i < bufsize; i++)
+		capbuf[i] &= retbuf[i];
+
+	/* Check _OSC errors to print debug messages if any. */
+	acpi_osc_error_check(handle, &guid, rev, cap, retbuf);
+
+out:
+	ACPI_FREE(out_obj);
+	return ret;
+}
+
 bool osc_sb_apei_support_acked;
 
 /*
@@ -342,19 +415,16 @@ EXPORT_SYMBOL_GPL(osc_sb_native_usb4_sup
 
 bool osc_sb_cppc2_support_acked;
 
-static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
 static void acpi_bus_osc_negotiate_platform_control(void)
 {
-	u32 capbuf[2], *capbuf_ret;
-	struct acpi_osc_context context = {
-		.uuid_str = sb_uuid_str,
-		.rev = 1,
-		.cap.length = 8,
-		.cap.pointer = capbuf,
+	static const u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
+	u32 capbuf[2];
+	struct acpi_buffer cap = {
+		.pointer = capbuf,
+		.length = sizeof(capbuf),
 	};
 	acpi_handle handle;
 
-	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
 	capbuf[OSC_SUPPORT_DWORD] = OSC_SB_PR3_SUPPORT; /* _PR3 is in use */
 	if (IS_ENABLED(CONFIG_ACPI_PROCESSOR_AGGREGATOR))
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PAD_SUPPORT;
@@ -400,43 +470,21 @@ static void acpi_bus_osc_negotiate_platf
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
 		return;
 
-	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;
-	}
-
-	/*
-	 * Now run _OSC again with query flag clear and with the caps
-	 * supported by both the OS and the platform.
-	 */
-	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)))
+	if (acpi_osc_handshake(handle, sb_uuid_str, 1, &cap))
 		return;
 
-	capbuf_ret = context.ret.pointer;
-	if (context.ret.length > OSC_SUPPORT_DWORD) {
 #ifdef CONFIG_ACPI_CPPC_LIB
-		osc_sb_cppc2_support_acked = capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_CPCV2_SUPPORT;
+	osc_sb_cppc2_support_acked = capbuf[OSC_SUPPORT_DWORD] & OSC_SB_CPCV2_SUPPORT;
 #endif
 
-		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_cpc_flexible_adr_space_confirmed =
-			capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_CPC_FLEXIBLE_ADR_SPACE;
-	}
-
-	kfree(context.ret.pointer);
+	osc_sb_apei_support_acked =
+			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
+	osc_pc_lpi_support_confirmed =
+			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
+	osc_sb_native_usb4_support_confirmed =
+			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
+	osc_cpc_flexible_adr_space_confirmed =
+			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_CPC_FLEXIBLE_ADR_SPACE;
 }
 
 /*

[PATCH v1 7/8] ACPI: bus: Adjust feature mask creation for \_SB._OSC

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The feature mask creation for \_SB._OSC platform features is messy
and hard to follow, so clean it up and make all of the CPPC-related
features depend on CONFIG_ACPI_CPPC_LIB as they will not work if it
is not set anyway.

Also make acpi_bus_osc_negotiate_platform_control() print a message
including a bit mask representing the features for which control has
been granted.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   83 ++++++++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 44 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -418,73 +418,68 @@ bool osc_sb_cppc2_support_acked;
 static void acpi_bus_osc_negotiate_platform_control(void)
 {
 	static const u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
-	u32 capbuf[2];
+	u32 capbuf[2], feature_mask;
 	struct acpi_buffer cap = {
 		.pointer = capbuf,
 		.length = sizeof(capbuf),
 	};
 	acpi_handle handle;
 
-	capbuf[OSC_SUPPORT_DWORD] = OSC_SB_PR3_SUPPORT; /* _PR3 is in use */
+	feature_mask = OSC_SB_PR3_SUPPORT | OSC_SB_HOTPLUG_OST_SUPPORT |
+			OSC_SB_PCLPI_SUPPORT | OSC_SB_OVER_16_PSTATES_SUPPORT |
+			OSC_SB_GED_SUPPORT | OSC_SB_IRQ_RESOURCE_SOURCE_SUPPORT;
+
+	if (IS_ENABLED(CONFIG_ARM64) || IS_ENABLED(CONFIG_X86))
+		feature_mask |= OSC_SB_GENERIC_INITIATOR_SUPPORT;
+
+	if (IS_ENABLED(CONFIG_ACPI_CPPC_LIB)) {
+		feature_mask |= OSC_SB_CPC_SUPPORT | OSC_SB_CPCV2_SUPPORT |
+				OSC_SB_CPC_FLEXIBLE_ADR_SPACE;
+		if (IS_ENABLED(CONFIG_SCHED_MC_PRIO))
+			feature_mask |= OSC_SB_CPC_DIVERSE_HIGH_SUPPORT;
+	}
+
 	if (IS_ENABLED(CONFIG_ACPI_PROCESSOR_AGGREGATOR))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PAD_SUPPORT;
+		feature_mask |= OSC_SB_PAD_SUPPORT;
+
 	if (IS_ENABLED(CONFIG_ACPI_PROCESSOR))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PPC_OST_SUPPORT;
+		feature_mask |= OSC_SB_PPC_OST_SUPPORT;
+
 	if (IS_ENABLED(CONFIG_ACPI_THERMAL))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_FAST_THERMAL_SAMPLING_SUPPORT;
+		feature_mask |= OSC_SB_FAST_THERMAL_SAMPLING_SUPPORT;
+
 	if (IS_ENABLED(CONFIG_ACPI_BATTERY))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_BATTERY_CHARGE_LIMITING_SUPPORT;
+		feature_mask |= OSC_SB_BATTERY_CHARGE_LIMITING_SUPPORT;
 
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_OVER_16_PSTATES_SUPPORT;
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_GED_SUPPORT;
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_IRQ_RESOURCE_SOURCE_SUPPORT;
 	if (IS_ENABLED(CONFIG_ACPI_PRMT))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PRM_SUPPORT;
-	if (IS_ENABLED(CONFIG_ACPI_FFH))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_FFH_OPR_SUPPORT;
-
-#ifdef CONFIG_ARM64
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_GENERIC_INITIATOR_SUPPORT;
-#endif
-#ifdef CONFIG_X86
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_GENERIC_INITIATOR_SUPPORT;
-#endif
-
-#ifdef CONFIG_ACPI_CPPC_LIB
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
-#endif
+		feature_mask |= OSC_SB_PRM_SUPPORT;
 
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_FLEXIBLE_ADR_SPACE;
-
-	if (IS_ENABLED(CONFIG_SCHED_MC_PRIO))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_DIVERSE_HIGH_SUPPORT;
+	if (IS_ENABLED(CONFIG_ACPI_FFH))
+		feature_mask |= OSC_SB_FFH_OPR_SUPPORT;
 
 	if (IS_ENABLED(CONFIG_USB4))
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_NATIVE_USB4_SUPPORT;
+		feature_mask |= OSC_SB_NATIVE_USB4_SUPPORT;
 
 	if (!ghes_disable)
-		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
+		feature_mask |= OSC_SB_APEI_SUPPORT;
+
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
 		return;
 
+	capbuf[OSC_SUPPORT_DWORD] = feature_mask;
+
 	if (acpi_osc_handshake(handle, sb_uuid_str, 1, &cap))
 		return;
 
-#ifdef CONFIG_ACPI_CPPC_LIB
-	osc_sb_cppc2_support_acked = capbuf[OSC_SUPPORT_DWORD] & OSC_SB_CPCV2_SUPPORT;
-#endif
-
-	osc_sb_apei_support_acked =
-			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
-	osc_pc_lpi_support_confirmed =
-			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
-	osc_sb_native_usb4_support_confirmed =
-			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_NATIVE_USB4_SUPPORT;
-	osc_cpc_flexible_adr_space_confirmed =
-			capbuf[OSC_SUPPORT_DWORD] & OSC_SB_CPC_FLEXIBLE_ADR_SPACE;
+	feature_mask = capbuf[OSC_SUPPORT_DWORD];
+
+	acpi_handle_info(handle, "platform _OSC feature mask [%08x]\n", feature_mask);
+
+	osc_sb_cppc2_support_acked = feature_mask & OSC_SB_CPCV2_SUPPORT;
+	osc_sb_apei_support_acked = feature_mask & OSC_SB_APEI_SUPPORT;
+	osc_pc_lpi_support_confirmed = feature_mask & OSC_SB_PCLPI_SUPPORT;
+	osc_sb_native_usb4_support_confirmed = feature_mask & OSC_SB_NATIVE_USB4_SUPPORT;
+	osc_cpc_flexible_adr_space_confirmed = feature_mask & OSC_SB_CPC_FLEXIBLE_ADR_SPACE;
 }
 
 /*

[PATCH v1 8/8] ACPI: bus: Rework the handling of \_SB._OSC USB4 features

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Use acpi_osc_handshake() introduced previously for implementing the
\_SB._OSC USB4 features control negotiation.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   58 ++++++++---------------------------------------------
 1 file changed, 9 insertions(+), 49 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -499,19 +499,15 @@ static void acpi_bus_decode_usb_osc(cons
 	       (bits & OSC_USB_XDOMAIN) ? '+' : '-');
 }
 
-static u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
 static void acpi_bus_osc_negotiate_usb_control(void)
 {
-	u32 capbuf[3], *capbuf_ret;
-	struct acpi_osc_context context = {
-		.uuid_str = sb_usb_uuid_str,
-		.rev = 1,
-		.cap.length = sizeof(capbuf),
-		.cap.pointer = capbuf,
+	static const u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A";
+	u32 capbuf[3], control, granted;
+	struct acpi_buffer cap = {
+		.pointer = capbuf,
+		.length = sizeof(capbuf),
 	};
 	acpi_handle handle;
-	acpi_status status;
-	u32 control;
 
 	if (!osc_sb_native_usb4_support_confirmed)
 		return;
@@ -522,54 +518,18 @@ static void acpi_bus_osc_negotiate_usb_c
 	control = OSC_USB_USB3_TUNNELING | OSC_USB_DP_TUNNELING |
 		  OSC_USB_PCIE_TUNNELING | OSC_USB_XDOMAIN;
 
-	/*
-	 * Run _OSC first with query bit set, trying to get control over
-	 * all tunneling. The platform can then clear out bits in the
-	 * control dword that it does not want to grant to the OS.
-	 */
-	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
 	capbuf[OSC_SUPPORT_DWORD] = 0;
 	capbuf[OSC_CONTROL_DWORD] = control;
 
-	status = acpi_run_osc(handle, &context);
-	if (ACPI_FAILURE(status))
+	if (acpi_osc_handshake(handle, sb_usb_uuid_str, 1, &cap))
 		return;
 
-	if (context.ret.length != sizeof(capbuf)) {
-		pr_info("USB4 _OSC: returned invalid length buffer\n");
-		goto out_free;
-	}
-
-	/*
-	 * Run _OSC again now with query bit clear and the control dword
-	 * matching what the platform granted (which may not have all
-	 * the control bits set).
-	 */
-	capbuf_ret = context.ret.pointer;
+	granted = capbuf[OSC_CONTROL_DWORD];
 
-	capbuf[OSC_QUERY_DWORD] = 0;
-	capbuf[OSC_CONTROL_DWORD] = capbuf_ret[OSC_CONTROL_DWORD];
-
-	kfree(context.ret.pointer);
-
-	status = acpi_run_osc(handle, &context);
-	if (ACPI_FAILURE(status))
-		return;
-
-	if (context.ret.length != sizeof(capbuf)) {
-		pr_info("USB4 _OSC: returned invalid length buffer\n");
-		goto out_free;
-	}
-
-	osc_sb_native_usb4_control =
-		control & acpi_osc_ctx_get_pci_control(&context);
+	osc_sb_native_usb4_control = control & granted;
 
 	acpi_bus_decode_usb_osc("USB4 _OSC: OS supports", control);
-	acpi_bus_decode_usb_osc("USB4 _OSC: OS controls",
-				osc_sb_native_usb4_control);
-
-out_free:
-	kfree(context.ret.pointer);
+	acpi_bus_decode_usb_osc("USB4 _OSC: OS controls", granted);
 }
 
 /* --------------------------------------------------------------------------

Re: [PATCH v1 1/8] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:34:26 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The handling of _OSC errors in acpi_run_osc() is inconsistent.

I wonder if this would be easier to follow with a brief statement
of why we threat OSC_CAPABILITIES_MASK_ERROR as an error in the first
place for non query cases?  It took me a brief think and spec read
to figure that out, but maybe more coffee needed.

> 
> For example, if OSC_CAPABILITIES_MASK_ERROR is set in the return buffer
> and OSC_QUERY_ENABLE is set in the capabilities buffer, all of the
> _OSC errors will be ignored even though some of them indicate that
> _OSC evaluation has failed.  However, if OSC_QUERY_ENABLE is not set in
> the capabilities buffer, all _OSC errors will be reported, but the error
> value returned by acpi_run_osc() depends on whether or not
> OSC_CAPABILITIES_MASK_ERROR is set in the return buffer.
> 
> Adress this by making acpi_run_osc() clear OSC_CAPABILITIES_MASK_ERROR
> in the return buffer if OSC_QUERY_ENABLE is set in the capabilities
> buffer and then check if any other _OSC errors have been returned.  Also
> make it use the same error return value for all _OSC errors and print
> a message for OSC_CAPABILITIES_MASK_ERROR like for the other error
> types.
> 
> Moreover, make acpi_run_osc() only take the defined _OSC error bits into
> account when checking _OSC errors.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Changes look good to me.
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH v1 2/8] ACPI: bus: Rework printing debug messages on _OSC errors

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:35:27 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of using one function, acpi_print_osc_error(), for printing a
> debug message and dumping the _OSC request data in one go, use
> acpi_handle_debug() directly for printing messages and a separate
> function called acpi_dump_osc_data() for dumping the _OSC request data
> after printing one or more of them.
> 
> This allows the message printing in the _OSC handling code to be
> organized so that the messages printed by it are easier to parse.
Hi Rafael,

Perhaps an example of the print to motivate this change clearly?
The absence of a guid on the error string line for instance may
or may not bother people. It's there in the dump but that comes
after the error print I think.

Thanks,

Jonathan


> 
> Also, use %pUL for UUID printing instead of printing UUIDs as strings
> and include the revision number into the dumped _OSC request data.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/bus.c |   35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -180,18 +180,15 @@ void acpi_bus_detach_private_data(acpi_h
>  }
>  EXPORT_SYMBOL_GPL(acpi_bus_detach_private_data);
>  
> -static void acpi_print_osc_error(acpi_handle handle,
> -				 struct acpi_osc_context *context, char *error)
> +static void acpi_dump_osc_data(acpi_handle handle, const guid_t *guid, int rev,
> +			       struct acpi_buffer *cap)
>  {
> +	u32 *capbuf = cap->pointer;
>  	int i;
>  
> -	acpi_handle_debug(handle, "(%s): %s\n", context->uuid_str, error);
> -
> -	pr_debug("_OSC request data:");
> -	for (i = 0; i < context->cap.length; i += sizeof(u32))
> -		pr_debug(" %x", *((u32 *)(context->cap.pointer + i)));
> -
> -	pr_debug("\n");
> +	for (i = 0; i < cap->length / sizeof(u32); i++)
> +		acpi_handle_debug(handle, "(%pUL, %d): capabilities DWORD %i: [%08x]\n",
> +				  guid, rev, i, capbuf[i]);
>  }
>  
>  #define OSC_ERROR_MASK 	(OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
> @@ -239,8 +236,8 @@ acpi_status acpi_run_osc(acpi_handle han
>  	out_obj = output.pointer;
>  	if (out_obj->type != ACPI_TYPE_BUFFER
>  		|| out_obj->buffer.length != context->cap.length) {
> -		acpi_print_osc_error(handle, context,
> -			"_OSC evaluation returned wrong type");
> +		acpi_handle_debug(handle, "_OSC evaluation returned wrong type");
> +		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
>  		status = AE_TYPE;
>  		goto out_kfree;
>  	}
> @@ -252,18 +249,18 @@ acpi_status acpi_run_osc(acpi_handle han
>  
>  	if (errors) {
>  		if (errors & OSC_REQUEST_ERROR)
> -			acpi_print_osc_error(handle, context,
> -				"_OSC request failed");
> +			acpi_handle_debug(handle, "_OSC request failed");
> +
>  		if (errors & OSC_INVALID_UUID_ERROR)
> -			acpi_print_osc_error(handle, context,
> -				"_OSC invalid UUID");
> +			acpi_handle_debug(handle, "_OSC invalid UUID");
> +
>  		if (errors & OSC_INVALID_REVISION_ERROR)
> -			acpi_print_osc_error(handle, context,
> -				"_OSC invalid revision");
> +			acpi_handle_debug(handle, "_OSC invalid revision");
> +
>  		if (errors & OSC_CAPABILITIES_MASK_ERROR)
> -			acpi_print_osc_error(handle, context,
> -				"_OSC capability bits masked");
> +			acpi_handle_debug(handle, "_OSC capability bits masked");
>  
> +		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
>  		status = AE_ERROR;
>  		goto out_kfree;
>  	}
> 
> 
> 
> 

Re: [PATCH v1 3/8] ACPI: bus: Split _OSC evaluation out of acpi_run_osc()

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:36:08 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Split a function for evaluating _OSL called acpi_eval_osc() out of

_OSC

> acpi_run_osc() to facilitate subsequent changes and add some more
> parameters sanity checks to the latter.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

One comment on the fun static keyword usage.  Next time I have
to ask/answer some silly C questions in an interview that one is definitely going
in :)
> ---
>  drivers/acpi/bus.c |   89 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 52 insertions(+), 37 deletions(-)
> 
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -195,52 +195,67 @@ static void acpi_dump_osc_data(acpi_hand
>  			 OSC_INVALID_REVISION_ERROR | \
>  			 OSC_CAPABILITIES_MASK_ERROR)
>  
> -acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
> +static int acpi_eval_osc(acpi_handle handle, guid_t *guid, int rev,
> +			 struct acpi_buffer *cap,
> +			 union acpi_object in_params[static 4],

This static usage has such non intuitive behavior maybe use
the new at_least marking in compiler_types.h to indicate
what protection against wrong sizes it can offer.

> +			 struct acpi_buffer *output)
>  {
> -	acpi_status status;
>  	struct acpi_object_list input;
> -	union acpi_object in_params[4];
>  	union acpi_object *out_obj;
> +	acpi_status status;
> +
> +	in_params[0].type = ACPI_TYPE_BUFFER;
> +	in_params[0].buffer.length = sizeof(*guid);
> +	in_params[0].buffer.pointer = (u8 *)guid;
> +	in_params[1].type = ACPI_TYPE_INTEGER;
> +	in_params[1].integer.value = rev;
> +	in_params[2].type = ACPI_TYPE_INTEGER;
> +	in_params[2].integer.value = cap->length / sizeof(u32);
> +	in_params[3].type = ACPI_TYPE_BUFFER;
> +	in_params[3].buffer.length = cap->length;
> +	in_params[3].buffer.pointer = cap->pointer;
> +	input.pointer = in_params;
> +	input.count = 4;
> +
> +	output->length = ACPI_ALLOCATE_BUFFER;
> +	output->pointer = NULL;
> +
> +	status = acpi_evaluate_object(handle, "_OSC", &input, output);
> +	if (ACPI_FAILURE(status) || !output->length)
> +		return -ENODATA;
> +
> +	out_obj = output->pointer;
> +	if (out_obj->type != ACPI_TYPE_BUFFER ||
> +	    out_obj->buffer.length != cap->length) {
> +		acpi_handle_debug(handle, "Invalid _OSC return buffer\n");
> +		acpi_dump_osc_data(handle, guid, rev, cap);
> +		ACPI_FREE(out_obj);
> +		return -ENODATA;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH v1 4/8] ACPI: bus: Split _OSC error processing out of acpi_run_osc()

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:36:57 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Split a function for processing _OSL errors called acpi_osc_error_check()
_OSC?
> out of acpi_run_osc() to facilitate subsequent changes.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Minor request inline. 
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
>  drivers/acpi/bus.c |   56 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
> 
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -236,13 +236,46 @@ static int acpi_eval_osc(acpi_handle han
>  	return 0;
>  }
>  
> +static bool acpi_osc_error_check(acpi_handle handle, guid_t *guid, int rev,
> +				 struct acpi_buffer *cap, u32 *retbuf)
> +{
> +	/* Bit 0 in the query DWORD of the return buffer is reserved. */
> +	u32 errors = retbuf[OSC_QUERY_DWORD] & OSC_ERROR_MASK;
> +	u32 *capbuf = cap->pointer;
> +
> +	/*
> +	 * If OSC_QUERY_ENABLE is set in the capabilities buffer, ignore bit 4.

Maybe add a tiny bit on why?

> +	 */
> +	if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)
> +		errors &= ~OSC_CAPABILITIES_MASK_ERROR;
> +
> +	if (!errors)
> +		return false;
> +
> +	if (errors & OSC_REQUEST_ERROR)
> +		acpi_handle_debug(handle, "_OSC: request failed\n");
> +
> +	if (errors & OSC_INVALID_UUID_ERROR)
> +		acpi_handle_debug(handle, "_OSC: invalid UUID\n");
> +
> +	if (errors & OSC_INVALID_REVISION_ERROR)
> +		acpi_handle_debug(handle, "_OSC: invalid revision\n");
> +
> +	if (errors & OSC_CAPABILITIES_MASK_ERROR)
> +		acpi_handle_debug(handle, "_OSC: capability bits masked\n");
> +
> +	acpi_dump_osc_data(handle, guid, rev, cap);
> +
> +	return true;
> +}

> 
> 

Re: [PATCH v1 5/8] ACPI: bus: Rename label and use ACPI_FREE() in acpi_run_osc()

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:37:36 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Use ACPI_FREE() for freeing an object coming from acpi_eval_osc()
> and rename the "out_free" to "out" because it does not involve
> kfree() any more.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH v1 6/8] ACPI: bus: Rework the handling of \_SB._OSC platform features

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:39:43 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The current handling of \_SB._OSC is susceptible to problems with

Maybe state 'firmware bug workaround' a bit more clearly in this description.
I briefly wondered if there was a non buggy path to this case.

> setting error bits in the output buffer by mistake if the platform
> firmware is supplied with a feature mask previously acknowledged
> by the analogous _OSC call with OSC_QUERY_ENABLE set.  If that
> happens, acpi_run_osc() will return an error and the kernel will
> assume that it cannot control any of the features it has asked
> for.  If an error bit has been set by mistake, however, the platform
> firmware may expect the kernel to actually take over the control of
> those features and nobody will take care of them going forward.

This 'may expect' seems like a nasty opening. I get that there is an oddity
if a firmware says it can do something and then when we try to ask
for that says no, but I'd be concerned that someone might have a bug
in the query instead so it promises more that is actually possible
and we grab control of things the firmware is still using with
may eat babies result.

At very least I think we should scream about any firmware that
does return an error in these cases.  You do that so I guess this
is making the best of a bad situation.

Otherwise one comment inline.
> 
> If the given feature mask has been already acknowledged once though,
> the kernel may reasonably expect the _OSC evaluation to succeed and
> acknowledge all of the features in the current mask again, but that
> is not generally guaranteed to happen, so it is actually good to
> verify the return buffer.  Still, it is sufficient to check the
> feature bits in the return buffer for this purpose.
> 
> Namely, the OSC_INVALID_UUID_ERROR and OSC_INVALID_REVISION_ERROR bits
> should not be set then because they were not set during the previous
> _OSC evaluation that has acknowledged the feature mask.  Moreover,
> if all of the feature bits that are set in the capabilities buffer
> are also set in the return buffer, the OSC_CAPABILITIES_MASK_ERROR
> should not be set either and the OSC_REQUEST_ERROR bit doesn't matter
> even if set.  Thus if that is the case, the kernel may regard the
> entire feature mask as acknowledged and take over the control of the
> given features as per Section 6.2.12 of ACPI 6.6 [1].
> 
> If the feature masks in the capabilities buffer and in the return
> buffer are different, the bits that are set in both masks may still
> be regarded as acknowledged and the corresponding features may be
> controlled by the kernel.
> 
> Introduce a new function carrying out an _OSC handshake along the
> lines of the above description and make the \_SB._OSC handling code
> use it to avoid failing in some cases in which it may succeed
> regardless of platform firmware deficiencies.
> 
> Link: https://uefi.org/specs/ACPI/6.6/06_Device_Configuration.html#osc-operating-system-capabilities
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/bus.c |  128 ++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 88 insertions(+), 40 deletions(-)
> 
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -311,6 +311,79 @@ out:
>  }
>  EXPORT_SYMBOL(acpi_run_osc);
>  
> +static int acpi_osc_handshake(acpi_handle handle, const char *uuid_str,
> +			      int rev, struct acpi_buffer *cap)
> +{
> +	union acpi_object in_params[4], *out_obj;
> +	size_t bufsize = cap->length / sizeof(u32);
> +	struct acpi_object_list input;
> +	struct acpi_buffer output;
> +	u32 *capbuf, *retbuf, test;
> +	guid_t guid;
> +	int ret, i;
> +
> +	if (!cap || cap->length < 2 * sizeof(32) || guid_parse(uuid_str, &guid))
> +		return -EINVAL;
> +
> +	/* First evaluate _OSC with OSC_QUERY_ENABLE set. */
> +	capbuf = cap->pointer;
> +	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
> +
> +	ret = acpi_eval_osc(handle, &guid, rev, cap, in_params, &output);
> +	if (ret)
> +		return ret;
> +
> +	out_obj = output.pointer;
> +	retbuf = (u32 *)out_obj->buffer.pointer;
> +
> +	if (acpi_osc_error_check(handle, &guid, rev, cap, retbuf)) {
> +		ret = -ENODATA;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Clear the feature bits in the capabilities buffer that have not been
> +	 * acknowledged and clear the return buffer.
> +	 */
> +	for (i = OSC_QUERY_DWORD + 1, test = 0; i < bufsize; i++) {
> +		capbuf[i] &= retbuf[i];
> +		test |= capbuf[i];
> +		retbuf[i] = 0;
> +	}
> +	/*
> +	 * If none of the feature bits have been acknowledged, there's nothing
> +	 * more to do.
> +	 */
> +	if (!test)
> +		goto out;
> +
> +	retbuf[OSC_QUERY_DWORD] = 0;
> +	/*
> +	 * Now evaluate _OSC again (directly) with OSC_QUERY_ENABLE clear and
> +	 * the updated input and output buffers used before.
> +	 */
> +	capbuf[OSC_QUERY_DWORD] = 0;
> +	/* Reuse in_params[] populated by acpi_eval_osc(). */
> +	input.pointer = in_params;
> +	input.count = 4;
> +
> +	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_OSC", &input, &output))) {
> +		ret = -ENODATA;
> +		goto out;
> +	}
> +
> +	/* Clear the feature bits that have not been acknowledged in capbuf[]. */
> +	for (i = OSC_QUERY_DWORD + 1; i < bufsize; i++)
> +		capbuf[i] &= retbuf[i];
> +
> +	/* Check _OSC errors to print debug messages if any. */
> +	acpi_osc_error_check(acpi_osc_error_checkhandle, &guid, rev, cap, retbuf);

Maybe it's worth a 'Muddling on anyway' message to say that we are ignoring those
errors?

> +
> +out:
> +	ACPI_FREE(out_obj);
> +	return ret;
> +}
> +

Re: [PATCH v1 7/8] ACPI: bus: Adjust feature mask creation for \_SB._OSC

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:41:42 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The feature mask creation for \_SB._OSC platform features is messy
> and hard to follow, so clean it up and make all of the CPPC-related
> features depend on CONFIG_ACPI_CPPC_LIB as they will not work if it
> is not set anyway.
> 
> Also make acpi_bus_osc_negotiate_platform_control() print a message
> including a bit mask representing the features for which control has
> been granted.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Nice.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH v1 8/8] ACPI: bus: Rework the handling of \_SB._OSC USB4 features

[Jonathan Cameron]

On Thu, 18 Dec 2025 21:42:32 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Use acpi_osc_handshake() introduced previously for implementing the
> \_SB._OSC USB4 features control negotiation.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH v1 2/8] ACPI: bus: Rework printing debug messages on _OSC errors

[Rafael J. Wysocki]

On Fri, Dec 19, 2025 at 1:33 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Thu, 18 Dec 2025 21:35:27 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Instead of using one function, acpi_print_osc_error(), for printing a
> > debug message and dumping the _OSC request data in one go, use
> > acpi_handle_debug() directly for printing messages and a separate
> > function called acpi_dump_osc_data() for dumping the _OSC request data
> > after printing one or more of them.
> >
> > This allows the message printing in the _OSC handling code to be
> > organized so that the messages printed by it are easier to parse.
>
> Hi Rafael,
>
> Perhaps an example of the print to motivate this change clearly?
> The absence of a guid on the error string line for instance may
> or may not bother people. It's there in the dump but that comes
> after the error print I think.

Yes, it's there.

I guess I can add examples of "before" and "after" printouts to the
patch changelog.

> > Also, use %pUL for UUID printing instead of printing UUIDs as strings
> > and include the revision number into the dumped _OSC request data.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/bus.c |   35 ++++++++++++++++-------------------
> >  1 file changed, 16 insertions(+), 19 deletions(-)
> >
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -180,18 +180,15 @@ void acpi_bus_detach_private_data(acpi_h
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_bus_detach_private_data);
> >
> > -static void acpi_print_osc_error(acpi_handle handle,
> > -                              struct acpi_osc_context *context, char *error)
> > +static void acpi_dump_osc_data(acpi_handle handle, const guid_t *guid, int rev,
> > +                            struct acpi_buffer *cap)
> >  {
> > +     u32 *capbuf = cap->pointer;
> >       int i;
> >
> > -     acpi_handle_debug(handle, "(%s): %s\n", context->uuid_str, error);
> > -
> > -     pr_debug("_OSC request data:");
> > -     for (i = 0; i < context->cap.length; i += sizeof(u32))
> > -             pr_debug(" %x", *((u32 *)(context->cap.pointer + i)));
> > -
> > -     pr_debug("\n");
> > +     for (i = 0; i < cap->length / sizeof(u32); i++)
> > +             acpi_handle_debug(handle, "(%pUL, %d): capabilities DWORD %i: [%08x]\n",
> > +                               guid, rev, i, capbuf[i]);
> >  }
> >
> >  #define OSC_ERROR_MASK       (OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
> > @@ -239,8 +236,8 @@ acpi_status acpi_run_osc(acpi_handle han
> >       out_obj = output.pointer;
> >       if (out_obj->type != ACPI_TYPE_BUFFER
> >               || out_obj->buffer.length != context->cap.length) {
> > -             acpi_print_osc_error(handle, context,
> > -                     "_OSC evaluation returned wrong type");
> > +             acpi_handle_debug(handle, "_OSC evaluation returned wrong type");
> > +             acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
> >               status = AE_TYPE;
> >               goto out_kfree;
> >       }
> > @@ -252,18 +249,18 @@ acpi_status acpi_run_osc(acpi_handle han
> >
> >       if (errors) {
> >               if (errors & OSC_REQUEST_ERROR)
> > -                     acpi_print_osc_error(handle, context,
> > -                             "_OSC request failed");
> > +                     acpi_handle_debug(handle, "_OSC request failed");
> > +
> >               if (errors & OSC_INVALID_UUID_ERROR)
> > -                     acpi_print_osc_error(handle, context,
> > -                             "_OSC invalid UUID");
> > +                     acpi_handle_debug(handle, "_OSC invalid UUID");
> > +
> >               if (errors & OSC_INVALID_REVISION_ERROR)
> > -                     acpi_print_osc_error(handle, context,
> > -                             "_OSC invalid revision");
> > +                     acpi_handle_debug(handle, "_OSC invalid revision");
> > +
> >               if (errors & OSC_CAPABILITIES_MASK_ERROR)
> > -                     acpi_print_osc_error(handle, context,
> > -                             "_OSC capability bits masked");
> > +                     acpi_handle_debug(handle, "_OSC capability bits masked");
> >
> > +             acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
> >               status = AE_ERROR;
> >               goto out_kfree;
> >       }

Re: [PATCH v1 3/8] ACPI: bus: Split _OSC evaluation out of acpi_run_osc()

[Rafael J. Wysocki]

On Fri, Dec 19, 2025 at 1:44 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Thu, 18 Dec 2025 21:36:08 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Split a function for evaluating _OSL called acpi_eval_osc() out of
>
> _OSC

Yup, thanks!

> > acpi_run_osc() to facilitate subsequent changes and add some more
> > parameters sanity checks to the latter.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> One comment on the fun static keyword usage.  Next time I have
> to ask/answer some silly C questions in an interview that one is definitely going
> in :)
> > ---
> >  drivers/acpi/bus.c |   89 ++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 52 insertions(+), 37 deletions(-)
> >
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -195,52 +195,67 @@ static void acpi_dump_osc_data(acpi_hand
> >                        OSC_INVALID_REVISION_ERROR | \
> >                        OSC_CAPABILITIES_MASK_ERROR)
> >
> > -acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
> > +static int acpi_eval_osc(acpi_handle handle, guid_t *guid, int rev,
> > +                      struct acpi_buffer *cap,
> > +                      union acpi_object in_params[static 4],
>
> This static usage has such non intuitive behavior maybe use
> the new at_least marking in compiler_types.h to indicate
> what protection against wrong sizes it can offer.

I'll have a look at that, thanks!

Re: [PATCH v1 4/8] ACPI: bus: Split _OSC error processing out of acpi_run_osc()

[Rafael J. Wysocki]

On Fri, Dec 19, 2025 at 1:46 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Thu, 18 Dec 2025 21:36:57 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Split a function for processing _OSL errors called acpi_osc_error_check()
> _OSC?
> > out of acpi_run_osc() to facilitate subsequent changes.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Minor request inline.
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > ---
> >  drivers/acpi/bus.c |   56 ++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 36 insertions(+), 20 deletions(-)
> >
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -236,13 +236,46 @@ static int acpi_eval_osc(acpi_handle han
> >       return 0;
> >  }
> >
> > +static bool acpi_osc_error_check(acpi_handle handle, guid_t *guid, int rev,
> > +                              struct acpi_buffer *cap, u32 *retbuf)
> > +{
> > +     /* Bit 0 in the query DWORD of the return buffer is reserved. */
> > +     u32 errors = retbuf[OSC_QUERY_DWORD] & OSC_ERROR_MASK;
> > +     u32 *capbuf = cap->pointer;
> > +
> > +     /*
> > +      * If OSC_QUERY_ENABLE is set in the capabilities buffer, ignore bit 4.
>
> Maybe add a tiny bit on why?

OK, I'll expand the comment.

Re: [PATCH v1 6/8] ACPI: bus: Rework the handling of \_SB._OSC platform features

[Rafael J. Wysocki]

On Fri, Dec 19, 2025 at 1:56 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Thu, 18 Dec 2025 21:39:43 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The current handling of \_SB._OSC is susceptible to problems with
>
> Maybe state 'firmware bug workaround' a bit more clearly in this description.
> I briefly wondered if there was a non buggy path to this case.

The main point to me is that the new code should work if the platform
firmware messes things up somewhat (but not too much) while the old
code fails in those cases.

There is also an assumption in the current code that setting error
bits in the return buffer means hard failures while it follows from
the spec (although not really clearly, which is a problem by itself)
that what matters is what bits are set in the return feature masks.
I'll add some information related to this to the changelog.

> > setting error bits in the output buffer by mistake if the platform
> > firmware is supplied with a feature mask previously acknowledged
> > by the analogous _OSC call with OSC_QUERY_ENABLE set.  If that
> > happens, acpi_run_osc() will return an error and the kernel will
> > assume that it cannot control any of the features it has asked
> > for.  If an error bit has been set by mistake, however, the platform
> > firmware may expect the kernel to actually take over the control of
> > those features and nobody will take care of them going forward.
>
> This 'may expect' seems like a nasty opening. I get that there is an oddity
> if a firmware says it can do something and then when we try to ask
> for that says no, but I'd be concerned that someone might have a bug
> in the query instead so it promises more that is actually possible
> and we grab control of things the firmware is still using with
> may eat babies result.

That of course is also possible.

> At very least I think we should scream about any firmware that
> does return an error in these cases.  You do that so I guess this
> is making the best of a bad situation.

Well, it doesn't actually scream, but that would be a matter of
changing the log level.

> Otherwise one comment inline.
> >
> > If the given feature mask has been already acknowledged once though,
> > the kernel may reasonably expect the _OSC evaluation to succeed and
> > acknowledge all of the features in the current mask again, but that
> > is not generally guaranteed to happen, so it is actually good to
> > verify the return buffer.  Still, it is sufficient to check the
> > feature bits in the return buffer for this purpose.
> >
> > Namely, the OSC_INVALID_UUID_ERROR and OSC_INVALID_REVISION_ERROR bits
> > should not be set then because they were not set during the previous
> > _OSC evaluation that has acknowledged the feature mask.  Moreover,
> > if all of the feature bits that are set in the capabilities buffer
> > are also set in the return buffer, the OSC_CAPABILITIES_MASK_ERROR
> > should not be set either and the OSC_REQUEST_ERROR bit doesn't matter
> > even if set.  Thus if that is the case, the kernel may regard the
> > entire feature mask as acknowledged and take over the control of the
> > given features as per Section 6.2.12 of ACPI 6.6 [1].
> >
> > If the feature masks in the capabilities buffer and in the return
> > buffer are different, the bits that are set in both masks may still
> > be regarded as acknowledged and the corresponding features may be
> > controlled by the kernel.
> >
> > Introduce a new function carrying out an _OSC handshake along the
> > lines of the above description and make the \_SB._OSC handling code
> > use it to avoid failing in some cases in which it may succeed
> > regardless of platform firmware deficiencies.
> >
> > Link: https://uefi.org/specs/ACPI/6.6/06_Device_Configuration.html#osc-operating-system-capabilities
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/bus.c |  128 ++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 88 insertions(+), 40 deletions(-)
> >
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -311,6 +311,79 @@ out:
> >  }
> >  EXPORT_SYMBOL(acpi_run_osc);
> >
> > +static int acpi_osc_handshake(acpi_handle handle, const char *uuid_str,
> > +                           int rev, struct acpi_buffer *cap)
> > +{
> > +     union acpi_object in_params[4], *out_obj;
> > +     size_t bufsize = cap->length / sizeof(u32);
> > +     struct acpi_object_list input;
> > +     struct acpi_buffer output;
> > +     u32 *capbuf, *retbuf, test;
> > +     guid_t guid;
> > +     int ret, i;
> > +
> > +     if (!cap || cap->length < 2 * sizeof(32) || guid_parse(uuid_str, &guid))
> > +             return -EINVAL;
> > +
> > +     /* First evaluate _OSC with OSC_QUERY_ENABLE set. */
> > +     capbuf = cap->pointer;
> > +     capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
> > +
> > +     ret = acpi_eval_osc(handle, &guid, rev, cap, in_params, &output);
> > +     if (ret)
> > +             return ret;
> > +
> > +     out_obj = output.pointer;
> > +     retbuf = (u32 *)out_obj->buffer.pointer;
> > +
> > +     if (acpi_osc_error_check(handle, &guid, rev, cap, retbuf)) {
> > +             ret = -ENODATA;
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * Clear the feature bits in the capabilities buffer that have not been
> > +      * acknowledged and clear the return buffer.
> > +      */
> > +     for (i = OSC_QUERY_DWORD + 1, test = 0; i < bufsize; i++) {
> > +             capbuf[i] &= retbuf[i];
> > +             test |= capbuf[i];
> > +             retbuf[i] = 0;
> > +     }
> > +     /*
> > +      * If none of the feature bits have been acknowledged, there's nothing
> > +      * more to do.
> > +      */
> > +     if (!test)
> > +             goto out;
> > +
> > +     retbuf[OSC_QUERY_DWORD] = 0;
> > +     /*
> > +      * Now evaluate _OSC again (directly) with OSC_QUERY_ENABLE clear and
> > +      * the updated input and output buffers used before.
> > +      */
> > +     capbuf[OSC_QUERY_DWORD] = 0;
> > +     /* Reuse in_params[] populated by acpi_eval_osc(). */
> > +     input.pointer = in_params;
> > +     input.count = 4;
> > +
> > +     if (ACPI_FAILURE(acpi_evaluate_object(handle, "_OSC", &input, &output))) {
> > +             ret = -ENODATA;
> > +             goto out;
> > +     }
> > +
> > +     /* Clear the feature bits that have not been acknowledged in capbuf[]. */
> > +     for (i = OSC_QUERY_DWORD + 1; i < bufsize; i++)
> > +             capbuf[i] &= retbuf[i];
> > +
> > +     /* Check _OSC errors to print debug messages if any. */
> > +     acpi_osc_error_check(acpi_osc_error_checkhandle, &guid, rev, cap, retbuf);
>
> Maybe it's worth a 'Muddling on anyway' message to say that we are ignoring those
> errors?

Sure, I'll add one, thanks!

> > +
> > +out:
> > +     ACPI_FREE(out_obj);
> > +     return ret;
> > +}
> > +

Re: [PATCH v1 1/8] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

[Rafael J. Wysocki]

On Fri, Dec 19, 2025 at 1:26 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Thu, 18 Dec 2025 21:34:26 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The handling of _OSC errors in acpi_run_osc() is inconsistent.
>
> I wonder if this would be easier to follow with a brief statement
> of why we threat OSC_CAPABILITIES_MASK_ERROR as an error in the first
> place for non query cases?  It took me a brief think and spec read
> to figure that out, but maybe more coffee needed.

Well, this is a good question and it is not obvious IMV.

The current code treats it as an error, but arguably it is not really an error.

If it is a query, it doesn't even make sense to print a debug message
for it, but if it is not a query, the feature mask in the _OSC return
buffer still represents the feature that the OS is expected to
control.  So print the debug messages, but do not fail in that case.

I'll update the patch.

[PATCH v2] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

[Rafael J. Wysocki]

On Friday, December 19, 2025 9:38:44 PM CET Rafael J. Wysocki wrote:
> On Fri, Dec 19, 2025 at 1:26 PM Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
> >
> > On Thu, 18 Dec 2025 21:34:26 +0100
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > The handling of _OSC errors in acpi_run_osc() is inconsistent.
> >
> > I wonder if this would be easier to follow with a brief statement
> > of why we threat OSC_CAPABILITIES_MASK_ERROR as an error in the first
> > place for non query cases?  It took me a brief think and spec read
> > to figure that out, but maybe more coffee needed.
> 
> Well, this is a good question and it is not obvious IMV.
> 
> The current code treats it as an error, but arguably it is not really an error.
> 
> If it is a query, it doesn't even make sense to print a debug message
> for it, but if it is not a query, the feature mask in the _OSC return
> buffer still represents the feature that the OS is expected to
> control.  So print the debug messages, but do not fail in that case.
> 
> I'll update the patch.

I have come to the conclusion that the underlying issue can be addressed in
this patch and it basically boils down to the compliance with the spec.

Please see below.  If we can agree that this is the way to go, I'll rework
the rest of the patch series as a follow-up on top of this one.

Thanks!

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH v2] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

The handling of _OSC errors in acpi_run_osc() is inconsistent and
arguably not compliant with the _OSC definition (cf. Section 6.2.12 of
ACPI 6.6 [1]).

Namely, if OSC_QUERY_ENABLE is not set in the capabilities buffer and
any of the error bits are set in the _OSC return buffer, acpi_run_osc()
returns an error code and the _OSC return buffer is discarded.  However,
in that case, depending on what error bits are set, the return buffer
may contain acknowledged bits for features that need to be controlled by
the kernel going forward.

If the OSC_INVALID_UUID_ERROR bit is set, the request could not be
processed at all and so in that particular case discarding the _OSC
return buffer and returning an error is the right thing to do regardless
of whether or not OSC_QUERY_ENABLE is set in the capabilities buffer.

If OSC_QUERY_ENABLE is set in the capabilities buffer and the
OSC_REQUEST_ERROR or OSC_INVALID_REVISION_ERROR bits are set in the
return buffer, acpi_run_osc() may return an error and discard the _OSC
return buffer because in that case the platform configuration does not
change.  However, if any of them is set in the return buffer when
OSC_QUERY_ENABLE is not set in the capabilities buffer, the feature
mask in the _OSC return buffer still representes a set of acknowleded
features as per the _OSC definition:

 The platform acknowledges the Capabilities Buffer by returning a
 buffer of DWORDs of the same length. Set bits indicate acknowledgment
 that OSPM may take control of the capability and cleared bits indicate
 that the platform either does not support the capability or that OSPM
 may not assume control.

which is not conditional on the error bits being clear, so in that case,
discarding the _OSC return buffer is questionable.  There is also no
reason to return an error and discard the _OSC return buffer if the
OSC_CAPABILITIES_MASK_ERROR bit is set in it, but printing diagnostic
messages is not unreasonable when that happens with OSC_QUERY_ENABLE
clear in the capabilities buffer.

Adress this issue by making acpi_run_osc() follow the rules outlined
above.

Moreover, make acpi_run_osc() only take the defined _OSC error bits into
account when checking _OSC errors.

Link: https://uefi.org/specs/ACPI/6.6/06_Device_Configuration.html#osc-operating-system-capabilities
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   50 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -194,14 +194,18 @@ static void acpi_print_osc_error(acpi_ha
 	pr_debug("\n");
 }
 
+#define OSC_ERROR_MASK 	(OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
+			 OSC_INVALID_REVISION_ERROR | \
+			 OSC_CAPABILITIES_MASK_ERROR)
+
 acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 {
+	u32 errors, *capbuf = context->cap.pointer;
 	acpi_status status;
 	struct acpi_object_list input;
 	union acpi_object in_params[4];
 	union acpi_object *out_obj;
 	guid_t guid;
-	u32 errors;
 	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
 
 	if (!context)
@@ -240,29 +244,47 @@ acpi_status acpi_run_osc(acpi_handle han
 		status = AE_TYPE;
 		goto out_kfree;
 	}
-	/* Need to ignore the bit0 in result code */
-	errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
+	/* Only take defined error bits into account. */
+	errors = *((u32 *)out_obj->buffer.pointer) & OSC_ERROR_MASK;
+	/*
+	 * If OSC_QUERY_ENABLE is set, ignore the "capabilities masked"
+	 * bit because it merely means that some features have not been
+	 * acknowledged which is not unexpected.
+	 */
+	if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)
+		errors &= ~OSC_CAPABILITIES_MASK_ERROR;
+
 	if (errors) {
+		if (errors & OSC_INVALID_UUID_ERROR) {
+			acpi_print_osc_error(handle, context,
+				"_OSC invalid UUID");
+			/*
+			 * Always fail if this bit is set because it means that
+			 * the request could not be processed.
+			 */
+			status = AE_ERROR;
+			goto out_kfree;
+		}
 		if (errors & OSC_REQUEST_ERROR)
 			acpi_print_osc_error(handle, context,
 				"_OSC request failed");
-		if (errors & OSC_INVALID_UUID_ERROR)
-			acpi_print_osc_error(handle, context,
-				"_OSC invalid UUID");
 		if (errors & OSC_INVALID_REVISION_ERROR)
 			acpi_print_osc_error(handle, context,
 				"_OSC invalid revision");
-		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
-			if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD]
-			    & OSC_QUERY_ENABLE)
-				goto out_success;
-			status = AE_SUPPORT;
+		if (errors & OSC_CAPABILITIES_MASK_ERROR)
+			acpi_print_osc_error(handle, context,
+				"_OSC capability bits masked");
+
+		/*
+		 * Fail only if OSC_QUERY_ENABLE is set because otherwise the
+		 * acknowledged features need to be controlled.
+		 */
+		if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE) {
+			status = AE_ERROR;
 			goto out_kfree;
 		}
-		status = AE_ERROR;
-		goto out_kfree;
 	}
-out_success:
+
 	context->ret.length = out_obj->buffer.length;
 	context->ret.pointer = kmemdup(out_obj->buffer.pointer,
 				       context->ret.length, GFP_KERNEL);

Re: [PATCH v2] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

[Jonathan Cameron]

On Fri, 19 Dec 2025 22:28:02 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Friday, December 19, 2025 9:38:44 PM CET Rafael J. Wysocki wrote:
> > On Fri, Dec 19, 2025 at 1:26 PM Jonathan Cameron
> > <jonathan.cameron@huawei.com> wrote:  
> > >
> > > On Thu, 18 Dec 2025 21:34:26 +0100
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >  
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > The handling of _OSC errors in acpi_run_osc() is inconsistent.  
> > >
> > > I wonder if this would be easier to follow with a brief statement
> > > of why we threat OSC_CAPABILITIES_MASK_ERROR as an error in the first
> > > place for non query cases?  It took me a brief think and spec read
> > > to figure that out, but maybe more coffee needed.  
> > 
> > Well, this is a good question and it is not obvious IMV.
> > 
> > The current code treats it as an error, but arguably it is not really an error.
> > 
> > If it is a query, it doesn't even make sense to print a debug message
> > for it, but if it is not a query, the feature mask in the _OSC return
> > buffer still represents the feature that the OS is expected to
> > control.  So print the debug messages, but do not fail in that case.
> > 
> > I'll update the patch.  
> 
> I have come to the conclusion that the underlying issue can be addressed in
> this patch and it basically boils down to the compliance with the spec.
> 
> Please see below.  If we can agree that this is the way to go, I'll rework
> the rest of the patch series as a follow-up on top of this one.
> 
> Thanks!
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH v2] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()
> 
> The handling of _OSC errors in acpi_run_osc() is inconsistent and
> arguably not compliant with the _OSC definition (cf. Section 6.2.12 of
> ACPI 6.6 [1]).
> 
> Namely, if OSC_QUERY_ENABLE is not set in the capabilities buffer and
> any of the error bits are set in the _OSC return buffer, acpi_run_osc()
> returns an error code and the _OSC return buffer is discarded.  However,
> in that case, depending on what error bits are set, the return buffer
> may contain acknowledged bits for features that need to be controlled by
> the kernel going forward.
> 
> If the OSC_INVALID_UUID_ERROR bit is set, the request could not be
> processed at all and so in that particular case discarding the _OSC
> return buffer and returning an error is the right thing to do regardless
> of whether or not OSC_QUERY_ENABLE is set in the capabilities buffer.
> 
> If OSC_QUERY_ENABLE is set in the capabilities buffer and the
> OSC_REQUEST_ERROR or OSC_INVALID_REVISION_ERROR bits are set in the

For the _OSC Failure = OSC_REQUEST_ERROR the 'capabilities bits may have been
masked' bothers me a little but other that that vagueness I think your
interpretation is correct.  The spec could definitely have been more tightly
written though so I suspect there may be implementations out there that
get this wrong in the corner cases.  Mind you we shouldn't see them anyway
assuming correct negotiation and nothing unexpected! 

> return buffer, acpi_run_osc() may return an error and discard the _OSC
> return buffer because in that case the platform configuration does not
> change.  However, if any of them is set in the return buffer when
> OSC_QUERY_ENABLE is not set in the capabilities buffer, the feature
> mask in the _OSC return buffer still representes a set of acknowleded
> features as per the _OSC definition:
> 
>  The platform acknowledges the Capabilities Buffer by returning a
>  buffer of DWORDs of the same length. Set bits indicate acknowledgment
>  that OSPM may take control of the capability and cleared bits indicate
>  that the platform either does not support the capability or that OSPM
>  may not assume control.
> 
> which is not conditional on the error bits being clear, so in that case,
> discarding the _OSC return buffer is questionable.  There is also no
> reason to return an error and discard the _OSC return buffer if the
> OSC_CAPABILITIES_MASK_ERROR bit is set in it, but printing diagnostic
> messages is not unreasonable when that happens with OSC_QUERY_ENABLE
> clear in the capabilities buffer.
I'd got further and say "printing diagnostic messages is appropriate"
given this happening is indicating something we don't expect to see.

Other than that this looks good to me.

Thanks

Jonathan

> 
> Adress this issue by making acpi_run_osc() follow the rules outlined
> above.
> 
> Moreover, make acpi_run_osc() only take the defined _OSC error bits into
> account when checking _OSC errors.
> 
> Link: https://uefi.org/specs/ACPI/6.6/06_Device_Configuration.html#osc-operating-system-capabilities
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/bus.c |   50 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 14 deletions(-)
> 
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -194,14 +194,18 @@ static void acpi_print_osc_error(acpi_ha
>  	pr_debug("\n");
>  }
>  
> +#define OSC_ERROR_MASK 	(OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
> +			 OSC_INVALID_REVISION_ERROR | \
> +			 OSC_CAPABILITIES_MASK_ERROR)
> +
>  acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
>  {
> +	u32 errors, *capbuf = context->cap.pointer;
>  	acpi_status status;
>  	struct acpi_object_list input;
>  	union acpi_object in_params[4];
>  	union acpi_object *out_obj;
>  	guid_t guid;
> -	u32 errors;
>  	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
>  
>  	if (!context)
> @@ -240,29 +244,47 @@ acpi_status acpi_run_osc(acpi_handle han
>  		status = AE_TYPE;
>  		goto out_kfree;
>  	}
> -	/* Need to ignore the bit0 in result code */
> -	errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
> +	/* Only take defined error bits into account. */
> +	errors = *((u32 *)out_obj->buffer.pointer) & OSC_ERROR_MASK;
> +	/*
> +	 * If OSC_QUERY_ENABLE is set, ignore the "capabilities masked"
> +	 * bit because it merely means that some features have not been
> +	 * acknowledged which is not unexpected.
> +	 */
> +	if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)
> +		errors &= ~OSC_CAPABILITIES_MASK_ERROR;
> +
>  	if (errors) {
> +		if (errors & OSC_INVALID_UUID_ERROR) {
> +			acpi_print_osc_error(handle, context,
> +				"_OSC invalid UUID");
> +			/*
> +			 * Always fail if this bit is set because it means that
> +			 * the request could not be processed.
> +			 */
> +			status = AE_ERROR;
> +			goto out_kfree;
> +		}
>  		if (errors & OSC_REQUEST_ERROR)
>  			acpi_print_osc_error(handle, context,
>  				"_OSC request failed");
> -		if (errors & OSC_INVALID_UUID_ERROR)
> -			acpi_print_osc_error(handle, context,
> -				"_OSC invalid UUID");
>  		if (errors & OSC_INVALID_REVISION_ERROR)
>  			acpi_print_osc_error(handle, context,
>  				"_OSC invalid revision");
> -		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
> -			if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD]
> -			    & OSC_QUERY_ENABLE)
> -				goto out_success;
> -			status = AE_SUPPORT;
> +		if (errors & OSC_CAPABILITIES_MASK_ERROR)
> +			acpi_print_osc_error(handle, context,
> +				"_OSC capability bits masked");
> +
> +		/*
> +		 * Fail only if OSC_QUERY_ENABLE is set because otherwise the
> +		 * acknowledged features need to be controlled.
> +		 */
> +		if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE) {
> +			status = AE_ERROR;
>  			goto out_kfree;
>  		}
> -		status = AE_ERROR;
> -		goto out_kfree;
>  	}
> -out_success:
> +
>  	context->ret.length = out_obj->buffer.length;
>  	context->ret.pointer = kmemdup(out_obj->buffer.pointer,
>  				       context->ret.length, GFP_KERNEL);
> 
> 
> 
> 
> 

Re: [PATCH v2] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

[Rafael J. Wysocki]

On Mon, Dec 22, 2025 at 12:18 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Fri, 19 Dec 2025 22:28:02 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Friday, December 19, 2025 9:38:44 PM CET Rafael J. Wysocki wrote:
> > > On Fri, Dec 19, 2025 at 1:26 PM Jonathan Cameron
> > > <jonathan.cameron@huawei.com> wrote:
> > > >
> > > > On Thu, 18 Dec 2025 21:34:26 +0100
> > > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > > >
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > The handling of _OSC errors in acpi_run_osc() is inconsistent.
> > > >
> > > > I wonder if this would be easier to follow with a brief statement
> > > > of why we threat OSC_CAPABILITIES_MASK_ERROR as an error in the first
> > > > place for non query cases?  It took me a brief think and spec read
> > > > to figure that out, but maybe more coffee needed.
> > >
> > > Well, this is a good question and it is not obvious IMV.
> > >
> > > The current code treats it as an error, but arguably it is not really an error.
> > >
> > > If it is a query, it doesn't even make sense to print a debug message
> > > for it, but if it is not a query, the feature mask in the _OSC return
> > > buffer still represents the feature that the OS is expected to
> > > control.  So print the debug messages, but do not fail in that case.
> > >
> > > I'll update the patch.
> >
> > I have come to the conclusion that the underlying issue can be addressed in
> > this patch and it basically boils down to the compliance with the spec.
> >
> > Please see below.  If we can agree that this is the way to go, I'll rework
> > the rest of the patch series as a follow-up on top of this one.
> >
> > Thanks!
> >
> > ---
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: [PATCH v2] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()
> >
> > The handling of _OSC errors in acpi_run_osc() is inconsistent and
> > arguably not compliant with the _OSC definition (cf. Section 6.2.12 of
> > ACPI 6.6 [1]).
> >
> > Namely, if OSC_QUERY_ENABLE is not set in the capabilities buffer and
> > any of the error bits are set in the _OSC return buffer, acpi_run_osc()
> > returns an error code and the _OSC return buffer is discarded.  However,
> > in that case, depending on what error bits are set, the return buffer
> > may contain acknowledged bits for features that need to be controlled by
> > the kernel going forward.
> >
> > If the OSC_INVALID_UUID_ERROR bit is set, the request could not be
> > processed at all and so in that particular case discarding the _OSC
> > return buffer and returning an error is the right thing to do regardless
> > of whether or not OSC_QUERY_ENABLE is set in the capabilities buffer.
> >
> > If OSC_QUERY_ENABLE is set in the capabilities buffer and the
> > OSC_REQUEST_ERROR or OSC_INVALID_REVISION_ERROR bits are set in the
>
> For the _OSC Failure = OSC_REQUEST_ERROR the 'capabilities bits may have been
> masked' bothers me a little but other that that vagueness I think your
> interpretation is correct.  The spec could definitely have been more tightly
> written though

Totally agreed and I'm going to do something on that front.

> so I suspect there may be implementations out there that
> get this wrong in the corner cases.  Mind you we shouldn't see them anyway
> assuming correct negotiation and nothing unexpected!

Right.

> > return buffer, acpi_run_osc() may return an error and discard the _OSC
> > return buffer because in that case the platform configuration does not
> > change.  However, if any of them is set in the return buffer when
> > OSC_QUERY_ENABLE is not set in the capabilities buffer, the feature
> > mask in the _OSC return buffer still representes a set of acknowleded
> > features as per the _OSC definition:
> >
> >  The platform acknowledges the Capabilities Buffer by returning a
> >  buffer of DWORDs of the same length. Set bits indicate acknowledgment
> >  that OSPM may take control of the capability and cleared bits indicate
> >  that the platform either does not support the capability or that OSPM
> >  may not assume control.
> >
> > which is not conditional on the error bits being clear, so in that case,
> > discarding the _OSC return buffer is questionable.  There is also no
> > reason to return an error and discard the _OSC return buffer if the
> > OSC_CAPABILITIES_MASK_ERROR bit is set in it, but printing diagnostic
> > messages is not unreasonable when that happens with OSC_QUERY_ENABLE
> > clear in the capabilities buffer.
> I'd got further and say "printing diagnostic messages is appropriate"
> given this happening is indicating something we don't expect to see.

Sure.

> Other than that this looks good to me.

Thanks!

> >
> > Adress this issue by making acpi_run_osc() follow the rules outlined
> > above.
> >
> > Moreover, make acpi_run_osc() only take the defined _OSC error bits into
> > account when checking _OSC errors.
> >
> > Link: https://uefi.org/specs/ACPI/6.6/06_Device_Configuration.html#osc-operating-system-capabilities
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/bus.c |   50 ++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 36 insertions(+), 14 deletions(-)
> >
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -194,14 +194,18 @@ static void acpi_print_osc_error(acpi_ha
> >       pr_debug("\n");
> >  }
> >
> > +#define OSC_ERROR_MASK       (OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
> > +                      OSC_INVALID_REVISION_ERROR | \
> > +                      OSC_CAPABILITIES_MASK_ERROR)
> > +
> >  acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
> >  {
> > +     u32 errors, *capbuf = context->cap.pointer;
> >       acpi_status status;
> >       struct acpi_object_list input;
> >       union acpi_object in_params[4];
> >       union acpi_object *out_obj;
> >       guid_t guid;
> > -     u32 errors;
> >       struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> >
> >       if (!context)
> > @@ -240,29 +244,47 @@ acpi_status acpi_run_osc(acpi_handle han
> >               status = AE_TYPE;
> >               goto out_kfree;
> >       }
> > -     /* Need to ignore the bit0 in result code */
> > -     errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
> > +     /* Only take defined error bits into account. */
> > +     errors = *((u32 *)out_obj->buffer.pointer) & OSC_ERROR_MASK;
> > +     /*
> > +      * If OSC_QUERY_ENABLE is set, ignore the "capabilities masked"
> > +      * bit because it merely means that some features have not been
> > +      * acknowledged which is not unexpected.
> > +      */
> > +     if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE)
> > +             errors &= ~OSC_CAPABILITIES_MASK_ERROR;
> > +
> >       if (errors) {
> > +             if (errors & OSC_INVALID_UUID_ERROR) {
> > +                     acpi_print_osc_error(handle, context,
> > +                             "_OSC invalid UUID");
> > +                     /*
> > +                      * Always fail if this bit is set because it means that
> > +                      * the request could not be processed.
> > +                      */
> > +                     status = AE_ERROR;
> > +                     goto out_kfree;
> > +             }
> >               if (errors & OSC_REQUEST_ERROR)
> >                       acpi_print_osc_error(handle, context,
> >                               "_OSC request failed");
> > -             if (errors & OSC_INVALID_UUID_ERROR)
> > -                     acpi_print_osc_error(handle, context,
> > -                             "_OSC invalid UUID");
> >               if (errors & OSC_INVALID_REVISION_ERROR)
> >                       acpi_print_osc_error(handle, context,
> >                               "_OSC invalid revision");
> > -             if (errors & OSC_CAPABILITIES_MASK_ERROR) {
> > -                     if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD]
> > -                         & OSC_QUERY_ENABLE)
> > -                             goto out_success;
> > -                     status = AE_SUPPORT;
> > +             if (errors & OSC_CAPABILITIES_MASK_ERROR)
> > +                     acpi_print_osc_error(handle, context,
> > +                             "_OSC capability bits masked");
> > +
> > +             /*
> > +              * Fail only if OSC_QUERY_ENABLE is set because otherwise the
> > +              * acknowledged features need to be controlled.
> > +              */
> > +             if (capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE) {
> > +                     status = AE_ERROR;
> >                       goto out_kfree;
> >               }
> > -             status = AE_ERROR;
> > -             goto out_kfree;
> >       }
> > -out_success:
> > +
> >       context->ret.length = out_obj->buffer.length;
> >       context->ret.pointer = kmemdup(out_obj->buffer.pointer,
> >                                      context->ret.length, GFP_KERNEL);
> >
> >
> >
> >
> >
>