diff mbox series

[v8,02/10] ACPI: property: Parse data node string references in properties

Message ID 20230329100951.1522322-3-sakari.ailus@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand

Commit Message

Sakari Ailus March 29, 2023, 10:09 a.m. UTC
Add support for parsing property references using strings, besides
reference objects that were previously supported. This allows also
referencing data nodes which was not possible with reference objects.

Also add pr_fmt() macro to prefix printouts.

While at it, update copyright.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
 1 file changed, 94 insertions(+), 16 deletions(-)

Comments

Rafael J. Wysocki May 11, 2023, 5:04 p.m. UTC | #1
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Add support for parsing property references using strings, besides
> reference objects that were previously supported. This allows also
> referencing data nodes which was not possible with reference objects.
>
> Also add pr_fmt() macro to prefix printouts.
>
> While at it, update copyright.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

This one looks good to me, so formally

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and please add this tag if you resubmit.

Alternatively, I can just queue it up, because IMV this is an
improvement regardless of the rest of the series.

> ---
>  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 94 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index b8d9eb9a433e..08831ffba26c 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -2,14 +2,17 @@
>  /*
>   * ACPI device specific properties support.
>   *
> - * Copyright (C) 2014, Intel Corporation
> + * Copyright (C) 2014-2023, Intel Corporation
>   * All rights reserved.
>   *
>   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> - *          Darren Hart <dvhart@linux.intel.com>
> - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *         Darren Hart <dvhart@linux.intel.com>
> + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *         Sakari Ailus <sakari.ailus@linux.intel.com>
>   */
>
> +#define pr_fmt(fmt) "ACPI: " fmt
> +
>  #include <linux/acpi.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> @@ -795,7 +798,8 @@ 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 subnode_string)
>  {
>         u32 nargs = 0, i;
>
> @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
>          * Find the referred data extension node under the
>          * referred device node.
>          */
> -       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;
> +       if (subnode_string) {
> +               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;
> +               }
>         }
>
>         /*
> @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
>         for (i = 0; (*element) + i < end && i < num_args; i++) {
>                 acpi_object_type type = (*element)[i].type;
>
> -               if (type == ACPI_TYPE_LOCAL_REFERENCE)
> +               if (type == ACPI_TYPE_LOCAL_REFERENCE ||
> +                   (!subnode_string && type == ACPI_TYPE_STRING))
>                         break;
>
>                 if (type == ACPI_TYPE_INTEGER)
> @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
>         return 0;
>  }
>
> +static struct fwnode_handle *
> +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> +{
> +       acpi_handle scope, handle;
> +       struct acpi_data_node *dn;
> +       struct acpi_device *device;
> +       acpi_status status;
> +
> +       if (is_acpi_device_node(fwnode)) {
> +               scope = to_acpi_device_node(fwnode)->handle;
> +       } else if (is_acpi_data_node(fwnode)) {
> +               scope = to_acpi_data_node(fwnode)->handle;
> +       } else {
> +               pr_debug("bad node type for node %pfw\n", fwnode);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       status = acpi_get_handle(scope, refstring, &handle);
> +       if (ACPI_FAILURE(status)) {
> +               acpi_handle_debug(scope, "can't get handle for %s", refstring);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       device = acpi_fetch_acpi_dev(handle);
> +       if (device)
> +               return acpi_fwnode_handle(device);
> +
> +       status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
> +                                   (void **)&dn, NULL);
> +       if (ACPI_FAILURE(status) || !dn) {
> +               acpi_handle_debug(handle, "can't find subnode");
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return &dn->fwnode;
> +}
> +
>  /**
>   * __acpi_node_get_property_reference - returns handle to the referenced object
>   * @fwnode: Firmware node to get the property from
> @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>         const union acpi_object *element, *end;
>         const union acpi_object *obj;
>         const struct acpi_device_data *data;
> +       struct fwnode_handle *ref_fwnode;
>         struct acpi_device *device;
>         int ret, idx = 0;
>
> @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
>                 args->fwnode = acpi_fwnode_handle(device);
>                 args->nargs = 0;
> +               return 0;
> +       case ACPI_TYPE_STRING:
> +               if (index)
> +                       return -ENOENT;
> +
> +               ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
> +               if (IS_ERR(ref_fwnode))
> +                       return PTR_ERR(ref_fwnode);
> +
> +               args->fwnode = ref_fwnode;
> +               args->nargs = 0;
> +
>                 return 0;
>         case ACPI_TYPE_PACKAGE:
>                 /*
>                  * If it is not a single reference, then it is a package of
> -                * references followed by number of ints as follows:
> +                * references, followed by number of ints as follows:
>                  *
>                  *  Package () { REF, INT, REF, INT, INT }
>                  *
> -                * The index argument is then used to determine which reference
> -                * the caller wants (along with the arguments).
> +                * Here, REF may be either a local reference or a string. The
> +                * index argument is then used to determine which reference the
> +                * caller wants (along with the arguments).
>                  */
>                 break;
>         default:
> @@ -942,7 +1001,26 @@ 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;
> +
> +                       if (idx == index)
> +                               return 0;
> +
> +                       break;
> +               case ACPI_TYPE_STRING:
> +                       ref_fwnode =
> +                               acpi_parse_string_ref(fwnode,
> +                                                     element->string.pointer);
> +                       if (IS_ERR(ref_fwnode))
> +                               return PTR_ERR(ref_fwnode);
> +
> +                       element++;
> +
> +                       ret = acpi_get_ref_args(idx == index ? args : NULL,
> +                                               ref_fwnode, &element, end,
> +                                               num_args, false);
>                         if (ret < 0)
>                                 return ret;
>
> --
> 2.30.2
>
Rafael J. Wysocki May 12, 2023, 4:04 p.m. UTC | #2
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Add support for parsing property references using strings, besides
> reference objects that were previously supported. This allows also
> referencing data nodes which was not possible with reference objects.
>
> Also add pr_fmt() macro to prefix printouts.
>
> While at it, update copyright.

