Message-ID: <CAJZ5v0gp8E9_P3n5126F9YD72oyimCOHt1Z0cv_s_rUuipLY+Q@mail.gmail.com>
Date: Fri, 19 Dec 2025 18:48:57 +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 v1 6/8] ACPI: bus: Rework the handling of \_SB._OSC
 platform features

On Fri, Dec 19, 2025 at 1:56 PM Jonathan Cameron
<jonathan.cameron@...wei.com> wrote:
>
> On Thu, 18 Dec 2025 21:39:43 +0100
> "Rafael J. Wysocki" <rafael@...nel.org> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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;
> > +}
> > +

Powered by blists - more mailing lists