Re: [PATCH 3/3] of: Use scope based of_node_put() cleanups

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 04 Apr 2024 09:15:12 -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.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

Hi Rob,

Looks good in general. A few suggestions inline.
First one is a little more complex, but I think will give you a better end result

The others are readability improvements enabled by the changes you've made
that I think you could validly change in this same patch.

Jonathan

> ---
>  drivers/of/address.c  | 60 ++++++++++++++++-----------------------------------
>  drivers/of/property.c | 22 ++++++-------------
>  2 files changed, 26 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index ae46a3605904..f7b2d535a6d1 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -491,7 +491,6 @@ static u64 __of_translate_address(struct device_node *dev,
>  				  const __be32 *in_addr, const char *rprop,
>  				  struct device_node **host)
>  {
> -	struct device_node *parent = NULL;
>  	struct of_bus *bus, *pbus;
>  	__be32 addr[OF_MAX_ADDR_CELLS];
>  	int na, ns, pna, pns;
> @@ -504,7 +503,7 @@ static u64 __of_translate_address(struct device_node *dev,
>  
>  	*host = NULL;
>  	/* Get parent & match bus type */
> -	parent = get_parent(dev);
> +	struct device_node *parent __free(device_node) = get_parent(dev);

This is an order change as we'll put this node after dev

Whilst probably fine, I'd call that out in the patch description or just throw
in an earlier
struct device_node *dev_node __free(device_node) = of_node_get(dev);


Obviously the release order doesn't actually matter but a reader has to think
about it briefly to be sure.

Bonus if you do that is that you get to do direct returns at the error condition
and potentially in the breaks out of the loop. To my eye that's a readability
advantage.

In at least one place you could also steal the pointer rather than
getting another reference to dev (taking it to at least 2) then dropping
one of them in the exit path. Can use no_free_ptr() for that.

From readability point of view you'd only want to do that with a direct return.
 		
			*host = no_free_ptr(dev_node);
			return result;
		}




>  	if (parent == NULL)
>  		goto bail;
>  	bus = of_match_bus(parent);
> @@ -573,7 +572,6 @@ 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);
>  


> @@ -1016,11 +999,9 @@ phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
>   */
>  bool of_dma_is_coherent(struct device_node *np)
>  {
> -	struct device_node *node;
> +	struct device_node *node __free(device_node) = of_node_get(np);
>  	bool is_coherent = dma_default_coherent;
>  
> -	node = of_node_get(np);
> -
>  	while (node) {
>  		if (of_property_read_bool(node, "dma-coherent")) {
>  			is_coherent = true;

			return true;

and same in the other branch given you break out the loop and return it directly.


> @@ -1032,7 +1013,6 @@ bool of_dma_is_coherent(struct device_node *np)
>  		}
>  		node = of_get_next_dma_parent(node);
>  	}
> -	of_node_put(node);
>  	return is_coherent;
With above early returns

	return dma_default_coherent;

and drop the is_coherent variable as no longer used.

This sort of related cleanup is one reason I really like the cleanup.h magic.


>  }
>  EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> @@ -1049,19 +1029,17 @@ EXPORT_SYMBOL_GPL(of_dma_is_coherent);
>   */
>  static bool of_mmio_is_nonposted(struct device_node *np)
>  {
> -	struct device_node *parent;
>  	bool nonposted;
>  
>  	if (!IS_ENABLED(CONFIG_ARCH_APPLE))
>  		return false;
>  
> -	parent = of_get_parent(np);
> +	struct device_node *parent __free(device_node) = of_get_parent(np);
>  	if (!parent)
>  		return false;
>  
>  	nonposted = of_property_read_bool(parent, "nonposted-mmio");
>  
> -	of_node_put(parent);
>  	return nonposted;

	return of_property_read_bool()

>  }
>