Although I said that it looked good to me, some minor improvements can
still be made.

First off, the above changelog is a bit terse.

I think that it would help to provide an example of device properties
that would not be parsed properly before the change and can be parsed
now.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 94 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index b8d9eb9a433e..08831ffba26c 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -2,14 +2,17 @@
>  /*
>   * ACPI device specific properties support.
>   *
> - * Copyright (C) 2014, Intel Corporation
> + * Copyright (C) 2014-2023, Intel Corporation
>   * All rights reserved.
>   *
>   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> - *          Darren Hart <dvhart@linux.intel.com>
> - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *         Darren Hart <dvhart@linux.intel.com>
> + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I'm not sure if the whitespace change here is really useful.

> + *         Sakari Ailus <sakari.ailus@linux.intel.com>
>   */
>
> +#define pr_fmt(fmt) "ACPI: " fmt
> +
>  #include <linux/acpi.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> @@ -795,7 +798,8 @@ 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 subnode_string)

The meaning of the new argument isn't really clear.  it would be good
to somehow help a casual reader of the code to find this out more
easily.

>  {
>         u32 nargs = 0, i;
>
> @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
>          * Find the referred data extension node under the
>          * referred device node.
>          */
> -       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;
> +       if (subnode_string) {
> +               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;
> +               }
>         }
>
>         /*
> @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
>         for (i = 0; (*element) + i < end && i < num_args; i++) {
>                 acpi_object_type type = (*element)[i].type;
>
> -               if (type == ACPI_TYPE_LOCAL_REFERENCE)
> +               if (type == ACPI_TYPE_LOCAL_REFERENCE ||
> +                   (!subnode_string && type == ACPI_TYPE_STRING))
>                         break;
>
>                 if (type == ACPI_TYPE_INTEGER)
> @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
>         return 0;
>  }
>
> +static struct fwnode_handle *
> +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> +{
> +       acpi_handle scope, handle;
> +       struct acpi_data_node *dn;
> +       struct acpi_device *device;
> +       acpi_status status;
> +
> +       if (is_acpi_device_node(fwnode)) {
> +               scope = to_acpi_device_node(fwnode)->handle;
> +       } else if (is_acpi_data_node(fwnode)) {
> +               scope = to_acpi_data_node(fwnode)->handle;
> +       } else {
> +               pr_debug("bad node type for node %pfw\n", fwnode);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       status = acpi_get_handle(scope, refstring, &handle);
> +       if (ACPI_FAILURE(status)) {
> +               acpi_handle_debug(scope, "can't get handle for %s", refstring);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       device = acpi_fetch_acpi_dev(handle);
> +       if (device)
> +               return acpi_fwnode_handle(device);
> +
> +       status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
> +                                   (void **)&dn, NULL);
> +       if (ACPI_FAILURE(status) || !dn) {
> +               acpi_handle_debug(handle, "can't find subnode");
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return &dn->fwnode;

So on failure this function always returns the same error code.  Can
it return NULL instead which can be translated into an error code by
the caller?

> +}
> +
>  /**
>   * __acpi_node_get_property_reference - returns handle to the referenced object
>   * @fwnode: Firmware node to get the property from
> @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>         const union acpi_object *element, *end;
>         const union acpi_object *obj;
>         const struct acpi_device_data *data;
> +       struct fwnode_handle *ref_fwnode;
>         struct acpi_device *device;
>         int ret, idx = 0;
>
> @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>
>                 args->fwnode = acpi_fwnode_handle(device);
>                 args->nargs = 0;
> +               return 0;
> +       case ACPI_TYPE_STRING:
> +               if (index)
> +                       return -ENOENT;
> +
> +               ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
> +               if (IS_ERR(ref_fwnode))
> +                       return PTR_ERR(ref_fwnode);
> +
> +               args->fwnode = ref_fwnode;
> +               args->nargs = 0;
> +
>                 return 0;
>         case ACPI_TYPE_PACKAGE:
>                 /*
>                  * If it is not a single reference, then it is a package of
> -                * references followed by number of ints as follows:
> +                * references, followed by number of ints as follows:
>                  *
>                  *  Package () { REF, INT, REF, INT, INT }
>                  *
> -                * The index argument is then used to determine which reference
> -                * the caller wants (along with the arguments).
> +                * Here, REF may be either a local reference or a string. The
> +                * index argument is then used to determine which reference the
> +                * caller wants (along with the arguments).
>                  */
>                 break;
>         default:
> @@ -942,7 +1001,26 @@ 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;
> +
> +                       if (idx == index)
> +                               return 0;
> +
> +                       break;
> +               case ACPI_TYPE_STRING:
> +                       ref_fwnode =
> +                               acpi_parse_string_ref(fwnode,
> +                                                     element->string.pointer);
> +                       if (IS_ERR(ref_fwnode))
> +                               return PTR_ERR(ref_fwnode);
> +
> +                       element++;
> +
> +                       ret = acpi_get_ref_args(idx == index ? args : NULL,
> +                                               ref_fwnode, &element, end,
> +                                               num_args, false);
>                         if (ret < 0)
>                                 return ret;
>
> --
Sakari Ailus May 16, 2023, 11:24 a.m. UTC | #3
Hi Rafael,

