[PATCH v1 0/4] ACPI: property: Two fixes, more documentation and a cleanup

[PATCH v1 0/4] ACPI: property: Two fixes, more documentation and a cleanup

[Rafael J. Wysocki]

Hi All,

A user report regarding "ACPI: \: Can't tag data node" error messages in dmesg
made me look at the ACPI property code and I've found a couple of issues in
it.

Also, it took me some time to figure out why the code was doing what it was
doing, so I decided to add some comments explaining it.

Finally, there's always something that can be cleaned up in every piece of
kernel code.

Hence, this series.

Please refer to the patch changelogs for details.

Thanks!

[PATCH v1 1/4] ACPI: property: Fix buffer properties extraction for subnodes

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The ACPI handle passed to acpi_extract_properties() as the first
argument represents the ACPI namespace scope in which to look for
objects returning buffers associated with buffer properties.

For _DSD objects located immediately under ACPI devices, this handle is
the same as the handle of the device object holding the _DSD, but for
data-only subnodes it is not so.

First of all, data-only subnodes are represented by objects that
cannot hold other objects in their scopes (like control methods).
Therefore a data-only subnode handle cannot be used for completing
relative pathname segments, so the current code in
in acpi_nondev_subnode_extract() passing a data-only subnode handle
to acpi_extract_properties() is invalid.

