On 6/2/22 07:06, Rob Herring wrote: > On Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler <tyreld@xxxxxxxxxxxxx> 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@xxxxxxxxxxx> >>>> --- >>>> drivers/of/dynamic.c | 101 ++++++++++++++++++++++++++++++------------- >>>> include/linux/of.h | 16 +++++++ >>>> 2 files changed, 88 insertions(+), 29 deletions(-) >>>> >>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c >>>> index cd3821a6444f..e8700e509d2e 100644 >>>> --- a/drivers/of/dynamic.c >>>> +++ b/drivers/of/dynamic.c >>>> @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list) >>>> >>>> for (prop = prop_list; prop != NULL; prop = next) { >>>> next = prop->next; >>>> - kfree(prop->name); >>>> - kfree(prop->value); >>>> - kfree(prop); >>>> + of_property_free(prop); >>>> } >>>> } >>>> >>>> @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj) >>>> } >>>> >>>> /** >>>> - * __of_prop_dup - Copy a property dynamically. >>>> - * @prop: Property to copy >>>> + * of_property_free - Free a property allocated dynamically. >>>> + * @prop: Property to be freed >>>> + */ >>>> +void of_property_free(const struct property *prop) >>>> +{ >>>> + kfree(prop->value); >>>> + kfree(prop->name); >>>> + kfree(prop); >>>> +} >>>> +EXPORT_SYMBOL(of_property_free); >>>> + >>>> +/** >>>> + * of_property_alloc - Allocate a property dynamically. >>>> + * @name: Name of the new property >>>> + * @value: Value that will be copied into the new property value >>>> + * @value_len: length of @value to be copied into the new property value >>>> + * @len: Length of new property value, must be greater than @value_len >>> >>> What's the usecase for the lengths being different? That doesn't seem >>> like a common case, so perhaps handle it with a NULL value and >>> non-zero length. Then the caller has to deal with populating >>> prop->value. >>> >>>> * @allocflags: Allocation flags (typically pass GFP_KERNEL) >>>> * >>>> - * Copy a property by dynamically allocating the memory of both the >>>> + * Create a property by dynamically allocating the memory of both the >>>> * property structure and the property name & contents. The property's >>>> * flags have the OF_DYNAMIC bit set so that we can differentiate between >>>> * dynamically allocated properties and not. >>>> * >>>> * Return: The newly allocated property or NULL on out of memory error. >>>> */ >>>> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags) >>>> +struct property *of_property_alloc(const char *name, const void *value, >>>> + int value_len, int len, gfp_t allocflags) >>>> { >>>> - struct property *new; >>>> + int alloc_len = len; >>>> + struct property *prop; >>>> + >>>> + if (len < value_len) >>>> + return NULL; >>>> >>>> - new = kzalloc(sizeof(*new), allocflags); >>>> - if (!new) >>>> + prop = kzalloc(sizeof(*prop), allocflags); >>>> + if (!prop) >>>> return NULL; >>>> >>>> + prop->name = kstrdup(name, allocflags); >>>> + if (!prop->name) >>>> + goto out_err; >>>> + >>>> /* >>>> - * NOTE: There is no check for zero length value. >>>> - * In case of a boolean property, this will allocate a value >>>> - * of zero bytes. We do this to work around the use >>>> - * of of_get_property() calls on boolean values. >>>> + * Even if the property has no value, it must be set to a >>>> + * non-null value since of_get_property() is used to check >>>> + * some values that might or not have a values (ranges for >>>> + * instance). Moreover, when the node is released, prop->value >>>> + * is kfreed so the memory must come from kmalloc. >>> >>> Allowing for NULL value didn't turn out well... >>> >>> We know that we can do the kfree because OF_DYNAMIC is set IIRC... >>> >>> If we do 1 allocation for prop and value, then we can test >>> for "prop->value == prop + 1" to determine if we need to free or not. >> >> 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