On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Add support for parsing property references using strings, besides
> > reference objects that were previously supported. This allows also
> > referencing data nodes which was not possible with reference objects.
> >
> > Also add pr_fmt() macro to prefix printouts.
> >
> > While at it, update copyright.
> 
> Although I said that it looked good to me, some minor improvements can
> still be made.
> 
> First off, the above changelog is a bit terse.
> 
> I think that it would help to provide an example of device properties
> that would not be parsed properly before the change and can be parsed
> now.
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 94 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index b8d9eb9a433e..08831ffba26c 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -2,14 +2,17 @@
> >  /*
> >   * ACPI device specific properties support.
> >   *
> > - * Copyright (C) 2014, Intel Corporation
> > + * Copyright (C) 2014-2023, Intel Corporation
> >   * All rights reserved.
> >   *
> >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > - *          Darren Hart <dvhart@linux.intel.com>
> > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + *         Darren Hart <dvhart@linux.intel.com>
> > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> I'm not sure if the whitespace change here is really useful.

I did that to address a comment from Andy --- the earlier lines used spaces
for indentation.

> 
> > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> >   */
> >
> > +#define pr_fmt(fmt) "ACPI: " fmt
> > +
> >  #include <linux/acpi.h>
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> > @@ -795,7 +798,8 @@ 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 subnode_string)
> 
> The meaning of the new argument isn't really clear.  it would be good
> to somehow help a casual reader of the code to find this out more
> easily.

I can add comments to v9.

> 
> >  {
> >         u32 nargs = 0, i;
> >
> > @@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> >          * Find the referred data extension node under the
> >          * referred device node.
> >          */
> > -       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;
> > +       if (subnode_string) {
> > +               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;
> > +               }
> >         }
> >
> >         /*
> > @@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> >         for (i = 0; (*element) + i < end && i < num_args; i++) {
> >                 acpi_object_type type = (*element)[i].type;
> >
> > -               if (type == ACPI_TYPE_LOCAL_REFERENCE)
> > +               if (type == ACPI_TYPE_LOCAL_REFERENCE ||
> > +                   (!subnode_string && type == ACPI_TYPE_STRING))
> >                         break;
> >
> >                 if (type == ACPI_TYPE_INTEGER)
> > @@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
> >         return 0;
> >  }
> >
> > +static struct fwnode_handle *
> > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> > +{
> > +       acpi_handle scope, handle;
> > +       struct acpi_data_node *dn;
> > +       struct acpi_device *device;
> > +       acpi_status status;
> > +
> > +       if (is_acpi_device_node(fwnode)) {
> > +               scope = to_acpi_device_node(fwnode)->handle;
> > +       } else if (is_acpi_data_node(fwnode)) {
> > +               scope = to_acpi_data_node(fwnode)->handle;
> > +       } else {
> > +               pr_debug("bad node type for node %pfw\n", fwnode);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       status = acpi_get_handle(scope, refstring, &handle);
> > +       if (ACPI_FAILURE(status)) {
> > +               acpi_handle_debug(scope, "can't get handle for %s", refstring);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       device = acpi_fetch_acpi_dev(handle);
> > +       if (device)
> > +               return acpi_fwnode_handle(device);
> > +
> > +       status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
> > +                                   (void **)&dn, NULL);
> > +       if (ACPI_FAILURE(status) || !dn) {
> > +               acpi_handle_debug(handle, "can't find subnode");
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       return &dn->fwnode;
> 
> So on failure this function always returns the same error code.  Can
> it return NULL instead which can be translated into an error code by
> the caller?

Sure, makes sense.

> 
> > +}
> > +
> >  /**
> >   * __acpi_node_get_property_reference - returns handle to the referenced object
> >   * @fwnode: Firmware node to get the property from
> > @@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> >         const union acpi_object *element, *end;
> >         const union acpi_object *obj;
> >         const struct acpi_device_data *data;
> > +       struct fwnode_handle *ref_fwnode;
> >         struct acpi_device *device;
> >         int ret, idx = 0;
> >
> > @@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
> >
> >                 args->fwnode = acpi_fwnode_handle(device);
> >                 args->nargs = 0;
> > +               return 0;
> > +       case ACPI_TYPE_STRING:
> > +               if (index)
> > +                       return -ENOENT;
> > +
> > +               ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
> > +               if (IS_ERR(ref_fwnode))
> > +                       return PTR_ERR(ref_fwnode);
> > +
> > +               args->fwnode = ref_fwnode;
> > +               args->nargs = 0;
> > +
> >                 return 0;
> >         case ACPI_TYPE_PACKAGE:
> >                 /*
> >                  * If it is not a single reference, then it is a package of
> > -                * references followed by number of ints as follows:
> > +                * references, followed by number of ints as follows:
> >                  *
> >                  *  Package () { REF, INT, REF, INT, INT }
> >                  *
> > -                * The index argument is then used to determine which reference
> > -                * the caller wants (along with the arguments).
> > +                * Here, REF may be either a local reference or a string. The
> > +                * index argument is then used to determine which reference the
> > +                * caller wants (along with the arguments).
> >                  */
> >                 break;
> >         default:
> > @@ -942,7 +1001,26 @@ 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;
> > +
> > +                       if (idx == index)
> > +                               return 0;
> > +
> > +                       break;
> > +               case ACPI_TYPE_STRING:
> > +                       ref_fwnode =
> > +                               acpi_parse_string_ref(fwnode,
> > +                                                     element->string.pointer);
> > +                       if (IS_ERR(ref_fwnode))
> > +                               return PTR_ERR(ref_fwnode);
> > +
> > +                       element++;
> > +
> > +                       ret = acpi_get_ref_args(idx == index ? args : NULL,
> > +                                               ref_fwnode, &element, end,
> > +                                               num_args, false);
> >                         if (ret < 0)
> >                                 return ret;
> >
> > --
Rafael J. Wysocki May 22, 2023, 3:29 p.m. UTC | #4
On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Add support for parsing property references using strings, besides
> > > reference objects that were previously supported. This allows also
> > > referencing data nodes which was not possible with reference objects.
> > >
> > > Also add pr_fmt() macro to prefix printouts.
> > >
> > > While at it, update copyright.
> >
> > Although I said that it looked good to me, some minor improvements can
> > still be made.
> >
> > First off, the above changelog is a bit terse.
> >
> > I think that it would help to provide an example of device properties
> > that would not be parsed properly before the change and can be parsed
> > now.
> >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 94 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index b8d9eb9a433e..08831ffba26c 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -2,14 +2,17 @@
> > >  /*
> > >   * ACPI device specific properties support.
> > >   *
> > > - * Copyright (C) 2014, Intel Corporation
> > > + * Copyright (C) 2014-2023, Intel Corporation
> > >   * All rights reserved.
> > >   *
> > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > - *          Darren Hart <dvhart@linux.intel.com>
> > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > + *         Darren Hart <dvhart@linux.intel.com>
> > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > I'm not sure if the whitespace change here is really useful.
>
> I did that to address a comment from Andy --- the earlier lines used spaces
> for indentation.
>
> >
> > > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> > >   */
> > >
> > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > +
> > >  #include <linux/acpi.h>
> > >  #include <linux/device.h>
> > >  #include <linux/export.h>
> > > @@ -795,7 +798,8 @@ 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 subnode_string)
> >
> > The meaning of the new argument isn't really clear.  it would be good
> > to somehow help a casual reader of the code to find this out more
> > easily.
>
> I can add comments to v9.

