On Wed, May 04, 2022 at 05:40:31PM +0200, Clément Léger wrote:
> 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.leger@xxxxxxxxxxx>
> ---
> 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
What's the usecase for the lengths being different? That doesn't seem
like a common case, so perhaps handle it with a NULL value and
non-zero length. Then the caller has to deal with populating
prop->value.
> * @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.
Allowing for NULL value didn't turn out well...
We know that we can do the kfree because OF_DYNAMIC is set IIRC...
If we do 1 allocation for prop and value, then we can test
for "prop->value == prop + 1" to determine if we need to free or not.
> */
> - 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);
> +
> + prop->length = len;
> + of_property_set_flag(prop, OF_DYNAMIC);
> +
> + return prop;
> +
> +out_err:
> + of_property_free(prop);
>
> - err_free:
> - kfree(new->name);
> - kfree(new->value);
> - kfree(new);
> return NULL;
> }
> +EXPORT_SYMBOL(of_property_alloc);
> +
> +/**
> + * __of_prop_dup - Copy a property dynamically.
> + * @prop: Property to copy
> + * @allocflags: Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Copy 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)
> +{
> + return of_property_alloc(prop->name, prop->value, prop->length,
> + prop->length, allocflags);
This can now be a static inline.
> +}
>
> /**
> * __of_node_dup() - Duplicate or create an empty device node dynamically.
> @@ -447,9 +492,7 @@ struct device_node *__of_node_dup(const struct device_node *np,
> if (!new_pp)
> goto err_prop;
> if (__of_add_property(node, new_pp)) {
> - kfree(new_pp->name);
> - kfree(new_pp->value);
> - kfree(new_pp);
> + of_property_free(new_pp);
> goto err_prop;
> }
> }
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 04971e85fbc9..6b345eb71c19 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1463,6 +1463,11 @@ enum of_reconfig_change {
> };
>
> #ifdef CONFIG_OF_DYNAMIC
> +extern struct property *of_property_alloc(const char *name, const void *value,
> + int value_len, int len,
> + gfp_t allocflags);
> +extern void of_property_free(const struct property *prop);
> +
> extern int of_reconfig_notifier_register(struct notifier_block *);
> extern int of_reconfig_notifier_unregister(struct notifier_block *);
> extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
> @@ -1507,6 +1512,17 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
> return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
> }
> #else /* CONFIG_OF_DYNAMIC */
> +
> +static inline struct property *of_property_alloc(const char *name,
> + const void *value,
> + int value_len, int len,
> + gfp_t allocflags)
> +{
> + return NULL;
> +}
> +
> +static inline void of_property_free(const struct property *prop) {}
> +
> static inline int of_reconfig_notifier_register(struct notifier_block *nb)
> {
> return -EINVAL;
> --
> 2.34.1
>
>
