Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler <tyreld@linux.ibm.com>]

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