[PATCH v3 0/8] ACPI: Buffer property and reference as string support

[PATCH v3 0/8] ACPI: Buffer property and reference as string support

[Sakari Ailus]

Hello everyone,

This set adds support for _DSD buffer properties (specified by DSD Guide
<URL:https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.md>) as well as
support for references as strings. Reference property type was previously
supported for device objects only, whereas string references enable
referencing also _DSD sub-node objects --- also included in the set.

The ACPICA patch has been submitted to upstream but not merged yet.

This set currently prepares for data node string reference support and
does not add it anymore.

since v2:

- Use C99 _Generic() in patch unifying reading integer arrays.

since v1:

- Drop the ACPICA, data node child list initialisation and data node
  string reference patches.

Sakari Ailus (8):
  ACPI: property: Return type of acpi_add_nondev_subnodes() should be
    bool
  ACPI: property: Tie data nodes to acpi handles
  ACPI: property: Use acpi_object_type consistently in property ref
    parsing
  ACPI: property: Move property ref argument parsing into a new function
  ACPI: property: Switch node property referencing from ifs to a switch
  ACPI: property: Unify integer value reading functions
  ACPI: property: Add support for parsing buffer property UUID
  ACPI: property: Read buffer properties as integers

 drivers/acpi/property.c | 462 +++++++++++++++++++++++++++-------------
 include/acpi/acpi_bus.h |   3 +-
 include/linux/acpi.h    |   2 +-
 3 files changed, 315 insertions(+), 152 deletions(-)

-- 
2.30.2

[PATCH v3 1/8] ACPI: property: Return type of acpi_add_nondev_subnodes() should be bool

[Sakari Ailus]

The value acpi_add_nondev_subnodes() returns is bool so change the return
type of the function to match that.

Fixes: 445b0eb058f5 ("ACPI / property: Add support for data-only subnodes")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index d3173811614ec..bc9a645f8bb77 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -155,10 +155,10 @@ static bool acpi_nondev_subnode_ok(acpi_handle scope,
 	return acpi_nondev_subnode_data_ok(handle, link, list, parent);
 }
 
-static int acpi_add_nondev_subnodes(acpi_handle scope,
-				    const union acpi_object *links,
-				    struct list_head *list,
-				    struct fwnode_handle *parent)
+static bool acpi_add_nondev_subnodes(acpi_handle scope,
+				     const union acpi_object *links,
+				     struct list_head *list,
+				     struct fwnode_handle *parent)
 {
 	bool ret = false;
 	int i;
-- 
2.30.2

Re: [PATCH v3 1/8] ACPI: property: Return type of acpi_add_nondev_subnodes() should be bool

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:16PM +0300, Sakari Ailus wrote:
> The value acpi_add_nondev_subnodes() returns is bool so change the return
> type of the function to match that.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 445b0eb058f5 ("ACPI / property: Add support for data-only subnodes")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index d3173811614ec..bc9a645f8bb77 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -155,10 +155,10 @@ static bool acpi_nondev_subnode_ok(acpi_handle scope,
>  	return acpi_nondev_subnode_data_ok(handle, link, list, parent);
>  }
>  
> -static int acpi_add_nondev_subnodes(acpi_handle scope,
> -				    const union acpi_object *links,
> -				    struct list_head *list,
> -				    struct fwnode_handle *parent)
> +static bool acpi_add_nondev_subnodes(acpi_handle scope,
> +				     const union acpi_object *links,
> +				     struct list_head *list,
> +				     struct fwnode_handle *parent)
>  {
>  	bool ret = false;
>  	int i;
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Sakari Ailus]

ACPICA allows associating additional information (i.e. pointers with
specific tag) to acpi_handles. The acpi_device's are associated to
acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the
_DSD data nodes.

This allows direct data node references in properties, implemented later on
in the series.

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

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index bc9a645f8bb77..f8c83ee6c8d59 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -340,6 +340,43 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
 	return props;
 }
 
