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 | 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);
+
+ 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);
+}
/**
* __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
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 <[email protected]>
> > ---
> > 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/[email protected]/
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 <[email protected]>
> ---
> 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
>
>
Le Thu, 5 May 2022 14:37:15 -0500,
Rob Herring <[email protected]> a écrit :
> > +
> > +/**
> > + * 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.
That was actually something used by powerpc code but agreed, letting
the user recopy it's values seems fine to me and the usage will be more
clear.
> > /*
> > - * 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.
Sounds like a good idea.
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
Le Thu, 5 May 2022 07:30:47 +0000,
Christophe Leroy <[email protected]> a écrit :
> > /*
> > - * 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 could but then, we won't be able to allocate a property value that is
larger than the original one. This is used by the powerpc code to
recopy an existing value and add some extra space after it.
> > 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' is pointless for function prototypes, you should not add new
> ones. Checkpatch complain about it:
I did so that, but I kept that since the existing code is full of them.
Since you mention it, I'll remove the extern.
>
> CHECK: extern prototypes should be avoided in .h files
> #172: FILE: include/linux/of.h:1466:
> +extern struct property *of_property_alloc(const char *name, const void
> *value,
>
> CHECK: extern prototypes should be avoided in .h files
> #175: FILE: include/linux/of.h:1469:
> +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)
>
> Can that fit on less lines ?
>
> May be:
>
> static inline struct property
> *of_property_alloc(const char *name, const void *value, int value_len,
> int len, gfp_t allocflags)
Yes, that seems a better split.
Thanks,
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
Le Thu, 5 May 2022 12:37:38 -0500,
Rob Herring <[email protected]> a écrit :
> > >
> > > - /* 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.
At least for value it should be easy indeed. i'll check what I can do
for the name.
>
> With memcpy, note this series[1].
Ok, good to know, should I base my series on that one ?
>
> Rob
>
> [1] https://lore.kernel.org/all/[email protected]/
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
On 5/5/22 12:37, Rob Herring wrote:
> 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 <[email protected]>
>> ---
>> 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.
If its a single allocation do we even need a test? Doesn't kfree(prop) take care
of the property and the trailing memory allocated for the value?
-Tyrel
>
>> */
>> - 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
>>
>>
On 6/2/22 07:06, Rob Herring wrote:
> On Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler <[email protected]> wrote:
>>
>> On 5/5/22 12:37, Rob Herring wrote:
>>> 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 <[email protected]>
>>>> ---
>>>> 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.
>>
>> If its a single allocation do we even need a test? Doesn't kfree(prop) take care
>> of the property and the trailing memory allocated for the value?
>
> Yes, it does when it's a single alloc, but it's testing for when
> prop->value is not a single allocation because we could have either.
>
Ok, that is the part I was missing. Thanks for the clarification.
-Tyrel
On Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler <[email protected]> wrote:
>
> On 5/5/22 12:37, Rob Herring wrote:
> > 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 <[email protected]>
> >> ---
> >> 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.
>
> If its a single allocation do we even need a test? Doesn't kfree(prop) take care
> of the property and the trailing memory allocated for the value?
Yes, it does when it's a single alloc, but it's testing for when
prop->value is not a single allocation because we could have either.
Rob