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

From: Saravana Kannan
Date: Thu Apr 04 2024 - 19:22:32 EST

Next message: Thorsten Blum: "[PATCH] x86/apic: Use max() to fix Coccinelle warning"
Previous message: Andrew Jeffery: "Re: [PATCH 1/4] ARM: dts: aspeed: greatlakes: correct Mellanox multi-host property"
In reply to: Rob Herring: "[PATCH 3/3] of: Use scope based of_node_put() cleanups"
Next in thread: Rob Herring: "Re: [PATCH 3/3] of: Use scope based of_node_put() cleanups"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Apr 4, 2024 at 7:15 AM Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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);

Can we leave the variable definition where it was? We generally define
all the variables up top. So, defining the one variable in the middle
feels weird. I at least get when we do this inside for/if blocks. But
randomly in the middle feels weird.

Similar comments in other places. Since both kfree() and of_put() can
both handle NULL pointers, I'd be surprised if we HAVE to combine
these lines.

Not a very strong position, but I'd rather we didn't. So,

Reviewed-by: Saravana Kannan <saravanak@xxxxxxxxxx>

-Saravana

> 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);
>
> return result;
> @@ -654,19 +652,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 +683,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 +881,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 +897,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 +907,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 +919,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 +942,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,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;
> @@ -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;
> }
> 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;
> }
>
> 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;
> }
>
> - 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);
>
> if (!port) {
> pr_debug("graph: no port node found in %pOF\n", parent);
> @@ -1052,15 +1046,13 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> struct fwnode_endpoint *endpoint)
> {
> const struct device_node *node = to_of_node(fwnode);
> - struct device_node *port_node = of_get_parent(node);
> + struct device_node *port_node __free(device_node) = of_get_parent(node);
>
> endpoint->local_fwnode = fwnode;
>
> of_property_read_u32(port_node, "reg", &endpoint->port);
> of_property_read_u32(node, "reg", &endpoint->id);
>
> - of_node_put(port_node);
> -
> return 0;
> }
>
>
> --
> 2.43.0
>

Next message: Thorsten Blum: "[PATCH] x86/apic: Use max() to fix Coccinelle warning"
Previous message: Andrew Jeffery: "Re: [PATCH 1/4] ARM: dts: aspeed: greatlakes: correct Mellanox multi-host property"
In reply to: Rob Herring: "[PATCH 3/3] of: Use scope based of_node_put() cleanups"
Next in thread: Rob Herring: "Re: [PATCH 3/3] of: Use scope based of_node_put() cleanups"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]