[PATCH 1/2] of: Make of_find_property_value_of_size take a length range

[PATCH 1/2] of: Make of_find_property_value_of_size take a length range

[Richard Fitzgerald]

In preparation for adding variable-length array reads, change
of_find_property_value_of_size so that it takes an optional
maximum length. If the maximum is passed as 0, the behaviour is
unchanged and it will return a property if it's >= the requested
minimum length. If maximum is non-zero it will only return a
property whose length is min <= l <= max.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 drivers/of/base.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3ce6953..b853737 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1146,16 +1146,18 @@ EXPORT_SYMBOL_GPL(of_property_count_elems_of_size);
  *
  * @np:		device node from which the property value is to be read.
  * @propname:	name of the property to be searched.
- * @len:	requested length of property value
+ * @min:	minimum allowed length of property value
+ * @max:	maximum allowed length of property value (0 means unlimited)
+ * @len:	if !=NULL, actual length is written to here
  *
  * Search for a property in a device node and valid the requested size.
  * Returns the property value on success, -EINVAL if the property does not
  *  exist, -ENODATA if property does not have a value, and -EOVERFLOW if the
- * property data isn't large enough.
+ * property data is too small or too large.
  *
  */
 static void *of_find_property_value_of_size(const struct device_node *np,
-			const char *propname, u32 len)
+			const char *propname, u32 min, u32 max, size_t *len)
 {
 	struct property *prop = of_find_property(np, propname, NULL);
 
@@ -1163,8 +1165,13 @@ static void *of_find_property_value_of_size(const struct device_node *np,
 		return ERR_PTR(-EINVAL);
 	if (!prop->value)
 		return ERR_PTR(-ENODATA);
-	if (len > prop->length)
+	if (prop->length < min)
 		return ERR_PTR(-EOVERFLOW);
+	if (max && prop->length > max)
+		return ERR_PTR(-EOVERFLOW);
+
+	if (len)
+		*len = prop->length;
 
 	return prop->value;
 }
@@ -1189,7 +1196,9 @@ int of_property_read_u32_index(const struct device_node *np,
 				       u32 index, u32 *out_value)
 {
 	const u32 *val = of_find_property_value_of_size(np, propname,
-					((index + 1) * sizeof(*out_value)));
+					((index + 1) * sizeof(*out_value)),
+					0,
+					NULL);
 
 	if (IS_ERR(val))
 		return PTR_ERR(val);
@@ -1221,7 +1230,9 @@ int of_property_read_u8_array(const struct device_node *np,
 			const char *propname, u8 *out_values, size_t sz)
 {
 	const u8 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)));
+						(sz * sizeof(*out_values)),
+						0,
+						NULL);
 
 	if (IS_ERR(val))
 		return PTR_ERR(val);
@@ -1254,7 +1265,9 @@ int of_property_read_u16_array(const struct device_node *np,
 			const char *propname, u16 *out_values, size_t sz)
 {
 	const __be16 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)));
+						(sz * sizeof(*out_values)),
+						0,
+						NULL);
 
 	if (IS_ERR(val))
 		return PTR_ERR(val);
@@ -1286,7 +1299,9 @@ int of_property_read_u32_array(const struct device_node *np,
 			       size_t sz)
 {
 	const __be32 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)));
+						(sz * sizeof(*out_values)),
+						0,
+						NULL);
 
 	if (IS_ERR(val))
 		return PTR_ERR(val);
@@ -1314,7 +1329,9 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
 			 u64 *out_value)
 {
 	const __be32 *val = of_find_property_value_of_size(np, propname,
-						sizeof(*out_value));
+						sizeof(*out_value),
+						0,
+						NULL);
 
 	if (IS_ERR(val))
 		return PTR_ERR(val);
@@ -1345,7 +1362,9 @@ int of_property_read_u64_array(const struct device_node *np,
 			       size_t sz)
 {
 	const __be32 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)));
+						(sz * sizeof(*out_values)),
+						0,
+						NULL);
 
 	if (IS_ERR(val))
 		return PTR_ERR(val);
-- 
1.9.1

