[PATCH 0/3] Do some cleanup with use of __free()

[PATCH 0/3] Do some cleanup with use of __free()

[Zhang Zekun]

__free() provides a scoped of_node_put() functionality to put the
device_node automatically. Let's simplify the code a bit with use of
__free().

Zhang Zekun (3):
  of: device: Do some clean up with use of __free()
  of: irq: Do some clean up with use of __free()
  of: property: Do some clean up with use of __free()

 drivers/of/device.c   |  7 +++----
 drivers/of/irq.c      | 15 +++++----------
 drivers/of/property.c | 28 ++++++++--------------------
 3 files changed, 16 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH 1/3] of: device: Do some clean up with use of __free()

[Zhang Zekun]

__free() provides a scoped of_node_put() functionality to put the
device_node automatically, and we don't need to call of_node_put()
directly. Let's simplify the code a bit with the use of __free().

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/of/device.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index edf3be197265..7a71ef2aa16e 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -35,7 +35,7 @@ EXPORT_SYMBOL(of_match_device);
 static void
 of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
 {
-	struct device_node *node, *of_node = dev->of_node;
+	struct device_node *of_node = dev->of_node;
 	int count, i;
 
 	if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
@@ -54,17 +54,16 @@ of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
 	}
 
 	for (i = 0; i < count; i++) {
-		node = of_parse_phandle(of_node, "memory-region", i);
+		struct device_node *node __free(device_node) =
+			of_parse_phandle(of_node, "memory-region", i);
 		/*
 		 * There might be multiple memory regions, but only one
 		 * restricted-dma-pool region is allowed.
 		 */
 		if (of_device_is_compatible(node, "restricted-dma-pool") &&
 		    of_device_is_available(node)) {
-			of_node_put(node);
 			break;
 		}
-		of_node_put(node);
 	}
 
 	/*
-- 
2.17.1

[PATCH 2/3] of: irq: Do some clean up with use of __free()

[Zhang Zekun]

__free() provides a scoped of_node_put() functionality to put the
device_node automatically, and we don't need to call of_node_put()
directly. Let's simplify the code a bit with the use of __free().

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/of/irq.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 36351ad6115e..3291f1ffea49 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -341,7 +341,7 @@ EXPORT_SYMBOL_GPL(of_irq_parse_raw);
  */
 int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_args *out_irq)
 {
-	struct device_node *p;
+	struct device_node *p __free(device_node) = NULL;
 	const __be32 *addr;
 	u32 intsize;
 	int i, res, addr_len;
@@ -374,10 +374,8 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 		return -EINVAL;
 
 	/* Get size of interrupt specifier */
-	if (of_property_read_u32(p, "#interrupt-cells", &intsize)) {
-		res = -EINVAL;
-		goto out;
-	}
+	if (of_property_read_u32(p, "#interrupt-cells", &intsize))
+		return -EINVAL;
 
 	pr_debug(" parent=%pOF, intsize=%d\n", p, intsize);
 
@@ -389,17 +387,14 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 						 (index * intsize) + i,
 						 out_irq->args + i);
 		if (res)
-			goto out;
+			return res;
 	}
 
 	pr_debug(" intspec=%d\n", *out_irq->args);
 
 
 	/* Check if there are any interrupt-map translations to process */
-	res = of_irq_parse_raw(addr_buf, out_irq);
- out:
-	of_node_put(p);
-	return res;
+	return of_irq_parse_raw(addr_buf, out_irq);
 }
 EXPORT_SYMBOL_GPL(of_irq_parse_one);
 
-- 
2.17.1

[PATCH 3/3] of: property: Do some clean up with use of __free()

[Zhang Zekun]

__free() provides a scoped of_node_put() functionality to put the
device_node automatically, and we don't need to call of_node_put()
directly. Let's simplify the code a bit with the use of __free().

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/of/property.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 164d77cb9445..940324225c34 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -773,16 +773,11 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
 struct device_node *of_graph_get_remote_port_parent(
 			       const struct device_node *node)
 {
-	struct device_node *np, *pp;
-
 	/* Get remote endpoint node. */
-	np = of_graph_get_remote_endpoint(node);
-
-	pp = of_graph_get_port_parent(np);
+	struct device_node *np __free(device_node) =
+		of_graph_get_remote_endpoint(node);
 
-	of_node_put(np);
-
-	return pp;
+	return of_graph_get_port_parent(np);
 }
 EXPORT_SYMBOL(of_graph_get_remote_port_parent);
 
