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

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

[Rafael J. Wysocki]

Hi All,

This is an update of

https://lore.kernel.org/linux-acpi/5049211.GXAFRqVoOG@rafael.j.wysocki/

and the version is 2.1 because I've already posted a v2 of the first patch:

https://lore.kernel.org/linux-acpi/5967663.DvuYhMxLoT@rafael.j.wysocki/

which is updated again (in a minor way) is this series.

The original motivation was 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
since it can be argued that the current implementation of acpi_run_osc()
does not follow the specification exactly (see the changelog of patch
[1/8] for details), this is now more in the fixes territory.  However,
all of the patches except for the [1/8] can be regarded as cleanups and
optimizations.

The essential changes takes place in patch [1/8] this time.  It fixes
the error handling in acpi_run_osc() to remove inconsistencies from it
and follow the _OSC definition more closely, which also addresses the
"robustness" issue as kind of a side effect.

The second one reworks the printing of debug messages from acpi_run_osc()
for clarity, like in v1.  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 helper function for handling _OSC handshakes
in a generic way that should allow some code duplication and unnecessary
overhead to be avoided going forward 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 [6/8].

Thanks!

[PATCH v2.1 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 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 appropriate 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 [1]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 -> v2.1:
   * Print diagnostic messages for all error bits set in the _OSC return buffer
     when OSC_INVALID_UUID_ERROR is set.
   * Strengthen the changelog language related to printing diagnostic messages
     when OSC_CAPABILITIES_MASK_ERROR is set in the _OSC return buffer (Jonathan).

v1 -> v2:
   * In addition to making the behavior consistent, also make the code
     follow the specification more closely in the cases when the "query
     enable" bit is not set in _OSC the capabilities buffer an error bits
     are set in the _OSC return buffer.
   * Update the changelog accordingly.

---
 drivers/acpi/bus.c |   52 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 38 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,49 @@ 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) {
+		/*
+		 * As a rule, fail only if OSC_QUERY_ENABLE is set because
+		 * otherwise the acknowledged features need to be controlled.
+		 */
+		bool fail = !!(capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE);
+
+		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.
+			 */
+			fail = true;
+			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");
+
+		if (fail) {
+			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.1 1/8] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

[Jonathan Cameron]

On Mon, 22 Dec 2025 20:05:44 +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 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

typo: represents

> 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 appropriate 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 [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

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

[Rafael J. Wysocki]

On Tue, Dec 23, 2025 at 12:12 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Mon, 22 Dec 2025 20:05:44 +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 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
>
> typo: represents

Thanks, fixed while applying.

> > 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 appropriate 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 [1]
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Thank you!

And thanks for all of the reviews!

[PATCH v2.1 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
before printing one or more of them.

This avoids
 * dumping _OSC request data multiple times when there are
   multiple error bits set in the return buffer,
 * wrapping message lines on terminals with 80 character line width,
 * mixing up unrelated messages by printing full lines only,
and generally helps to make the messages 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.

This is how the debug printout looks like when the
OSC_REQUEST_ERROR and OSC_INVALID_REVISION_ERROR bits
are set in the return buffer before the change:

 ACPI: \_SB_: ACPI: (0811B06E-4A27-44F9-8D60-3CBBC22E7B48): _OSC request failed
 ACPI: _OSC request data:
 ACPI:  1
 ACPI:  2e7eff
 ACPI: 
 ACPI: \_SB_: ACPI: (0811B06E-4A27-44F9-8D60-3CBBC22E7B48): _OSC invalid revision
 ACPI: _OSC request data:
 ACPI:  1
 ACPI:  2e7eff
 ACPI:

and this is how it is going to look like afterward:

 ACPI: \_SB_: ACPI: _OSC: UUID: 0811B06E-4A27-44F9-8D60-3CBBC22E7B48, rev: 1
 ACPI: \_SB_: ACPI: _OSC: capabilities DWORD 0: [00000001]
 ACPI: \_SB_: ACPI: _OSC: capabilities DWORD 1: [002e7eff]
 ACPI: \_SB_: ACPI: _OSC: request failed
 ACPI: \_SB_: ACPI: _OSC: invalid revision

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2.1:
   * Dump _OSC request data before printing diagnostic messages.
   * Add example "before" and "after" debug output to the changelog (Jonathan).
   * Reformat messages to avoid crossing the 80 characters boundary.
   * Update the changelog.

---
 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,16 @@ 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");
+	acpi_handle_debug(handle, "_OSC: UUID: %pUL, rev: %d\n", guid, rev);
+	for (i = 0; i < cap->length / sizeof(u32); i++)
+		acpi_handle_debug(handle, "_OSC: capabilities DWORD %i: [%08x]\n",
+				  i, capbuf[i]);
 }
 
 #define OSC_ERROR_MASK 	(OSC_REQUEST_ERROR | OSC_INVALID_UUID_ERROR | \
@@ -239,8 +237,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_dump_osc_data(handle, &guid, context->rev, &context->cap);
+		acpi_handle_debug(handle, "_OSC: evaluation returned wrong type");
 		status = AE_TYPE;
 		goto out_kfree;
 	}
@@ -261,9 +259,9 @@ acpi_status acpi_run_osc(acpi_handle han
 		 */
 		bool fail = !!(capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE);
 
+		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
 		if (errors & OSC_INVALID_UUID_ERROR) {
-			acpi_print_osc_error(handle, context,
-				"_OSC invalid UUID");
+			acpi_handle_debug(handle, "_OSC: invalid UUID");
 			/*
 			 * Always fail if this bit is set because it means that
 			 * the request could not be processed.
@@ -272,14 +270,13 @@ acpi_status acpi_run_osc(acpi_handle han
 			goto out_kfree;
 		}
 		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_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");
 
 		if (fail) {
 			status = AE_ERROR;

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

[Jonathan Cameron]

On Mon, 22 Dec 2025 20:11:08 +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
> before printing one or more of them.
> 
> This avoids
>  * dumping _OSC request data multiple times when there are
>    multiple error bits set in the return buffer,
>  * wrapping message lines on terminals with 80 character line width,
>  * mixing up unrelated messages by printing full lines only,
> and generally helps to make the messages 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.
> 
> This is how the debug printout looks like when the
> OSC_REQUEST_ERROR and OSC_INVALID_REVISION_ERROR bits
> are set in the return buffer before the change:
> 
>  ACPI: \_SB_: ACPI: (0811B06E-4A27-44F9-8D60-3CBBC22E7B48): _OSC request failed
>  ACPI: _OSC request data:
>  ACPI:  1
>  ACPI:  2e7eff
>  ACPI: 
>  ACPI: \_SB_: ACPI: (0811B06E-4A27-44F9-8D60-3CBBC22E7B48): _OSC invalid revision
>  ACPI: _OSC request data:
>  ACPI:  1
>  ACPI:  2e7eff
>  ACPI:
> 
> and this is how it is going to look like afterward:
> 
>  ACPI: \_SB_: ACPI: _OSC: UUID: 0811B06E-4A27-44F9-8D60-3CBBC22E7B48, rev: 1
>  ACPI: \_SB_: ACPI: _OSC: capabilities DWORD 0: [00000001]
>  ACPI: \_SB_: ACPI: _OSC: capabilities DWORD 1: [002e7eff]
>  ACPI: \_SB_: ACPI: _OSC: request failed
>  ACPI: \_SB_: ACPI: _OSC: invalid revision
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

[PATCH v2.1 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 _OSC 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>
---

v1 -> v2.1:
   * Fix typo in the changelog (Jonathan).
   * Use at_least to enforce compiler checking of in_params[] size instead of
     using "static" directly (Jonathan).

---
 drivers/acpi/bus.c |   91 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 38 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -196,52 +196,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[at_least 4],
+			 struct acpi_buffer *output)
 {
-	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;
-	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
-
-	if (!context)
-		return AE_ERROR;
-	if (guid_parse(context->uuid_str, &guid))
-		return AE_ERROR;
-	context->ret.length = ACPI_ALLOCATE_BUFFER;
-	context->ret.pointer = NULL;
+	acpi_status status;
 
-	/* Setting up input parameters */
-	input.count = 4;
+	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;
-	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;
+	input.count = 4;
 
-	if (!output.length)
-		return AE_NULL_OBJECT;
+	output->length = ACPI_ALLOCATE_BUFFER;
+	output->pointer = NULL;
 
-	out_obj = output.pointer;
-	if (out_obj->type != ACPI_TYPE_BUFFER
-		|| out_obj->buffer.length != context->cap.length) {
-		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
-		acpi_handle_debug(handle, "_OSC: evaluation returned wrong type");
-		status = AE_TYPE;
-		goto out_kfree;
+	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)
+{
+	u32 errors, *capbuf = context->cap.pointer;
+	union acpi_object in_params[4], *out_obj;
+	struct acpi_buffer output;
+	acpi_status status = AE_OK;
+	guid_t guid;
+	int ret;
+
+	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;
+
+	out_obj = output.pointer;
 	/* Only take defined error bits into account. */
 	errors = *((u32 *)out_obj->buffer.pointer) & OSC_ERROR_MASK;
 	/*

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

[Jonathan Cameron]

On Mon, 22 Dec 2025 20:14:16 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Split a function for evaluating _OSC 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>
Hi Rafael,

There is a little bit of reformatting / whitespace cleanup in here
that makes it a tiny bit harder to review than if that had been
done separately.  Not worth a respin though.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
> 
> v1 -> v2.1:
>    * Fix typo in the changelog (Jonathan).
>    * Use at_least to enforce compiler checking of in_params[] size instead of
>      using "static" directly (Jonathan).
> 
> ---
>  drivers/acpi/bus.c |   91 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 53 insertions(+), 38 deletions(-)
> 
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -196,52 +196,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[at_least 4],
> +			 struct acpi_buffer *output)
>  {
> -	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;
> -	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> -
> -	if (!context)
> -		return AE_ERROR;
> -	if (guid_parse(context->uuid_str, &guid))
> -		return AE_ERROR;
> -	context->ret.length = ACPI_ALLOCATE_BUFFER;
> -	context->ret.pointer = NULL;
> +	acpi_status status;
>  
> -	/* Setting up input parameters */
> -	input.count = 4;
> +	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;
> -	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;
Not sure I'd have made this formatting change in here because it
'might' have hidden real functional changes from reviewers.

> -
> -	status = acpi_evaluate_object(handle, "_OSC", &input, &output);
> -	if (ACPI_FAILURE(status))
> -		return status;
> +	input.count = 4;
>  
> -	if (!output.length)
> -		return AE_NULL_OBJECT;
> +	output->length = ACPI_ALLOCATE_BUFFER;
> +	output->pointer = NULL;
>  
> -	out_obj = output.pointer;
> -	if (out_obj->type != ACPI_TYPE_BUFFER
> -		|| out_obj->buffer.length != context->cap.length) {
> -		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
> -		acpi_handle_debug(handle, "_OSC: evaluation returned wrong type");
> -		status = AE_TYPE;
> -		goto out_kfree;
> +	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;
> +}

[PATCH v2.1 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 _OSC 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>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
---

v1 -> v2.1:
   * Rebase on the modified first patch.
   * Fix typo in the changelog (Jonathan).
   * Expand the comment regarding ignoring OSC_CAPABILITIES_MASK_ERROR
     when OSC_QUERY_DWORD is set (Jonathan).
   * Add R-by from Jonathan.

---
 drivers/acpi/bus.c |   95 +++++++++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -237,13 +237,60 @@ 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)
+{
+	/* Only take defined error bits into account. */
+	u32 errors = retbuf[OSC_QUERY_DWORD] & OSC_ERROR_MASK;
+	u32 *capbuf = cap->pointer;
+	bool fail;
+
+	/*
+	 * 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)
+		return false;
+
+	acpi_dump_osc_data(handle, guid, rev, cap);
+	/*
+	 * As a rule, fail only if OSC_QUERY_ENABLE is set because otherwise the
+	 * acknowledged features need to be controlled.
+	 */
+	fail = !!(capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE);
+
+	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");
+		/*
+		 * Always fail if this bit is set because it means that the
+		 * request could not be processed.
+		 */
+		fail = true;
+	}
+
+	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");
+
+	return fail;
+}
+
 acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 {
-	u32 errors, *capbuf = context->cap.pointer;
 	union acpi_object in_params[4], *out_obj;
 	struct acpi_buffer output;
 	acpi_status status = AE_OK;
 	guid_t guid;
+	u32 *retbuf;
 	int ret;
 
 	if (!context || !context->cap.pointer ||
@@ -257,51 +304,15 @@ 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 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;
+	retbuf = (u32 *)out_obj->buffer.pointer;
 
-	if (errors) {
-		/*
-		 * As a rule, fail only if OSC_QUERY_ENABLE is set because
-		 * otherwise the acknowledged features need to be controlled.
-		 */
-		bool fail = !!(capbuf[OSC_QUERY_DWORD] & OSC_QUERY_ENABLE);
-
-		acpi_dump_osc_data(handle, &guid, context->rev, &context->cap);
-		if (errors & OSC_INVALID_UUID_ERROR) {
-			acpi_handle_debug(handle, "_OSC: invalid UUID");
-			/*
-			 * Always fail if this bit is set because it means that
-			 * the request could not be processed.
-			 */
-			fail = true;
-			goto out_kfree;
-		}
-		if (errors & OSC_REQUEST_ERROR)
-			acpi_handle_debug(handle, "_OSC: request failed");
-
-		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");
-
-		if (fail) {
-			status = AE_ERROR;
-			goto out_kfree;
-		}
+	if (acpi_osc_error_check(handle, &guid, context->rev, &context->cap, retbuf)) {
+		status = AE_ERROR;
+		goto out_kfree;
 	}
 
 	context->ret.length = out_obj->buffer.length;
-	context->ret.pointer = kmemdup(out_obj->buffer.pointer,
-				       context->ret.length, GFP_KERNEL);
+	context->ret.pointer = kmemdup(retbuf, context->ret.length, GFP_KERNEL);
 	if (!context->ret.pointer) {
 		status =  AE_NO_MEMORY;
 		goto out_kfree;

[PATCH v2.1 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>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
---

v1 -> v2.1: Add R-by from Jonathan.

---
 drivers/acpi/bus.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -308,19 +308,19 @@ 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;
 	context->ret.pointer = kmemdup(retbuf, 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 v2.1 6/8] ACPI: bus: Rework the handling of \_SB._OSC platform features

[Rafael J. Wysocki]

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

Both acpi_bus_osc_negotiate_platform_control() and
acpi_bus_osc_negotiate_usb_control() first call acpi_run_osc() to
evaluate _OSC in "query mode", with OSC_QUERY_ENABLE set in the
capabilities buffer, and then use the resultant feature mask as
the input buffer for requesting control of those features by
calling acpi_run_osc() to evaluate _OSC with OSC_QUERY_ENABLE clear.

This involves some code duplication and unnecessary memory
allocations, so introduce a new helper function carrying out an
_OSC handshake along the lines of the above description in a simpler
way and update acpi_bus_osc_negotiate_platform_control() to use it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2.1:
   * Add printing error messages regarding unexpected _OSC errors when
     processing a feature mask acknowledged previously (Jonathan).
   * Update the changelog after moving the essential change to patch [1/8].

---
 drivers/acpi/bus.c |  141 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 101 insertions(+), 40 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -325,6 +325,92 @@ 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.  capbuf[] contains a feature mask of all zeros.
+	 */
+	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.  Since the feature
+	 * bits that were clear in the return buffer from the previous _OSC
+	 * evaluation are also clear in the capabilities buffer now, this _OSC
+	 * evaluation is not expected to fail.
+	 */
+	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 in capbuf[] that have not been acknowledged.
+	 * After that, capbuf[] contains the resultant feature mask.
+	 */
+	for (i = OSC_QUERY_DWORD + 1; i < bufsize; i++)
+		capbuf[i] &= retbuf[i];
+
+	if (retbuf[OSC_QUERY_DWORD] & OSC_ERROR_MASK) {
+		/*
+		 * Complain about the unexpected errors and print diagnostic
+		 * information related to them.
+		 */
+		acpi_handle_err(handle, "_OSC: errors while processing control request\n");
+		acpi_handle_err(handle, "_OSC: some features may be missing\n");
+		acpi_osc_error_check(handle, &guid, rev, cap, retbuf);
+	}
+
+out:
+	ACPI_FREE(out_obj);
+	return ret;
+}
+
 bool osc_sb_apei_support_acked;
 
 /*
@@ -356,19 +442,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;
@@ -414,43 +497,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);
+	if (acpi_osc_handshake(handle, sb_uuid_str, 1, &cap))
 		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)))
-		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;
 }
 
 /*

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

[Jonathan Cameron]

On Mon, 22 Dec 2025 20:21:19 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Both acpi_bus_osc_negotiate_platform_control() and
> acpi_bus_osc_negotiate_usb_control() first call acpi_run_osc() to
> evaluate _OSC in "query mode", with OSC_QUERY_ENABLE set in the
> capabilities buffer, and then use the resultant feature mask as
> the input buffer for requesting control of those features by
> calling acpi_run_osc() to evaluate _OSC with OSC_QUERY_ENABLE clear.
> 
> This involves some code duplication and unnecessary memory
> allocations, so introduce a new helper function carrying out an
> _OSC handshake along the lines of the above description in a simpler
> way and update acpi_bus_osc_negotiate_platform_control() to use it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

[PATCH v2.1 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 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 representig the features for which control has
been granted.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
---

v1 -> v2.1:
   * Print one more message including the initial feature mask before processing.
   * Add R-by from Jonathan (assuming it still applies).

---
 drivers/acpi/bus.c |   85 +++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -445,73 +445,70 @@ 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
-
-	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_FLEXIBLE_ADR_SPACE;
+		feature_mask |= OSC_SB_PRM_SUPPORT;
 
-	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;
+
+	acpi_handle_info(handle, "platform _OSC: OS support mask [%08x]\n", 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: OS control 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;
 }
 
 /*

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

[Jonathan Cameron]

On Mon, 22 Dec 2025 20:23:29 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The feature mask creation for \_SB._OSC 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 representig the features for which control has
> been granted.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
> 
> v1 -> v2.1:
>    * Print one more message including the initial feature mask before processing.
>    * Add R-by from Jonathan (assuming it still applies).
FWIW the extra message makes sense to me. So RB definitely still applies.

[PATCH v2.1 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>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
---

v1 -> v2.1:
   * Drop a redundant local variable.
   * Add R-by from Jonathan (assuming it still applies).

---
 drivers/acpi/bus.c |   58 +++++++----------------------------------------------
 1 file changed, 8 insertions(+), 50 deletions(-)

--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -528,19 +528,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;
+	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;
@@ -551,54 +547,16 @@ 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;
-
-	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 = capbuf[OSC_CONTROL_DWORD];
 
 	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", osc_sb_native_usb4_control);
 }
 
 /* --------------------------------------------------------------------------