If you can send me an example of ASL that will be parsed correctly
after this change, but not before, it will help a bit.
Sakari Ailus May 22, 2023, 4:28 p.m. UTC | #5
Hi Rafael,

On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Add support for parsing property references using strings, besides
> > > > reference objects that were previously supported. This allows also
> > > > referencing data nodes which was not possible with reference objects.
> > > >
> > > > Also add pr_fmt() macro to prefix printouts.
> > > >
> > > > While at it, update copyright.
> > >
> > > Although I said that it looked good to me, some minor improvements can
> > > still be made.
> > >
> > > First off, the above changelog is a bit terse.
> > >
> > > I think that it would help to provide an example of device properties
> > > that would not be parsed properly before the change and can be parsed
> > > now.
> > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > >  1 file changed, 94 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > --- a/drivers/acpi/property.c
> > > > +++ b/drivers/acpi/property.c
> > > > @@ -2,14 +2,17 @@
> > > >  /*
> > > >   * ACPI device specific properties support.
> > > >   *
> > > > - * Copyright (C) 2014, Intel Corporation
> > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > >   * All rights reserved.
> > > >   *
> > > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > - *          Darren Hart <dvhart@linux.intel.com>
> > > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > + *         Darren Hart <dvhart@linux.intel.com>
> > > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > I'm not sure if the whitespace change here is really useful.
> >
> > I did that to address a comment from Andy --- the earlier lines used spaces
> > for indentation.
> >
> > >
> > > > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> > > >   */
> > > >
> > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > +
> > > >  #include <linux/acpi.h>
> > > >  #include <linux/device.h>
> > > >  #include <linux/export.h>
> > > > @@ -795,7 +798,8 @@ 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 subnode_string)
> > >
> > > The meaning of the new argument isn't really clear.  it would be good
> > > to somehow help a casual reader of the code to find this out more
> > > easily.
> >
> > I can add comments to v9.
> 
> If you can send me an example of ASL that will be parsed correctly
> after this change, but not before, it will help a bit.

E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):

	Package () {
	    "mipi-img-flash-leds",
	    Package () {
		"\\_SB.PCI0.I2C2.LEDD.LED0",
		"\\_SB.PCI0.I2C2.LEDD.LED1"
	    },
	},

It's a property with a string reference to an ACPI non-device node,
although you can refer to device nodes as well.

You can get the spec from here:
<URL:https://www.mipi.org/mipi-disco-for-imaging-download>.
Rafael J. Wysocki May 22, 2023, 4:38 p.m. UTC | #6
Hi Sakari,