@@ -1064,19 +1059,15 @@ static void of_link_to_phandle(struct device_node *con_np,
 			      struct device_node *sup_np,
 			      u8 flags)
 {
-	struct device_node *tmp_np = of_node_get(sup_np);
+	struct device_node *tmp_np __free(device_node) = of_node_get(sup_np);
 
 	/* Check that sup_np and its ancestors are available. */
 	while (tmp_np) {
-		if (of_fwnode_handle(tmp_np)->dev) {
-			of_node_put(tmp_np);
+		if (of_fwnode_handle(tmp_np)->dev)
 			break;
-		}
 
-		if (!of_device_is_available(tmp_np)) {
-			of_node_put(tmp_np);
+		if (!of_device_is_available(tmp_np))
 			return;
-		}
 
 		tmp_np = of_get_next_parent(tmp_np);
 	}
@@ -1440,16 +1431,13 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
 		}
 
 		while ((phandle = s->parse_prop(con_np, prop_name, i))) {
-			struct device_node *con_dev_np;
+			struct device_node *con_dev_np __free(device_node) =
+				s->get_con_dev ? s->get_con_dev(con_np) : of_node_get(con_np);
 
-			con_dev_np = s->get_con_dev
-					? s->get_con_dev(con_np)
-					: of_node_get(con_np);
 			matched = true;
 			i++;
 			of_link_to_phandle(con_dev_np, phandle, s->fwlink_flags);
 			of_node_put(phandle);
-			of_node_put(con_dev_np);
 		}
 		s++;
 	}
-- 
2.17.1

Re: [PATCH 2/3] of: irq: Do some clean up with use of __free()

[Rob Herring]

On Thu, Aug 29, 2024 at 9:19 PM Zhang Zekun <zhangzekun11@huawei.com> wrote:
>
> __free() provides a scoped of_node_put() functionality to put the
> device_node automatically, and we don't need to call of_node_put()
> directly. Let's simplify the code a bit with the use of __free().
>
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>  drivers/of/irq.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 36351ad6115e..3291f1ffea49 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -341,7 +341,7 @@ EXPORT_SYMBOL_GPL(of_irq_parse_raw);
>   */
>  int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_args *out_irq)
>  {
> -       struct device_node *p;
> +       struct device_node *p __free(device_node) = NULL;

It is desired that the declaration and (real) assignment be together.
See discussions when adding the device_node cleanup support.

Rob

Re: [PATCH 1/3] of: device: Do some clean up with use of __free()

[Rob Herring]

On Fri, Aug 30, 2024 at 10:06:24AM +0800, Zhang Zekun wrote:
> __free() provides a scoped of_node_put() functionality to put the
> device_node automatically, and we don't need to call of_node_put()
> directly. Let's simplify the code a bit with the use of __free().
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>  drivers/of/device.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index edf3be197265..7a71ef2aa16e 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -35,7 +35,7 @@ EXPORT_SYMBOL(of_match_device);
>  static void
>  of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
>  {
> -	struct device_node *node, *of_node = dev->of_node;
> +	struct device_node *of_node = dev->of_node;
>  	int count, i;
>  
>  	if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
> @@ -54,17 +54,16 @@ of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
>  	}
>  
>  	for (i = 0; i < count; i++) {
> -		node = of_parse_phandle(of_node, "memory-region", i);
> +		struct device_node *node __free(device_node) =
> +			of_parse_phandle(of_node, "memory-region", i);
>  		/*
>  		 * There might be multiple memory regions, but only one
>  		 * restricted-dma-pool region is allowed.
>  		 */
>  		if (of_device_is_compatible(node, "restricted-dma-pool") &&
>  		    of_device_is_available(node)) {
> -			of_node_put(node);
>  			break;
>  		}
> -		of_node_put(node);
>  	}

Actually, I'd re-write this function like this (untested):

static void
of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
{
	struct device_node *of_node = dev->of_node;
	struct of_phandle_iterator it;
	int rc, match = -1, i = 0;

	if (!IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL))
		return;

	/*
	 * If dev->of_node doesn't exist or doesn't contain memory-region, try
	 * the OF node having DMA configuration.
	 */
	if (!of_property_present(of_node, "memory-region"))
		of_node = np;

	of_for_each_phandle(&it, of_node, rc, "memory-region", NULL, 0) {
		/*
		 * There might be multiple memory regions, but only one
		 * restricted-dma-pool region is allowed.
		 */
		if ((match < 0) && of_device_is_compatible(it.node, "restricted-dma-pool") &&
		    of_device_is_available(it.node)) {
			match = i;
			if (of_reserved_mem_device_init_by_idx(dev, of_node, i))
				dev_warn(dev, "failed to initialise \"restricted-dma-pool\" memory node\n");
		}
		i++;
	}
}


