[PATCH v2 0/3] Fwnode property API fixes for OF, pset and ACPI

[PATCH v2 0/3] Fwnode property API fixes for OF, pset and ACPI

[Sakari Ailus]

Hi folks,

Here are three patches that fix a few issues in the fwnode property API
implementation. They were formerly a part of a larger patchset "Move
firmware specific code to firmware specific locations" here:

<URL:http://www.spinics.net/lists/linux-acpi/msg72174.html>

The v1 (previous version) of the set is also available here:

<URL:http://www.spinics.net/lists/linux-acpi/msg72327.html>

These patches apply cleanly on v4.11-rc1 and linux-next with or without
the ACPI graph support patchset I've sent a moment ago.

changes since v1.1:

- Move fixes to fwnode_property_read_string_array() return value fixes to
  respective implementations of those functions, rather than working
  around them in the fwnode property interface functions.

- fwnode_property_read_string_array() now may return fewer strings than
  requested also on ACPI in order to make behaviour of
  fwnode_property_read_string_array() fully consistent.

changes since v1:

- Make prop const. pset_prop_get() returns const struct property_entry *
  now in __fwnode_property_read_string_array().

- Make fwnode_property_read_string_array() behave as
  of_property_read_string_array() does in terms of the return value.

Sakari Ailus (3):
  device property: fwnode_property_read_string_array() may return
    -EILSEQ
  device property: Fix reading pset strings using array access functions
  device property: fwnode_property_read_string_array() returns nr of
    strings

 drivers/acpi/property.c | 22 +++++++++++++++++-----
 drivers/base/property.c | 46 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 49 insertions(+), 19 deletions(-)

-- 
Kind regards,
Sakari

[PATCH v2 1/3] device property: fwnode_property_read_string_array() may return -EILSEQ

[Sakari Ailus]

fwnode_property_read_string_array() may return -EILSEQ through
of_property_read_string_array(). Document this.

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

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c458c63..19a3dc5 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -589,7 +589,7 @@ static int __fwnode_property_read_string(struct fwnode_handle *fwnode,
  *         %0 if the property was found (success),
  *	   %-EINVAL if given arguments are not valid,
  *	   %-ENODATA if the property does not have a value,
- *	   %-EPROTO if the property is not an array of strings,
+ *	   %-EPROTO or %-EILSEQ if the property is not an array of strings,
  *	   %-EOVERFLOW if the size of the property is not as expected,
  *	   %-ENXIO if no suitable firmware interface is present.
  */
-- 
2.7.4

[PATCH v2 2/3] device property: Fix reading pset strings using array access functions

[Sakari Ailus]

The length field value of non-array string properties is the length of the
string itself. Non-array string properties thus require specific handling.
Fix this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/base/property.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 19a3dc5..686e985 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -146,14 +146,36 @@ static int pset_prop_read_string_array(struct property_set *pset,
 				       const char *propname,
 				       const char **strings, size_t nval)
 {
+	const struct property_entry *prop;
 	const void *pointer;
-	size_t length = nval * sizeof(*strings);
+	size_t array_len, length;
+
+	/* Find out the array length. */
+	prop = pset_prop_get(pset, propname);
+	if (!prop)
+		return -EINVAL;
+
+	if (!prop->is_array)
+		/* The array length for a non-array string property is 1. */
+		array_len = 1;
+	else
+		/* Find the length of an array. */
+		array_len = pset_prop_count_elems_of_size(pset, propname,
+							  sizeof(const char *));
+
+	/* Return how many there are if strings is NULL. */
+	if (!strings)
+		return array_len;
+
+	array_len = min(nval, array_len);
+	length = array_len * sizeof(*strings);
 
 	pointer = pset_prop_find(pset, propname, length);
 	if (IS_ERR(pointer))
 		return PTR_ERR(pointer);
 
 	memcpy(strings, pointer, length);
+
 	return 0;
 }
 
@@ -553,12 +575,8 @@ static int __fwnode_property_read_string_array(struct fwnode_handle *fwnode,
 		return acpi_node_prop_read(fwnode, propname, DEV_PROP_STRING,
 					   val, nval);
 	else if (is_pset_node(fwnode))
-		return val ?
-			pset_prop_read_string_array(to_pset_node(fwnode),
-						    propname, val, nval) :
-			pset_prop_count_elems_of_size(to_pset_node(fwnode),
-						      propname,
-						      sizeof(const char *));
+		return pset_prop_read_string_array(to_pset_node(fwnode),
+						   propname, val, nval);
 	return -ENXIO;
 }
 
-- 
2.7.4

Re: [PATCH v2 2/3] device property: Fix reading pset strings using array access functions

[Mika Westerberg]

On Tue, Mar 28, 2017 at 03:22:18PM +0300, Sakari Ailus wrote:
> The length field value of non-array string properties is the length of the
> string itself. Non-array string properties thus require specific handling.
> Fix this.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v2 3/3] device property: fwnode_property_read_string_array() returns nr of strings

[Sakari Ailus]

Functionally fwnode_property_read_string_array() should match
of_property_read_string_array() and work as a drop-in substitute for the
latter. of_property_read_string_array() returns the number of strings read
if the target string pointer array is non-NULL. Make
fwnode_property_read_string_array() do the same.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/acpi/property.c | 22 +++++++++++++++++-----
 drivers/base/property.c | 12 ++++++------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 3afddcd..06f8fda 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -699,6 +699,8 @@ static int acpi_data_prop_read_single(struct acpi_device_data *data,
 			return ret;
 
 		*(char **)val = obj->string.pointer;
+
+		return 1;
 	} else {
 		ret = -EINVAL;
 	}
