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:
+ /*
+ * 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:
+ /*
+ * 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);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.
> + /*
> + * 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).
> + /*
> + * 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);
>
--
Kind regards,
Sakari Ailus
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.
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);
> >
>
> --
Hi Rafael, On Mon, Sep 15, 2025 at 02:27:16PM +0200, Rafael J. Wysocki 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: > > > + /* > > > + * 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? As long as the target is a non-device object (or name or method object at least), which is required by DSD-guide for (non-string-)referenced objects. > > But the comment would still be valid in that case, wouldn't it? After re-reading the first paragraph, I agree. > > > > + /* > > > + * 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. Is the "original AML" relevant? Aren't we just interested in how the evaluation result was reached instead of what was its actual path? We won't know the latter in any case. What would you think of: /* * Evaluating a reference results in a package object * (required by DSD guide) allocated on the fly. The * actual target object of the reference isn't * available. */ I guess nothing prevents having further string references within the object? I think it'd be best to deprecate direct references in the DSD guide. > > > + */ > > > desc = &link->package.elements[1]; > > > result = acpi_nondev_subnode_extract(desc, NULL, link, > > > list, parent); > > > > > -- Kind regards, Sakari Ailus
Hi Sakari, On Mon, Sep 15, 2025 at 10:19 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Mon, Sep 15, 2025 at 02:27:16PM +0200, Rafael J. Wysocki 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: > > > > + /* > > > > + * 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? > > As long as the target is a non-device object (or name or method object at > least), which is required by DSD-guide for (non-string-)referenced objects. > > > > > But the comment would still be valid in that case, wouldn't it? > > After re-reading the first paragraph, I agree. > > > > > > > + /* > > > > + * 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. > > Is the "original AML" relevant? Aren't we just interested in how the > evaluation result was reached instead of what was its actual path? So long as the node in question is not referred to via a namepath from a different place (for instance, a reference property in a different node), we don't. However, if there is such a reference to it somewhere, we need to know that this is the target node of that reference. > We won't know the latter in any case. What would you think of: > > /* > * Evaluating a reference results in a package object > * (required by DSD guide) allocated on the fly. The > * actual target object of the reference isn't > * available. > */ The target object actually is available, but the path to it isn't known at this point. > > I guess nothing prevents having further string references within the > object? The _DSD guide forbids that and they would only work if they were full namepaths (because of the unknown scope). Anyway, I think that the comment is as good as it gets in its current form. > I think it'd be best to deprecate direct references in the DSD guide. That I agree with, but the code needs to be retained for compatibility with the installed base. > > > > + */ > > > > desc = &link->package.elements[1]; > > > > result = acpi_nondev_subnode_extract(desc, NULL, link, > > > > list, parent); > > > > > > > > > --
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.
Thanks!
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
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.
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
© 2016 - 2025 Red Hat, Inc.