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:   Tue, 12 Apr 2022 12:23:49 +0200
From:   Slawomir Stepien <sst@poczta.fm>
To:     Frank Rowand <frowand.list@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>, pantelis.antoniou@konsulko.com,
        Slawomir Stepien <slawomir.stepien@nokia.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Alan Tull <atull@kernel.org>,
        Jan Kiszka <jan.kiszka@siemens.com>
Subject: Re: [PATCH v2 1/2] of: overlay: rename variables to be consistent
Message-ID: <YlVTNXXtXtm0syLm@t480s.localdomain>
References: <20220410210833.441504-1-frowand.list@gmail.com>
 <20220410210833.441504-2-frowand.list@gmail.com>
 <bcc7cbbb-eb5c-d089-31aa-efdd4868a177@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bcc7cbbb-eb5c-d089-31aa-efdd4868a177@gmail.com>
Precedence: bulk

On kwi 10, 2022 23:10, Frank Rowand wrote:
> adding cc: Jan Kiszka <jan.kiszka@siemens.com>
> 
> On 4/10/22 16:08, frowand.list@gmail.com wrote:
> > From: Frank Rowand <frank.rowand@sony.com>
> > 
> > Variables change name across function calls when there is not a good
> > reason to do so.  Fix by changing "fdt" to "new_fdt" and "tree" to
> > "overlay_tree".
> > 
> > The name disparity was confusing when creating the following commit.
> > The name changes are in this separate commit to make review of the
> > following commmit less complex.
> > 
> > Signed-off-by: Frank Rowand <frank.rowand@sony.com>

Hi Frank

It looks good to me.

Reviewed-by: Slawomir Stepien <slawomir.stepien@nokia.com>

