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.
> + * 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.
> {
> 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;
>
> --