[PATCH 2/2] of: Add array read functions with min/max size limits

[Richard Fitzgerald]

Add a new set of array reading functions that take a minimum and
maximum size limit and will fail if the property size is not within
the size limits. This makes it more convenient for drivers that
use variable-size DT arrays which must be bounded at both ends -
data must be at least N entries but must not overflow the array
it is being copied into. It is also more efficient than making this
functionality out of existing public functions and avoids duplication.

The existing array functions have been left in the API, since there
are a very large number of clients of those functions and their
existing functionality is still useful. This avoids turning a small
API improvement into a major kernel rework.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 drivers/of/base.c  | 206 ++++++++++++++++++++++++++++++++++++++++++-----------
 include/linux/of.h |  16 +++++
 2 files changed, 180 insertions(+), 42 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b853737..cbad5cf 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1209,6 +1209,47 @@ int of_property_read_u32_index(const struct device_node *np,
 EXPORT_SYMBOL_GPL(of_property_read_u32_index);
 
 /**
+ * of_property_read_variable_u8_array - Find and read an array of u8 from a
+ * property, with bounds on the minimum and maximum array size.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @out_values:	pointer to return value, modified only if return value is 0.
+ * @sz_min:	minimum number of array elements to read
+ * @sz_max:	maximum number of array elements to read
+ *
+ * Search for a property in a device node and read 8-bit value(s) from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data is smaller than sz_min or longer than sz_max.
+ *
+ * dts entry of array should be like:
+ *	property = /bits/ 8 <0x50 0x60 0x70>;
+ *
+ * The out_values is modified only if a valid u8 value can be decoded.
+ */
+int of_property_read_variable_u8_array(const struct device_node *np,
+					const char *propname, u8 *out_values,
+					size_t sz_min, size_t sz_max)
+{
+	size_t sz;
+	const u8 *val = of_find_property_value_of_size(np, propname,
+						(sz_min * sizeof(*out_values)),
+						(sz_max * sizeof(*out_values)),
+						&sz);
+
+	if (IS_ERR(val))
+		return PTR_ERR(val);
+
+	sz /= sizeof(*out_values);
+
+	while (sz--)
+		*out_values++ = *val++;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_property_read_variable_u8_array);
+
+/**
  * of_property_read_u8_array - Find and read an array of u8 from a property.
  *
  * @np:		device node from which the property value is to be read.
@@ -1229,21 +1270,53 @@ EXPORT_SYMBOL_GPL(of_property_read_u32_index);
 int of_property_read_u8_array(const struct device_node *np,
 			const char *propname, u8 *out_values, size_t sz)
 {
-	const u8 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)),
-						0,
-						NULL);
-
-	if (IS_ERR(val))
-		return PTR_ERR(val);
-
-	while (sz--)
-		*out_values++ = *val++;
-	return 0;
+	return of_property_read_variable_u8_array(np, propname, out_values,
+						  sz, 0);
 }
 EXPORT_SYMBOL_GPL(of_property_read_u8_array);
 
 /**
+ * of_property_read_variable_u16_array - Find and read an array of u16 from a
+ * property, with bounds on the minimum and maximum array size.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @out_values:	pointer to return value, modified only if return value is 0.
+ * @sz_min:	minimum number of array elements to read
+ * @sz_max:	maximum number of array elements to read
+ *
+ * Search for a property in a device node and read 16-bit value(s) from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data is smaller than sz_min or longer than sz_max.
+ *
+ * dts entry of array should be like:
+ *	property = /bits/ 16 <0x5000 0x6000 0x7000>;
+ *
+ * The out_values is modified only if a valid u16 value can be decoded.
+ */
+int of_property_read_variable_u16_array(const struct device_node *np,
+					const char *propname, u16 *out_values,
+					size_t sz_min, size_t sz_max)
+{
+	size_t sz;
+	const __be16 *val = of_find_property_value_of_size(np, propname,
+						(sz_min * sizeof(*out_values)),
+						(sz_max * sizeof(*out_values)),
+						&sz);
+
+	if (IS_ERR(val))
+		return PTR_ERR(val);
+
+	sz /= sizeof(*out_values);
+
+	while (sz--)
+		*out_values++ = be16_to_cpup(val++);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_property_read_variable_u16_array);
+
+/**
  * of_property_read_u16_array - Find and read an array of u16 from a property.
  *
  * @np:		device node from which the property value is to be read.
@@ -1264,21 +1337,50 @@ EXPORT_SYMBOL_GPL(of_property_read_u8_array);
 int of_property_read_u16_array(const struct device_node *np,
 			const char *propname, u16 *out_values, size_t sz)
 {
-	const __be16 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)),
-						0,
-						NULL);
-
-	if (IS_ERR(val))
-		return PTR_ERR(val);
-
-	while (sz--)
-		*out_values++ = be16_to_cpup(val++);
-	return 0;
+	return of_property_read_variable_u16_array(np, propname, out_values,
+						   sz, 0);
 }
 EXPORT_SYMBOL_GPL(of_property_read_u16_array);
 
 /**
+ * of_property_read_variable_u32_array - Find and read an array of 32 bit
+ * integers from a property, with bounds on the minimum and maximum array size.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @out_values:	pointer to return value, modified only if return value is 0.
+ * @sz_min:	minimum number of array elements to read
+ * @sz_max:	maximum number of array elements to read
+ *
+ * Search for a property in a device node and read 32-bit value(s) from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data is smaller than sz_min or longer than sz_max.
+ *
+ * The out_values is modified only if a valid u32 value can be decoded.
+ */
+int of_property_read_variable_u32_array(const struct device_node *np,
+			       const char *propname, u32 *out_values,
+			       size_t sz_min, size_t sz_max)
+{
+	size_t sz;
+	const __be32 *val = of_find_property_value_of_size(np, propname,
+						(sz_min * sizeof(*out_values)),
+						(sz_max * sizeof(*out_values)),
+						&sz);
+
+	if (IS_ERR(val))
+		return PTR_ERR(val);
+
+	sz /= sizeof(*out_values);
+
+	while (sz--)
+		*out_values++ = be32_to_cpup(val++);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_property_read_variable_u32_array);
+
+/**
  * of_property_read_u32_array - Find and read an array of 32 bit integers
  * from a property.
  *
@@ -1298,17 +1400,8 @@ int of_property_read_u32_array(const struct device_node *np,
 			       const char *propname, u32 *out_values,
 			       size_t sz)
 {
-	const __be32 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)),
-						0,
-						NULL);
-
-	if (IS_ERR(val))
-		return PTR_ERR(val);
-
-	while (sz--)
-		*out_values++ = be32_to_cpup(val++);
-	return 0;
+	return of_property_read_variable_u32_array(np, propname, out_values,
+						   sz, 0);
 }
 EXPORT_SYMBOL_GPL(of_property_read_u32_array);
 
@@ -1342,39 +1435,68 @@ int of_property_read_u64(const struct device_node *np, const char *propname,
 EXPORT_SYMBOL_GPL(of_property_read_u64);
 
 /**
- * of_property_read_u64_array - Find and read an array of 64 bit integers
- * from a property.
+ * of_property_read_variable_u64_array - Find and read an array of 64 bit
+ * integers from a property, with bounds on the minimum and maximum array size.
  *
  * @np:		device node from which the property value is to be read.
  * @propname:	name of the property to be searched.
  * @out_values:	pointer to return value, modified only if return value is 0.
- * @sz:		number of array elements to read
+ * @sz_min:	minimum number of array elements to read
+ * @sz_max:	maximum number of array elements to read
  *
  * Search for a property in a device node and read 64-bit value(s) from
  * it. Returns 0 on success, -EINVAL if the property does not exist,
  * -ENODATA if property does not have a value, and -EOVERFLOW if the
- * property data isn't large enough.
+ * property data is smaller than sz_min or longer than sz_max.
  *
  * The out_values is modified only if a valid u64 value can be decoded.
  */
-int of_property_read_u64_array(const struct device_node *np,
+int of_property_read_variable_u64_array(const struct device_node *np,
 			       const char *propname, u64 *out_values,
-			       size_t sz)
+			       size_t sz_min, size_t sz_max)
 {
+	size_t sz;
 	const __be32 *val = of_find_property_value_of_size(np, propname,
-						(sz * sizeof(*out_values)),
-						0,
-						NULL);
+						(sz_min * sizeof(*out_values)),
+						(sz_max * sizeof(*out_values)),
+						&sz);
 
 	if (IS_ERR(val))
 		return PTR_ERR(val);
 
+	sz /= sizeof(*out_values);
+
 	while (sz--) {
 		*out_values++ = of_read_number(val, 2);
 		val += 2;
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(of_property_read_variable_u64_array);
+
+/**
+ * of_property_read_u64_array - Find and read an array of 64 bit integers
+ * from a property.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @out_values:	pointer to return value, modified only if return value is 0.
+ * @sz:		number of array elements to read
+ *
+ * Search for a property in a device node and read 64-bit value(s) from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data isn't large enough.
+ *
+ * The out_values is modified only if a valid u64 value can be decoded.
+ */
+int of_property_read_u64_array(const struct device_node *np,
+			       const char *propname, u64 *out_values,
+			       size_t sz)
+{
+	return of_property_read_variable_u64_array(np, propname, out_values,
+						   sz, 0);
+}
 EXPORT_SYMBOL_GPL(of_property_read_u64_array);
 
 /**
diff --git a/include/linux/of.h b/include/linux/of.h
index 3d9ff8e..8e3c508 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -291,16 +291,32 @@ extern int of_property_count_elems_of_size(const struct device_node *np,
 extern int of_property_read_u32_index(const struct device_node *np,
 				       const char *propname,
 				       u32 index, u32 *out_value);
+extern int of_property_read_variable_u8_array(const struct device_node *np,
+					const char *propname, u8 *out_values,
+					size_t sz_min, size_t sz_max);
 extern int of_property_read_u8_array(const struct device_node *np,
 			const char *propname, u8 *out_values, size_t sz);
+extern int of_property_read_variable_u16_array(const struct device_node *np,
+					const char *propname, u16 *out_values,
+					size_t sz_min, size_t sz_max);
 extern int of_property_read_u16_array(const struct device_node *np,
 			const char *propname, u16 *out_values, size_t sz);
+extern int of_property_read_variable_u32_array(const struct device_node *np,
+					const char *propname,
+					u32 *out_values,
+					size_t sz_min,
+					size_t sz_max);
 extern int of_property_read_u32_array(const struct device_node *np,
 				      const char *propname,
 				      u32 *out_values,
 				      size_t sz);
 extern int of_property_read_u64(const struct device_node *np,
 				const char *propname, u64 *out_value);
+extern int of_property_read_variable_u64_array(const struct device_node *np,
+					const char *propname,
+					u64 *out_values,
+					size_t sz_min,
+					size_t sz_max);
 extern int of_property_read_u64_array(const struct device_node *np,
 				      const char *propname,
 				      u64 *out_values,
-- 
1.9.1

Re: [PATCH 2/2] of: Add array read functions with min/max size limits

[Rob Herring]

On Tue, Sep 6, 2016 at 10:02 AM, Richard Fitzgerald
<rf@opensource.wolfsonmicro.com> wrote:
> Add a new set of array reading functions that take a minimum and
> maximum size limit and will fail if the property size is not within
> the size limits. This makes it more convenient for drivers that
> use variable-size DT arrays which must be bounded at both ends -
> data must be at least N entries but must not overflow the array
> it is being copied into. It is also more efficient than making this
> functionality out of existing public functions and avoids duplication.
>
> The existing array functions have been left in the API, since there
> are a very large number of clients of those functions and their
> existing functionality is still useful. This avoids turning a small
> API improvement into a major kernel rework.

Thanks for doing this.

> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> ---
>  drivers/of/base.c  | 206 ++++++++++++++++++++++++++++++++++++++++++-----------
>  include/linux/of.h |  16 +++++
>  2 files changed, 180 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index b853737..cbad5cf 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1209,6 +1209,47 @@ int of_property_read_u32_index(const struct device_node *np,
>  EXPORT_SYMBOL_GPL(of_property_read_u32_index);
>
>  /**
> + * of_property_read_variable_u8_array - Find and read an array of u8 from a
> + * property, with bounds on the minimum and maximum array size.
> + *
> + * @np:                device node from which the property value is to be read.
> + * @propname:  name of the property to be searched.
> + * @out_values:        pointer to return value, modified only if return value is 0.
> + * @sz_min:    minimum number of array elements to read
> + * @sz_max:    maximum number of array elements to read
> + *
> + * Search for a property in a device node and read 8-bit value(s) from
> + * it. Returns 0 on success, -EINVAL if the property does not exist,
> + * -ENODATA if property does not have a value, and -EOVERFLOW if the
> + * property data is smaller than sz_min or longer than sz_max.
> + *
> + * dts entry of array should be like:
> + *     property = /bits/ 8 <0x50 0x60 0x70>;
> + *
> + * The out_values is modified only if a valid u8 value can be decoded.
> + */
> +int of_property_read_variable_u8_array(const struct device_node *np,
> +                                       const char *propname, u8 *out_values,
> +                                       size_t sz_min, size_t sz_max)
> +{
> +       size_t sz;
> +       const u8 *val = of_find_property_value_of_size(np, propname,
> +                                               (sz_min * sizeof(*out_values)),
> +                                               (sz_max * sizeof(*out_values)),
> +                                               &sz);
> +
> +       if (IS_ERR(val))
> +               return PTR_ERR(val);
> +
> +       sz /= sizeof(*out_values);
> +
> +       while (sz--)
> +               *out_values++ = *val++;
> +       return 0;

I think this needs to return the actual number of elements filled.

> +}
> +EXPORT_SYMBOL_GPL(of_property_read_variable_u8_array);
> +
> +/**
>   * of_property_read_u8_array - Find and read an array of u8 from a property.
>   *
>   * @np:                device node from which the property value is to be read.
> @@ -1229,21 +1270,53 @@ EXPORT_SYMBOL_GPL(of_property_read_u32_index);
>  int of_property_read_u8_array(const struct device_node *np,
>                         const char *propname, u8 *out_values, size_t sz)
>  {
> -       const u8 *val = of_find_property_value_of_size(np, propname,
> -                                               (sz * sizeof(*out_values)),
> -                                               0,
> -                                               NULL);
> -
> -       if (IS_ERR(val))
> -               return PTR_ERR(val);
> -
> -       while (sz--)
> -               *out_values++ = *val++;
> -       return 0;
> +       return of_property_read_variable_u8_array(np, propname, out_values,
> +                                                 sz, 0);

This should be min and max both set to sz.

All these can be made a static inline and then we don't need the
declaration and the dummy empty function. Changing the return value
here will complicate things as this function should maintain the
existing return values (i.e. 0 for success).

Similar comments for the other flavors.

Rob

Re: [PATCH 2/2] of: Add array read functions with min/max size limits

[Richard Fitzgerald]

On Thu, 2016-09-08 at 09:46 -0500, Rob Herring wrote:
> On Tue, Sep 6, 2016 at 10:02 AM, Richard Fitzgerald
> <rf@opensource.wolfsonmicro.com> wrote:
> > Add a new set of array reading functions that take a minimum and
> > maximum size limit and will fail if the property size is not within
> > the size limits. This makes it more convenient for drivers that
> > use variable-size DT arrays which must be bounded at both ends -
> > data must be at least N entries but must not overflow the array
> > it is being copied into. It is also more efficient than making this
> > functionality out of existing public functions and avoids duplication.
> >
> > The existing array functions have been left in the API, since there
> > are a very large number of clients of those functions and their
> > existing functionality is still useful. This avoids turning a small
> > API improvement into a major kernel rework.
> 
> Thanks for doing this.
> 
> > Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > ---
> >  drivers/of/base.c  | 206 ++++++++++++++++++++++++++++++++++++++++++-----------
> >  include/linux/of.h |  16 +++++
> >  2 files changed, 180 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index b853737..cbad5cf 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1209,6 +1209,47 @@ int of_property_read_u32_index(const struct device_node *np,
> >  EXPORT_SYMBOL_GPL(of_property_read_u32_index);
> >
> >  /**
> > + * of_property_read_variable_u8_array - Find and read an array of u8 from a
> > + * property, with bounds on the minimum and maximum array size.
> > + *
> > + * @np:                device node from which the property value is to be read.
> > + * @propname:  name of the property to be searched.
> > + * @out_values:        pointer to return value, modified only if return value is 0.
> > + * @sz_min:    minimum number of array elements to read
> > + * @sz_max:    maximum number of array elements to read
> > + *
> > + * Search for a property in a device node and read 8-bit value(s) from
> > + * it. Returns 0 on success, -EINVAL if the property does not exist,
> > + * -ENODATA if property does not have a value, and -EOVERFLOW if the
> > + * property data is smaller than sz_min or longer than sz_max.
> > + *
> > + * dts entry of array should be like:
> > + *     property = /bits/ 8 <0x50 0x60 0x70>;
> > + *
> > + * The out_values is modified only if a valid u8 value can be decoded.
> > + */
> > +int of_property_read_variable_u8_array(const struct device_node *np,
> > +                                       const char *propname, u8 *out_values,
> > +                                       size_t sz_min, size_t sz_max)
> > +{
> > +       size_t sz;
> > +       const u8 *val = of_find_property_value_of_size(np, propname,
> > +                                               (sz_min * sizeof(*out_values)),
> > +                                               (sz_max * sizeof(*out_values)),
> > +                                               &sz);
> > +
> > +       if (IS_ERR(val))
> > +               return PTR_ERR(val);
> > +
> > +       sz /= sizeof(*out_values);
> > +
> > +       while (sz--)
> > +               *out_values++ = *val++;
> > +       return 0;
> 
> I think this needs to return the actual number of elements filled.

You're right.

> 
> > +}
> > +EXPORT_SYMBOL_GPL(of_property_read_variable_u8_array);
> > +
> > +/**
> >   * of_property_read_u8_array - Find and read an array of u8 from a property.
> >   *
> >   * @np:                device node from which the property value is to be read.
> > @@ -1229,21 +1270,53 @@ EXPORT_SYMBOL_GPL(of_property_read_u32_index);
> >  int of_property_read_u8_array(const struct device_node *np,
> >                         const char *propname, u8 *out_values, size_t sz)
> >  {
> > -       const u8 *val = of_find_property_value_of_size(np, propname,
> > -                                               (sz * sizeof(*out_values)),
> > -                                               0,
> > -                                               NULL);
> > -
> > -       if (IS_ERR(val))
> > -               return PTR_ERR(val);
> > -
> > -       while (sz--)
> > -               *out_values++ = *val++;
> > -       return 0;
> > +       return of_property_read_variable_u8_array(np, propname, out_values,
> > +                                                 sz, 0);
> 
> This should be min and max both set to sz.

Passing 0 as max preserves the existing behaviour of these functions of
only requiring the array to be at least sz long, but not caring if it's
longer.

> 
> All these can be made a static inline and then we don't need the
> declaration and the dummy empty function. Changing the return value
> here will complicate things as this function should maintain the
> existing return values (i.e. 0 for success).
> 
> Similar comments for the other flavors.
> 
> Rob

I'll push a new version of these patches.

Re: [PATCH 2/2] of: Add array read functions with min/max size limits

[Rob Herring]

On Thu, Sep 8, 2016 at 10:34 AM, Richard Fitzgerald
<rf@opensource.wolfsonmicro.com> wrote:
> On Thu, 2016-09-08 at 09:46 -0500, Rob Herring wrote:
>> On Tue, Sep 6, 2016 at 10:02 AM, Richard Fitzgerald
>> <rf@opensource.wolfsonmicro.com> wrote:
>> > Add a new set of array reading functions that take a minimum and
>> > maximum size limit and will fail if the property size is not within
>> > the size limits. This makes it more convenient for drivers that
>> > use variable-size DT arrays which must be bounded at both ends -
>> > data must be at least N entries but must not overflow the array
>> > it is being copied into. It is also more efficient than making this
>> > functionality out of existing public functions and avoids duplication.
>> >
>> > The existing array functions have been left in the API, since there
>> > are a very large number of clients of those functions and their
>> > existing functionality is still useful. This avoids turning a small
>> > API improvement into a major kernel rework.

[...]

>> > @@ -1229,21 +1270,53 @@ EXPORT_SYMBOL_GPL(of_property_read_u32_index);
>> >  int of_property_read_u8_array(const struct device_node *np,
>> >                         const char *propname, u8 *out_values, size_t sz)
>> >  {
>> > -       const u8 *val = of_find_property_value_of_size(np, propname,
>> > -                                               (sz * sizeof(*out_values)),
>> > -                                               0,
>> > -                                               NULL);
>> > -
>> > -       if (IS_ERR(val))
>> > -               return PTR_ERR(val);
>> > -
>> > -       while (sz--)
>> > -               *out_values++ = *val++;
>> > -       return 0;
>> > +       return of_property_read_variable_u8_array(np, propname, out_values,
>> > +                                                 sz, 0);
>>
>> This should be min and max both set to sz.
>
> Passing 0 as max preserves the existing behaviour of these functions of
> only requiring the array to be at least sz long, but not caring if it's
> longer.

Yes, I was just writing to say that after reading patch 1 more carefully.

Rob

Re: [PATCH 2/2] of: Add array read functions with min/max size limits

[Richard Fitzgerald]

On Thu, 2016-09-08 at 10:38 -0500, Rob Herring wrote:
> On Thu, Sep 8, 2016 at 10:34 AM, Richard Fitzgerald
> <rf@opensource.wolfsonmicro.com> wrote:
> > On Thu, 2016-09-08 at 09:46 -0500, Rob Herring wrote:
> >> On Tue, Sep 6, 2016 at 10:02 AM, Richard Fitzgerald
> >> <rf@opensource.wolfsonmicro.com> wrote:
> >> > Add a new set of array reading functions that take a minimum and
> >> > maximum size limit and will fail if the property size is not within
> >> > the size limits. This makes it more convenient for drivers that
> >> > use variable-size DT arrays which must be bounded at both ends -
> >> > data must be at least N entries but must not overflow the array
> >> > it is being copied into. It is also more efficient than making this
> >> > functionality out of existing public functions and avoids duplication.
> >> >
> >> > The existing array functions have been left in the API, since there
> >> > are a very large number of clients of those functions and their
> >> > existing functionality is still useful. This avoids turning a small
> >> > API improvement into a major kernel rework.
> 
> [...]
> 
> >> > @@ -1229,21 +1270,53 @@ EXPORT_SYMBOL_GPL(of_property_read_u32_index);
> >> >  int of_property_read_u8_array(const struct device_node *np,
> >> >                         const char *propname, u8 *out_values, size_t sz)
> >> >  {
> >> > -       const u8 *val = of_find_property_value_of_size(np, propname,
> >> > -                                               (sz * sizeof(*out_values)),
> >> > -                                               0,
> >> > -                                               NULL);
> >> > -
> >> > -       if (IS_ERR(val))
> >> > -               return PTR_ERR(val);
> >> > -
> >> > -       while (sz--)
> >> > -               *out_values++ = *val++;
> >> > -       return 0;
> >> > +       return of_property_read_variable_u8_array(np, propname, out_values,
> >> > +                                                 sz, 0);
> >>
> >> This should be min and max both set to sz.
> >
> > Passing 0 as max preserves the existing behaviour of these functions of
> > only requiring the array to be at least sz long, but not caring if it's
> > longer.
> 
> Yes, I was just writing to say that after reading patch 1 more carefully.
> 

Although at the same time I was realizing that actually my code is
broken for that and somehow I missed validating it in my testing.
The new functions validate the min and max against the DT entry size and
copy the actual number of elements. The old functions effectively
validate only the min and return that number of elements.

I'm just considering whether it's worth fixing my new functions to keep
the intention that max=0 duplicates the old behaviour, or not bother and
just keep the old functions.

> Rob