> > ---
> > Changes since v1:
> >   - This patch is added to the series.
> > 
> >  drivers/of/overlay.c | 94 ++++++++++++++++++++++----------------------
> >  1 file changed, 47 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index d80160cf34bb..f74aa9ff67aa 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -57,7 +57,7 @@ struct fragment {
> >   * struct overlay_changeset
> >   * @id:			changeset identifier
> >   * @ovcs_list:		list on which we are located
> > - * @fdt:		base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
> > + * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> >   * @overlay_tree:	expanded device tree that contains the fragment nodes
> >   * @count:		count of fragment structures
> >   * @fragments:		fragment nodes in the overlay expanded device tree
> > @@ -67,7 +67,7 @@ struct fragment {
> >  struct overlay_changeset {
> >  	int id;
> >  	struct list_head ovcs_list;
> > -	const void *fdt;
> > +	const void *new_fdt;
> >  	struct device_node *overlay_tree;
> >  	int count;
> >  	struct fragment *fragments;
> > @@ -718,19 +718,20 @@ static struct device_node *find_target(struct device_node *info_node)
> >  
> >  /**
> >   * init_overlay_changeset() - initialize overlay changeset from overlay tree
> > - * @ovcs:	Overlay changeset to build
> > - * @fdt:	base of memory allocated to hold aligned FDT that was unflattened to create @tree
> > - * @tree:	Contains the overlay fragments and overlay fixup nodes
> > + * @ovcs:		Overlay changeset to build
> > + * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> > + * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
> >   *
> >   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
> > - * the top level of @tree.  The relevant top level nodes are the fragment
> > - * nodes and the __symbols__ node.  Any other top level node will be ignored.
> > + * the top level of @overlay_tree.  The relevant top level nodes are the
> > + * fragment nodes and the __symbols__ node.  Any other top level node will
> > + * be ignored.
> >   *
> >   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
> > - * detected in @tree, or -ENOSPC if idr_alloc() error.
> > + * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
> >   */
> >  static int init_overlay_changeset(struct overlay_changeset *ovcs,
> > -		const void *fdt, struct device_node *tree)
> > +		const void *new_fdt, struct device_node *overlay_tree)
> >  {
> >  	struct device_node *node, *overlay_node;
> >  	struct fragment *fragment;
> > @@ -741,17 +742,17 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	 * Warn for some issues.  Can not return -EINVAL for these until
> >  	 * of_unittest_apply_overlay() is fixed to pass these checks.
> >  	 */
> > -	if (!of_node_check_flag(tree, OF_DYNAMIC))
> > -		pr_debug("%s() tree is not dynamic\n", __func__);
> > +	if (!of_node_check_flag(overlay_tree, OF_DYNAMIC))
> > +		pr_debug("%s() overlay_tree is not dynamic\n", __func__);
> >  
> > -	if (!of_node_check_flag(tree, OF_DETACHED))
> > -		pr_debug("%s() tree is not detached\n", __func__);
> > +	if (!of_node_check_flag(overlay_tree, OF_DETACHED))
> > +		pr_debug("%s() overlay_tree is not detached\n", __func__);
> >  
> > -	if (!of_node_is_root(tree))
> > -		pr_debug("%s() tree is not root\n", __func__);
> > +	if (!of_node_is_root(overlay_tree))
> > +		pr_debug("%s() overlay_tree is not root\n", __func__);
> >  
> > -	ovcs->overlay_tree = tree;
> > -	ovcs->fdt = fdt;
> > +	ovcs->overlay_tree = overlay_tree;
> > +	ovcs->new_fdt = new_fdt;
> >  
> >  	INIT_LIST_HEAD(&ovcs->ovcs_list);
> >  
> > @@ -764,7 +765,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	cnt = 0;
> >  
> >  	/* fragment nodes */
> > -	for_each_child_of_node(tree, node) {
> > +	for_each_child_of_node(overlay_tree, node) {
> >  		overlay_node = of_get_child_by_name(node, "__overlay__");
> >  		if (overlay_node) {
> >  			cnt++;
> > @@ -772,7 +773,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  		}
> >  	}
> >  
> > -	node = of_get_child_by_name(tree, "__symbols__");
> > +	node = of_get_child_by_name(overlay_tree, "__symbols__");
> >  	if (node) {
> >  		cnt++;
> >  		of_node_put(node);
> > @@ -785,7 +786,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	}
> >  
> >  	cnt = 0;
> > -	for_each_child_of_node(tree, node) {
> > +	for_each_child_of_node(overlay_tree, node) {
> >  		overlay_node = of_get_child_by_name(node, "__overlay__");
> >  		if (!overlay_node)
> >  			continue;
> > @@ -807,7 +808,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	 * if there is a symbols fragment in ovcs->fragments[i] it is
> >  	 * the final element in the array
> >  	 */
> > -	node = of_get_child_by_name(tree, "__symbols__");
> > +	node = of_get_child_by_name(overlay_tree, "__symbols__");
> >  	if (node) {
> >  		ovcs->symbols_fragment = 1;
> >  		fragment = &fragments[cnt];
> > @@ -862,11 +863,11 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >  	kfree(ovcs->fragments);
> >  	/*
> >  	 * There should be no live pointers into ovcs->overlay_tree and
> > -	 * ovcs->fdt due to the policy that overlay notifiers are not allowed
> > -	 * to retain pointers into the overlay devicetree.
> > +	 * ovcs->new_fdt due to the policy that overlay notifiers are not
> > +	 * allowed to retain pointers into the overlay devicetree.
> >  	 */
> >  	kfree(ovcs->overlay_tree);
> > -	kfree(ovcs->fdt);
> > +	kfree(ovcs->new_fdt);
> >  	kfree(ovcs);
> >  }
> >  
> > @@ -874,16 +875,15 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >   * internal documentation
> >   *
> >   * of_overlay_apply() - Create and apply an overlay changeset
> > - * @fdt:	base of memory allocated to hold the aligned FDT
> > - * @tree:	Expanded overlay device tree
> > - * @ovcs_id:	Pointer to overlay changeset id
> > + * @new_fdt:		Memory allocated to hold the aligned FDT
> > + * @overlay_tree:	Expanded overlay device tree
> > + * @ovcs_id:		Pointer to overlay changeset id
> >   *
> >   * Creates and applies an overlay changeset.
> >   *
> >   * If an error occurs in a pre-apply notifier, then no changes are made
> >   * to the device tree.
> >   *
> > -
> >   * A non-zero return value will not have created the changeset if error is from:
> >   *   - parameter checks
> >   *   - building the changeset
> > @@ -913,29 +913,29 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >   * id is returned to *ovcs_id.
> >   */
> >  
> > -static int of_overlay_apply(const void *fdt, struct device_node *tree,
> > -		int *ovcs_id)
> > +static int of_overlay_apply(const void *new_fdt,
> > +		struct device_node *overlay_tree, int *ovcs_id)
> >  {
> >  	struct overlay_changeset *ovcs;
> >  	int ret = 0, ret_revert, ret_tmp;
> >  
> >  	/*
> > -	 * As of this point, fdt and tree belong to the overlay changeset.
> > -	 * overlay changeset code is responsible for freeing them.
> > +	 * As of this point, new_fdt and overlay_tree belong to the overlay
> > +	 * changeset.  overlay changeset code is responsible for freeing them.
> >  	 */
> >  
> >  	if (devicetree_corrupt()) {
> >  		pr_err("devicetree state suspect, refuse to apply overlay\n");
> > -		kfree(fdt);
> > -		kfree(tree);
> > +		kfree(new_fdt);
> > +		kfree(overlay_tree);
> >  		ret = -EBUSY;
> >  		goto out;
> >  	}
> >  
> >  	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
> >  	if (!ovcs) {
> > -		kfree(fdt);
> > -		kfree(tree);
> > +		kfree(new_fdt);
> > +		kfree(overlay_tree);
> >  		ret = -ENOMEM;
> >  		goto out;
> >  	}
> > @@ -943,20 +943,20 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
> >  	of_overlay_mutex_lock();
> >  	mutex_lock(&of_mutex);
> >  
> > -	ret = of_resolve_phandles(tree);
> > +	ret = of_resolve_phandles(overlay_tree);
> >  	if (ret)
> > -		goto err_free_tree;
> > +		goto err_free_overlay_tree;
> >  
> > -	ret = init_overlay_changeset(ovcs, fdt, tree);
> > +	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
> >  	if (ret)
> > -		goto err_free_tree;
> > +		goto err_free_overlay_tree;
> >  
> >  	/*
> >  	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
> > -	 * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
> > -	 * and can not free memory containing aligned fdt.  The aligned fdt
> > -	 * is contained within the memory at ovcs->fdt, possibly at an offset
> > -	 * from ovcs->fdt.
> > +	 * leaked to drivers, so can not kfree() overlay_tree,
> > +	 * aka ovcs->overlay_tree; and can not free memory containing aligned
> > +	 * fdt.  The aligned fdt is contained within the memory at
> > +	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
> >  	 */
> >  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
> >  	if (ret) {
> > @@ -997,9 +997,9 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
> >  
> >  	goto out_unlock;
> >  
> > -err_free_tree:
> > -	kfree(fdt);
> > -	kfree(tree);
> > +err_free_overlay_tree:
> > +	kfree(new_fdt);
> > +	kfree(overlay_tree);
> >  
> >  err_free_overlay_changeset:
> >  	free_overlay_changeset(ovcs);
> 

-- 
Slawomir Stepien