On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Add support for parsing property references using strings, besides
> > > > > reference objects that were previously supported. This allows also
> > > > > referencing data nodes which was not possible with reference objects.
> > > > >
> > > > > Also add pr_fmt() macro to prefix printouts.
> > > > >
> > > > > While at it, update copyright.
> > > >
> > > > Although I said that it looked good to me, some minor improvements can
> > > > still be made.
> > > >
> > > > First off, the above changelog is a bit terse.
> > > >
> > > > I think that it would help to provide an example of device properties
> > > > that would not be parsed properly before the change and can be parsed
> > > > now.
> > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > >  1 file changed, 94 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > --- a/drivers/acpi/property.c
> > > > > +++ b/drivers/acpi/property.c
> > > > > @@ -2,14 +2,17 @@
> > > > >  /*
> > > > >   * ACPI device specific properties support.
> > > > >   *
> > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > >   * All rights reserved.
> > > > >   *
> > > > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > - *          Darren Hart <dvhart@linux.intel.com>
> > > > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > + *         Darren Hart <dvhart@linux.intel.com>
> > > > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > I'm not sure if the whitespace change here is really useful.
> > >
> > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > for indentation.
> > >
> > > >
> > > > > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > >   */
> > > > >
> > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > +
> > > > >  #include <linux/acpi.h>
> > > > >  #include <linux/device.h>
> > > > >  #include <linux/export.h>
> > > > > @@ -795,7 +798,8 @@ 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 subnode_string)
> > > >
> > > > The meaning of the new argument isn't really clear.  it would be good
> > > > to somehow help a casual reader of the code to find this out more
> > > > easily.
> > >
> > > I can add comments to v9.
> >
> > If you can send me an example of ASL that will be parsed correctly
> > after this change, but not before, it will help a bit.
>
> E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
>
>         Package () {
>             "mipi-img-flash-leds",
>             Package () {
>                 "\\_SB.PCI0.I2C2.LEDD.LED0",
>                 "\\_SB.PCI0.I2C2.LEDD.LED1"
>             },
>         },
>
> It's a property with a string reference to an ACPI non-device node,
> although you can refer to device nodes as well.

This example is missing the definition of LED0 or LED1 from which it
would be clear that they are data nodes (or at least one of them is a
data node).

Also I'm kind of wondering about the "reference with arguments" part
which seems to work differently depending on whether the reference is
represented by a string or by a reference object.

> You can get the spec from here:
> <URL:https://www.mipi.org/mipi-disco-for-imaging-download>.

Sure, but it alone won't help me much with documenting this code change.
Sakari Ailus May 22, 2023, 8:25 p.m. UTC | #7
Hi Rafael,

On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Add support for parsing property references using strings, besides
> > > > > > reference objects that were previously supported. This allows also
> > > > > > referencing data nodes which was not possible with reference objects.
> > > > > >
> > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > >
> > > > > > While at it, update copyright.
> > > > >
> > > > > Although I said that it looked good to me, some minor improvements can
> > > > > still be made.
> > > > >
> > > > > First off, the above changelog is a bit terse.
> > > > >
> > > > > I think that it would help to provide an example of device properties
> > > > > that would not be parsed properly before the change and can be parsed
> > > > > now.
> > > > >
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > >  1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > --- a/drivers/acpi/property.c
> > > > > > +++ b/drivers/acpi/property.c
> > > > > > @@ -2,14 +2,17 @@
> > > > > >  /*
> > > > > >   * ACPI device specific properties support.
> > > > > >   *
> > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > >   * All rights reserved.
> > > > > >   *
> > > > > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > - *          Darren Hart <dvhart@linux.intel.com>
> > > > > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > + *         Darren Hart <dvhart@linux.intel.com>
> > > > > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > I'm not sure if the whitespace change here is really useful.
> > > >
> > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > for indentation.
> > > >
> > > > >
> > > > > > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > >   */
> > > > > >
> > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > +
> > > > > >  #include <linux/acpi.h>
> > > > > >  #include <linux/device.h>
> > > > > >  #include <linux/export.h>
> > > > > > @@ -795,7 +798,8 @@ 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 subnode_string)
> > > > >
> > > > > The meaning of the new argument isn't really clear.  it would be good
> > > > > to somehow help a casual reader of the code to find this out more
> > > > > easily.
> > > >
> > > > I can add comments to v9.
> > >
> > > If you can send me an example of ASL that will be parsed correctly
> > > after this change, but not before, it will help a bit.
> >
> > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> >
> >         Package () {
> >             "mipi-img-flash-leds",
> >             Package () {
> >                 "\\_SB.PCI0.I2C2.LEDD.LED0",
> >                 "\\_SB.PCI0.I2C2.LEDD.LED1"
> >             },
> >         },
> >
> > It's a property with a string reference to an ACPI non-device node,
> > although you can refer to device nodes as well.
> 
> This example is missing the definition of LED0 or LED1 from which it
> would be clear that they are data nodes (or at least one of them is a
> data node).

Ok, perhaps this one could work as a complete example, with a single
reference:

	Package ()
        {
	    "mipi-img-flash-leds",  "\\_SB.PCI0.I2C2.LEDD.LED0",
	}

        Device (LEDD)
        {
            Name (_DSD, Package ()  // _DSD: Device-Specific Data
            {
                ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
                Package ()
                {
                    Package ()
                    {
                        "mipi-img-flash-led-0", 
                        "LED0",
                    }
                },
            })
            Name (LED0, Package ()  // _DSD: Device-Specific Data
            {
                ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
                Package ()
                {
                    Package ()
                    {
                        "mipi-img-max-current", 
                        1000000,
                    }
                }
            })
	}


> 
> Also I'm kind of wondering about the "reference with arguments" part
> which seems to work differently depending on whether the reference is
> represented by a string or by a reference object.

Yes. With (device) reference objects, it is possible currently to refer to
subnodes with the _DSD data extension child names of those nodes. This is
not done with string references as 1) any node can already be referenced so
there's no need to and 2) as node references are strings already, it's not
possible to distinguish node string references from _DSD data node names.
E.g.

	"\\_SB.I2C0.LED0", "LED1"

			   ^ ACPI object name or _DSD data node name?

