[PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Sakari Ailus]

Restore parsing of ACPI data node references consisting of a device node
reference followed by one or more child data node names.

Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
Cc: stable@vger.kernel.org # for 6.8 and later
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Rafael,

It seems that support for REF STR... references got accidentally removed
when pure STR reference were added. The former are documented in
Documentation/firmware-guide/acpi/dsd/graph.rst .

- Sakari

 drivers/acpi/property.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 436019d96027..4e3202a0b305 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -807,10 +807,27 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 static int acpi_get_ref_args(struct fwnode_reference_args *args,
 			     struct fwnode_handle *ref_fwnode,
 			     const union acpi_object **element,
-			     const union acpi_object *end, size_t num_args)
+			     const union acpi_object *end, size_t num_args,
+			     bool follow_strings)
 {
 	u32 nargs = 0, i;
 
+	/*
+	 * Parse REF STR... references by following named child nodes below the
+	 * device node pointed by REF.
+	 */
+	if (follow_strings) {
+		for (; (*element) < end && (*element)->type == ACPI_TYPE_STRING;
+		     (*element)++) {
+			const char *child_name = (*element)->string.pointer;
+
+			ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
+								      child_name);
+			if (!ref_fwnode)
+				return -EINVAL;
+		}
+	}
+
 	/*
 	 * Assume the following integer elements are all args. Stop counting on
 	 * the first reference (possibly represented as a string) or end of the
@@ -999,7 +1016,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 
 			ret = acpi_get_ref_args(idx == index ? args : NULL,
 						acpi_fwnode_handle(device),
-						&element, end, num_args);
+						&element, end, num_args, true);
 			if (ret < 0)
 				return ret;
 
@@ -1017,7 +1034,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 
 			ret = acpi_get_ref_args(idx == index ? args : NULL,
 						ref_fwnode, &element, end,
-						num_args);
+						num_args, false);
 			if (ret < 0)
 				return ret;
 
-- 
2.39.5

Re: [PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Rafael J. Wysocki]

On Mon, Mar 31, 2025 at 2:18 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Restore parsing of ACPI data node references consisting of a device node
> reference followed by one or more child data node names.
>
> Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
> Cc: stable@vger.kernel.org # for 6.8 and later
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Rafael,
>
> It seems that support for REF STR... references got accidentally removed
> when pure STR reference were added. The former are documented in
> Documentation/firmware-guide/acpi/dsd/graph.rst .

It would be good to provide an ASL example that is not parsed as
expected before the change and will be parsed correctly after it.

Admittedly, I can't quite recall the history leading to the above
commit, but this paragraphs is present in its changelog:

"Also remove the mechanism allowing data nodes to be referenced
 indirectly, with the help of an object reference pointing to the
 "ancestor" device and a path relative to it (this mechanism is not
 expected to be in use in any production platform firmware in the field)."

so the change in question appears to be intentional rather than accidental.

>  drivers/acpi/property.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 436019d96027..4e3202a0b305 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -807,10 +807,27 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
>  static int acpi_get_ref_args(struct fwnode_reference_args *args,
>                              struct fwnode_handle *ref_fwnode,
>                              const union acpi_object **element,
> -                            const union acpi_object *end, size_t num_args)
> +                            const union acpi_object *end, size_t num_args,
> +                            bool follow_strings)
>  {
>         u32 nargs = 0, i;
>
> +       /*
> +        * Parse REF STR... references by following named child nodes below the
> +        * device node pointed by REF.
> +        */
> +       if (follow_strings) {
> +               for (; (*element) < end && (*element)->type == ACPI_TYPE_STRING;
> +                    (*element)++) {
> +                       const char *child_name = (*element)->string.pointer;
> +
> +                       ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
> +                                                                     child_name);
> +                       if (!ref_fwnode)
> +                               return -EINVAL;
> +               }
> +       }
> +
>         /*
>          * Assume the following integer elements are all args. Stop counting on
>          * the first reference (possibly represented as a string) or end of the
> @@ -999,7 +1016,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
>                         ret = acpi_get_ref_args(idx == index ? args : NULL,
>                                                 acpi_fwnode_handle(device),
> -                                               &element, end, num_args);
> +                                               &element, end, num_args, true);
>                         if (ret < 0)
>                                 return ret;
>
> @@ -1017,7 +1034,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
>                         ret = acpi_get_ref_args(idx == index ? args : NULL,
>                                                 ref_fwnode, &element, end,
> -                                               num_args);
> +                                               num_args, false);
>                         if (ret < 0)
>                                 return ret;
>
> --
> 2.39.5
>
>

Re: [PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Sakari Ailus]

Hi Rafael,

On Mon, Mar 31, 2025 at 03:19:07PM +0200, Rafael J. Wysocki wrote:
> On Mon, Mar 31, 2025 at 2:18 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Restore parsing of ACPI data node references consisting of a device node
> > reference followed by one or more child data node names.
> >
> > Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
> > Cc: stable@vger.kernel.org # for 6.8 and later
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Rafael,
> >
> > It seems that support for REF STR... references got accidentally removed
> > when pure STR reference were added. The former are documented in
> > Documentation/firmware-guide/acpi/dsd/graph.rst .
> 
> It would be good to provide an ASL example that is not parsed as
> expected before the change and will be parsed correctly after it.
> 
> Admittedly, I can't quite recall the history leading to the above
> commit, but this paragraphs is present in its changelog:
> 
> "Also remove the mechanism allowing data nodes to be referenced
>  indirectly, with the help of an object reference pointing to the
>  "ancestor" device and a path relative to it (this mechanism is not
>  expected to be in use in any production platform firmware in the field)."
> 
> so the change in question appears to be intentional rather than accidental.

Right. This still continues to be documented in
Documentation/firmware-guide/acpi/dsd/graph.rst as noted in my previous
e-mail. A sample ASL snippet is can also be found in the same file. I only
noticed this when I tried to use such references.

The other option indeed is to change the documentation and hope no-one
depends on it.

> 
> >  drivers/acpi/property.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index 436019d96027..4e3202a0b305 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -807,10 +807,27 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> >  static int acpi_get_ref_args(struct fwnode_reference_args *args,
> >                              struct fwnode_handle *ref_fwnode,
> >                              const union acpi_object **element,
> > -                            const union acpi_object *end, size_t num_args)
> > +                            const union acpi_object *end, size_t num_args,
> > +                            bool follow_strings)
> >  {
> >         u32 nargs = 0, i;
> >
> > +       /*
> > +        * Parse REF STR... references by following named child nodes below the
> > +        * device node pointed by REF.
> > +        */
> > +       if (follow_strings) {
> > +               for (; (*element) < end && (*element)->type == ACPI_TYPE_STRING;
> > +                    (*element)++) {
> > +                       const char *child_name = (*element)->string.pointer;
> > +
> > +                       ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
> > +                                                                     child_name);
> > +                       if (!ref_fwnode)
> > +                               return -EINVAL;
> > +               }
> > +       }
> > +
> >         /*
> >          * Assume the following integer elements are all args. Stop counting on
> >          * the first reference (possibly represented as a string) or end of the
> > @@ -999,7 +1016,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> >
> >                         ret = acpi_get_ref_args(idx == index ? args : NULL,
> >                                                 acpi_fwnode_handle(device),
> > -                                               &element, end, num_args);
> > +                                               &element, end, num_args, true);
> >                         if (ret < 0)
> >                                 return ret;
> >
> > @@ -1017,7 +1034,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> >
> >                         ret = acpi_get_ref_args(idx == index ? args : NULL,
> >                                                 ref_fwnode, &element, end,
> > -                                               num_args);
> > +                                               num_args, false);
> >                         if (ret < 0)
> >                                 return ret;
> >

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Rafael J. Wysocki]

Hi Sakari,

On Mon, Mar 31, 2025 at 3:35 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, Mar 31, 2025 at 03:19:07PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Mar 31, 2025 at 2:18 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Restore parsing of ACPI data node references consisting of a device node
> > > reference followed by one or more child data node names.
> > >
> > > Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
> > > Cc: stable@vger.kernel.org # for 6.8 and later
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > Hi Rafael,
> > >
> > > It seems that support for REF STR... references got accidentally removed
> > > when pure STR reference were added. The former are documented in
> > > Documentation/firmware-guide/acpi/dsd/graph.rst .
> >
> > It would be good to provide an ASL example that is not parsed as
> > expected before the change and will be parsed correctly after it.
> >
> > Admittedly, I can't quite recall the history leading to the above
> > commit, but this paragraphs is present in its changelog:
> >
> > "Also remove the mechanism allowing data nodes to be referenced
> >  indirectly, with the help of an object reference pointing to the
> >  "ancestor" device and a path relative to it (this mechanism is not
> >  expected to be in use in any production platform firmware in the field)."
> >
> > so the change in question appears to be intentional rather than accidental.
>
> Right. This still continues to be documented in
> Documentation/firmware-guide/acpi/dsd/graph.rst as noted in my previous
> e-mail. A sample ASL snippet is can also be found in the same file. I only
> noticed this when I tried to use such references.
>
> The other option indeed is to change the documentation and hope no-one
> depends on it.

I thought that this was the original plan.

Re: [PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Sakari Ailus]

Hi Rafael,

On Mon, Mar 31, 2025 at 05:00:52PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, Mar 31, 2025 at 3:35 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, Mar 31, 2025 at 03:19:07PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Mar 31, 2025 at 2:18 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Restore parsing of ACPI data node references consisting of a device node
> > > > reference followed by one or more child data node names.
> > > >
> > > > Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
> > > > Cc: stable@vger.kernel.org # for 6.8 and later
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > Hi Rafael,
> > > >
> > > > It seems that support for REF STR... references got accidentally removed
> > > > when pure STR reference were added. The former are documented in
> > > > Documentation/firmware-guide/acpi/dsd/graph.rst .
> > >
> > > It would be good to provide an ASL example that is not parsed as
> > > expected before the change and will be parsed correctly after it.
> > >
> > > Admittedly, I can't quite recall the history leading to the above
> > > commit, but this paragraphs is present in its changelog:
> > >
> > > "Also remove the mechanism allowing data nodes to be referenced
> > >  indirectly, with the help of an object reference pointing to the
> > >  "ancestor" device and a path relative to it (this mechanism is not
> > >  expected to be in use in any production platform firmware in the field)."
> > >
> > > so the change in question appears to be intentional rather than accidental.
> >
> > Right. This still continues to be documented in
> > Documentation/firmware-guide/acpi/dsd/graph.rst as noted in my previous
> > e-mail. A sample ASL snippet is can also be found in the same file. I only
> > noticed this when I tried to use such references.
> >
> > The other option indeed is to change the documentation and hope no-one
> > depends on it.
> 
> I thought that this was the original plan.

I wasn't aware of the plan. But given that no-one has complained yet,
there's a fair chance no-one will. I can post patches to switch to
string-only references.

-- 
Sakari Ailus

Re: [PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Rafael J. Wysocki]

Hi Sakari,

On Mon, Mar 31, 2025 at 5:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, Mar 31, 2025 at 05:00:52PM +0200, Rafael J. Wysocki wrote:
> > Hi Sakari,
> >
> > On Mon, Mar 31, 2025 at 3:35 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Mon, Mar 31, 2025 at 03:19:07PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, Mar 31, 2025 at 2:18 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Restore parsing of ACPI data node references consisting of a device node
> > > > > reference followed by one or more child data node names.
> > > > >
> > > > > Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
> > > > > Cc: stable@vger.kernel.org # for 6.8 and later
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > > Hi Rafael,
> > > > >
> > > > > It seems that support for REF STR... references got accidentally removed
> > > > > when pure STR reference were added. The former are documented in
> > > > > Documentation/firmware-guide/acpi/dsd/graph.rst .
> > > >
> > > > It would be good to provide an ASL example that is not parsed as
> > > > expected before the change and will be parsed correctly after it.
> > > >
> > > > Admittedly, I can't quite recall the history leading to the above
> > > > commit, but this paragraphs is present in its changelog:
> > > >
> > > > "Also remove the mechanism allowing data nodes to be referenced
> > > >  indirectly, with the help of an object reference pointing to the
> > > >  "ancestor" device and a path relative to it (this mechanism is not
> > > >  expected to be in use in any production platform firmware in the field)."
> > > >
> > > > so the change in question appears to be intentional rather than accidental.
> > >
> > > Right. This still continues to be documented in
> > > Documentation/firmware-guide/acpi/dsd/graph.rst as noted in my previous
> > > e-mail. A sample ASL snippet is can also be found in the same file. I only
> > > noticed this when I tried to use such references.
> > >
> > > The other option indeed is to change the documentation and hope no-one
> > > depends on it.
> >
> > I thought that this was the original plan.
>
> I wasn't aware of the plan. But given that no-one has complained yet,
> there's a fair chance no-one will. I can post patches to switch to
> string-only references.

I think that it should be sufficient to update the documentation so it
does not describe the indirect mechanism mentioned in the changelog of
commit 9880702d123f, shouldn't it?

Re: [PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Sakari Ailus]

Hi Rafael,

On Mon, Mar 31, 2025 at 05:35:50PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, Mar 31, 2025 at 5:10 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, Mar 31, 2025 at 05:00:52PM +0200, Rafael J. Wysocki wrote:
> > > Hi Sakari,
> > >
> > > On Mon, Mar 31, 2025 at 3:35 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > On Mon, Mar 31, 2025 at 03:19:07PM +0200, Rafael J. Wysocki wrote:
> > > > > On Mon, Mar 31, 2025 at 2:18 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Restore parsing of ACPI data node references consisting of a device node
> > > > > > reference followed by one or more child data node names.
> > > > > >
> > > > > > Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
> > > > > > Cc: stable@vger.kernel.org # for 6.8 and later
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > > Hi Rafael,
> > > > > >
> > > > > > It seems that support for REF STR... references got accidentally removed
> > > > > > when pure STR reference were added. The former are documented in
> > > > > > Documentation/firmware-guide/acpi/dsd/graph.rst .
> > > > >
> > > > > It would be good to provide an ASL example that is not parsed as
> > > > > expected before the change and will be parsed correctly after it.
> > > > >
> > > > > Admittedly, I can't quite recall the history leading to the above
> > > > > commit, but this paragraphs is present in its changelog:
> > > > >
> > > > > "Also remove the mechanism allowing data nodes to be referenced
> > > > >  indirectly, with the help of an object reference pointing to the
> > > > >  "ancestor" device and a path relative to it (this mechanism is not
> > > > >  expected to be in use in any production platform firmware in the field)."
> > > > >
> > > > > so the change in question appears to be intentional rather than accidental.
> > > >
> > > > Right. This still continues to be documented in
> > > > Documentation/firmware-guide/acpi/dsd/graph.rst as noted in my previous
> > > > e-mail. A sample ASL snippet is can also be found in the same file. I only
> > > > noticed this when I tried to use such references.
> > > >
> > > > The other option indeed is to change the documentation and hope no-one
> > > > depends on it.
> > >
> > > I thought that this was the original plan.
> >
> > I wasn't aware of the plan. But given that no-one has complained yet,
> > there's a fair chance no-one will. I can post patches to switch to
> > string-only references.
> 
> I think that it should be sufficient to update the documentation so it
> does not describe the indirect mechanism mentioned in the changelog of
> commit 9880702d123f, shouldn't it?

I'd think so.

The REF+STR data node references actually go back to
Documentation/firmware-guide/acpi/dsd/data-node-references.rst merged in
2018 and their documented use includes LEDs (besides graphs) as well. Just
FYI though.

-- 
Sakari Ailus

Re: [PATCH 1/1] ACPI: property: Fix REF STR... reference parsing

[Sakari Ailus]

On Mon, Mar 31, 2025 at 04:10:32PM +0000, Sakari Ailus wrote:
> Hi Rafael,
> 
> On Mon, Mar 31, 2025 at 05:35:50PM +0200, Rafael J. Wysocki wrote:
> > Hi Sakari,
> > 
> > On Mon, Mar 31, 2025 at 5:10 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Mon, Mar 31, 2025 at 05:00:52PM +0200, Rafael J. Wysocki wrote:
> > > > Hi Sakari,
> > > >
> > > > On Mon, Mar 31, 2025 at 3:35 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > On Mon, Mar 31, 2025 at 03:19:07PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Mon, Mar 31, 2025 at 2:18 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > Restore parsing of ACPI data node references consisting of a device node
> > > > > > > reference followed by one or more child data node names.
> > > > > > >
> > > > > > > Fixes: 9880702d123f ("ACPI: property: Support using strings in reference properties")
> > > > > > > Cc: stable@vger.kernel.org # for 6.8 and later
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > ---
> > > > > > > Hi Rafael,
> > > > > > >
> > > > > > > It seems that support for REF STR... references got accidentally removed
> > > > > > > when pure STR reference were added. The former are documented in
> > > > > > > Documentation/firmware-guide/acpi/dsd/graph.rst .
> > > > > >
> > > > > > It would be good to provide an ASL example that is not parsed as
> > > > > > expected before the change and will be parsed correctly after it.
> > > > > >
> > > > > > Admittedly, I can't quite recall the history leading to the above
> > > > > > commit, but this paragraphs is present in its changelog:
> > > > > >
> > > > > > "Also remove the mechanism allowing data nodes to be referenced
> > > > > >  indirectly, with the help of an object reference pointing to the
> > > > > >  "ancestor" device and a path relative to it (this mechanism is not
> > > > > >  expected to be in use in any production platform firmware in the field)."
> > > > > >
> > > > > > so the change in question appears to be intentional rather than accidental.
> > > > >
> > > > > Right. This still continues to be documented in
> > > > > Documentation/firmware-guide/acpi/dsd/graph.rst as noted in my previous
> > > > > e-mail. A sample ASL snippet is can also be found in the same file. I only
> > > > > noticed this when I tried to use such references.
> > > > >
> > > > > The other option indeed is to change the documentation and hope no-one
> > > > > depends on it.
> > > >
> > > > I thought that this was the original plan.
> > >
> > > I wasn't aware of the plan. But given that no-one has complained yet,
> > > there's a fair chance no-one will. I can post patches to switch to
> > > string-only references.
> > 
> > I think that it should be sufficient to update the documentation so it
> > does not describe the indirect mechanism mentioned in the changelog of
> > commit 9880702d123f, shouldn't it?
> 
> I'd think so.
> 
> The REF+STR data node references actually go back to
> Documentation/firmware-guide/acpi/dsd/data-node-references.rst merged in
> 2018 and their documented use includes LEDs (besides graphs) as well. Just
> FYI though.

Oops. I missed Laurent's comments while replying the previous time. I'll
send v2 soon.

-- 
Sakari Ailus