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

Posted by Rafael J. Wysocki 2 weeks, 5 days ago

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

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

Posted by Sakari Ailus 2 weeks, 3 days ago

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 3/4] ACPI: property: Do not pass NULL handles to acpi_attach_data()

Posted by Rafael J. Wysocki 2 weeks, 3 days ago

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 3/4] ACPI: property: Do not pass NULL handles to acpi_attach_data()

Posted by Sakari Ailus 2 weeks, 2 days ago

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