Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 5 May 2022 12:37:38 -0500
From:   Rob Herring <robh@kernel.org>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
Cc:     =?iso-8859-1?Q?Cl=E9ment_L=E9ger?= <clement.leger@bootlin.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Nathan Lynch <nathanl@linux.ibm.com>,
        Laurent Dufour <ldufour@linux.ibm.com>,
        Daniel Henrique Barboza <danielhb413@gmail.com>,
        David Gibson <david@gibson.dropbear.id.au>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Ohhoon Kwon <ohoono.kwon@samsung.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
        YueHaibing <yuehaibing@huawei.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Steen Hegelund <steen.hegelund@microchip.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Allan Nielsen <allan.nielsen@microchip.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        Horatiu Vultur <horatiu.vultur@microchip.com>
Subject: Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and
 of_property_free()
Message-ID: <YnQLYjcIc0PjvWYR@robh.at.kernel.org>
References: <20220504154033.750511-1-clement.leger@bootlin.com>
 <20220504154033.750511-2-clement.leger@bootlin.com>
 <9e470414-67a5-10ce-95eb-f8093fde70d4@csgroup.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9e470414-67a5-10ce-95eb-f8093fde70d4@csgroup.eu>
Precedence: bulk

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/