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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

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;
> +}
> +