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

Rafael J. Wysocki
2025-12-22T17:57:48Z

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);
> >
> >
> >
> >
> >
>