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 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?

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.

-- 
Kind regards,

Sakari Ailus