Message-ID: <20240414175539.517d677f@jic23-huawei>
Date: Sun, 14 Apr 2024 17:55:39 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Rob Herring <robh@...nel.org>
Cc: Saravana Kannan <saravanak@...gle.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] of: Use scope based of_node_put() cleanups

On Tue, 09 Apr 2024 13:59:41 -0500
Rob Herring <robh@...nel.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@...gle.com>
> Signed-off-by: Rob Herring <robh@...nel.org>
> ---
> 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);
>  	struct of_bus *bus, *pbus;
>  	__be32 addr[OF_MAX_ADDR_CELLS];
>  	int na, ns, pna, pns;
> -	u64 result = OF_BAD_ADDR;
>  
>  	pr_debug("** translation for device %pOF **\n", dev);
>  
> -	/* Increase refcount at current level */
> -	of_node_get(dev);
> -
>  	*host = NULL;
> -	/* Get parent & match bus type */
> -	parent = get_parent(dev);
> +
>  	if (parent == NULL)
> -		goto bail;
> +		return OF_BAD_ADDR;
>  	bus = of_match_bus(parent);
>  
>  	/* Count address cells & copy address locally */
>  	bus->count_cells(dev, &na, &ns);
>  	if (!OF_CHECK_COUNTS(na, ns)) {
>  		pr_debug("Bad cell count for %pOF\n", dev);
> -		goto bail;
> +		return OF_BAD_ADDR;
>  	}
>  	memcpy(addr, in_addr, na * 4);
>  
> @@ -533,8 +529,7 @@ static u64 __of_translate_address(struct device_node *dev,
>  		/* If root, we have finished */
>  		if (parent == NULL) {
>  			pr_debug("reached root node\n");
> -			result = of_read_number(addr, na);
> -			break;
> +			return of_read_number(addr, na);
>  		}
>  
>  		/*
> @@ -543,11 +538,11 @@ static u64 __of_translate_address(struct device_node *dev,
>  		 */
>  		iorange = find_io_range_by_fwnode(&dev->fwnode);
>  		if (iorange && (iorange->flags != LOGIC_PIO_CPU_MMIO)) {
> -			result = of_read_number(addr + 1, na - 1);
> +			u64 result = of_read_number(addr + 1, na - 1);
>  			pr_debug("indirectIO matched(%pOF) 0x%llx\n",
>  				 dev, result);
> -			*host = of_node_get(dev);
> -			break;
> +			*host = no_free_ptr(dev);
> +			return result;
>  		}
>  
>  		/* Get new parent bus and counts */
> @@ -555,7 +550,7 @@ static u64 __of_translate_address(struct device_node *dev,
>  		pbus->count_cells(dev, &pna, &pns);
>  		if (!OF_CHECK_COUNTS(pna, pns)) {
>  			pr_err("Bad cell count for %pOF\n", dev);
> -			break;
> +			return OF_BAD_ADDR;
>  		}
>  
>  		pr_debug("parent bus is %s (na=%d, ns=%d) on %pOF\n",
> @@ -563,7 +558,7 @@ static u64 __of_translate_address(struct device_node *dev,
>  
>  		/* Apply bus translation */
>  		if (of_translate_one(dev, bus, pbus, addr, na, ns, pna, rprop))
> -			break;
> +			return OF_BAD_ADDR;
>  
>  		/* Complete the move up one level */
>  		na = pna;
> @@ -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;
>  }
>  
>  u64 of_translate_address(struct device_node *dev, const __be32 *in_addr)
> @@ -654,19 +646,16 @@ EXPORT_SYMBOL(of_translate_dma_address);
>  const __be32 *of_translate_dma_region(struct device_node *dev, const __be32 *prop,
>  				      phys_addr_t *start, size_t *length)
>  {
> -	struct device_node *parent;
> +	struct device_node *parent __free(device_node) = __of_get_dma_parent(dev);
>  	u64 address, size;
>  	int na, ns;
>  
> -	parent = __of_get_dma_parent(dev);
>  	if (!parent)
>  		return NULL;
>  
>  	na = of_bus_n_addr_cells(parent);
>  	ns = of_bus_n_size_cells(parent);
>  
> -	of_node_put(parent);
> -
>  	address = of_translate_dma_address(dev, prop);
>  	if (address == OF_BAD_ADDR)
>  		return NULL;
> @@ -688,21 +677,19 @@ const __be32 *__of_get_address(struct device_node *dev, int index, int bar_no,
>  {
>  	const __be32 *prop;
>  	unsigned int psize;
> -	struct device_node *parent;
> +	struct device_node *parent __free(device_node) = of_get_parent(dev);
>  	struct of_bus *bus;
>  	int onesize, i, na, ns;
>  
> -	/* Get parent & match bus type */
> -	parent = of_get_parent(dev);
>  	if (parent == NULL)
>  		return NULL;
> +
> +	/* match the parent's bus type */
>  	bus = of_match_bus(parent);
> -	if (strcmp(bus->name, "pci") && (bar_no >= 0)) {
> -		of_node_put(parent);
> +	if (strcmp(bus->name, "pci") && (bar_no >= 0))
>  		return NULL;
> -	}
> +
>  	bus->count_cells(dev, &na, &ns);
> -	of_node_put(parent);
>  	if (!OF_CHECK_ADDR_COUNT(na))
>  		return NULL;
>  
> @@ -888,14 +875,13 @@ static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
>   */
>  int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
>  {
> -	struct device_node *node = of_node_get(np);
> +	struct device_node *node __free(device_node) = of_node_get(np);
>  	const __be32 *ranges = NULL;
>  	bool found_dma_ranges = false;
>  	struct of_range_parser parser;
>  	struct of_range range;
>  	struct bus_dma_region *r;
>  	int len, num_ranges = 0;
> -	int ret = 0;
>  
>  	while (node) {
>  		ranges = of_get_property(node, "dma-ranges", &len);
> @@ -905,10 +891,9 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
>  			break;
>  
>  		/* Once we find 'dma-ranges', then a missing one is an error */
> -		if (found_dma_ranges && !ranges) {
> -			ret = -ENODEV;
> -			goto out;
> -		}
> +		if (found_dma_ranges && !ranges)
> +			return -ENODEV;
> +
>  		found_dma_ranges = true;
>  
>  		node = of_get_next_dma_parent(node);
> @@ -916,10 +901,8 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
>  
>  	if (!node || !ranges) {
>  		pr_debug("no dma-ranges found for node(%pOF)\n", np);
> -		ret = -ENODEV;
> -		goto out;
> +		return -ENODEV;
>  	}
> -
>  	of_dma_range_parser_init(&parser, node);
>  	for_each_of_range(&parser, &range) {
>  		if (range.cpu_addr == OF_BAD_ADDR) {
> @@ -930,16 +913,12 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
>  		num_ranges++;
>  	}
>  
> -	if (!num_ranges) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (!num_ranges)
> +		return -EINVAL;
>  
>  	r = kcalloc(num_ranges + 1, sizeof(*r), GFP_KERNEL);
> -	if (!r) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (!r)
> +		return -ENOMEM;
>  
>  	/*
>  	 * Record all info in the generic DMA ranges array for struct device,
> @@ -957,9 +936,7 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
>  		r->size = range.size;
>  		r++;
>  	}
> -out:
> -	of_node_put(node);
> -	return ret;
> +	return 0;
>  }
>  #endif /* CONFIG_HAS_DMA */
>  
> @@ -1016,24 +993,18 @@ 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;
> -	bool is_coherent = dma_default_coherent;
> -
> -	node = of_node_get(np);
> +	struct device_node *node __free(device_node) = of_node_get(np);
>  
>  	while (node) {
> -		if (of_property_read_bool(node, "dma-coherent")) {
> -			is_coherent = true;
> -			break;
> -		}
> -		if (of_property_read_bool(node, "dma-noncoherent")) {
> -			is_coherent = false;
> -			break;
> -		}
> +		if (of_property_read_bool(node, "dma-coherent"))
> +			return true;
> +
> +		if (of_property_read_bool(node, "dma-noncoherent"))
> +			return false;
> +
>  		node = of_get_next_dma_parent(node);
>  	}
> -	of_node_put(node);
> -	return is_coherent;
> +	return dma_default_coherent;
>  }
>  EXPORT_SYMBOL_GPL(of_dma_is_coherent);
>  
> @@ -1049,20 +1020,14 @@ 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(parent, "nonposted-mmio");
>  }
>  
>  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
> @@ -40,15 +40,12 @@
>   */
>  bool of_graph_is_present(const struct device_node *node)
>  {
> -	struct device_node *ports, *port;
> +	struct device_node *ports __free(device_node) = of_get_child_by_name(node, "ports");
>  
> -	ports = of_get_child_by_name(node, "ports");
>  	if (ports)
>  		node = ports;
>  
> -	port = of_get_child_by_name(node, "port");
> -	of_node_put(ports);
> -	of_node_put(port);
> +	struct device_node *port __free(device_node) = of_get_child_by_name(node, "port");
>  
>  	return !!port;
>  }
> @@ -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;
Trivial but you could use
			return port;
	}

	return NULL;

which I think would make the flow a tiny bit more obvious to anyone who doesn't
know that for_each_child_of_node() leaves it set to NULL on exit.

Maybe use the scoped form a a return_ptr() to make it obvious you are intentionally
holding on to the port node on exit?


>  	}
>  
> -	of_node_put(node);
> -
>  	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);
Completely trivial but I'd drop the blank line to bring the error check
up into same block of code as the line that gets what it is checking.

>  
>  		if (!port) {
>  			pr_debug("graph: no port node found in %pOF\n", parent);


> 

Powered by blists - more mailing lists