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@xxxxxxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  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.