Re: [PATCH v1 2/4] ACPI: property: Add code comments explaining what is going on

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Rafael,

On Mon, Sep 15, 2025 at 08:05:34PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, Sep 15, 2025 at 7:52 PM Sakari Ailus
> <sakari.ailus@linux.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@kernel.org> wrote:
> > > >
> > > > Hi Sakari,
> > > >
> > > > On Mon, Sep 15, 2025 at 1:32 PM Sakari Ailus
> > > > <sakari.ailus@linux.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@intel.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@intel.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.

Conceivably, ACPICA could offer an evaluation function similar to
acpi_evaluate_object_typed() that simply wouldn't resolve references. I'm
not saying we should try to change existing behaviour elsewhere.

I don't really have an opinion how to address this, just that this is a
possibility as well.

-- 
Kind regards,

Sakari Ailus