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 <[email protected]>
---
drivers/of/dynamic.c | 82 ++++++++++++++++++++++++-----------------
drivers/of/of_private.h | 21 ++++++++++-
include/linux/of.h | 14 +++++++
3 files changed, 82 insertions(+), 35 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..c0dcbea31d28 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,66 @@ 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)
+{
+ if (!of_property_check_flag(prop, OF_DYNAMIC))
+ return;
+
+ if (prop->value != prop + 1)
+ 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 or NULL
+ * if only @len allocation is needed.
+ * @len: Length of new property value and if @value is provided, the
+ * length of the value to be copied
* @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,
+ size_t len, gfp_t allocflags)
{
- struct property *new;
+ struct property *prop;
- new = kzalloc(sizeof(*new), allocflags);
- if (!new)
+ prop = kzalloc(sizeof(*prop) + len, allocflags);
+ if (!prop)
return NULL;
- /*
- * 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.
- */
- 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;
-
- /* mark the property as dynamic */
- of_property_set_flag(new, OF_DYNAMIC);
-
- return new;
-
- err_free:
- kfree(new->name);
- kfree(new->value);
- kfree(new);
+ prop->name = kstrdup(name, allocflags);
+ if (!prop->name)
+ goto out_err;
+
+ prop->value = prop + 1;
+ if (value)
+ memcpy(prop->value, value, len);
+
+ prop->length = len;
+ of_property_set_flag(prop, OF_DYNAMIC);
+
+ return prop;
+
+out_err:
+ of_property_free(prop);
+
return NULL;
}
+EXPORT_SYMBOL(of_property_alloc);
/**
* __of_node_dup() - Duplicate or create an empty device node dynamically.
@@ -447,9 +463,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/drivers/of/of_private.h b/drivers/of/of_private.h
index 9324483397f6..1d6459bf705d 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -115,7 +115,26 @@ extern void *__unflatten_device_tree(const void *blob,
* without taking node references, so you either have to
* own the devtree lock or work on detached trees only.
*/
-struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
+
+/**
+ * __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.
+ */
+static inline
+struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
+{
+ return of_property_alloc(prop->name, prop->value, prop->length,
+ allocflags);
+}
+
struct device_node *__of_node_dup(const struct device_node *np,
const char *full_name);
diff --git a/include/linux/of.h b/include/linux/of.h
index d74fd82a6963..f1966f3c3847 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1464,6 +1464,10 @@ enum of_reconfig_change {
};
#ifdef CONFIG_OF_DYNAMIC
+struct property *of_property_alloc(const char *name, const void *value,
+ size_t len, gfp_t allocflags);
+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);
@@ -1508,6 +1512,16 @@ 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,
+ size_t 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.36.0
On 6/1/22 01:17, 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 <[email protected]>
> ---
> drivers/of/dynamic.c | 82 ++++++++++++++++++++++++-----------------
> drivers/of/of_private.h | 21 ++++++++++-
> include/linux/of.h | 14 +++++++
> 3 files changed, 82 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..c0dcbea31d28 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,66 @@ 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)
> +{
> + if (!of_property_check_flag(prop, OF_DYNAMIC))
> + return;
> +
This looks wrong to me. From what I understand the value data is allocated as
trailing memory that is part of the property allocation itself. (ie. prop =
kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
of the trailing value data. Calling kfree(prop->value) is bogus since
prop->value wasn't dynamically allocated on its own.
Also, this condition will always fail. You explicitly set prop->value = prop + 1
in alloc.
Maybe I need to go back and look at v1 again.
-Tyrel
> + if (prop->value != prop + 1)
> + 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 or NULL
> + * if only @len allocation is needed.
> + * @len: Length of new property value and if @value is provided, the
> + * length of the value to be copied
> * @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,
> + size_t len, gfp_t allocflags)
> {
> - struct property *new;
> + struct property *prop;
>
> - new = kzalloc(sizeof(*new), allocflags);
> - if (!new)
> + prop = kzalloc(sizeof(*prop) + len, allocflags);
> + if (!prop)
> return NULL;
>
> - /*
> - * 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.
> - */
> - 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;
> -
> - /* mark the property as dynamic */
> - of_property_set_flag(new, OF_DYNAMIC);
> -
> - return new;
> -
> - err_free:
> - kfree(new->name);
> - kfree(new->value);
> - kfree(new);
> + prop->name = kstrdup(name, allocflags);
> + if (!prop->name)
> + goto out_err;
> +
> + prop->value = prop + 1;
> + if (value)
> + memcpy(prop->value, value, len);
> +
> + prop->length = len;
> + of_property_set_flag(prop, OF_DYNAMIC);
> +
> + return prop;
> +
> +out_err:
> + of_property_free(prop);
> +
> return NULL;
> }
> +EXPORT_SYMBOL(of_property_alloc);
>
> /**
> * __of_node_dup() - Duplicate or create an empty device node dynamically.
> @@ -447,9 +463,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/drivers/of/of_private.h b/drivers/of/of_private.h
> index 9324483397f6..1d6459bf705d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -115,7 +115,26 @@ extern void *__unflatten_device_tree(const void *blob,
> * without taking node references, so you either have to
> * own the devtree lock or work on detached trees only.
> */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
> +
> +/**
> + * __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.
> + */
> +static inline
> +struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +{
> + return of_property_alloc(prop->name, prop->value, prop->length,
> + allocflags);
> +}
> +
> struct device_node *__of_node_dup(const struct device_node *np,
> const char *full_name);
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index d74fd82a6963..f1966f3c3847 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1464,6 +1464,10 @@ enum of_reconfig_change {
> };
>
> #ifdef CONFIG_OF_DYNAMIC
> +struct property *of_property_alloc(const char *name, const void *value,
> + size_t len, gfp_t allocflags);
> +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);
> @@ -1508,6 +1512,16 @@ 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,
> + size_t 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;
Le Wed, 1 Jun 2022 15:32:29 -0700,
Tyrel Datwyler <[email protected]> a écrit :
> > /**
> > - * __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)
> > +{
> > + if (!of_property_check_flag(prop, OF_DYNAMIC))
> > + return;
> > +
>
> This looks wrong to me. From what I understand the value data is allocated as
> trailing memory that is part of the property allocation itself. (ie. prop =
> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
> of the trailing value data. Calling kfree(prop->value) is bogus since
> prop->value wasn't dynamically allocated on its own.
kfree(prop->value) is only called if the value is not the trailing data
of the property so I don't see what is wrong there. In that case, only
kfree(prop) is called.
>
> Also, this condition will always fail. You explicitly set prop->value = prop + 1
> in alloc.
The user that did allocated the property might want to provide its own
"value". In that case, prop->value would be overwritten by the user
allocated value and thus the check would be true, hence calling
kfree(prop->value).
>
> Maybe I need to go back and look at v1 again.
>
> -Tyrel
>
> > + if (prop->value != prop + 1)
> > + kfree(prop->value);
> > +
> > + kfree(prop->name);
> > + kfree(prop);
> > +}
> > +EXPORT_SYMBOL(of_property_free);
> > +
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
On 6/1/22 23:58, Clément Léger wrote:
> Le Wed, 1 Jun 2022 15:32:29 -0700,
> Tyrel Datwyler <[email protected]> a écrit :
>
>>> /**
>>> - * __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)
>>> +{
>>> + if (!of_property_check_flag(prop, OF_DYNAMIC))
>>> + return;
>>> +
>>
>> This looks wrong to me. From what I understand the value data is allocated as
>> trailing memory that is part of the property allocation itself. (ie. prop =
>> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
>> of the trailing value data. Calling kfree(prop->value) is bogus since
>> prop->value wasn't dynamically allocated on its own.
>
> kfree(prop->value) is only called if the value is not the trailing data
> of the property so I don't see what is wrong there. In that case, only
> kfree(prop) is called.
Right, Rob clarified for me in the v1 patch.
>
>>
>> Also, this condition will always fail. You explicitly set prop->value = prop + 1
>> in alloc.
>
> The user that did allocated the property might want to provide its own
> "value". In that case, prop->value would be overwritten by the user
> allocated value and thus the check would be true, hence calling
> kfree(prop->value).
So, that was the part I was missing. I think a comment would be helpful so its
clear value can be either trailing or user assigned.
-Tyrel
>
>>
>> Maybe I need to go back and look at v1 again.
>>
>> -Tyrel
>>
>>> + if (prop->value != prop + 1)
>>> + kfree(prop->value);
>>> +
>>> + kfree(prop->name);
>>> + kfree(prop);
>>> +}
>>> +EXPORT_SYMBOL(of_property_free);
>>> +
>
>