Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3c40e495-2474-b7a6-6a5c-7ad29e8324bc@gmail.com>
Date:   Fri, 16 Sep 2022 18:15:31 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH RFC 1/2] of: dynamic: add of_node_alloc()
Content-Language: en-US
To:     Lizhi Hou <lizhi.hou@amd.com>, linux-pci@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        robh@kernel.org, helgaas@kernel.org
Cc:     =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>,
        max.zhen@amd.com, sonal.santan@amd.com, larry.liu@amd.com,
        brian.xu@amd.com, stefano.stabellini@xilinx.com, trix@redhat.com
References: <1661809417-11370-1-git-send-email-lizhi.hou@amd.com>
 <1661809417-11370-2-git-send-email-lizhi.hou@amd.com>
From:   Frank Rowand <frowand.list@gmail.com>
In-Reply-To: <1661809417-11370-2-git-send-email-lizhi.hou@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/29/22 16:43, Lizhi Hou wrote:
> From: Clément Léger <clement.leger@bootlin.com>
> 
> Add of_node_alloc() which allows to create nodes. The node full_name
> field is allocated as part of the node allocation and the kfree() for
> this field is checked at release time to allow users using their own
> allocated name.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/of/dynamic.c | 50 +++++++++++++++++++++++++++++++++++---------
>  include/linux/of.h   |  7 +++++++
>  2 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..0e08283fd4fd 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -362,10 +362,49 @@ void of_node_release(struct kobject *kobj)
>  	fwnode_links_purge(of_fwnode_handle(node));
>  
>  	kfree(node->full_name);
> +	if (node->full_name != (const char *)(node + 1))
> +		kfree(node->full_name);
> +

Why free node->full_name a second time?

>  	kfree(node->data);k>  	kfree(node);
>  }
>  
> +/**
> + * of_node_alloc - Allocate a node dynamically.
> + * @name:	Node name
> + *
> + * Create a node by dynamically allocating the memory of both the
> + * node structure and the node name & contents. The node's
> + * flags have the OF_DYNAMIC & OF_DETACHED bit set so that we can
> + * differentiate between dynamically allocated nodes and not.
> + *
> + * Return: The newly allocated node or NULL on out of memory error.
> + */
> +struct device_node *of_node_alloc(const char *name)
> +{

The body of this function is already implemented by __of_node_dup(NULL, ...)

> +	struct device_node *node;
> +	int name_len = 0;
> +
> +	if (name)
> +		name_len = strlen(name) + 1;
> +
> +	node = kzalloc(sizeof(*node) + name_len, GFP_KERNEL);
> +	if (!node)
> +		return NULL;
> +
> +	if (name) {
> +		node->full_name = (const char *)(node + 1);
> +		strcpy((char *)node->full_name, name);
> +	}
> +
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +	of_node_init(node);
> +
> +	return node;
> +}
> +EXPORT_SYMBOL(of_node_alloc);
> +
>  /**
>   * __of_prop_dup - Copy a property dynamically.
>   * @prop:	Property to copy
> @@ -426,18 +465,9 @@ struct device_node *__of_node_dup(const struct device_node *np,
>  {
>  	struct device_node *node;
>  
> -	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	node = of_node_alloc(full_name);
>  	if (!node)
>  		return NULL;
> -	node->full_name = kstrdup(full_name, GFP_KERNEL);
> -	if (!node->full_name) {
> -		kfree(node);
> -		return NULL;
> -	}
> -
> -	of_node_set_flag(node, OF_DYNAMIC);
> -	of_node_set_flag(node, OF_DETACHED);
> -	of_node_init(node);
>  
>  	/* Iterate over and duplicate all properties */
>  	if (np) {
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 766d002bddb9..fc71e0355f05 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1462,6 +1462,8 @@ enum of_reconfig_change {
>  };
>  
>  #ifdef CONFIG_OF_DYNAMIC
> +struct device_node *of_node_alloc(const char *name);
> +
>  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);
> @@ -1506,6 +1508,11 @@ 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 device_node *of_node_alloc(const char *name)
> +{
> +	return NULL;
> +}
> +
>  static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>  {
>  	return -EINVAL;