Re: [PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Rob Herring <robh@kernel.org>]

On Wed, Jun 01, 2022 at 10:18:01AM +0200, Clément Léger wrote:
> Use of_property_alloc/free() and of_node_alloc() to create and free
> device-tree nodes and properties.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  arch/powerpc/platforms/pseries/dlpar.c        | 51 +++----------------
>  .../platforms/pseries/hotplug-memory.c        | 21 +-------
>  arch/powerpc/platforms/pseries/reconfig.c     | 45 +++++-----------
>  3 files changed, 21 insertions(+), 96 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index 498d6efcb5ae..5a04566e98a4 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -38,61 +38,25 @@ struct cc_workarea {
>  	__be32	prop_offset;
>  };
>  
> -void dlpar_free_cc_property(struct property *prop)
> -{
> -	kfree(prop->name);
> -	kfree(prop->value);
> -	kfree(prop);
> -}
> -
>  static struct property *dlpar_parse_cc_property(struct cc_workarea *ccwa)
>  {
> -	struct property *prop;
> -	char *name;
> -	char *value;
> -
> -	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> -	if (!prop)
> -		return NULL;
> +	int length;
> +	char *name, *value;
>  
>  	name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
> -	prop->name = kstrdup(name, GFP_KERNEL);
> -	if (!prop->name) {
> -		dlpar_free_cc_property(prop);
> -		return NULL;
> -	}
> -
> -	prop->length = be32_to_cpu(ccwa->prop_length);
> +	length = be32_to_cpu(ccwa->prop_length);
>  	value = (char *)ccwa + be32_to_cpu(ccwa->prop_offset);
> -	prop->value = kmemdup(value, prop->length, GFP_KERNEL);
> -	if (!prop->value) {
> -		dlpar_free_cc_property(prop);
> -		return NULL;
> -	}
>  
> -	return prop;
> +	return of_property_alloc(name, value, length, GFP_KERNEL);
>  }
>  
>  static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa)
>  {
> -	struct device_node *dn;
>  	const char *name;
>  
> -	dn = kzalloc(sizeof(*dn), GFP_KERNEL);
> -	if (!dn)
> -		return NULL;
> -
>  	name = (const char *)ccwa + be32_to_cpu(ccwa->name_offset);
> -	dn->full_name = kstrdup(name, GFP_KERNEL);
> -	if (!dn->full_name) {
> -		kfree(dn);
> -		return NULL;
> -	}
>  
> -	of_node_set_flag(dn, OF_DYNAMIC);
> -	of_node_init(dn);
> -
> -	return dn;
> +	return of_node_alloc(name, GFP_KERNEL);

Do you have any need for different flags? I can't really see a need for 
atomic or dma allocs or ???, so drop it I think.

>  }
>  
>  static void dlpar_free_one_cc_node(struct device_node *dn)
> @@ -102,11 +66,10 @@ static void dlpar_free_one_cc_node(struct device_node *dn)
>  	while (dn->properties) {
>  		prop = dn->properties;
>  		dn->properties = prop->next;
> -		dlpar_free_cc_property(prop);
> +		of_property_free(prop);

We should be able to just put the node and all the properties already 
attached will be freed.

Looking at the history of this code, it originally did the kref_init 
much later in dlpar_attach_node(). So there was a window of allocating 
the node and adding properties where you'd need to manually free 
everything. Now that the node is referenced from the start, a put should 
free everything.

>  	}
>  
> -	kfree(dn->full_name);
> -	kfree(dn);
> +	of_node_put(dn);
>  }
>  
>  void dlpar_free_cc_nodes(struct device_node *dn)
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 2e3a317722a8..2ddf2a0ba048 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -69,33 +69,16 @@ unsigned long pseries_memory_block_size(void)
>  	return memblock_size;
>  }
>  
> -static void dlpar_free_property(struct property *prop)
> -{
> -	kfree(prop->name);
> -	kfree(prop->value);
> -	kfree(prop);
> -}
> -
>  static struct property *dlpar_clone_property(struct property *prop,
>  					     u32 prop_size)
>  {
> -	struct property *new_prop;
> -
> -	new_prop = kzalloc(sizeof(*new_prop), GFP_KERNEL);
> +	struct property *new_prop = of_property_alloc(prop->name, NULL,
> +						      prop_size, GFP_KERNEL);
>  	if (!new_prop)
>  		return NULL;
>  
> -	new_prop->name = kstrdup(prop->name, GFP_KERNEL);
> -	new_prop->value = kzalloc(prop_size, GFP_KERNEL);
> -	if (!new_prop->name || !new_prop->value) {
> -		dlpar_free_property(new_prop);
> -		return NULL;
> -	}
> -
>  	memcpy(new_prop->value, prop->value, prop->length);
> -	new_prop->length = prop_size;
>  
> -	of_property_set_flag(new_prop, OF_DYNAMIC);
>  	return new_prop;
>  }
>  
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index cad7a0c93117..f1a364995e82 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -24,17 +24,9 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
>  	struct device_node *np;
>  	int err = -ENOMEM;
>  
> -	np = kzalloc(sizeof(*np), GFP_KERNEL);
> +	np = of_node_alloc(kbasename(path), GFP_KERNEL);
>  	if (!np)
> -		goto out_err;
> -
> -	np->full_name = kstrdup(kbasename(path), GFP_KERNEL);
> -	if (!np->full_name)
> -		goto out_err;
> -
> -	np->properties = proplist;
> -	of_node_set_flag(np, OF_DYNAMIC);
> -	of_node_init(np);
> +		return -ENOMEM;
>  
>  	np->parent = pseries_of_derive_parent(path);
>  	if (IS_ERR(np->parent)) {
> @@ -55,8 +47,7 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
>  out_err:
>  	if (np) {
>  		of_node_put(np->parent);
> -		kfree(np->full_name);
> -		kfree(np);
> +		of_node_put(np);
>  	}
>  	return err;
>  }
> @@ -91,9 +82,7 @@ static void release_prop_list(const struct property *prop)
>  	struct property *next;
>  	for (; prop; prop = next) {
>  		next = prop->next;
> -		kfree(prop->name);
> -		kfree(prop->value);
> -		kfree(prop);
> +		of_property_free(prop);

Looks like you need this because code does: alloc properties, alloc 
node, add properties, attach node. It would need to be refactored to 
alloc the node first, but that's a bit more complex needing someone to 
test on pSeries.

>  	}
>  
>  }
> @@ -167,27 +156,17 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
>  static struct property *new_property(const char *name, const int length,
>  				     const unsigned char *value, struct property *last)
>  {
> -	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
> +	struct property *prop;
>  
> -	if (!new)
> +	prop = of_property_alloc(name, NULL, length + 1, GFP_KERNEL);
> +	if (!prop)
>  		return NULL;
>  
> -	if (!(new->name = kstrdup(name, GFP_KERNEL)))
> -		goto cleanup;
> -	if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
> -		goto cleanup;
> -
> -	memcpy(new->value, value, length);
> -	*(((char *)new->value) + length) = 0;
> -	new->length = length;
> -	new->next = last;
> -	return new;
> -
> -cleanup:
> -	kfree(new->name);
> -	kfree(new->value);
> -	kfree(new);
> -	return NULL;
> +	memcpy(prop->value, value, length);
> +	*(((char *)prop->value) + length) = 0;

Looks to me like this could be avoided with this change:

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index cad7a0c93117..614753fc5f27 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -148,7 +148,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
        /* now we're on the value */
        *value = tmp;
        tmp += *length;
-       if (tmp > end) {
+       if (tmp >= end) {
                printk(KERN_ERR "property parse failed in %s at line %d\n",
                       __func__, __LINE__);
                return NULL;
@@ -158,6 +158,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
                       __func__, __LINE__);
                return NULL;
        }
+       *tmp = '\0';
        tmp++;
 
        /* and now we should be on the next name, or the end */


Based on the comments, 'buf' should be nul terminated, so I would think 
that tmp == end would be an error. But I really don't know.

Really need some pSeries people to comment on all this.

Another option is if value is NULL, then of_property_alloc() should 
ensure the buffer is zeroed. Then you just need the memcpy.

Rob