On Thu, May 05, 2022 at 07:30:47AM +0000, Christophe Leroy wrote:
>
>
> Le 04/05/2022 à 17:40, Clément Léger a écrit :
> > Add function which allows to dynamically allocate and free properties.
> > Use this function internally for all code that used the same logic
> > (mainly __of_prop_dup()).
> >
> > Signed-off-by: Clément Léger <clement.le...@bootlin.com>
> > ---
> > drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++-------------
> > include/linux/of.h | 16 +++++++
> > 2 files changed, 88 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index cd3821a6444f..e8700e509d2e 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -313,9 +313,7 @@ static void property_list_free(struct property
> > *prop_list)
> >
> > for (prop = prop_list; prop != NULL; prop = next) {
> > next = prop->next;
> > - kfree(prop->name);
> > - kfree(prop->value);
> > - kfree(prop);
> > + of_property_free(prop);
> > }
> > }
> >
> > @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
> > }
> >
> > /**
> > - * __of_prop_dup - Copy a property dynamically.
> > - * @prop: Property to copy
> > + * of_property_free - Free a property allocated dynamically.
> > + * @prop: Property to be freed
> > + */
> > +void of_property_free(const struct property *prop)
> > +{
> > + kfree(prop->value);
> > + kfree(prop->name);
> > + kfree(prop);
> > +}
> > +EXPORT_SYMBOL(of_property_free);
> > +
> > +/**
> > + * of_property_alloc - Allocate a property dynamically.
> > + * @name: Name of the new property
> > + * @value: Value that will be copied into the new property value
> > + * @value_len: length of @value to be copied into the new property
> > value
> > + * @len: Length of new property value, must be greater than @value_len
> > * @allocflags: Allocation flags (typically pass GFP_KERNEL)
> > *
> > - * Copy a property by dynamically allocating the memory of both the
> > + * Create a property by dynamically allocating the memory of both the
> > * property structure and the property name & contents. The property's
> > * flags have the OF_DYNAMIC bit set so that we can differentiate between
> > * dynamically allocated properties and not.
> > *
> > * Return: The newly allocated property or NULL on out of memory error.
> > */
> > -struct property *__of_prop_dup(const struct property *prop, gfp_t
> > allocflags)
> > +struct property *of_property_alloc(const char *name, const void *value,
> > + int value_len, int len, gfp_t allocflags)
> > {
> > - struct property *new;
> > + int alloc_len = len;
> > + struct property *prop;
> > +
> > + if (len < value_len)
> > + return NULL;
> >
> > - new = kzalloc(sizeof(*new), allocflags);
> > - if (!new)
> > + prop = kzalloc(sizeof(*prop), allocflags);
> > + if (!prop)
> > return NULL;
> >
> > + prop->name = kstrdup(name, allocflags);
> > + if (!prop->name)
> > + goto out_err;
> > +
> > /*
> > - * NOTE: There is no check for zero length value.
> > - * In case of a boolean property, this will allocate a value
> > - * of zero bytes. We do this to work around the use
> > - * of of_get_property() calls on boolean values.
> > + * Even if the property has no value, it must be set to a
> > + * non-null value since of_get_property() is used to check
> > + * some values that might or not have a values (ranges for
> > + * instance). Moreover, when the node is released, prop->value
> > + * is kfreed so the memory must come from kmalloc.
> > */
> > - new->name = kstrdup(prop->name, allocflags);
> > - new->value = kmemdup(prop->value, prop->length, allocflags);
> > - new->length = prop->length;
> > - if (!new->name || !new->value)
> > - goto err_free;
> > + if (!alloc_len)
> > + alloc_len = 1;
> >
> > - /* mark the property as dynamic */
> > - of_property_set_flag(new, OF_DYNAMIC);
> > + prop->value = kzalloc(alloc_len, allocflags);
> > + if (!prop->value)
> > + goto out_err;
> >
> > - return new;
> > + if (value)
> > + memcpy(prop->value, value, value_len);
>
> Could you use kmemdup() instead of kzalloc+memcpy ?
I'd prefer there be 1 alloc for struct property and value instead of 2.
And maybe 'name' gets rolled into it too, but that gets a bit more
complicated to manage I think.
With memcpy, note this series[1].
Rob
[1] https://lore.kernel.org/all/20220504014440.3697851-30-keesc...@chromium.org/