> 
> > You can get the spec from here:
> > <URL:https://www.mipi.org/mipi-disco-for-imaging-download>.
> 
> Sure, but it alone won't help me much with documenting this code change.

Not as such but the spec is publicly available now.
Rafael J. Wysocki May 23, 2023, 11:21 a.m. UTC | #8
Hi Sakari,

On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > Add support for parsing property references using strings, besides
> > > > > > > reference objects that were previously supported. This allows also
> > > > > > > referencing data nodes which was not possible with reference objects.
> > > > > > >
> > > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > > >
> > > > > > > While at it, update copyright.
> > > > > >
> > > > > > Although I said that it looked good to me, some minor improvements can
> > > > > > still be made.
> > > > > >
> > > > > > First off, the above changelog is a bit terse.
> > > > > >
> > > > > > I think that it would help to provide an example of device properties
> > > > > > that would not be parsed properly before the change and can be parsed
> > > > > > now.
> > > > > >
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > > >  1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > > --- a/drivers/acpi/property.c
> > > > > > > +++ b/drivers/acpi/property.c
> > > > > > > @@ -2,14 +2,17 @@
> > > > > > >  /*
> > > > > > >   * ACPI device specific properties support.
> > > > > > >   *
> > > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > > >   * All rights reserved.
> > > > > > >   *
> > > > > > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > > - *          Darren Hart <dvhart@linux.intel.com>
> > > > > > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > + *         Darren Hart <dvhart@linux.intel.com>
> > > > > > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > I'm not sure if the whitespace change here is really useful.
> > > > >
> > > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > > for indentation.
> > > > >
> > > > > >
> > > > > > > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > >   */
> > > > > > >
> > > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > > +
> > > > > > >  #include <linux/acpi.h>
> > > > > > >  #include <linux/device.h>
> > > > > > >  #include <linux/export.h>
> > > > > > > @@ -795,7 +798,8 @@ 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 subnode_string)
> > > > > >
> > > > > > The meaning of the new argument isn't really clear.  it would be good
> > > > > > to somehow help a casual reader of the code to find this out more
> > > > > > easily.
> > > > >
> > > > > I can add comments to v9.
> > > >
> > > > If you can send me an example of ASL that will be parsed correctly
> > > > after this change, but not before, it will help a bit.
> > >
> > > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> > >
> > >         Package () {
> > >             "mipi-img-flash-leds",
> > >             Package () {
> > >                 "\\_SB.PCI0.I2C2.LEDD.LED0",
> > >                 "\\_SB.PCI0.I2C2.LEDD.LED1"
> > >             },
> > >         },
> > >
> > > It's a property with a string reference to an ACPI non-device node,
> > > although you can refer to device nodes as well.
> >
> > This example is missing the definition of LED0 or LED1 from which it
> > would be clear that they are data nodes (or at least one of them is a
> > data node).
>
> Ok, perhaps this one could work as a complete example, with a single
> reference:
>
>         Package ()
>         {
>             "mipi-img-flash-leds",  "\\_SB.PCI0.I2C2.LEDD.LED0",
>         }
>
>         Device (LEDD)
>         {
>             Name (_DSD, Package ()  // _DSD: Device-Specific Data
>             {
>                 ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
>                 Package ()
>                 {
>                     Package ()
>                     {
>                         "mipi-img-flash-led-0",
>                         "LED0",
>                     }
>                 },
>             })
>             Name (LED0, Package ()  // _DSD: Device-Specific Data
>             {
>                 ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
>                 Package ()
>                 {
>                     Package ()
>                     {
>                         "mipi-img-max-current",
>                         1000000,
>                     }
>                 }
>             })
>         }
>

This works, thanks!

> >
> > Also I'm kind of wondering about the "reference with arguments" part
> > which seems to work differently depending on whether the reference is
> > represented by a string or by a reference object.
>
> Yes. With (device) reference objects, it is possible currently to refer to
> subnodes with the _DSD data extension child names of those nodes. This is
> not done with string references as 1) any node can already be referenced so
> there's no need to and 2) as node references are strings already, it's not
> possible to distinguish node string references from _DSD data node names.
> E.g.
>
>         "\\_SB.I2C0.LED0", "LED1"
>
>                            ^ ACPI object name or _DSD data node name?
>

Has this behavior been documented anywhere?  Or is there any
expectation to see anything like this shipping in production platform
firmware?

If any of the above isn't the case, I would be inclined to simply
remove this special case and make both the "object reference" and
"string" cases work in the same way and if someone needs to refer to a
data node, they will just need to use a string (in which case it will
be the only option).
Sakari Ailus May 23, 2023, 11:43 a.m. UTC | #9
Hi Rafael,