Moreover, a data-only subnode of device A may be represented by an
object located in the scope of device B (which kind of makes sense,
for instance, if A is a B's child).  In that case, the scope in
question would be the one of device B.  In other words, the scope
mentioned above is the same as the scope used for subnode object
lookup in acpi_nondev_subnode_extract().

Accordingly, rearrange that function to use the same scope for the
extraction of properties and subnode object lookup.

Fixes: 103e10c69c61 ("ACPI: property: Add support for parsing buffer property UUID")
Cc: 6.0+ <stable@vger.kernel.org> # 6.0+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/property.c |   30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -83,6 +83,7 @@ static bool acpi_nondev_subnode_extract(
 					struct fwnode_handle *parent)
 {
 	struct acpi_data_node *dn;
+	acpi_handle scope = NULL;
 	bool result;
 
 	if (acpi_graph_ignore_port(handle))
@@ -98,27 +99,18 @@ static bool acpi_nondev_subnode_extract(
 	INIT_LIST_HEAD(&dn->data.properties);
 	INIT_LIST_HEAD(&dn->data.subnodes);
 
-	result = acpi_extract_properties(handle, desc, &dn->data);
+	/*
+	 * The scope for the completion of relative pathname segments and
+	 * subnode object lookup is the one of the namespace node (device)
+	 * containing the object that has returned the package.  That is, it's
+	 * the scope of that object's parent device.
+	 */
+	if (handle)
+		acpi_get_parent(handle, &scope);
 
-	if (handle) {
-		acpi_handle scope;
-		acpi_status status;
-
-		/*
-		 * The scope for the subnode object lookup is the one of the
-		 * namespace node (device) containing the object that has
-		 * returned the package.  That is, it's the scope of that
-		 * object's parent.
-		 */
-		status = acpi_get_parent(handle, &scope);
-		if (ACPI_SUCCESS(status)
-		    && acpi_enumerate_nondev_subnodes(scope, desc, &dn->data,
-						      &dn->fwnode))
-			result = true;
-	} else if (acpi_enumerate_nondev_subnodes(NULL, desc, &dn->data,
-						  &dn->fwnode)) {
+	result = acpi_extract_properties(scope, desc, &dn->data);
+	if (acpi_enumerate_nondev_subnodes(scope, desc, &dn->data, &dn->fwnode))
 		result = true;
-	}
 
 	if (result) {
 		dn->handle = handle;

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

[Rafael J. Wysocki]

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);

[PATCH v1 3/4] ACPI: property: Do not pass NULL handles to acpi_attach_data()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In certain circumstances, the ACPI handle of a data-only node may be
NULL, in which case it does not make sense to attempt to attach that
node to an ACPI namespace object, so update the code to avoid attempts
to do so.

This prevents confusing and unuseful error messages from being printed.

Also document the fact that the ACPI handle of a data-only node may be
NULL, and when that happens, in a code comment.

In addition, make acpi_add_nondev_subnodes() print a diagnostic message
for each data-only node with an unknown ACPI namespace scope.

Fixes: 1d52f10917a7 ("ACPI: property: Tie data nodes to acpi handles")
Cc: 6.0+ <stable@vger.kernel.org> # 6.0+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/property.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -124,6 +124,10 @@ static bool acpi_nondev_subnode_extract(
 		result = true;
 
 	if (result) {
+		/*
+		 * This will be NULL if the desc package is embedded in an outer
+		 * _DSD-equivalent package and its scope cannot be determined.
+		 */
 		dn->handle = handle;
 		dn->data.pointer = desc;
 		list_add_tail(&dn->sibling, list);
@@ -245,6 +249,8 @@ static bool acpi_add_nondev_subnodes(acp
 			 * strings because there is no way to build full
 			 * pathnames out of them.
 			 */
+			acpi_handle_info(scope, "Unknown namespace scope of node %s\n",
+					 link->package.elements[0].string.pointer);
 			desc = &link->package.elements[1];
 			result = acpi_nondev_subnode_extract(desc, NULL, link,
 							     list, parent);
@@ -408,6 +414,9 @@ static void acpi_untie_nondev_subnodes(s
 	struct acpi_data_node *dn;
 
 	list_for_each_entry(dn, &data->subnodes, sibling) {
+		if (!dn->handle)
+			continue;
+
 		acpi_detach_data(dn->handle, acpi_nondev_subnode_tag);
 
 		acpi_untie_nondev_subnodes(&dn->data);
@@ -422,6 +431,9 @@ static bool acpi_tie_nondev_subnodes(str
 		acpi_status status;
 		bool ret;
 
+		if (!dn->handle)
+			continue;
+
 		status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
 		if (ACPI_FAILURE(status) && status != AE_ALREADY_EXISTS) {
 			acpi_handle_err(dn->handle, "Can't tag data node\n");

[PATCH v1 4/4] ACPI: property: Adjust failure handling in acpi_nondev_subnode_extract()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make acpi_nondev_subnode_extract() follow the usual code flow pattern
in which failure is handled at the point where it is detected.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/property.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -123,20 +123,21 @@ static bool acpi_nondev_subnode_extract(
 	if (acpi_enumerate_nondev_subnodes(scope, desc, &dn->data, &dn->fwnode))
 		result = true;
 
-	if (result) {
-		/*
-		 * This will be NULL if the desc package is embedded in an outer
-		 * _DSD-equivalent package and its scope cannot be determined.
-		 */
-		dn->handle = handle;
-		dn->data.pointer = desc;
-		list_add_tail(&dn->sibling, list);
-		return true;
+	if (!result) {
+		kfree(dn);
+		acpi_handle_debug(handle, "Invalid properties/subnodes data, skipping\n");
+		return false;
 	}
 
-	kfree(dn);
-	acpi_handle_debug(handle, "Invalid properties/subnodes data, skipping\n");
-	return false;
+	/*
+	 * This will be NULL if the desc package is embedded in an outer
+	 * _DSD-equivalent package and its scope cannot be determined.
+	 */
+	dn->handle = handle;
+	dn->data.pointer = desc;
+	list_add_tail(&dn->sibling, list);
+
+	return true;
 }
 
 static bool acpi_nondev_subnode_data_ok(acpi_handle handle,

Re: [PATCH v1 1/4] ACPI: property: Fix buffer properties extraction for subnodes

[Sakari Ailus]

Hi Rafael,

On Fri, Sep 12, 2025 at 09:39:52PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The ACPI handle passed to acpi_extract_properties() as the first
> argument represents the ACPI namespace scope in which to look for
> objects returning buffers associated with buffer properties.
> 
> For _DSD objects located immediately under ACPI devices, this handle is
> the same as the handle of the device object holding the _DSD, but for
> data-only subnodes it is not so.
> 
> First of all, data-only subnodes are represented by objects that
> cannot hold other objects in their scopes (like control methods).
> Therefore a data-only subnode handle cannot be used for completing
> relative pathname segments, so the current code in
> in acpi_nondev_subnode_extract() passing a data-only subnode handle
> to acpi_extract_properties() is invalid.
> 
> Moreover, a data-only subnode of device A may be represented by an
> object located in the scope of device B (which kind of makes sense,
> for instance, if A is a B's child).  In that case, the scope in
> question would be the one of device B.  In other words, the scope
> mentioned above is the same as the scope used for subnode object
> lookup in acpi_nondev_subnode_extract().
> 
> Accordingly, rearrange that function to use the same scope for the
> extraction of properties and subnode object lookup.
> 
> Fixes: 103e10c69c61 ("ACPI: property: Add support for parsing buffer property UUID")

I believe the commit introducing this is
99db5ff7fe0b4e1657423d7bbe2aa8f655dd02d1 .

> Cc: 6.0+ <stable@vger.kernel.org> # 6.0+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/property.c |   30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -83,6 +83,7 @@ static bool acpi_nondev_subnode_extract(
>  					struct fwnode_handle *parent)
>  {
>  	struct acpi_data_node *dn;
> +	acpi_handle scope = NULL;
>  	bool result;
>  
>  	if (acpi_graph_ignore_port(handle))
> @@ -98,27 +99,18 @@ static bool acpi_nondev_subnode_extract(
>  	INIT_LIST_HEAD(&dn->data.properties);
>  	INIT_LIST_HEAD(&dn->data.subnodes);
>  
> -	result = acpi_extract_properties(handle, desc, &dn->data);
> +	/*
> +	 * The scope for the completion of relative pathname segments and
> +	 * subnode object lookup is the one of the namespace node (device)
> +	 * containing the object that has returned the package.  That is, it's
> +	 * the scope of that object's parent device.
> +	 */
> +	if (handle)
> +		acpi_get_parent(handle, &scope);
>  
> -	if (handle) {
> -		acpi_handle scope;
> -		acpi_status status;
> -
> -		/*
> -		 * The scope for the subnode object lookup is the one of the
> -		 * namespace node (device) containing the object that has
> -		 * returned the package.  That is, it's the scope of that
> -		 * object's parent.
> -		 */
> -		status = acpi_get_parent(handle, &scope);
> -		if (ACPI_SUCCESS(status)
> -		    && acpi_enumerate_nondev_subnodes(scope, desc, &dn->data,
> -						      &dn->fwnode))
> -			result = true;
> -	} else if (acpi_enumerate_nondev_subnodes(NULL, desc, &dn->data,
> -						  &dn->fwnode)) {
> +	result = acpi_extract_properties(scope, desc, &dn->data);
> +	if (acpi_enumerate_nondev_subnodes(scope, desc, &dn->data, &dn->fwnode))
>  		result = true;
> -	}
>  
>  	if (result) {
>  		dn->handle = handle;
> 
> 

-- 
Kind regards,

Sakari Ailus

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

[Sakari Ailus]

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

Re: [PATCH v1 3/4] ACPI: property: Do not pass NULL handles to acpi_attach_data()

[Sakari Ailus]

Hi Rafael,

On Fri, Sep 12, 2025 at 09:42:55PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In certain circumstances, the ACPI handle of a data-only node may be
> NULL, in which case it does not make sense to attempt to attach that
> node to an ACPI namespace object, so update the code to avoid attempts
> to do so.
> 
> This prevents confusing and unuseful error messages from being printed.
> 
> Also document the fact that the ACPI handle of a data-only node may be
> NULL, and when that happens, in a code comment.
> 
> In addition, make acpi_add_nondev_subnodes() print a diagnostic message
> for each data-only node with an unknown ACPI namespace scope.
> 
> Fixes: 1d52f10917a7 ("ACPI: property: Tie data nodes to acpi handles")
> Cc: 6.0+ <stable@vger.kernel.org> # 6.0+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/property.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -124,6 +124,10 @@ static bool acpi_nondev_subnode_extract(
>  		result = true;
>  
>  	if (result) {
> +		/*
> +		 * This will be NULL if the desc package is embedded in an outer
> +		 * _DSD-equivalent package and its scope cannot be determined.
> +		 */

I think indeed this happens in particular when when references to
non-device nodes; there's no handle because when you get is basically a
dynamically allocated copy of a node.

>  		dn->handle = handle;
>  		dn->data.pointer = desc;
>  		list_add_tail(&dn->sibling, list);
> @@ -245,6 +249,8 @@ static bool acpi_add_nondev_subnodes(acp
>  			 * strings because there is no way to build full
>  			 * pathnames out of them.
>  			 */
> +			acpi_handle_info(scope, "Unknown namespace scope of node %s\n",
> +					 link->package.elements[0].string.pointer);
>  			desc = &link->package.elements[1];
>  			result = acpi_nondev_subnode_extract(desc, NULL, link,
>  							     list, parent);
> @@ -408,6 +414,9 @@ static void acpi_untie_nondev_subnodes(s
>  	struct acpi_data_node *dn;
>  
>  	list_for_each_entry(dn, &data->subnodes, sibling) {
> +		if (!dn->handle)
> +			continue;
> +
>  		acpi_detach_data(dn->handle, acpi_nondev_subnode_tag);
>  
>  		acpi_untie_nondev_subnodes(&dn->data);
> @@ -422,6 +431,9 @@ static bool acpi_tie_nondev_subnodes(str
>  		acpi_status status;
>  		bool ret;
>  
> +		if (!dn->handle)
> +			continue;
> +
>  		status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
>  		if (ACPI_FAILURE(status) && status != AE_ALREADY_EXISTS) {
>  			acpi_handle_err(dn->handle, "Can't tag data node\n");
> 
> 
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v1 1/4] ACPI: property: Fix buffer properties extraction for subnodes

[Rafael J. Wysocki]

Hi Sakari,

On Mon, Sep 15, 2025 at 1:04 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Fri, Sep 12, 2025 at 09:39:52PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The ACPI handle passed to acpi_extract_properties() as the first
> > argument represents the ACPI namespace scope in which to look for
> > objects returning buffers associated with buffer properties.
> >
> > For _DSD objects located immediately under ACPI devices, this handle is
> > the same as the handle of the device object holding the _DSD, but for
> > data-only subnodes it is not so.
> >
> > First of all, data-only subnodes are represented by objects that
> > cannot hold other objects in their scopes (like control methods).
> > Therefore a data-only subnode handle cannot be used for completing
> > relative pathname segments, so the current code in
> > in acpi_nondev_subnode_extract() passing a data-only subnode handle
> > to acpi_extract_properties() is invalid.
> >
> > Moreover, a data-only subnode of device A may be represented by an
> > object located in the scope of device B (which kind of makes sense,
> > for instance, if A is a B's child).  In that case, the scope in
> > question would be the one of device B.  In other words, the scope
> > mentioned above is the same as the scope used for subnode object
> > lookup in acpi_nondev_subnode_extract().
> >
> > Accordingly, rearrange that function to use the same scope for the
> > extraction of properties and subnode object lookup.
> >
> > Fixes: 103e10c69c61 ("ACPI: property: Add support for parsing buffer property UUID")
>
> I believe the commit introducing this is
> 99db5ff7fe0b4e1657423d7bbe2aa8f655dd02d1 .

No, it isn't.  Prior to commit 103e10c69c61, scope was not passed to
acpi_extract_properties().

> > Cc: 6.0+ <stable@vger.kernel.org> # 6.0+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/property.c |   30 +++++++++++-------------------
> >  1 file changed, 11 insertions(+), 19 deletions(-)
> >
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -83,6 +83,7 @@ static bool acpi_nondev_subnode_extract(
> >                                       struct fwnode_handle *parent)
> >  {
> >       struct acpi_data_node *dn;
> > +     acpi_handle scope = NULL;
> >       bool result;
> >
> >       if (acpi_graph_ignore_port(handle))
> > @@ -98,27 +99,18 @@ static bool acpi_nondev_subnode_extract(
> >       INIT_LIST_HEAD(&dn->data.properties);
> >       INIT_LIST_HEAD(&dn->data.subnodes);
> >
> > -     result = acpi_extract_properties(handle, desc, &dn->data);
> > +     /*
> > +      * The scope for the completion of relative pathname segments and
> > +      * subnode object lookup is the one of the namespace node (device)
> > +      * containing the object that has returned the package.  That is, it's
> > +      * the scope of that object's parent device.
> > +      */
> > +     if (handle)
> > +             acpi_get_parent(handle, &scope);
> >
> > -     if (handle) {
> > -             acpi_handle scope;
> > -             acpi_status status;
> > -
> > -             /*
> > -              * The scope for the subnode object lookup is the one of the
> > -              * namespace node (device) containing the object that has
> > -              * returned the package.  That is, it's the scope of that
> > -              * object's parent.
> > -              */
> > -             status = acpi_get_parent(handle, &scope);
> > -             if (ACPI_SUCCESS(status)
> > -                 && acpi_enumerate_nondev_subnodes(scope, desc, &dn->data,
> > -                                                   &dn->fwnode))
> > -                     result = true;
> > -     } else if (acpi_enumerate_nondev_subnodes(NULL, desc, &dn->data,
> > -                                               &dn->fwnode)) {
> > +     result = acpi_extract_properties(scope, desc, &dn->data);
> > +     if (acpi_enumerate_nondev_subnodes(scope, desc, &dn->data, &dn->fwnode))
> >               result = true;
> > -     }
> >
> >       if (result) {
> >               dn->handle = handle;
> >
> >
>
> --

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

[Rafael J. Wysocki]

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);
> >
>
> --

Re: [PATCH v1 3/4] ACPI: property: Do not pass NULL handles to acpi_attach_data()

[Rafael J. Wysocki]

Hi Sakari,

On Mon, Sep 15, 2025 at 1:50 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Fri, Sep 12, 2025 at 09:42:55PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In certain circumstances, the ACPI handle of a data-only node may be
> > NULL, in which case it does not make sense to attempt to attach that
> > node to an ACPI namespace object, so update the code to avoid attempts
> > to do so.
> >
> > This prevents confusing and unuseful error messages from being printed.
> >
> > Also document the fact that the ACPI handle of a data-only node may be
> > NULL, and when that happens, in a code comment.
> >
> > In addition, make acpi_add_nondev_subnodes() print a diagnostic message
> > for each data-only node with an unknown ACPI namespace scope.
> >
> > Fixes: 1d52f10917a7 ("ACPI: property: Tie data nodes to acpi handles")
> > Cc: 6.0+ <stable@vger.kernel.org> # 6.0+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/property.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -124,6 +124,10 @@ static bool acpi_nondev_subnode_extract(
> >               result = true;
> >
> >       if (result) {
> > +             /*
> > +              * This will be NULL if the desc package is embedded in an outer
> > +              * _DSD-equivalent package and its scope cannot be determined.
> > +              */
>
> I think indeed this happens in particular when when references to
> non-device nodes; there's no handle because when you get is basically a
> dynamically allocated copy of a node.

I know for a fact that this happens, that's why I'm adding the comment here.

> >               dn->handle = handle;
> >               dn->data.pointer = desc;
> >               list_add_tail(&dn->sibling, list);
> > @@ -245,6 +249,8 @@ static bool acpi_add_nondev_subnodes(acp
> >                        * strings because there is no way to build full
> >                        * pathnames out of them.
> >                        */
> > +                     acpi_handle_info(scope, "Unknown namespace scope of node %s\n",
> > +                                      link->package.elements[0].string.pointer);
> >                       desc = &link->package.elements[1];
> >                       result = acpi_nondev_subnode_extract(desc, NULL, link,
> >                                                            list, parent);
> > @@ -408,6 +414,9 @@ static void acpi_untie_nondev_subnodes(s
> >       struct acpi_data_node *dn;
> >
> >       list_for_each_entry(dn, &data->subnodes, sibling) {
> > +             if (!dn->handle)
> > +                     continue;
> > +
> >               acpi_detach_data(dn->handle, acpi_nondev_subnode_tag);
> >
> >               acpi_untie_nondev_subnodes(&dn->data);
> > @@ -422,6 +431,9 @@ static bool acpi_tie_nondev_subnodes(str
> >               acpi_status status;
> >               bool ret;
> >
> > +             if (!dn->handle)
> > +                     continue;
> > +
> >               status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> >               if (ACPI_FAILURE(status) && status != AE_ALREADY_EXISTS) {
> >                       acpi_handle_err(dn->handle, "Can't tag data node\n");
> >
> >
> >
>
> --
> Kind regards,
>
> Sakari Ailus

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

[Rafael J. Wysocki]

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!

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

[Sakari Ailus]

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

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

[Rafael J. Wysocki]

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.

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

[Sakari Ailus]

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

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

[Sakari Ailus]

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

Re: [PATCH v1 3/4] ACPI: property: Do not pass NULL handles to acpi_attach_data()

[Sakari Ailus]

Hi Rafael,

On Mon, Sep 15, 2025 at 02:57:31PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, Sep 15, 2025 at 1:50 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Fri, Sep 12, 2025 at 09:42:55PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > In certain circumstances, the ACPI handle of a data-only node may be
> > > NULL, in which case it does not make sense to attempt to attach that
> > > node to an ACPI namespace object, so update the code to avoid attempts
> > > to do so.
> > >
> > > This prevents confusing and unuseful error messages from being printed.
> > >
> > > Also document the fact that the ACPI handle of a data-only node may be
> > > NULL, and when that happens, in a code comment.
> > >
> > > In addition, make acpi_add_nondev_subnodes() print a diagnostic message
> > > for each data-only node with an unknown ACPI namespace scope.
> > >
> > > Fixes: 1d52f10917a7 ("ACPI: property: Tie data nodes to acpi handles")
> > > Cc: 6.0+ <stable@vger.kernel.org> # 6.0+
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/acpi/property.c |   12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -124,6 +124,10 @@ static bool acpi_nondev_subnode_extract(
> > >               result = true;
> > >
> > >       if (result) {
> > > +             /*
> > > +              * This will be NULL if the desc package is embedded in an outer
> > > +              * _DSD-equivalent package and its scope cannot be determined.
> > > +              */
> >
> > I think indeed this happens in particular when when references to
> > non-device nodes; there's no handle because when you get is basically a
> > dynamically allocated copy of a node.
> 
> I know for a fact that this happens, that's why I'm adding the comment here.

I wanted to say it could be helpful to have this written in the comment.

> 
> > >               dn->handle = handle;
> > >               dn->data.pointer = desc;
> > >               list_add_tail(&dn->sibling, list);
> > > @@ -245,6 +249,8 @@ static bool acpi_add_nondev_subnodes(acp
> > >                        * strings because there is no way to build full
> > >                        * pathnames out of them.
> > >                        */
> > > +                     acpi_handle_info(scope, "Unknown namespace scope of node %s\n",
> > > +                                      link->package.elements[0].string.pointer);
> > >                       desc = &link->package.elements[1];
> > >                       result = acpi_nondev_subnode_extract(desc, NULL, link,
> > >                                                            list, parent);
> > > @@ -408,6 +414,9 @@ static void acpi_untie_nondev_subnodes(s
> > >       struct acpi_data_node *dn;
> > >
> > >       list_for_each_entry(dn, &data->subnodes, sibling) {
> > > +             if (!dn->handle)
> > > +                     continue;
> > > +
> > >               acpi_detach_data(dn->handle, acpi_nondev_subnode_tag);
> > >
> > >               acpi_untie_nondev_subnodes(&dn->data);
> > > @@ -422,6 +431,9 @@ static bool acpi_tie_nondev_subnodes(str
> > >               acpi_status status;
> > >               bool ret;
> > >
> > > +             if (!dn->handle)
> > > +                     continue;
> > > +
> > >               status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> > >               if (ACPI_FAILURE(status) && status != AE_ALREADY_EXISTS) {
> > >                       acpi_handle_err(dn->handle, "Can't tag data node\n");
> > >
> > >
> > >
> >

-- 
Kind regards,

Sakari Ailus

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

[Rafael J. Wysocki]

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);
> > > >
> > >
>
> --