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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0jVeSrDO6hrZhKgRZrH=FpGD4vNUjFD8hV9WwN9TLHjzQ@mail.gmail.com>
Date: Mon, 15 Sep 2025 19:12:44 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: 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

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.

Thanks!