On Tue, May 23, 2023 at 01:21:12PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > Add support for parsing property references using strings, besides
> > > > > > > > reference objects that were previously supported. This allows also
> > > > > > > > referencing data nodes which was not possible with reference objects.
> > > > > > > >
> > > > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > > > >
> > > > > > > > While at it, update copyright.
> > > > > > >
> > > > > > > Although I said that it looked good to me, some minor improvements can
> > > > > > > still be made.
> > > > > > >
> > > > > > > First off, the above changelog is a bit terse.
> > > > > > >
> > > > > > > I think that it would help to provide an example of device properties
> > > > > > > that would not be parsed properly before the change and can be parsed
> > > > > > > now.
> > > > > > >
> > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > > > >  1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > > > --- a/drivers/acpi/property.c
> > > > > > > > +++ b/drivers/acpi/property.c
> > > > > > > > @@ -2,14 +2,17 @@
> > > > > > > >  /*
> > > > > > > >   * ACPI device specific properties support.
> > > > > > > >   *
> > > > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > > > >   * All rights reserved.
> > > > > > > >   *
> > > > > > > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > > > - *          Darren Hart <dvhart@linux.intel.com>
> > > > > > > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > + *         Darren Hart <dvhart@linux.intel.com>
> > > > > > > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > >
> > > > > > > I'm not sure if the whitespace change here is really useful.
> > > > > >
> > > > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > > > for indentation.
> > > > > >
> > > > > > >
> > > > > > > > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > >   */
> > > > > > > >
> > > > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > > > +
> > > > > > > >  #include <linux/acpi.h>
> > > > > > > >  #include <linux/device.h>
> > > > > > > >  #include <linux/export.h>
> > > > > > > > @@ -795,7 +798,8 @@ 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 subnode_string)
> > > > > > >
> > > > > > > The meaning of the new argument isn't really clear.  it would be good
> > > > > > > to somehow help a casual reader of the code to find this out more
> > > > > > > easily.
> > > > > >
> > > > > > I can add comments to v9.
> > > > >
> > > > > If you can send me an example of ASL that will be parsed correctly
> > > > > after this change, but not before, it will help a bit.
> > > >
> > > > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> > > >
> > > >         Package () {
> > > >             "mipi-img-flash-leds",
> > > >             Package () {
> > > >                 "\\_SB.PCI0.I2C2.LEDD.LED0",
> > > >                 "\\_SB.PCI0.I2C2.LEDD.LED1"
> > > >             },
> > > >         },
> > > >
> > > > It's a property with a string reference to an ACPI non-device node,
> > > > although you can refer to device nodes as well.
> > >
> > > This example is missing the definition of LED0 or LED1 from which it
> > > would be clear that they are data nodes (or at least one of them is a
> > > data node).
> >
> > Ok, perhaps this one could work as a complete example, with a single
> > reference:
> >
> >         Package ()
> >         {
> >             "mipi-img-flash-leds",  "\\_SB.PCI0.I2C2.LEDD.LED0",
> >         }
> >
> >         Device (LEDD)
> >         {
> >             Name (_DSD, Package ()  // _DSD: Device-Specific Data
> >             {
> >                 ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
> >                 Package ()
> >                 {
> >                     Package ()
> >                     {
> >                         "mipi-img-flash-led-0",
> >                         "LED0",
> >                     }
> >                 },
> >             })
> >             Name (LED0, Package ()  // _DSD: Device-Specific Data
> >             {
> >                 ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> >                 Package ()
> >                 {
> >                     Package ()
> >                     {
> >                         "mipi-img-max-current",
> >                         1000000,
> >                     }
> >                 }
> >             })
> >         }
> >
> 
> This works, thanks!
> 
> > >
> > > Also I'm kind of wondering about the "reference with arguments" part
> > > which seems to work differently depending on whether the reference is
> > > represented by a string or by a reference object.
> >
> > Yes. With (device) reference objects, it is possible currently to refer to
> > subnodes with the _DSD data extension child names of those nodes. This is
> > not done with string references as 1) any node can already be referenced so
> > there's no need to and 2) as node references are strings already, it's not
> > possible to distinguish node string references from _DSD data node names.
> > E.g.
> >
> >         "\\_SB.I2C0.LED0", "LED1"
> >
> >                            ^ ACPI object name or _DSD data node name?
> >
> 
> Has this behavior been documented anywhere?  Or is there any
> expectation to see anything like this shipping in production platform
> firmware?

Good question. Support for this was added by commit 4eb0c3bf5ee52f . AFAIR
it was intended to use this in DisCo for Imaging but after review (in a
rather liberal sense of the term) it was decided to use string-only
references, as in this patch.

I'm not aware of anyone needing this. They've been there for about five
years but I'd guess someone would complain if it stops working for them.

> 
> If any of the above isn't the case, I would be inclined to simply
> remove this special case and make both the "object reference" and
> "string" cases work in the same way and if someone needs to refer to a
> data node, they will just need to use a string (in which case it will
> be the only option).

Works for me.
Rafael J. Wysocki May 23, 2023, 1:40 p.m. UTC | #10
Hi Sakari,

On Tue, May 23, 2023 at 1:43 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Tue, May 23, 2023 at 01:21:12PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:

[cut]

> > > >
> > > > Also I'm kind of wondering about the "reference with arguments" part
> > > > which seems to work differently depending on whether the reference is
> > > > represented by a string or by a reference object.
> > >
> > > Yes. With (device) reference objects, it is possible currently to refer to
> > > subnodes with the _DSD data extension child names of those nodes. This is
> > > not done with string references as 1) any node can already be referenced so
> > > there's no need to and 2) as node references are strings already, it's not
> > > possible to distinguish node string references from _DSD data node names.
> > > E.g.
> > >
> > >         "\\_SB.I2C0.LED0", "LED1"
> > >
> > >                            ^ ACPI object name or _DSD data node name?
> > >
> >
> > Has this behavior been documented anywhere?  Or is there any
> > expectation to see anything like this shipping in production platform
> > firmware?
>
> Good question. Support for this was added by commit 4eb0c3bf5ee52f . AFAIR
> it was intended to use this in DisCo for Imaging but after review (in a
> rather liberal sense of the term) it was decided to use string-only
> references, as in this patch.

