Message-ID: <CAJZ5v0jkdi_+zyqUKCb8pUzSYA9P5u4Ob3WgoJ8FMjMA=r0nug@mail.gmail.com>
Date: Mon, 22 Dec 2025 18:57:31 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux ACPI <linux-acpi@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Linux PCI <linux-pci@...r.kernel.org>, 
	Bjorn Helgaas <helgaas@...nel.org>, 
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>, Hans de Goede <hansg@...nel.org>, 
	Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v2] ACPI: bus: Fix handling of _OSC errors in acpi_run_osc()

On Mon, Dec 22, 2025 at 12:18 PM Jonathan Cameron
<jonathan.cameron@...wei.com> wrote:
>
> On Fri, 19 Dec 2025 22:28:02 +0100
> "Rafael J. Wysocki" <rafael@...nel.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@...wei.com> wrote:
> > > >
> > > > On Thu, 18 Dec 2025 21:34:26 +0100
> > > > "Rafael J. Wysocki" <rafael@...nel.org> wrote:
> > > >
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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@...el.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);
> >
> >
> >
> >
> >
>

Powered by blists - more mailing lists