Message-ID: <CAJZ5v0iAG8PfeejzL3wUsW4b_9oakgAVi2vOQbvLkB7=rU85=g@mail.gmail.com>
Date: Mon, 15 Sep 2025 20:05:34 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux ACPI <linux-acpi@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v1 2/4] ACPI: property: Add code comments explaining what
 is going on

Hi Sakari,

On Mon, Sep 15, 2025 at 7:52 PM Sakari Ailus
<sakari.ailus@...ux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, Sep 15, 2025 at 07:12:44PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Sep 15, 2025 at 2:27 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > >
> > > Hi Sakari,
> > >
> > > On Mon, Sep 15, 2025 at 1:32 PM Sakari Ailus
> > > <sakari.ailus@...ux.intel.com> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > On Fri, Sep 12, 2025 at 09:40:58PM +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > > >
> > > > > In some places in the ACPI device properties handling code, it is
> > > > > unclear why the code is what it is.  Some assumptions are not documented
> > > > > and some pieces of code are based on experience that is not mentioned
> > > > > anywhere.
> > > > >
> > > > > Add code comments explaining these things.
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > > > ---
> > > > >  drivers/acpi/property.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 49 insertions(+), 2 deletions(-)
> > > > >
> > > > > --- a/drivers/acpi/property.c
> > > > > +++ b/drivers/acpi/property.c
> > > > > @@ -108,7 +108,18 @@ static bool acpi_nondev_subnode_extract(
> > > > >       if (handle)
> > > > >               acpi_get_parent(handle, &scope);
> > > > >
> > > > > +     /*
> > > > > +      * Extract properties from the _DSD-equivalent package pointed to by
> > > > > +      * desc and use scope (if not NULL) for the completion of relative
> > > > > +      * pathname segments.
> > > > > +      *
> > > > > +      * The extracted properties will be held in the new data node dn.
> > > > > +      */
> > > > >       result = acpi_extract_properties(scope, desc, &dn->data);
> > > > > +     /*
> > > > > +      * Look for subnodes in the _DSD-equivalent package pointed to by desc
> > > > > +      * and create child nodes of dn if there are any.
> > > > > +      */
> > > > >       if (acpi_enumerate_nondev_subnodes(scope, desc, &dn->data, &dn->fwnode))
> > > > >               result = true;
> > > > >
> > > > > @@ -153,6 +164,12 @@ static bool acpi_nondev_subnode_ok(acpi_
> > > > >       acpi_handle handle;
> > > > >       acpi_status status;
> > > > >
> > > > > +     /*
> > > > > +      * If the scope is unknown, the _DSD-equivalent package being parsed
> > > > > +      * was embedded in an outer _DSD-equivalent package as a result of
> > > > > +      * direct evaluation of an object pointed to by a reference.  In that
> > > > > +      * case, using a pathname as the target object pointer is invalid.
> > > > > +      */
> > > > >       if (!scope)
> > > > >               return false;
> > > > >
> > > > > @@ -172,6 +189,10 @@ static bool acpi_add_nondev_subnodes(acp
> > > > >       bool ret = false;
> > > > >       int i;
> > > > >
> > > > > +     /*
> > > > > +      * Every element in the links package is expected to represent a link
> > > > > +      * to a non-device node in a tree containing device-specific data.
> > > > > +      */
> > > > >       for (i = 0; i < links->package.count; i++) {
> > > > >               union acpi_object *link, *desc;
> > > > >               acpi_handle handle;
> > > > > @@ -182,22 +203,48 @@ static bool acpi_add_nondev_subnodes(acp
> > > > >               if (link->package.count != 2)
> > > > >                       continue;
> > > > >
> > > > > -             /* The first one must be a string. */
> > > > > +             /* The first one (the key) must be a string. */
> > > > >               if (link->package.elements[0].type != ACPI_TYPE_STRING)
> > > > >                       continue;
> > > > >
> > > > > -             /* The second one may be a string, a reference or a package. */
> > > > > +             /*
> > > > > +              * The second one (the target) may be a string, a reference or
> > > > > +              * a package.
> > > > > +              */
> > > > >               switch (link->package.elements[1].type) {
> > > > >               case ACPI_TYPE_STRING:
> > > > > +                     /*
> > > > > +                      * The string is expected to be a full pathname or a
> > > > > +                      * pathname segment relative to the given scope.  That
> > > > > +                      * pathname is expected to point to an object returning
> > > > > +                      * a package that contains _DSD-equivalent information.
> > > > > +                      */
> > > > >                       result = acpi_nondev_subnode_ok(scope, link, list,
> > > > >                                                        parent);
> > > > >                       break;
> > > > >               case ACPI_TYPE_LOCAL_REFERENCE:
> > > >
> > > > I think you get ACPI_TYPE_LOCAL_REFERENCE only when you evaluate a
> > > > reference to a device object.
> > >
> > > If it is so, the code below is just dead because the target here is
> > > expected to be a named object (not a device), in which case it would
> > > just be better to delete this code.
> >
> > Well, unless there's a bug in the ACPI tables attempting to add a
> > reference to a device as a data-only subnode.  Of course, this won't
> > work, but printing a message in that case may help.
> >
> > > Is this what you mean?
> >
> > I think it is and you are right: Referencing a named object will cause
> > that object to be evaluated automatically and its return data to be
> > embedded into the return package at the location of the reference.
> >
> > So I think that this piece is confusing and I'm going to get rid of it.
>
> Sounds reasonable. Maybe this change would be worth its own patch?

Yes, it would.

> The DSD guide indeed requires the target evaluates to a package object
> while a device object does not. The ACPICA doesn't document this behaviour
> (or at least I'm not aware of it), which is probably why we have this code
> here.

This is what generally happens when AML is evaluated.

For instance, on SMP platforms, each CPU object is expected to contain
multiple named objects like _CST, _CPC, _PSS etc.  Typically, each of
these objects returns the same data for every CPU and typically, there
is one internal named object referred to by, say, _CST for each CPU.
Had references been returned in such cases, that wouldn't have worked.

Powered by blists - more mailing lists