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.
Rob