@@ -708,7 +710,15 @@ static int acpi_data_prop_read_single(struct acpi_device_data *data,
 int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname,
 			      enum dev_prop_type proptype, void *val)
 {
-	return adev ? acpi_data_prop_read_single(&adev->data, propname, proptype, val) : -EINVAL;
+	int ret;
+
+	if (!adev)
+		return -EINVAL;
+
+	ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val);
+	if (ret < 0 || proptype != ACPI_TYPE_STRING)
+		return ret;
+	return 0;
 }
 
 static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
@@ -784,7 +794,7 @@ static int acpi_copy_property_array_string(const union acpi_object *items,
 
 		val[i] = items[i].string.pointer;
 	}
-	return 0;
+	return nval;
 }
 
 static int acpi_data_prop_read(struct acpi_device_data *data,
@@ -798,7 +808,7 @@ static int acpi_data_prop_read(struct acpi_device_data *data,
 
 	if (val && nval == 1) {
 		ret = acpi_data_prop_read_single(data, propname, proptype, val);
-		if (!ret)
+		if (ret >= 0)
 			return ret;
 	}
 
@@ -809,7 +819,7 @@ static int acpi_data_prop_read(struct acpi_device_data *data,
 	if (!val)
 		return obj->package.count;
 
-	if (nval > obj->package.count)
+	if (proptype != DEV_PROP_STRING && nval > obj->package.count)
 		return -EOVERFLOW;
 	else if (nval <= 0)
 		return -EINVAL;
@@ -830,7 +840,9 @@ static int acpi_data_prop_read(struct acpi_device_data *data,
 		ret = acpi_copy_property_array_u64(items, (u64 *)val, nval);
 		break;
 	case DEV_PROP_STRING:
-		ret = acpi_copy_property_array_string(items, (char **)val, nval);
+		ret = acpi_copy_property_array_string(
+			items, (char **)val,
+			min_t(u32, nval, obj->package.count));
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 686e985..b517485 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -176,7 +176,7 @@ static int pset_prop_read_string_array(struct property_set *pset,
 
 	memcpy(strings, pointer, length);
 
-	return 0;
+	return array_len;
 }
 
 static int pset_prop_read_string(struct property_set *pset,
@@ -362,8 +362,8 @@ EXPORT_SYMBOL_GPL(device_property_read_u64_array);
  * Function reads an array of string properties with @propname from the device
  * firmware description and stores them to @val if found.
  *
- * Return: number of values if @val was %NULL,
- *         %0 if the property was found (success),
+ * Return: number of values read on success if @val is non-NULL,
+ *	   number of values available on success if @val is NULL,
  *	   %-EINVAL if given arguments are not valid,
  *	   %-ENODATA if the property does not have a value,
  *	   %-EPROTO or %-EILSEQ if the property is not an array of strings,
@@ -603,8 +603,8 @@ static int __fwnode_property_read_string(struct fwnode_handle *fwnode,
  * Read an string list property @propname from the given firmware node and store
  * them to @val if found.
  *
- * Return: number of values if @val was %NULL,
- *         %0 if the property was found (success),
+ * Return: number of values read on success if @val is non-NULL,
+ *	   number of values available on success if @val is NULL,
  *	   %-EINVAL if given arguments are not valid,
  *	   %-ENODATA if the property does not have a value,
  *	   %-EPROTO or %-EILSEQ if the property is not an array of strings,
@@ -651,7 +651,7 @@ int fwnode_property_read_string(struct fwnode_handle *fwnode,
 	    !IS_ERR_OR_NULL(fwnode->secondary))
 		ret = __fwnode_property_read_string(fwnode->secondary,
 						    propname, val);
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 EXPORT_SYMBOL_GPL(fwnode_property_read_string);
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] device property: fwnode_property_read_string_array() returns nr of strings

[Mika Westerberg]

On Tue, Mar 28, 2017 at 03:22:19PM +0300, Sakari Ailus wrote:
> -		ret = acpi_copy_property_array_string(items, (char **)val, nval);
> +		ret = acpi_copy_property_array_string(
> +			items, (char **)val,
> +			min_t(u32, nval, obj->package.count));

I think this looks better if written like:

		ret = acpi_copy_property_array_string(items, (char **)val,
					min_t(u32, nval, obj->package.count));

Regardless of that,

Reviewed-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] device property: fwnode_property_read_string_array() returns nr of strings

[Sakari Ailus]

Hi Mika,

Thank you for the review!

On Tue, Mar 28, 2017 at 03:52:31PM +0300, Mika Westerberg wrote:
> On Tue, Mar 28, 2017 at 03:22:19PM +0300, Sakari Ailus wrote:
> > -		ret = acpi_copy_property_array_string(items, (char **)val, nval);
> > +		ret = acpi_copy_property_array_string(
> > +			items, (char **)val,
> > +			min_t(u32, nval, obj->package.count));
> 
> I think this looks better if written like:
> 
> 		ret = acpi_copy_property_array_string(items, (char **)val,
> 					min_t(u32, nval, obj->package.count));

I prefer the former although I have no strong opinion either way. Using an
indentation that is automatically intended by a text editor is also an
advantage.

I can change it if you insist. :-)

> 
> Regardless of that,
> 
> Reviewed-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Thanks!

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html