Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <42d9e1af-5576-ed8a-be3a-9dfea6ce1041@linux.ibm.com>
Date:   Wed, 1 Jun 2022 15:30:47 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and
 of_property_free()
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>,
        =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>
Cc:     Nathan Lynch <nathanl@linux.ibm.com>, devicetree@vger.kernel.org,
        Ohhoon Kwon <ohoono.kwon@samsung.com>,
        David Hildenbrand <david@redhat.com>,
        Steen Hegelund <steen.hegelund@microchip.com>,
        Daniel Henrique Barboza <danielhb413@gmail.com>,
        YueHaibing <yuehaibing@huawei.com>,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        Paul Mackerras <paulus@samba.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Allan Nielsen <allan.nielsen@microchip.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Laurent Dufour <ldufour@linux.ibm.com>,
        Frank Rowand <frowand.list@gmail.com>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        David Gibson <david@gibson.dropbear.id.au>
References: <20220504154033.750511-1-clement.leger@bootlin.com>
 <20220504154033.750511-2-clement.leger@bootlin.com>
 <YnQnayouXw9/jp/E@robh.at.kernel.org>
From:   Tyrel Datwyler <tyreld@linux.ibm.com>
In-Reply-To: <YnQnayouXw9/jp/E@robh.at.kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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