Received-SPF: pass (google.com: domain of linux-kernel+bounces-144303-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 14 Apr 2024 18:11:19 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Rob Herring <robh@kernel.org>
Cc: Saravana Kannan <saravanak@google.com>, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/3] of: Use scope based of_node_put() cleanups
Message-ID: <20240414181119.32246670@jic23-huawei>
In-Reply-To: <20240409-dt-cleanup-free-v2-3-5b419a4af38d@kernel.org>
References: <20240409-dt-cleanup-free-v2-0-5b419a4af38d@kernel.org>
	<20240409-dt-cleanup-free-v2-3-5b419a4af38d@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 09 Apr 2024 13:59:41 -0500
Rob Herring <robh@kernel.org> wrote:

> Use the relatively new scope based of_node_put() cleanup to simplify
> function exit handling. Doing so reduces the chances of forgetting an
> of_node_put() and simplifies error paths by avoiding the need for goto
> statements.
> 
> Reviewed-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
Hi Rob,

A few follow up comments, but they are all in the 'you could' category.
Either way.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> v2:
>  - Also use cleanup for 'dev' in __of_translate_address()
>  - Further simplify of_dma_is_coherent() and of_mmio_is_nonposted()
> ---
>  drivers/of/address.c  | 113 +++++++++++++++++---------------------------------
>  drivers/of/property.c |  22 ++++------
>  2 files changed, 46 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index ae46a3605904..c350185ceaeb 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -486,34 +486,30 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>   * device that had registered logical PIO mapping, and the return code is
>   * relative to that node.
>   */
> -static u64 __of_translate_address(struct device_node *dev,
> +static u64 __of_translate_address(struct device_node *node,
>  				  struct device_node *(*get_parent)(const struct device_node *),
>  				  const __be32 *in_addr, const char *rprop,
>  				  struct device_node **host)
>  {
> -	struct device_node *parent = NULL;
> +	struct device_node *dev __free(device_node) = of_node_get(node);
> +	struct device_node *parent __free(device_node) = get_parent(dev);
..

> @@ -572,11 +567,8 @@ static u64 __of_translate_address(struct device_node *dev,
>  
>  		of_dump_addr("one level translation:", addr, na);
>  	}
> - bail:
> -	of_node_put(parent);
> -	of_node_put(dev);
>  
> -	return result;
> +	return OF_BAD_ADDR;

I think this is unreachable as there is no way to exist the loop without returning.
If so (and the compiler complains if you remove this) you could mark it as such
with unreachable();


>  }


>  static int __of_address_to_resource(struct device_node *dev, int index, int bar_no,
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index a6358ee99b74..b73daf81c99d 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c


> @@ -610,9 +607,9 @@ EXPORT_SYMBOL(of_graph_parse_endpoint);
>   */
>  struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id)
>  {
> -	struct device_node *node, *port;
> +	struct device_node *port;
> +	struct device_node *node __free(device_node) = of_get_child_by_name(parent, "ports");
>  
> -	node = of_get_child_by_name(parent, "ports");
>  	if (node)
>  		parent = node;
>  
> @@ -626,8 +623,6 @@ struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id)
>  			break;
>  	}
>  
> -	of_node_put(node);
> -

You 'could' (feel free to ignore) make this perhaps more obvious wiht.

	for_each_child_of_node_scoped(parent, port) {
		u32 port_id = 0;

		if (!of_node_name_eq(port, "port"))
			continue;
		of_property_read_u32(port, "reg", &port_id);
		if (id == port_id)
			return_ptr(port);
	}
	
	return NULL;

Makes it explicit you are holding a reference on exit if you go vial
the return_ptr() route and that if you don't then in all cases the return value
will be NULL.

>  	return port;
>  }
>  EXPORT_SYMBOL(of_graph_get_port_by_id);
> @@ -655,14 +650,13 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>  	 * parent port node.
>  	 */
>  	if (!prev) {
> -		struct device_node *node;
> +		struct device_node *node __free(device_node) =
> +			of_get_child_by_name(parent, "ports");
>  
> -		node = of_get_child_by_name(parent, "ports");
>  		if (node)
>  			parent = node;
>  
>  		port = of_get_child_by_name(parent, "port");
> -		of_node_put(node);
Very trivial but I'd drop the blank line here to more closely associate the check
with retrieval of the port.
>  
>  		if (!port) {
>  			pr_debug("graph: no port node found in %pOF\n", parent);

>