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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx>
> > - *          Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> > - *          Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > + *         Darren Hart <dvhart@xxxxxxxxxxxxxxx>
> > + *         Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
> >   */
> >
> > +#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;
> >
> > --

-- 
Regards,

Sakari Ailus



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux