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