It looks like this is sort of a placeholder then.

> I'm not aware of anyone needing this. They've been there for about five
> years but I'd guess someone would complain if it stops working for them.

It is also quite straightforward to restore if need be.

> >
> > If any of the above isn't the case, I would be inclined to simply
> > remove this special case and make both the "object reference" and
> > "string" cases work in the same way and if someone needs to refer to a
> > data node, they will just need to use a string (in which case it will
> > be the only option).
>
> Works for me.

OK, I'll make this change then.
diff mbox series

Patch

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index b8d9eb9a433e..08831ffba26c 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -2,14 +2,17 @@ 
 /*
  * ACPI device specific properties support.
  *
- * Copyright (C) 2014, Intel Corporation
+ * Copyright (C) 2014-2023, Intel Corporation
  * All rights reserved.
  *
  * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
- *          Darren Hart <dvhart@linux.intel.com>
- *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *	    Darren Hart <dvhart@linux.intel.com>
+ *	    Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *	    Sakari Ailus <sakari.ailus@linux.intel.com>
  */
 
+#define pr_fmt(fmt) "ACPI: " fmt
+
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/export.h>
@@ -795,7 +798,8 @@  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 subnode_string)
 {
 	u32 nargs = 0, i;
 
@@ -803,13 +807,16 @@  static int acpi_get_ref_args(struct fwnode_reference_args *args,
 	 * Find the referred data extension node under the
 	 * referred device node.
 	 */
-	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;
+	if (subnode_string) {
+		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;
+		}
 	}
 
 	/*
@@ -820,7 +827,8 @@  static int acpi_get_ref_args(struct fwnode_reference_args *args,
 	for (i = 0; (*element) + i < end && i < num_args; i++) {
 		acpi_object_type type = (*element)[i].type;
 
-		if (type == ACPI_TYPE_LOCAL_REFERENCE)
+		if (type == ACPI_TYPE_LOCAL_REFERENCE ||
+		    (!subnode_string && type == ACPI_TYPE_STRING))
 			break;
 
 		if (type == ACPI_TYPE_INTEGER)
@@ -844,6 +852,43 @@  static int acpi_get_ref_args(struct fwnode_reference_args *args,
 	return 0;
 }
 
+static struct fwnode_handle *
+acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
+{
+	acpi_handle scope, handle;
+	struct acpi_data_node *dn;
+	struct acpi_device *device;
+	acpi_status status;
+
+	if (is_acpi_device_node(fwnode)) {
+		scope = to_acpi_device_node(fwnode)->handle;
+	} else if (is_acpi_data_node(fwnode)) {
+		scope = to_acpi_data_node(fwnode)->handle;
+	} else {
+		pr_debug("bad node type for node %pfw\n", fwnode);
+		return ERR_PTR(-EINVAL);
+	}
+
+	status = acpi_get_handle(scope, refstring, &handle);
+	if (ACPI_FAILURE(status)) {
+		acpi_handle_debug(scope, "can't get handle for %s", refstring);
+		return ERR_PTR(-EINVAL);
+	}
+
+	device = acpi_fetch_acpi_dev(handle);
+	if (device)
+		return acpi_fwnode_handle(device);
+
+	status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
+				    (void **)&dn, NULL);
+	if (ACPI_FAILURE(status) || !dn) {
+		acpi_handle_debug(handle, "can't find subnode");
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &dn->fwnode;
+}
+
 /**
  * __acpi_node_get_property_reference - returns handle to the referenced object
  * @fwnode: Firmware node to get the property from
@@ -886,6 +931,7 @@  int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 	const union acpi_object *element, *end;
 	const union acpi_object *obj;
 	const struct acpi_device_data *data;
+	struct fwnode_handle *ref_fwnode;
 	struct acpi_device *device;
 	int ret, idx = 0;
 
@@ -909,16 +955,29 @@  int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 
 		args->fwnode = acpi_fwnode_handle(device);
 		args->nargs = 0;
+		return 0;
+	case ACPI_TYPE_STRING:
+		if (index)
+			return -ENOENT;
+
+		ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
+		if (IS_ERR(ref_fwnode))
+			return PTR_ERR(ref_fwnode);
+
+		args->fwnode = ref_fwnode;
+		args->nargs = 0;
+
 		return 0;
 	case ACPI_TYPE_PACKAGE:
 		/*
 		 * If it is not a single reference, then it is a package of
-		 * references followed by number of ints as follows:
+		 * references, followed by number of ints as follows:
 		 *
 		 *  Package () { REF, INT, REF, INT, INT }
 		 *
-		 * The index argument is then used to determine which reference
-		 * the caller wants (along with the arguments).
+		 * Here, REF may be either a local reference or a string. The
+		 * index argument is then used to determine which reference the
+		 * caller wants (along with the arguments).
 		 */
 		break;
 	default:
@@ -942,7 +1001,26 @@  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;
+
+			if (idx == index)
+				return 0;
+
+			break;
+		case ACPI_TYPE_STRING:
+			ref_fwnode =
+				acpi_parse_string_ref(fwnode,
+						      element->string.pointer);
+			if (IS_ERR(ref_fwnode))
+				return PTR_ERR(ref_fwnode);
+
+			element++;
+
+			ret = acpi_get_ref_args(idx == index ? args : NULL,
+						ref_fwnode, &element, end,
+						num_args, false);
 			if (ret < 0)
 				return ret;