+static void acpi_nondev_subnode_tag(acpi_handle handle, void *context)
+{
+}
+
+static void acpi_untie_nondev_subnodes(struct acpi_device_data *data)
+{
+	struct acpi_data_node *dn;
+
+	list_for_each_entry(dn, &data->subnodes, sibling) {
+		acpi_detach_data(dn->handle, acpi_nondev_subnode_tag);
+
+		acpi_untie_nondev_subnodes(&dn->data);
+	}
+}
+
+static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
+{
+	struct acpi_data_node *dn;
+
+	list_for_each_entry(dn, &data->subnodes, sibling) {
+		acpi_status status;
+		int ret;
+
+		status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
+		if (ACPI_FAILURE(status)) {
+			acpi_handle_err(dn->handle, "Can't tag data node\n");
+			return 0;
+		}
+
+		ret = acpi_tie_nondev_subnodes(&dn->data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static bool acpi_extract_properties(const union acpi_object *desc,
 				    struct acpi_device_data *data)
 {
@@ -419,7 +456,9 @@ void acpi_init_properties(struct acpi_device *adev)
 					&adev->data, acpi_fwnode_handle(adev)))
 		adev->data.pointer = buf.pointer;
 
-	if (!adev->data.pointer) {
+	if (!adev->data.pointer ||
+	    acpi_tie_nondev_subnodes(&adev->data) < 0) {
+		acpi_untie_nondev_subnodes(&adev->data);
 		acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n");
 		ACPI_FREE(buf.pointer);
 	}
@@ -462,6 +501,7 @@ static void acpi_destroy_nondev_subnodes(struct list_head *list)
 
 void acpi_free_properties(struct acpi_device *adev)
 {
+	acpi_untie_nondev_subnodes(&adev->data);
 	acpi_destroy_nondev_subnodes(&adev->data.subnodes);
 	ACPI_FREE((void *)adev->data.pointer);
 	adev->data.of_compatible = NULL;
-- 
2.30.2

Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:17PM +0300, Sakari Ailus wrote:
> ACPICA allows associating additional information (i.e. pointers with
> specific tag) to acpi_handles. The acpi_device's are associated to
> acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the
> _DSD data nodes.
> 
> This allows direct data node references in properties, implemented later on
> in the series.

...

> +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
> +{
> +	struct acpi_data_node *dn;
> +
> +	list_for_each_entry(dn, &data->subnodes, sibling) {
> +		acpi_status status;
> +		int ret;
> +
> +		status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> +		if (ACPI_FAILURE(status)) {
> +			acpi_handle_err(dn->handle, "Can't tag data node\n");
> +			return 0;
> +		}
> +
> +		ret = acpi_tie_nondev_subnodes(&dn->data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

...

> -	if (!adev->data.pointer) {
> +	if (!adev->data.pointer ||

> +	    acpi_tie_nondev_subnodes(&adev->data) < 0) {
> +		acpi_untie_nondev_subnodes(&adev->data);

I don't know this part of the code, but this looks unusual. Shouldn't _tie()
take care of proper error path itself?

Also, it's a bit strange to call _untie() when _tie() wasn't called.

>  		acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n");
>  		ACPI_FREE(buf.pointer);
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Sakari Ailus]

Hi Andy,

On Wed, May 25, 2022 at 08:20:04PM +0300, Andy Shevchenko wrote:
> On Wed, May 25, 2022 at 04:01:17PM +0300, Sakari Ailus wrote:
> > ACPICA allows associating additional information (i.e. pointers with
> > specific tag) to acpi_handles. The acpi_device's are associated to
> > acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the
> > _DSD data nodes.
> > 
> > This allows direct data node references in properties, implemented later on
> > in the series.
> 
> ...
> 
> > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
> > +{
> > +	struct acpi_data_node *dn;
> > +
> > +	list_for_each_entry(dn, &data->subnodes, sibling) {
> > +		acpi_status status;
> > +		int ret;
> > +
> > +		status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> > +		if (ACPI_FAILURE(status)) {
> > +			acpi_handle_err(dn->handle, "Can't tag data node\n");
> > +			return 0;
> > +		}
> > +
> > +		ret = acpi_tie_nondev_subnodes(&dn->data);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > -	if (!adev->data.pointer) {
> > +	if (!adev->data.pointer ||
> 
> > +	    acpi_tie_nondev_subnodes(&adev->data) < 0) {
> > +		acpi_untie_nondev_subnodes(&adev->data);
> 
> I don't know this part of the code, but this looks unusual. Shouldn't _tie()
> take care of proper error path itself?

It could, but I'd need another function for recursive use. You're basically
asking to move these two lines into a new function called from here only.

> 
> Also, it's a bit strange to call _untie() when _tie() wasn't called.

How does this happen?

If you mean not keeping track which nodes have been tied and which have
not, it could be done. But there's no harm from detaching data that has not
been attached, it's a nop.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Rafael J. Wysocki]

On Wed, May 25, 2022 at 3:01 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> ACPICA allows associating additional information (i.e. pointers with
> specific tag) to acpi_handles. The acpi_device's are associated to
> acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the
> _DSD data nodes.
>
> This allows direct data node references in properties, implemented later on
> in the series.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index bc9a645f8bb77..f8c83ee6c8d59 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -340,6 +340,43 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
>         return props;
>  }
>
> +static void acpi_nondev_subnode_tag(acpi_handle handle, void *context)
> +{
> +}
> +
> +static void acpi_untie_nondev_subnodes(struct acpi_device_data *data)
> +{
> +       struct acpi_data_node *dn;
> +
> +       list_for_each_entry(dn, &data->subnodes, sibling) {
> +               acpi_detach_data(dn->handle, acpi_nondev_subnode_tag);
> +
> +               acpi_untie_nondev_subnodes(&dn->data);
> +       }
> +}
> +
> +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
> +{
> +       struct acpi_data_node *dn;
> +
> +       list_for_each_entry(dn, &data->subnodes, sibling) {
> +               acpi_status status;
> +               int ret;
> +
> +               status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> +               if (ACPI_FAILURE(status)) {
> +                       acpi_handle_err(dn->handle, "Can't tag data node\n");
> +                       return 0;
> +               }
> +
> +               ret = acpi_tie_nondev_subnodes(&dn->data);
> +               if (ret)
> +                       return ret;

Is it actually possible that this returns anything different from 0?

> +       }
> +
> +       return 0;
> +}
> +
>  static bool acpi_extract_properties(const union acpi_object *desc,
>                                     struct acpi_device_data *data)
>  {
> @@ -419,7 +456,9 @@ void acpi_init_properties(struct acpi_device *adev)
>                                         &adev->data, acpi_fwnode_handle(adev)))
>                 adev->data.pointer = buf.pointer;
>
> -       if (!adev->data.pointer) {
> +       if (!adev->data.pointer ||
> +           acpi_tie_nondev_subnodes(&adev->data) < 0) {
> +               acpi_untie_nondev_subnodes(&adev->data);
>                 acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n");
>                 ACPI_FREE(buf.pointer);
>         }
> @@ -462,6 +501,7 @@ static void acpi_destroy_nondev_subnodes(struct list_head *list)
>
>  void acpi_free_properties(struct acpi_device *adev)
>  {
> +       acpi_untie_nondev_subnodes(&adev->data);
>         acpi_destroy_nondev_subnodes(&adev->data.subnodes);
>         ACPI_FREE((void *)adev->data.pointer);
>         adev->data.of_compatible = NULL;
> --
> 2.30.2
>

Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Sakari Ailus]

Hi Rafael,

On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote:
> > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
> > +{
> > +       struct acpi_data_node *dn;
> > +
> > +       list_for_each_entry(dn, &data->subnodes, sibling) {
> > +               acpi_status status;
> > +               int ret;
> > +
> > +               status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> > +               if (ACPI_FAILURE(status)) {
> > +                       acpi_handle_err(dn->handle, "Can't tag data node\n");
> > +                       return 0;
> > +               }
> > +
> > +               ret = acpi_tie_nondev_subnodes(&dn->data);
> > +               if (ret)
> > +                       return ret;
> 
> Is it actually possible that this returns anything different from 0?

acpi_attach_data() involves allocating memory and resolving a reference.
Both can fail.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Rafael J. Wysocki]

On Fri, May 27, 2022 at 11:02 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote:
> > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
> > > +{
> > > +       struct acpi_data_node *dn;
> > > +
> > > +       list_for_each_entry(dn, &data->subnodes, sibling) {
> > > +               acpi_status status;
> > > +               int ret;
> > > +
> > > +               status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> > > +               if (ACPI_FAILURE(status)) {
> > > +                       acpi_handle_err(dn->handle, "Can't tag data node\n");
> > > +                       return 0;
> > > +               }
> > > +
> > > +               ret = acpi_tie_nondev_subnodes(&dn->data);
> > > +               if (ret)
> > > +                       return ret;
> >
> > Is it actually possible that this returns anything different from 0?
>
> acpi_attach_data() involves allocating memory and resolving a reference.
> Both can fail.

Yes, they can, but the value returned by acpi_attach_data() is
effectively ignored above (except for printing the error message,
which BTW could be "info" and provide more information).

I don't see how acpi_tie_nondev_subnodes() can produce a nonzero return value.

Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Sakari Ailus]

Hi Rafael,

On Fri, May 27, 2022 at 07:04:39PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 27, 2022 at 11:02 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote:
> > > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
> > > > +{
> > > > +       struct acpi_data_node *dn;
> > > > +
> > > > +       list_for_each_entry(dn, &data->subnodes, sibling) {
> > > > +               acpi_status status;
> > > > +               int ret;
> > > > +
> > > > +               status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> > > > +               if (ACPI_FAILURE(status)) {
> > > > +                       acpi_handle_err(dn->handle, "Can't tag data node\n");
> > > > +                       return 0;
> > > > +               }
> > > > +
> > > > +               ret = acpi_tie_nondev_subnodes(&dn->data);
> > > > +               if (ret)
> > > > +                       return ret;
> > >
> > > Is it actually possible that this returns anything different from 0?
> >
> > acpi_attach_data() involves allocating memory and resolving a reference.
> > Both can fail.
> 
> Yes, they can, but the value returned by acpi_attach_data() is
> effectively ignored above (except for printing the error message,
> which BTW could be "info" and provide more information).

Oops. Good point.

I intended this to return an error here. I don't have strong opinion on
which way to go though. How about changing that to -ENOMEM?

I think this is basically a decision on whether any subnodes could be
referenced if ore or more of them could not. I don't expect this to happen
in practice.

> 
> I don't see how acpi_tie_nondev_subnodes() can produce a nonzero return value.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

[Rafael J. Wysocki]

On Fri, May 27, 2022 at 10:59 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Fri, May 27, 2022 at 07:04:39PM +0200, Rafael J. Wysocki wrote:
> > On Fri, May 27, 2022 at 11:02 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote:
> > > > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
> > > > > +{
> > > > > +       struct acpi_data_node *dn;
> > > > > +
> > > > > +       list_for_each_entry(dn, &data->subnodes, sibling) {
> > > > > +               acpi_status status;
> > > > > +               int ret;
> > > > > +
> > > > > +               status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn);
> > > > > +               if (ACPI_FAILURE(status)) {
> > > > > +                       acpi_handle_err(dn->handle, "Can't tag data node\n");
> > > > > +                       return 0;
> > > > > +               }
> > > > > +
> > > > > +               ret = acpi_tie_nondev_subnodes(&dn->data);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > >
> > > > Is it actually possible that this returns anything different from 0?
> > >
> > > acpi_attach_data() involves allocating memory and resolving a reference.
> > > Both can fail.
> >
> > Yes, they can, but the value returned by acpi_attach_data() is
> > effectively ignored above (except for printing the error message,
> > which BTW could be "info" and provide more information).
>
> Oops. Good point.
>
> I intended this to return an error here. I don't have strong opinion on
> which way to go though. How about changing that to -ENOMEM?

It might as well return bool and let the caller worry about the error handling.

> I think this is basically a decision on whether any subnodes could be
> referenced if ore or more of them could not. I don't expect this to happen
> in practice.

So is having a partial description of something useful?  I guess it
may or may not be, depending on the use case.

If there's any use case in which it may be useful, I would ignore the
attach errors and address missing stuff elsewhere.

[PATCH v3 3/8] ACPI: property: Use acpi_object_type consistently in property ref parsing

[Sakari Ailus]

The type of union acpi_object field type is acpi_object_type. Use that
instead of int.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f8c83ee6c8d59..b36cb7e36e420 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -793,7 +793,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 			 * nor integer, return an error, we can't parse it.
 			 */
 			for (i = 0; element + i < end && i < num_args; i++) {
-				int type = element[i].type;
+				acpi_object_type type = element[i].type;
 
 				if (type == ACPI_TYPE_LOCAL_REFERENCE)
 					break;
-- 
2.30.2

Re: [PATCH v3 3/8] ACPI: property: Use acpi_object_type consistently in property ref parsing

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:18PM +0300, Sakari Ailus wrote:
> The type of union acpi_object field type is acpi_object_type. Use that
> instead of int.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index f8c83ee6c8d59..b36cb7e36e420 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -793,7 +793,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>  			 * nor integer, return an error, we can't parse it.
>  			 */
>  			for (i = 0; element + i < end && i < num_args; i++) {
> -				int type = element[i].type;
> +				acpi_object_type type = element[i].type;
>  
>  				if (type == ACPI_TYPE_LOCAL_REFERENCE)
>  					break;
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/8] ACPI: property: Move property ref argument parsing into a new function

[Sakari Ailus]

Split out property reference argument parsing out of the
__acpi_node_get_property_reference() function into a new one,
acpi_get_ref_args(). The new function will be needed also for parsing
string references soon.

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

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index b36cb7e36e420..dd6cce955ee28 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -673,6 +673,60 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 	return NULL;
 }
 
+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)
+{
+	u32 nargs = 0, i;
+
+	/*
+	 * 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;
+	}
+
+	/*
+	 * Assume the following integer elements are all args. Stop counting on
+	 * the first reference or end of the package arguments. In case of
+	 * neither reference, nor integer, return an error, we can't parse it.
+	 */
+	for (i = 0; (*element) + i < end && i < num_args; i++) {
+		acpi_object_type type = (*element)[i].type;
+
+		if (type == ACPI_TYPE_LOCAL_REFERENCE)
+			break;
+
+		if (type == ACPI_TYPE_INTEGER)
+			nargs++;
+		else
+			return -EINVAL;
+	}
+
+	if (nargs > NR_FWNODE_REFERENCE_ARGS)
+		return -EINVAL;
+
+	if (args) {
+		args->fwnode = ref_fwnode;
+		args->nargs = nargs;
+		for (i = 0; i < nargs; i++)
+			args->args[i] = (*element)[i].integer.value;
+	}
+
+	(*element) += nargs;
+
+	return 0;
+}
+
 /**
  * __acpi_node_get_property_reference - returns handle to the referenced object
  * @fwnode: Firmware node to get the property from
@@ -761,61 +815,22 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 	end = element + obj->package.count;
 
 	while (element < end) {
-		u32 nargs, i;
-
 		if (element->type == ACPI_TYPE_LOCAL_REFERENCE) {
-			struct fwnode_handle *ref_fwnode;
-
 			device = acpi_fetch_acpi_dev(element->reference.handle);
 			if (!device)
 				return -EINVAL;
 
-			nargs = 0;
 			element++;
 
-			/*
-			 * Find the referred data extension node under the
-			 * referred device node.
-			 */
-			for (ref_fwnode = acpi_fwnode_handle(device);
-			     element < end && element->type == ACPI_TYPE_STRING;
-			     element++) {
-				ref_fwnode = acpi_fwnode_get_named_child_node(
-					ref_fwnode, element->string.pointer);
-				if (!ref_fwnode)
-					return -EINVAL;
-			}
-
-			/*
-			 * Assume the following integer elements are all args.
-			 * Stop counting on the first reference or end of the
-			 * package arguments. In case of neither reference,
-			 * nor integer, return an error, we can't parse it.
-			 */
-			for (i = 0; element + i < end && i < num_args; i++) {
-				acpi_object_type type = element[i].type;
-
-				if (type == ACPI_TYPE_LOCAL_REFERENCE)
-					break;
-				if (type == ACPI_TYPE_INTEGER)
-					nargs++;
-				else
-					return -EINVAL;
-			}
-
-			if (nargs > NR_FWNODE_REFERENCE_ARGS)
-				return -EINVAL;
-
-			if (idx == index) {
-				args->fwnode = ref_fwnode;
-				args->nargs = nargs;
-				for (i = 0; i < nargs; i++)
-					args->args[i] = element[i].integer.value;
+			ret = acpi_get_ref_args(idx == index ? args : NULL,
+						acpi_fwnode_handle(device),
+						&element, end, num_args);
+			if (ret < 0)
+				return ret;
 
+			if (idx == index)
 				return 0;
-			}
 
-			element += nargs;
 		} else if (element->type == ACPI_TYPE_INTEGER) {
 			if (idx == index)
 				return -ENOENT;
-- 
2.30.2

Re: [PATCH v3 4/8] ACPI: property: Move property ref argument parsing into a new function

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:19PM +0300, Sakari Ailus wrote:
> Split out property reference argument parsing out of the
> __acpi_node_get_property_reference() function into a new one,
> acpi_get_ref_args(). The new function will be needed also for parsing
> string references soon.

...

> +static int
> +acpi_get_ref_args(struct fwnode_reference_args *args,

You can at least make these two on one line.

> +		  struct fwnode_handle *ref_fwnode,

Calling it fwnode would save a few lines of code even with your strictness
of 80.

> +		  const union acpi_object **element,
> +		  const union acpi_object *end, size_t num_args)
> +{
> +	u32 nargs = 0, i;
> +
> +	/*
> +	 * 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;

I believe this on one line is better to read.

> +		ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
> +							      child_name);

This too.

> +		if (!ref_fwnode)
> +			return -EINVAL;
> +	}
> +
> +	/*
> +	 * Assume the following integer elements are all args. Stop counting on
> +	 * the first reference or end of the package arguments. In case of
> +	 * neither reference, nor integer, return an error, we can't parse it.
> +	 */
> +	for (i = 0; (*element) + i < end && i < num_args; i++) {
> +		acpi_object_type type = (*element)[i].type;
> +
> +		if (type == ACPI_TYPE_LOCAL_REFERENCE)
> +			break;
> +
> +		if (type == ACPI_TYPE_INTEGER)
> +			nargs++;
> +		else
> +			return -EINVAL;
> +	}
> +
> +	if (nargs > NR_FWNODE_REFERENCE_ARGS)
> +		return -EINVAL;
> +
> +	if (args) {
> +		args->fwnode = ref_fwnode;
> +		args->nargs = nargs;
> +		for (i = 0; i < nargs; i++)
> +			args->args[i] = (*element)[i].integer.value;
> +	}
> +
> +	(*element) += nargs;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/8] ACPI: property: Move property ref argument parsing into a new function

[Sakari Ailus]

Hi Andy,

On Wed, May 25, 2022 at 08:28:57PM +0300, Andy Shevchenko wrote:
> On Wed, May 25, 2022 at 04:01:19PM +0300, Sakari Ailus wrote:
> > Split out property reference argument parsing out of the
> > __acpi_node_get_property_reference() function into a new one,
> > acpi_get_ref_args(). The new function will be needed also for parsing
> > string references soon.
> 
> ...
> 
> > +static int
> > +acpi_get_ref_args(struct fwnode_reference_args *args,
> 
> You can at least make these two on one line.

Agreed.

> 
> > +		  struct fwnode_handle *ref_fwnode,
> 
> Calling it fwnode would save a few lines of code even with your strictness
> of 80.

I'm just moving the code as much as possible as-is from elsewhere, thus
preferring to keep the naming.

> 
> > +		  const union acpi_object **element,
> > +		  const union acpi_object *end, size_t num_args)
> > +{
> > +	u32 nargs = 0, i;
> > +
> > +	/*
> > +	 * 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;
> 
> I believe this on one line is better to read.

I assume you mean the for loop.

I have to disargee. Everything doesn't automatically become better by
putting more stuff on the same line.

> 
> > +		ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
> > +							      child_name);
> 
> This too.

I think this would be affected less than the loop. 

-- 
Kind regards,

Sakari Ailus

[PATCH v3 5/8] ACPI: property: Switch node property referencing from ifs to a switch

[Sakari Ailus]

__acpi_node_get_property_reference() uses a series of if () statements for
testing the same variable. There's soon going to be one more value to be
tested.

Switch to use switch() instead.

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

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index dd6cce955ee28..a8e8a214a524f 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -780,11 +780,9 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 	if (ret)
 		return ret == -EINVAL ? -ENOENT : -EINVAL;
 
-	/*
-	 * The simplest case is when the value is a single reference.  Just
-	 * return that reference then.
-	 */
-	if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) {
+	switch (obj->type) {
+	case ACPI_TYPE_LOCAL_REFERENCE:
+		/* Plain single reference without arguments. */
 		if (index)
 			return -ENOENT;
 
@@ -795,19 +793,21 @@ 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_PACKAGE:
+		/*
+		 * If it is not a single reference, then it is a package of
+		 * 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).
+		 */
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	/*
-	 * If it is not a single reference, then it is a package of
-	 * 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).
-	 */
-	if (obj->type != ACPI_TYPE_PACKAGE)
-		return -EINVAL;
 	if (index >= obj->package.count)
 		return -ENOENT;
 
@@ -815,7 +815,8 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 	end = element + obj->package.count;
 
 	while (element < end) {
-		if (element->type == ACPI_TYPE_LOCAL_REFERENCE) {
+		switch (element->type) {
+		case ACPI_TYPE_LOCAL_REFERENCE:
 			device = acpi_fetch_acpi_dev(element->reference.handle);
 			if (!device)
 				return -EINVAL;
@@ -831,11 +832,13 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 			if (idx == index)
 				return 0;
 
-		} else if (element->type == ACPI_TYPE_INTEGER) {
+			break;
+		case ACPI_TYPE_INTEGER:
 			if (idx == index)
 				return -ENOENT;
 			element++;
-		} else {
+			break;
+		default:
 			return -EINVAL;
 		}
 
-- 
2.30.2

Re: [PATCH v3 5/8] ACPI: property: Switch node property referencing from ifs to a switch

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:20PM +0300, Sakari Ailus wrote:
> __acpi_node_get_property_reference() uses a series of if () statements for
> testing the same variable. There's soon going to be one more value to be
> tested.
> 
> Switch to use switch() instead.

Looks like sanitization over the different parts of the code in this file.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/property.c | 41 ++++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index dd6cce955ee28..a8e8a214a524f 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -780,11 +780,9 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>  	if (ret)
>  		return ret == -EINVAL ? -ENOENT : -EINVAL;
>  
> -	/*
> -	 * The simplest case is when the value is a single reference.  Just
> -	 * return that reference then.
> -	 */
> -	if (obj->type == ACPI_TYPE_LOCAL_REFERENCE) {
> +	switch (obj->type) {
> +	case ACPI_TYPE_LOCAL_REFERENCE:
> +		/* Plain single reference without arguments. */
>  		if (index)
>  			return -ENOENT;
>  
> @@ -795,19 +793,21 @@ 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_PACKAGE:
> +		/*
> +		 * If it is not a single reference, then it is a package of
> +		 * 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).
> +		 */
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> -	/*
> -	 * If it is not a single reference, then it is a package of
> -	 * 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).
> -	 */
> -	if (obj->type != ACPI_TYPE_PACKAGE)
> -		return -EINVAL;
>  	if (index >= obj->package.count)
>  		return -ENOENT;
>  
> @@ -815,7 +815,8 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>  	end = element + obj->package.count;
>  
>  	while (element < end) {
> -		if (element->type == ACPI_TYPE_LOCAL_REFERENCE) {
> +		switch (element->type) {
> +		case ACPI_TYPE_LOCAL_REFERENCE:
>  			device = acpi_fetch_acpi_dev(element->reference.handle);
>  			if (!device)
>  				return -EINVAL;
> @@ -831,11 +832,13 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
>  			if (idx == index)
>  				return 0;
>  
> -		} else if (element->type == ACPI_TYPE_INTEGER) {
> +			break;
> +		case ACPI_TYPE_INTEGER:
>  			if (idx == index)
>  				return -ENOENT;
>  			element++;
> -		} else {
> +			break;
> +		default:
>  			return -EINVAL;
>  		}
>  
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 6/8] ACPI: property: Unify integer value reading functions

[Sakari Ailus]

Unify functions reading ACPI property integer values into a single macro
using C99 _Generic().

Also use size_t for the counter instead of int.

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

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index a8e8a214a524f..5d9805a6ff12d 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -910,67 +910,30 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
 	return ret;
 }
 
-static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
-				       size_t nval)
-{
-	int i;
-
-	for (i = 0; i < nval; i++) {
-		if (items[i].type != ACPI_TYPE_INTEGER)
-			return -EPROTO;
-		if (items[i].integer.value > U8_MAX)
-			return -EOVERFLOW;
-
-		val[i] = items[i].integer.value;
-	}
-	return 0;
-}
-
-static int acpi_copy_property_array_u16(const union acpi_object *items,
-					u16 *val, size_t nval)
-{
-	int i;
-
-	for (i = 0; i < nval; i++) {
-		if (items[i].type != ACPI_TYPE_INTEGER)
-			return -EPROTO;
-		if (items[i].integer.value > U16_MAX)
-			return -EOVERFLOW;
-
-		val[i] = items[i].integer.value;
-	}
-	return 0;
-}
-
-static int acpi_copy_property_array_u32(const union acpi_object *items,
-					u32 *val, size_t nval)
-{
-	int i;
-
-	for (i = 0; i < nval; i++) {
-		if (items[i].type != ACPI_TYPE_INTEGER)
-			return -EPROTO;
-		if (items[i].integer.value > U32_MAX)
-			return -EOVERFLOW;
-
-		val[i] = items[i].integer.value;
-	}
-	return 0;
-}
-
-static int acpi_copy_property_array_u64(const union acpi_object *items,
-					u64 *val, size_t nval)
-{
-	int i;
-
-	for (i = 0; i < nval; i++) {
-		if (items[i].type != ACPI_TYPE_INTEGER)
-			return -EPROTO;
-
-		val[i] = items[i].integer.value;
-	}
-	return 0;
-}
+#define acpi_copy_property_array_uint(items, val, nval)			\
+	({								\
+		size_t i;						\
+		int ret = 0;						\
+									\
+		for (i = 0; i < (nval); i++) {				\
+			if ((items)[i].type != ACPI_TYPE_INTEGER) {	\
+				ret = -EPROTO;				\
+				break;					\
+			}						\
+			if ((items)[i].integer.value > _Generic((val),	\
+								u8: U8_MAX, \
+								u16: U16_MAX, \
+								u32: U32_MAX, \
+								u64: U64_MAX, \
+								default: 0U)) { \
+				ret = -EOVERFLOW;			\
+				break;					\
+			}						\
+									\
+			(val)[i] = (items)[i].integer.value;		\
+		}							\
+		ret;							\
+	})
 
 static int acpi_copy_property_array_string(const union acpi_object *items,
 					   char **val, size_t nval)
@@ -1027,16 +990,16 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
 
 	switch (proptype) {
 	case DEV_PROP_U8:
-		ret = acpi_copy_property_array_u8(items, (u8 *)val, nval);
+		ret = acpi_copy_property_array_uint(items, (u8 *)val, nval);
 		break;
 	case DEV_PROP_U16:
-		ret = acpi_copy_property_array_u16(items, (u16 *)val, nval);
+		ret = acpi_copy_property_array_uint(items, (u16 *)val, nval);
 		break;
 	case DEV_PROP_U32:
-		ret = acpi_copy_property_array_u32(items, (u32 *)val, nval);
+		ret = acpi_copy_property_array_uint(items, (u32 *)val, nval);
 		break;
 	case DEV_PROP_U64:
-		ret = acpi_copy_property_array_u64(items, (u64 *)val, nval);
+		ret = acpi_copy_property_array_uint(items, (u64 *)val, nval);
 		break;
 	case DEV_PROP_STRING:
 		ret = acpi_copy_property_array_string(
-- 
2.30.2

Re: [PATCH v3 6/8] ACPI: property: Unify integer value reading functions

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:21PM +0300, Sakari Ailus wrote:
> Unify functions reading ACPI property integer values into a single macro
> using C99 _Generic().
> 
> Also use size_t for the counter instead of int.

Thanks for an update!

...

> +#define acpi_copy_property_array_uint(items, val, nval)			\
> +	({								\

You can define local copies of (read-only) parameters and avoid adding
parentheses each time you access them.

> +		size_t i;						\
> +		int ret = 0;						\
> +									\
> +		for (i = 0; i < (nval); i++) {				\
> +			if ((items)[i].type != ACPI_TYPE_INTEGER) {	\
> +				ret = -EPROTO;				\
> +				break;					\
> +			}						\
> +			if ((items)[i].integer.value > _Generic((val),	\
> +								u8: U8_MAX, \
> +								u16: U16_MAX, \
> +								u32: U32_MAX, \
> +								u64: U64_MAX, \
> +								default: 0U)) { \

I think nobody will die if you add one more TAB to each line and make \ be
consistent column wise.

> +				ret = -EOVERFLOW;			\
> +				break;					\
> +			}						\
> +									\
> +			(val)[i] = (items)[i].integer.value;		\
> +		}							\
> +		ret;							\
> +	})

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/8] ACPI: property: Unify integer value reading functions

[Sakari Ailus]

Hi Andy,

On Wed, May 25, 2022 at 08:14:36PM +0300, Andy Shevchenko wrote:
> On Wed, May 25, 2022 at 04:01:21PM +0300, Sakari Ailus wrote:
> > Unify functions reading ACPI property integer values into a single macro
> > using C99 _Generic().
> > 
> > Also use size_t for the counter instead of int.
> 
> Thanks for an update!
> 
> ...
> 
> > +#define acpi_copy_property_array_uint(items, val, nval)			\
> > +	({								\
> 
> You can define local copies of (read-only) parameters and avoid adding
> parentheses each time you access them.

Sounds good.

> 
> > +		size_t i;						\
> > +		int ret = 0;						\
> > +									\
> > +		for (i = 0; i < (nval); i++) {				\
> > +			if ((items)[i].type != ACPI_TYPE_INTEGER) {	\
> > +				ret = -EPROTO;				\
> > +				break;					\
> > +			}						\
> > +			if ((items)[i].integer.value > _Generic((val),	\
> > +								u8: U8_MAX, \
> > +								u16: U16_MAX, \
> > +								u32: U32_MAX, \
> > +								u64: U64_MAX, \
> > +								default: 0U)) { \
> 
> I think nobody will die if you add one more TAB to each line and make \ be
> consistent column wise.

I think it's unlikely, too. Still the above is consistent with the rest of
recently merged patches adding similar constructs.

-- 
Kind regards,

Sakari Ailus

[PATCH v3 7/8] ACPI: property: Add support for parsing buffer property UUID

[Sakari Ailus]

Add support for newly added buffer property UUID, as defined in the DSD
guide.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 141 ++++++++++++++++++++++++++++++++++++----
 include/acpi/acpi_bus.h |   3 +-
 include/linux/acpi.h    |   2 +-
 3 files changed, 131 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 5d9805a6ff12d..b07a8c9fee876 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -55,14 +55,19 @@ static const guid_t ads_guid =
 	GUID_INIT(0xdbb8e3e6, 0x5886, 0x4ba6,
 		  0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
 
+static const guid_t buffer_prop_guid =
+	GUID_INIT(0xedb12dd0, 0x363d, 0x4085,
+		  0xa3, 0xd2, 0x49, 0x52, 0x2c, 0xa1, 0x60, 0xc4);
+
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
-					   const union acpi_object *desc,
+					   union acpi_object *desc,
 					   struct acpi_device_data *data,
 					   struct fwnode_handle *parent);
-static bool acpi_extract_properties(const union acpi_object *desc,
+static bool acpi_extract_properties(acpi_handle handle,
+				    union acpi_object *desc,
 				    struct acpi_device_data *data);
 
-static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
+static bool acpi_nondev_subnode_extract(union acpi_object *desc,
 					acpi_handle handle,
 					const union acpi_object *link,
 					struct list_head *list,
@@ -81,7 +86,7 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
 	INIT_LIST_HEAD(&dn->data.properties);
 	INIT_LIST_HEAD(&dn->data.subnodes);
 
-	result = acpi_extract_properties(desc, &dn->data);
+	result = acpi_extract_properties(handle, desc, &dn->data);
 
 	if (handle) {
 		acpi_handle scope;
@@ -156,7 +161,7 @@ static bool acpi_nondev_subnode_ok(acpi_handle scope,
 }
 
 static bool acpi_add_nondev_subnodes(acpi_handle scope,
-				     const union acpi_object *links,
+				     union acpi_object *links,
 				     struct list_head *list,
 				     struct fwnode_handle *parent)
 {
@@ -164,7 +169,7 @@ static bool acpi_add_nondev_subnodes(acpi_handle scope,
 	int i;
 
 	for (i = 0; i < links->package.count; i++) {
-		const union acpi_object *link, *desc;
+		union acpi_object *link, *desc;
 		acpi_handle handle;
 		bool result;
 
@@ -204,7 +209,7 @@ static bool acpi_add_nondev_subnodes(acpi_handle scope,
 }
 
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
-					   const union acpi_object *desc,
+					   union acpi_object *desc,
 					   struct acpi_device_data *data,
 					   struct fwnode_handle *parent)
 {
@@ -212,7 +217,8 @@ static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 
 	/* Look for the ACPI data subnodes GUID. */
 	for (i = 0; i < desc->package.count; i += 2) {
-		const union acpi_object *guid, *links;
+		const union acpi_object *guid;
+		union acpi_object *links;
 
 		guid = &desc->package.elements[i];
 		links = &desc->package.elements[i + 1];
@@ -325,7 +331,7 @@ static bool acpi_is_property_guid(const guid_t *guid)
 
 struct acpi_device_properties *
 acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
-		    const union acpi_object *properties)
+		    union acpi_object *properties)
 {
 	struct acpi_device_properties *props;
 
@@ -377,7 +383,103 @@ static int acpi_tie_nondev_subnodes(struct acpi_device_data *data)
 	return 0;
 }
 
-static bool acpi_extract_properties(const union acpi_object *desc,
+static void acpi_data_add_buffer_props(acpi_handle handle,
+				       struct acpi_device_data *data,
+				       union acpi_object *properties)
+{
+	struct acpi_device_properties *props;
+	union acpi_object *package;
+	size_t alloc_size;
+	unsigned int i;
+	u32 *count;
+
+	if (check_mul_overflow((size_t)properties->package.count,
+			       sizeof(*package) + sizeof(void *),
+			       &alloc_size) ||
+	    check_add_overflow(sizeof(*props) + sizeof(*package), alloc_size,
+			       &alloc_size)) {
+		acpi_handle_warn(handle,
+				 "can't allocate memory for %u buffer props",
+				 properties->package.count);
+		return;
+	}
+
+	props = kvzalloc(alloc_size, GFP_KERNEL);
+	if (!props)
+		return;
+
+	props->guid = &buffer_prop_guid;
+	props->bufs = (void *)(props + 1);
+	props->properties = (void *)(props->bufs + properties->package.count);
+
+	/* Outer package */
+	package = props->properties;
+	package->type = ACPI_TYPE_PACKAGE;
+	package->package.elements = package + 1;
+	count = &package->package.count;
+	*count = 0;
+
+	/* Inner packages */
+	package++;
+
+	for (i = 0; i < properties->package.count; i++) {
+		struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+		union acpi_object *property = &properties->package.elements[i];
+		union acpi_object *prop, *obj, *buf_obj;
+		acpi_status status;
+
+		if (property->type != ACPI_TYPE_PACKAGE ||
+		    property->package.count != 2) {
+			acpi_handle_warn(handle,
+					 "buffer property %u has %u entries\n",
+					 i, property->package.count);
+			continue;
+		}
+
+		prop = &property->package.elements[0];
+		obj = &property->package.elements[1];
+
+		if (prop->type != ACPI_TYPE_STRING ||
+		    obj->type != ACPI_TYPE_STRING) {
+			acpi_handle_warn(handle,
+					 "wrong object types %u and %u\n",
+					 prop->type, obj->type);
+			continue;
+		}
+
+		status = acpi_evaluate_object_typed(handle, obj->string.pointer,
+						    NULL, &buf,
+						    ACPI_TYPE_BUFFER);
+		if (ACPI_FAILURE(status)) {
+			acpi_handle_warn(handle,
+					 "can't evaluate \"%s\" as buffer\n",
+					 obj->string.pointer);
+			continue;
+		}
+
+		package->type = ACPI_TYPE_PACKAGE;
+		package->package.elements = prop;
+		package->package.count = 2;
+
+		buf_obj = buf.pointer;
+
+		/* Replace the string object with a buffer object */
+		obj->type = ACPI_TYPE_BUFFER;
+		obj->buffer.length = buf_obj->buffer.length;
+		obj->buffer.pointer = buf_obj->buffer.pointer;
+
+		props->bufs[i] = buf.pointer;
+		package++;
+		(*count)++;
+	}
+
+	if (*count)
+		list_add(&props->list, &data->properties);
+	else
+		kvfree(props);
+}
+
+static bool acpi_extract_properties(acpi_handle scope, union acpi_object *desc,
 				    struct acpi_device_data *data)
 {
 	int i;
@@ -387,7 +489,8 @@ static bool acpi_extract_properties(const union acpi_object *desc,
 
 	/* Look for the device properties GUID. */
 	for (i = 0; i < desc->package.count; i += 2) {
-		const union acpi_object *guid, *properties;
+		const union acpi_object *guid;
+		union acpi_object *properties;
 
 		guid = &desc->package.elements[i];
 		properties = &desc->package.elements[i + 1];
@@ -401,6 +504,12 @@ static bool acpi_extract_properties(const union acpi_object *desc,
 		    properties->type != ACPI_TYPE_PACKAGE)
 			break;
 
+		if (guid_equal((guid_t *)guid->buffer.pointer,
+			       &buffer_prop_guid)) {
+			acpi_data_add_buffer_props(scope, data, properties);
+			continue;
+		}
+
 		if (!acpi_is_property_guid((guid_t *)guid->buffer.pointer))
 			continue;
 
@@ -447,7 +556,7 @@ void acpi_init_properties(struct acpi_device *adev)
 	if (ACPI_FAILURE(status))
 		goto out;
 
-	if (acpi_extract_properties(buf.pointer, &adev->data)) {
+	if (acpi_extract_properties(adev->handle, buf.pointer, &adev->data)) {
 		adev->data.pointer = buf.pointer;
 		if (acpi_of)
 			acpi_init_of_compatible(adev);
@@ -477,8 +586,14 @@ static void acpi_free_device_properties(struct list_head *list)
 	struct acpi_device_properties *props, *tmp;
 
 	list_for_each_entry_safe(props, tmp, list, list) {
+		u32 i;
+
 		list_del(&props->list);
-		kfree(props);
+		/* Buffer data properties were separately allocated */
+		if (props->bufs)
+			for (i = 0; i < props->properties->package.count; i++)
+				ACPI_FREE(props->bufs[i]);
+		kvfree(props);
 	}
 }
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 772590e2eddb8..d5c60537982d8 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -344,8 +344,9 @@ struct acpi_device_physical_node {
 
 struct acpi_device_properties {
 	const guid_t *guid;
-	const union acpi_object *properties;
+	union acpi_object *properties;
 	struct list_head list;
+	void **bufs;
 };
 
 /* ACPI Device Specific Data (_DSD) */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index fadda404bcc9b..2213a8f046571 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1201,7 +1201,7 @@ static inline bool acpi_dev_has_props(const struct acpi_device *adev)
 
 struct acpi_device_properties *
 acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
-		    const union acpi_object *properties);
+		    union acpi_object *properties);
 
 int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
 		       void **valptr);
-- 
2.30.2

Re: [PATCH v3 7/8] ACPI: property: Add support for parsing buffer property UUID

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:22PM +0300, Sakari Ailus wrote:
> Add support for newly added buffer property UUID, as defined in the DSD
> guide.

...

> +	if (check_mul_overflow((size_t)properties->package.count,

Why do you need casting? Any issues on 32-bit compilation?

Looking at the below code snippets, it seems you also can have a local
copy with needed type and use it everywhere (as outer_package_count or so).
But first question first...

> +			       sizeof(*package) + sizeof(void *),
> +			       &alloc_size) ||
> +	    check_add_overflow(sizeof(*props) + sizeof(*package), alloc_size,
> +			       &alloc_size)) {
> +		acpi_handle_warn(handle,
> +				 "can't allocate memory for %u buffer props",
> +				 properties->package.count);
> +		return;
> +	}

...

> +		if (ACPI_FAILURE(status)) {
> +			acpi_handle_warn(handle,
> +					 "can't evaluate \"%s\" as buffer\n",
> +					 obj->string.pointer);

I'm wondering if better to use %*pE here to show the full data of the buffer.

> +			continue;
> +		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 7/8] ACPI: property: Add support for parsing buffer property UUID

[Sakari Ailus]

Hi Andy,

On Wed, May 25, 2022 at 08:44:11PM +0300, Andy Shevchenko wrote:
> On Wed, May 25, 2022 at 04:01:22PM +0300, Sakari Ailus wrote:
> > Add support for newly added buffer property UUID, as defined in the DSD
> > guide.
> 
> ...
> 
> > +	if (check_mul_overflow((size_t)properties->package.count,
> 
> Why do you need casting? Any issues on 32-bit compilation?

I think it can be removed. I'll see.

> 
> Looking at the below code snippets, it seems you also can have a local
> copy with needed type and use it everywhere (as outer_package_count or so).
> But first question first...

u32 should be fine.

> 
> > +			       sizeof(*package) + sizeof(void *),
> > +			       &alloc_size) ||
> > +	    check_add_overflow(sizeof(*props) + sizeof(*package), alloc_size,
> > +			       &alloc_size)) {
> > +		acpi_handle_warn(handle,
> > +				 "can't allocate memory for %u buffer props",
> > +				 properties->package.count);
> > +		return;
> > +	}
> 
> ...
> 
> > +		if (ACPI_FAILURE(status)) {
> > +			acpi_handle_warn(handle,
> > +					 "can't evaluate \"%s\" as buffer\n",
> > +					 obj->string.pointer);
> 
> I'm wondering if better to use %*pE here to show the full data of the buffer.

The string is supposed to be printable. If it's not, something is seriously
wrong. There's no harm though to prepare for this possibility. It'll make
backslashes printing twice but that is hardly an issue in practice.

> 
> > +			continue;
> > +		}
> 

-- 
Sakari Ailus

[PATCH v3 8/8] ACPI: property: Read buffer properties as integers

[Sakari Ailus]

Instead of adding a new property type, read buffer properties as integers.
Even though the internal representation in ACPI is different, the data
type is the same (byte) than on 8-bit integers.

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

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index b07a8c9fee876..9ac5cb03e4914 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1031,6 +1031,10 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
 		int ret = 0;						\
 									\
 		for (i = 0; i < (nval); i++) {				\
+			if ((items)->type == ACPI_TYPE_BUFFER) {	\
+				(val)[i] = (items)->buffer.pointer[i];	\
+				continue;				\
+			}						\
 			if ((items)[i].type != ACPI_TYPE_INTEGER) {	\
 				ret = -EPROTO;				\
 				break;					\
@@ -1090,18 +1094,40 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
 	}
 
 	ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
+	if (ret && proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64)
+		ret = acpi_data_get_property(data, propname, ACPI_TYPE_BUFFER,
+					     &obj);
 	if (ret)
 		return ret;
 
-	if (!val)
+	if (!val) {
+		if (obj->type == ACPI_TYPE_BUFFER)
+			return obj->buffer.length;
+
 		return obj->package.count;
+	}
 
-	if (proptype != DEV_PROP_STRING && nval > obj->package.count)
-		return -EOVERFLOW;
+	switch (proptype) {
+	case DEV_PROP_STRING:
+		break;
+	case DEV_PROP_U8 ... DEV_PROP_U64:
+		if (obj->type == ACPI_TYPE_BUFFER) {
+			if (nval <= obj->buffer.length)
+				break;
+			return -EOVERFLOW;
+		}
+		fallthrough;
+	default:
+		if (nval > obj->package.count)
+			return -EOVERFLOW;
+	}
 	if (nval == 0)
 		return -EINVAL;
 
-	items = obj->package.elements;
+	if (obj->type != ACPI_TYPE_BUFFER)
+		items = obj->package.elements;
+	else
+		items = obj;
 
 	switch (proptype) {
 	case DEV_PROP_U8:
-- 
2.30.2

Re: [PATCH v3 8/8] ACPI: property: Read buffer properties as integers

[Andy Shevchenko]

On Wed, May 25, 2022 at 04:01:23PM +0300, Sakari Ailus wrote:
> Instead of adding a new property type, read buffer properties as integers.
> Even though the internal representation in ACPI is different, the data
> type is the same (byte) than on 8-bit integers.

...

> +	switch (proptype) {
> +	case DEV_PROP_STRING:
> +		break;
> +	case DEV_PROP_U8 ... DEV_PROP_U64:
> +		if (obj->type == ACPI_TYPE_BUFFER) {

> +			if (nval <= obj->buffer.length)
> +				break;
> +			return -EOVERFLOW;

Why not traditional pattern and be consistent with default case?

			if (nval > obj->buffer.length)
				return -EOVERFLOW;
			break;

> +		}
> +		fallthrough;
> +	default:
> +		if (nval > obj->package.count)
> +			return -EOVERFLOW;

I would add break statement here.

> +	}
>  	if (nval == 0)
>  		return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/8] ACPI: property: Read buffer properties as integers

[Sakari Ailus]

Hi Andy,

On Wed, May 25, 2022 at 08:36:54PM +0300, Andy Shevchenko wrote:
> On Wed, May 25, 2022 at 04:01:23PM +0300, Sakari Ailus wrote:
> > Instead of adding a new property type, read buffer properties as integers.
> > Even though the internal representation in ACPI is different, the data
> > type is the same (byte) than on 8-bit integers.
> 
> ...
> 
> > +	switch (proptype) {
> > +	case DEV_PROP_STRING:
> > +		break;
> > +	case DEV_PROP_U8 ... DEV_PROP_U64:
> > +		if (obj->type == ACPI_TYPE_BUFFER) {
> 
> > +			if (nval <= obj->buffer.length)
> > +				break;
> > +			return -EOVERFLOW;
> 
> Why not traditional pattern and be consistent with default case?
> 
> 			if (nval > obj->buffer.length)
> 				return -EOVERFLOW;
> 			break;

Agreed.

> 
> > +		}
> > +		fallthrough;
> > +	default:
> > +		if (nval > obj->package.count)
> > +			return -EOVERFLOW;
> 
> I would add break statement here.

Sounds good.

-- 
Sakari Ailus

Re: [PATCH v3 0/8] ACPI: Buffer property and reference as string support

[Rafael J. Wysocki]

On Wed, May 25, 2022 at 3:01 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hello everyone,
>
> This set adds support for _DSD buffer properties (specified by DSD Guide
> <URL:https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.md>) as well as
> support for references as strings. Reference property type was previously
> supported for device objects only, whereas string references enable
> referencing also _DSD sub-node objects --- also included in the set.
>
> The ACPICA patch has been submitted to upstream but not merged yet.
>
> This set currently prepares for data node string reference support and
> does not add it anymore.
>
> since v2:
>
> - Use C99 _Generic() in patch unifying reading integer arrays.
>
> since v1:
>
> - Drop the ACPICA, data node child list initialisation and data node
>   string reference patches.
>
> Sakari Ailus (8):
>   ACPI: property: Return type of acpi_add_nondev_subnodes() should be
>     bool
>   ACPI: property: Tie data nodes to acpi handles
>   ACPI: property: Use acpi_object_type consistently in property ref
>     parsing
>   ACPI: property: Move property ref argument parsing into a new function
>   ACPI: property: Switch node property referencing from ifs to a switch
>   ACPI: property: Unify integer value reading functions
>   ACPI: property: Add support for parsing buffer property UUID
>   ACPI: property: Read buffer properties as integers
>
>  drivers/acpi/property.c | 462 +++++++++++++++++++++++++++-------------
>  include/acpi/acpi_bus.h |   3 +-
>  include/linux/acpi.h    |   2 +-
>  3 files changed, 315 insertions(+), 152 deletions(-)
>
> --

FYI, I'm expecting a v4 of this series to be sent.