of_parse_phandle() is implemented using of_for_each_phandle(), so every 
call to of_parse_phandle() is iterating i times.

Rob

Re: [PATCH 3/3] of: property: Do some clean up with use of __free()

[Krzysztof Kozlowski]

On Fri, Aug 30, 2024 at 10:06:26AM +0800, Zhang Zekun wrote:
> __free() provides a scoped of_node_put() functionality to put the
> device_node automatically, and we don't need to call of_node_put()
> directly. Let's simplify the code a bit with the use of __free().
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>  drivers/of/property.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 164d77cb9445..940324225c34 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -773,16 +773,11 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
>  struct device_node *of_graph_get_remote_port_parent(
>  			       const struct device_node *node)
>  {
> -	struct device_node *np, *pp;
> -
>  	/* Get remote endpoint node. */
> -	np = of_graph_get_remote_endpoint(node);
> -
> -	pp = of_graph_get_port_parent(np);
> +	struct device_node *np __free(device_node) =
> +		of_graph_get_remote_endpoint(node);
>  
> -	of_node_put(np);
> -
> -	return pp;
> +	return of_graph_get_port_parent(np);

Original code was obvious and simple. This is not an improvement.

>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port_parent);
>  
> @@ -1064,19 +1059,15 @@ static void of_link_to_phandle(struct device_node *con_np,
>  			      struct device_node *sup_np,
>  			      u8 flags)
>  {
> -	struct device_node *tmp_np = of_node_get(sup_np);
> +	struct device_node *tmp_np __free(device_node) = of_node_get(sup_np);
>  
>  	/* Check that sup_np and its ancestors are available. */
>  	while (tmp_np) {
> -		if (of_fwnode_handle(tmp_np)->dev) {
> -			of_node_put(tmp_np);
> +		if (of_fwnode_handle(tmp_np)->dev)
>  			break;
> -		}
>  
> -		if (!of_device_is_available(tmp_np)) {
> -			of_node_put(tmp_np);
> +		if (!of_device_is_available(tmp_np))
>  			return;
> -		}
>  
>  		tmp_np = of_get_next_parent(tmp_np);
>  	}
> @@ -1440,16 +1431,13 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
>  		}
>  
>  		while ((phandle = s->parse_prop(con_np, prop_name, i))) {
> -			struct device_node *con_dev_np;
> +			struct device_node *con_dev_np __free(device_node) =
> +				s->get_con_dev ? s->get_con_dev(con_np) : of_node_get(con_np);
>  
> -			con_dev_np = s->get_con_dev
> -					? s->get_con_dev(con_np)
> -					: of_node_get(con_np);
>  			matched = true;
>  			i++;
>  			of_link_to_phandle(con_dev_np, phandle, s->fwlink_flags);
>  			of_node_put(phandle);
> -			of_node_put(con_dev_np);

Neither is this.

Best regards,
Krzysztof

Re: [PATCH 3/3] of: property: Do some clean up with use of __free()

[Rob Herring (Arm)]

On Fri, 30 Aug 2024 10:06:26 +0800, Zhang Zekun wrote:
> __free() provides a scoped of_node_put() functionality to put the
> device_node automatically, and we don't need to call of_node_put()
> directly. Let's simplify the code a bit with the use of __free().
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>  drivers/of/property.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 

Applied, thanks!