| Message ID | 20220504154033.750511-2-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | of: add of_property_alloc/free() and of_node_alloc/free() | expand |
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.leger@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 ? > + > + 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' is pointless for function prototypes, you should not add new ones. Checkpatch complain about it: 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) > +{ > + 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 Thu, 5 May 2022 07:30:47 +0000, Christophe Leroy <christophe.leroy@csgroup.eu> 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,
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.leger@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-keescook@chromium.org/
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@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 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 12:37:38 -0500, Rob Herring <robh@kernel.org> 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/20220504014440.3697851-30-keescook@chromium.org/
Le Thu, 5 May 2022 14:37:15 -0500, Rob Herring <robh@kernel.org> 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.
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 <clement.leger@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 > > 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 Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler <tyreld@linux.ibm.com> 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 <clement.leger@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 > > > > 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
On 6/2/22 07:06, Rob Herring wrote: > On Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler <tyreld@linux.ibm.com> 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 <clement.leger@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 >>> >>> 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
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;
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@bootlin.com> --- drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++------------- include/linux/of.h | 16 +++++++ 2 files changed, 88 insertions(+), 29 deletions(-)