| 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
| ||
|
Message-ID: <CAJZ5v0ivX3s=pChGZ_+zeUswJgMPDH2Wi_cGeATyh+M9Tb0LYw@mail.gmail.com>
Date: Mon, 15 Sep 2025 14:27:16 +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 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.
Is this what you mean?
> > + /*
> > + * The reference is expected to point to an object
> > + * returning a package that contains _DSD-equivalent
> > + * information.
> > + */
> > handle = link->package.elements[1].reference.handle;
> > result = acpi_nondev_subnode_data_ok(handle, link, list,
> > parent);
> > break;
> > case ACPI_TYPE_PACKAGE:
>
> And similarly, the result of an evaluation here is a package when a
> reference points to a name object (i.e. a data node).
Well, I'm not sure how this remark is related to the new comment below.
Do you mean that this always happens when a reference is used in ASL
to point to the target here?
But the comment would still be valid in that case, wouldn't it?
> > + /*
> > + * This happens when the target package is embedded
> > + * within the links package as a result of direct
> > + * evaluation of an object pointed to by a reference.
> > + *
> > + * The target package is expected to contain _DSD-
> > + * equivalent information, but the scope in which it
> > + * is located in the original AML is unknown. Thus
> > + * it cannot contain pathname segments represented as
> > + * strings because there is no way to build full
> > + * pathnames out of them.
> > + */
> > desc = &link->package.elements[1];
> > result = acpi_nondev_subnode_extract(desc, NULL, link,
> > list, parent);
> >
>
> --
Powered by blists - more mailing lists