mbox series

[RFC,0/5] of: automate of_node_put() - new approach to loops.

Message ID 20240128160542.178315-1-jic23@kernel.org (mailing list archive)
Headers show
Series of: automate of_node_put() - new approach to loops. | expand

Message

Jonathan Cameron Jan. 28, 2024, 4:05 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

+CC includes peopleinterested in property.h equivalents to minimize
duplication of discussion.  Outcome of this discussion will affect:
https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
[PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.

In discussion of previous approach with Rob Herring we talked about various
ways to avoid a disconnect between the declaration of the __free(device_node)
and the first non NULL assignment. Making this connection clear is useful for 2
reasons:
1) Avoids out of order cleanup with respect to other cleanup.h usage.
2) Avoids disconnect between how cleanup is to be done and how the reference
   was acquired in the first place.

https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/

The options we discussed are:

1) Ignore this issue and merge original set.

2) Always put the declaration just before the for loop and don't set it NULL.

{
	int ret;

	ret = ... and other fun code.

	struct device_node *child __free(device_node);
	for_each_child_of_node(np, child) {
	}
}

This works but careful review is needed to ensure that this unusual pattern is
followed.  We don't set it to NULL as the loop will do that anyway if there are
no child nodes, or the loop finishes without an early break or return.

3) Introduced the pointer to auto put device_node only within the
   for loop scope.

+#define for_each_child_of_node_scoped(parent, child) \
+	for (struct device_node *child __free(device_node) =		\
+	     of_get_next_child(parent, NULL);				\
+	     child != NULL;						\
+	     child = of_get_next_available_child(parent, child))
+

This series is presenting option 3.  I only implemented this loop out of
all the similar ones and it is only compile tested.

Disadvantage Rob raised is that it isn't obvious this macro will instantiate
a struct device_node *child.  I can't see a way around that other than option 2
above, but all suggestions welcome.  Note that if a conversion leaves an
'external' struct device_node *child variable, in many cases the compiler
will catch that as an unused variable. We don't currently run shaddow
variable detection in normal kernel builds, but that could also be used
to catch such bugs.

All comments welcome.

Jonathan Cameron (5):
  of: Add cleanup.h based auto release via __free(device_node) markings.
  of: Introduce for_each_child_of_node_scoped() to automate
    of_node_put() handling
  of: unittest: Use __free(device_node)
  iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped()
  iio: adc: rcar-gyroadc: use for_each_child_node_scoped()

 drivers/iio/adc/fsl-imx25-gcq.c | 13 +++----------
 drivers/iio/adc/rcar-gyroadc.c  | 21 ++++++---------------
 drivers/of/unittest.c           | 11 +++--------
 include/linux/of.h              |  8 ++++++++
 4 files changed, 20 insertions(+), 33 deletions(-)

Comments

Julia Lawall Jan. 28, 2024, 6:06 p.m. UTC | #1 
On Sun, 28 Jan 2024, Jonathan Cameron wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> +CC includes peopleinterested in property.h equivalents to minimize
> duplication of discussion.  Outcome of this discussion will affect:
> https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
>
> In discussion of previous approach with Rob Herring we talked about various
> ways to avoid a disconnect between the declaration of the __free(device_node)
> and the first non NULL assignment. Making this connection clear is useful for 2
> reasons:
> 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> 2) Avoids disconnect between how cleanup is to be done and how the reference
>    was acquired in the first place.
>
> https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
>
> The options we discussed are:
>
> 1) Ignore this issue and merge original set.
>
> 2) Always put the declaration just before the for loop and don't set it NULL.
>
> {
> 	int ret;
>
> 	ret = ... and other fun code.
>
> 	struct device_node *child __free(device_node);
> 	for_each_child_of_node(np, child) {
> 	}
> }
>
> This works but careful review is needed to ensure that this unusual pattern is
> followed.  We don't set it to NULL as the loop will do that anyway if there are
> no child nodes, or the loop finishes without an early break or return.
>
> 3) Introduced the pointer to auto put device_node only within the
>    for loop scope.
>
> +#define for_each_child_of_node_scoped(parent, child) \
> +	for (struct device_node *child __free(device_node) =		\
> +	     of_get_next_child(parent, NULL);				\
> +	     child != NULL;						\
> +	     child = of_get_next_available_child(parent, child))
> +
>
> This series is presenting option 3.  I only implemented this loop out of
> all the similar ones and it is only compile tested.
>
> Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> a struct device_node *child.  I can't see a way around that other than option 2
> above, but all suggestions welcome.  Note that if a conversion leaves an
> 'external' struct device_node *child variable, in many cases the compiler
> will catch that as an unused variable. We don't currently run shaddow
> variable detection in normal kernel builds, but that could also be used
> to catch such bugs.
>
> All comments welcome.

It looks promising to get rid of a lot of clunky and error-prone
error-handling code.

I guess that

for_each_child_of_node_scoped(parent, struct device_node *, child)

would seem too verbose?

There are a lot of opportunities for device_node loops, but also for some
more obscure loops over other types.  And there are a lot of of_node_puts
that could be eliminated independent of loops.

julia

>
> Jonathan Cameron (5):
>   of: Add cleanup.h based auto release via __free(device_node) markings.
>   of: Introduce for_each_child_of_node_scoped() to automate
>     of_node_put() handling
>   of: unittest: Use __free(device_node)
>   iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped()
>   iio: adc: rcar-gyroadc: use for_each_child_node_scoped()
>
>  drivers/iio/adc/fsl-imx25-gcq.c | 13 +++----------
>  drivers/iio/adc/rcar-gyroadc.c  | 21 ++++++---------------
>  drivers/of/unittest.c           | 11 +++--------
>  include/linux/of.h              |  8 ++++++++
>  4 files changed, 20 insertions(+), 33 deletions(-)
>
> --
> 2.43.0
>
>
Jonathan Cameron Jan. 29, 2024, 11:42 a.m. UTC | #2 
On Sun, 28 Jan 2024 19:06:53 +0100 (CET)
Julia Lawall <julia.lawall@inria.fr> wrote:

> On Sun, 28 Jan 2024, Jonathan Cameron wrote:
> 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > +CC includes peopleinterested in property.h equivalents to minimize
> > duplication of discussion.  Outcome of this discussion will affect:
> > https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> >
> > In discussion of previous approach with Rob Herring we talked about various
> > ways to avoid a disconnect between the declaration of the __free(device_node)
> > and the first non NULL assignment. Making this connection clear is useful for 2
> > reasons:
> > 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> > 2) Avoids disconnect between how cleanup is to be done and how the reference
> >    was acquired in the first place.
> >
> > https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> >
> > The options we discussed are:
> >
> > 1) Ignore this issue and merge original set.
> >
> > 2) Always put the declaration just before the for loop and don't set it NULL.
> >
> > {
> > 	int ret;
> >
> > 	ret = ... and other fun code.
> >
> > 	struct device_node *child __free(device_node);
> > 	for_each_child_of_node(np, child) {
> > 	}
> > }
> >
> > This works but careful review is needed to ensure that this unusual pattern is
> > followed.  We don't set it to NULL as the loop will do that anyway if there are
> > no child nodes, or the loop finishes without an early break or return.
> >
> > 3) Introduced the pointer to auto put device_node only within the
> >    for loop scope.
> >
> > +#define for_each_child_of_node_scoped(parent, child) \
> > +	for (struct device_node *child __free(device_node) =		\
> > +	     of_get_next_child(parent, NULL);				\
> > +	     child != NULL;						\
> > +	     child = of_get_next_available_child(parent, child))
> > +
> >
> > This series is presenting option 3.  I only implemented this loop out of
> > all the similar ones and it is only compile tested.
> >
> > Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> > a struct device_node *child.  I can't see a way around that other than option 2
> > above, but all suggestions welcome.  Note that if a conversion leaves an
> > 'external' struct device_node *child variable, in many cases the compiler
> > will catch that as an unused variable. We don't currently run shaddow
> > variable detection in normal kernel builds, but that could also be used
> > to catch such bugs.
> >
> > All comments welcome.  
> 
> It looks promising to get rid of a lot of clunky and error-prone
> error-handling code.

Absolutely. I think I spotted 2 bugs whilst just looking for places this pattern
doesn't apply.  Will circle back to those once this discussion is resolved.
I think I've taken dozen's of fixes for cases where these were missed over the years
so hoping this means I'll never see another one!

> 
> I guess that
> 
> for_each_child_of_node_scoped(parent, struct device_node *, child)
> 
> would seem too verbose?

Intent just to make the allocated internal type clear?  Sure we can do that if
it helps with making it clear something is being allocated.
I can't think of a way this could be used with anything other than
a struct device_node * as the second parameter but I guess it still helps
'hint' at what is going on..

> 
> There are a lot of opportunities for device_node loops, but also for some
> more obscure loops over other types.  

> And there are a lot of of_node_puts
> that could be eliminated independent of loops.

The non loop cases should be handled via the __free(device_node) as provided
by patch 1.  We'll need to keep the declaration local and initial assignment
together but that is easy enough to do and similar to the many other cleanup.h
usecases that are surfacing.

Jonathan

> 
> julia
> 
> >
> > Jonathan Cameron (5):
> >   of: Add cleanup.h based auto release via __free(device_node) markings.
> >   of: Introduce for_each_child_of_node_scoped() to automate
> >     of_node_put() handling
> >   of: unittest: Use __free(device_node)
> >   iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped()
> >   iio: adc: rcar-gyroadc: use for_each_child_node_scoped()
> >
> >  drivers/iio/adc/fsl-imx25-gcq.c | 13 +++----------
> >  drivers/iio/adc/rcar-gyroadc.c  | 21 ++++++---------------
> >  drivers/of/unittest.c           | 11 +++--------
> >  include/linux/of.h              |  8 ++++++++
> >  4 files changed, 20 insertions(+), 33 deletions(-)
> >
> > --
> > 2.43.0
> >
> >
Julia Lawall Jan. 29, 2024, 2:02 p.m. UTC | #3 
On Mon, 29 Jan 2024, Jonathan Cameron wrote:

> On Sun, 28 Jan 2024 19:06:53 +0100 (CET)
> Julia Lawall <julia.lawall@inria.fr> wrote:
>
> > On Sun, 28 Jan 2024, Jonathan Cameron wrote:
> >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > +CC includes peopleinterested in property.h equivalents to minimize
> > > duplication of discussion.  Outcome of this discussion will affect:
> > > https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> > > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> > >
> > > In discussion of previous approach with Rob Herring we talked about various
> > > ways to avoid a disconnect between the declaration of the __free(device_node)
> > > and the first non NULL assignment. Making this connection clear is useful for 2
> > > reasons:
> > > 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> > > 2) Avoids disconnect between how cleanup is to be done and how the reference
> > >    was acquired in the first place.
> > >
> > > https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> > >
> > > The options we discussed are:
> > >
> > > 1) Ignore this issue and merge original set.
> > >
> > > 2) Always put the declaration just before the for loop and don't set it NULL.
> > >
> > > {
> > > 	int ret;
> > >
> > > 	ret = ... and other fun code.
> > >
> > > 	struct device_node *child __free(device_node);
> > > 	for_each_child_of_node(np, child) {
> > > 	}
> > > }
> > >
> > > This works but careful review is needed to ensure that this unusual pattern is
> > > followed.  We don't set it to NULL as the loop will do that anyway if there are
> > > no child nodes, or the loop finishes without an early break or return.
> > >
> > > 3) Introduced the pointer to auto put device_node only within the
> > >    for loop scope.
> > >
> > > +#define for_each_child_of_node_scoped(parent, child) \
> > > +	for (struct device_node *child __free(device_node) =		\
> > > +	     of_get_next_child(parent, NULL);				\
> > > +	     child != NULL;						\
> > > +	     child = of_get_next_available_child(parent, child))
> > > +
> > >
> > > This series is presenting option 3.  I only implemented this loop out of
> > > all the similar ones and it is only compile tested.
> > >
> > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> > > a struct device_node *child.  I can't see a way around that other than option 2
> > > above, but all suggestions welcome.  Note that if a conversion leaves an
> > > 'external' struct device_node *child variable, in many cases the compiler
> > > will catch that as an unused variable. We don't currently run shaddow
> > > variable detection in normal kernel builds, but that could also be used
> > > to catch such bugs.
> > >
> > > All comments welcome.
> >
> > It looks promising to get rid of a lot of clunky and error-prone
> > error-handling code.
>
> Absolutely. I think I spotted 2 bugs whilst just looking for places this pattern
> doesn't apply.  Will circle back to those once this discussion is resolved.
> I think I've taken dozen's of fixes for cases where these were missed over the years
> so hoping this means I'll never see another one!
>
> >
> > I guess that
> >
> > for_each_child_of_node_scoped(parent, struct device_node *, child)
> >
> > would seem too verbose?
>
> Intent just to make the allocated internal type clear?  Sure we can do that if
> it helps with making it clear something is being allocated.
> I can't think of a way this could be used with anything other than
> a struct device_node * as the second parameter but I guess it still helps
> 'hint' at what is going on..
>
> >
> > There are a lot of opportunities for device_node loops, but also for some
> > more obscure loops over other types.
>
> > And there are a lot of of_node_puts
> > that could be eliminated independent of loops.
>
> The non loop cases should be handled via the __free(device_node) as provided
> by patch 1.  We'll need to keep the declaration local and initial assignment
> together but that is easy enough to do and similar to the many other cleanup.h
> usecases that are surfacing.

I tried the following semantic patch:

@@
identifier x,f;
attribute name __free;
expression list es;
expression e;
statement S;
@@

{
... when != S
struct device_node *x
+ __free(device_node)
;
... when strict
x = f(es);
<... when any
     when != x = e
-of_node_put(x);
...>
-of_node_put(x);
}

It is written defensively in various ways:

when != S means tha tthe device_node declaration has t be at the top of
the block, perhaps with other declarations before it.

when strict means that all paths must lead from the declaration to the
initialization.  So there is no need to intiialize the variable to NULL,
as far as I understand.

when != x = e means that the declared variable is not reinitialized, which
would require keeping the previous of_node_put.

There is an of_node_put at the end of the block, so the use of __free
doesn't change the lifetime.

An unfortunate aspect of the last constraint is that some functions may
declare multiple device_node variables, and only one of the of_not_puts
can come at the very end of the block.  This can be solved by just running
the semantic patch again.

An alternative would be to move the initialization up to the declaration,
but the result was often a bit ugly, due to the various initialization
function calls having long names and argument lists.

The output is below.  I have looked quickly through all of the changes and
they all look reasonable, but have not tried compiling anything (which I
guess wouldn't currently work anyway).

julia

diff -u -p a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -4973,7 +4973,7 @@ static int dsi_probe(struct platform_dev

 	if (dsi->data->model == DSI_MODEL_OMAP4 ||
 	    dsi->data->model == DSI_MODEL_OMAP5) {
-		struct device_node *np;
+		struct device_node *np __free(device_node);

 		/*
 		 * The OMAP4/5 display DT bindings don't reference the padconf
@@ -4986,7 +4986,6 @@ static int dsi_probe(struct platform_dev
 			return -ENODEV;

 		dsi->syscon = syscon_node_to_regmap(np);
-		of_node_put(np);
 	}

 	/* DSI VCs initialization */
diff -u -p a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
--- a/arch/powerpc/sysdev/xics/xics-common.c
+++ b/arch/powerpc/sysdev/xics/xics-common.c
@@ -43,7 +43,7 @@ static struct ics *xics_ics;
 void xics_update_irq_servers(void)
 {
 	int i, j;
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	u32 ilen;
 	const __be32 *ireg;
 	u32 hcpuid;
@@ -59,7 +59,6 @@ void xics_update_irq_servers(void)

 	ireg = of_get_property(np, "ibm,ppc-interrupt-gserver#s", &ilen);
 	if (!ireg) {
-		of_node_put(np);
 		return;
 	}

@@ -78,7 +77,6 @@ void xics_update_irq_servers(void)
 	}
 	pr_devel("xics: xics_default_distrib_server = 0x%x\n",
 		 xics_default_distrib_server);
-	of_node_put(np);
 }

 /* GIQ stuff, currently only supported on RTAS setups, will have
@@ -485,7 +483,7 @@ void __init xics_register_ics(struct ics

 static void __init xics_get_server_size(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	const __be32 *isize;

 	/* We fetch the interrupt server size from the first ICS node
@@ -498,8 +496,6 @@ static void __init xics_get_server_size(
 	isize = of_get_property(np, "ibm,interrupt-server#-size", NULL);
 	if (isize)
 		xics_interrupt_server_size = be32_to_cpu(*isize);
-
-	of_node_put(np);
 }

 void __init xics_init(void)
diff -u -p a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -146,12 +146,11 @@ static const struct of_device_id exynos_

 static void exynos_map_pmu(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	np = of_find_matching_node(NULL, exynos_dt_pmu_match);
 	if (np)
 		pmu_base_addr = of_iomap(np, 0);
-	of_node_put(np);
 }

 static void __init exynos_init_irq(void)
diff -u -p a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -236,12 +236,11 @@ SYSCALL_DEFINE3(pciconfig_iobase, long,
 	 * (bus 0 is HT root), we return the AGP one instead.
 	 */
 	if (in_bus == 0 && of_machine_is_compatible("MacRISC4")) {
-		struct device_node *agp;
+		struct device_node *agp __free(device_node);

 		agp = of_find_compatible_node(NULL, NULL, "u3-agp");
 		if (agp)
 			in_bus = 0xf0;
-		of_node_put(agp);
 	}

 	/* That syscall isn't quite compatible with PCI domains, but it's
diff -u -p a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
@@ -25,7 +25,7 @@ omapdss_of_get_next_port(const struct de
 		return NULL;

 	if (!prev) {
-		struct device_node *ports;
+		struct device_node *ports __free(device_node);
 		/*
 		 * It's the first call, we have to find a port subnode
 		 * within this node or within an optional 'ports' node.
@@ -35,11 +35,8 @@ omapdss_of_get_next_port(const struct de
 			parent = ports;

 		port = of_get_child_by_name(parent, "port");
-
-		/* release the 'ports' node */
-		of_node_put(ports);
 	} else {
-		struct device_node *ports;
+		struct device_node *ports __free(device_node);

 		ports = of_get_parent(prev);
 		if (!ports)
@@ -48,13 +45,10 @@ omapdss_of_get_next_port(const struct de
 		do {
 			port = of_get_next_child(ports, prev);
 			if (!port) {
-				of_node_put(ports);
 				return NULL;
 			}
 			prev = port;
 		} while (!of_node_name_eq(port, "port"));
-
-		of_node_put(ports);
 	}

 	return port;
diff -u -p a/drivers/opp/of.c b/drivers/opp/of.c
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -163,7 +163,7 @@ static void _opp_table_alloc_required_ta
 					     struct device_node *opp_np)
 {
 	struct opp_table **required_opp_tables;
-	struct device_node *required_np, *np;
+	struct device_node *required_np, *np __free(device_node);
 	bool lazy = false;
 	int count, i, size;

@@ -216,7 +216,6 @@ static void _opp_table_alloc_required_ta
 free_required_tables:
 	_opp_table_free_required_tables(opp_table);
 put_np:
-	of_node_put(np);
 }

 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
diff -u -p a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -253,7 +253,7 @@ static enum ofdrm_model display_get_mode
 		   of_device_is_compatible(of_node, "pci1014,21c")) {
 		model = OFDRM_MODEL_GXT2000;
 	} else if (of_node_name_prefix(of_node, "vga,Display-")) {
-		struct device_node *of_parent;
+		struct device_node *of_parent __free(device_node);
 		const __be32 *vendor_p, *device_p;

 		/* Look for AVIVO initialized by SLOF */
@@ -267,7 +267,6 @@ static enum ofdrm_model display_get_mode
 			if (is_avivo(vendor, device))
 				model = OFDRM_MODEL_AVIVO;
 		}
-		of_node_put(of_parent);
 	} else if (of_device_is_compatible(of_node, "qemu,std-vga")) {
 		model = OFDRM_MODEL_QEMU;
 	}
diff -u -p a/arch/arm/mach-versatile/versatile.c b/arch/arm/mach-versatile/versatile.c
--- a/arch/arm/mach-versatile/versatile.c
+++ b/arch/arm/mach-versatile/versatile.c
@@ -123,7 +123,7 @@ static void __init versatile_init_early(
 static void __init versatile_dt_pci_init(void)
 {
 	u32 val;
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	struct property *newprop;

 	np = of_find_compatible_node(NULL, NULL, "arm,versatile-pci");
@@ -154,7 +154,6 @@ static void __init versatile_dt_pci_init
 	pr_info("Not plugged into PCI backplane!\n");

 out_put_node:
-	of_node_put(np);
 }

 static void __init versatile_dt_init(void)
diff -u -p a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -646,7 +646,7 @@ static int imx6_pm_stby_poweroff_probe(v

 void __init imx6_pm_ccm_init(const char *ccm_compat)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	u32 val;

 	np = of_find_compatible_node(NULL, NULL, ccm_compat);
@@ -663,8 +663,6 @@ void __init imx6_pm_ccm_init(const char

 	if (of_property_read_bool(np, "fsl,pmic-stby-poweroff"))
 		imx6_pm_stby_poweroff_probe();
-
-	of_node_put(np);
 }

 void __init imx6q_pm_init(void)
diff -u -p a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -210,7 +210,7 @@ static void __init pnv_setup_arch(void)

 static void __init pnv_add_hw_description(void)
 {
-	struct device_node *dn;
+	struct device_node *dn __free(device_node);
 	const char *s;

 	dn = of_find_node_by_path("/ibm,opal/firmware");
@@ -223,8 +223,6 @@ static void __init pnv_add_hw_descriptio

 	if (of_property_read_string(dn, "mi-version", &s) == 0)
 		seq_buf_printf(&ppc_hw_desc, "mi:%s ", s);
-
-	of_node_put(dn);
 }

 static void __init pnv_init(void)
diff -u -p a/arch/mips/mti-malta/malta-time.c b/arch/mips/mti-malta/malta-time.c
--- a/arch/mips/mti-malta/malta-time.c
+++ b/arch/mips/mti-malta/malta-time.c
@@ -202,7 +202,7 @@ static struct property gic_frequency_pro

 static void update_gic_frequency_dt(void)
 {
-	struct device_node *node;
+	struct device_node *node __free(device_node);

 	gic_frequency_dt = cpu_to_be32(gic_frequency);

@@ -214,8 +214,6 @@ static void update_gic_frequency_dt(void

 	if (of_update_property(node, &gic_frequency_prop) < 0)
 		pr_err("error updating gic frequency property\n");
-
-	of_node_put(node);
 }

 #endif
diff -u -p a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
--- a/arch/powerpc/sysdev/mpic_msgr.c
+++ b/arch/powerpc/sysdev/mpic_msgr.c
@@ -146,16 +146,14 @@ static int mpic_msgr_block_number(struct

 	for (index = 0; index < number_of_blocks; ++index) {
 		struct property *prop;
-		struct device_node *tn;
+		struct device_node *tn __free(device_node);

 		snprintf(buf, sizeof(buf), "mpic-msgr-block%d", index);
 		prop = of_find_property(aliases, buf, NULL);
 		tn = of_find_node_by_path(prop->value);
 		if (node == tn) {
-			of_node_put(tn);
 			break;
 		}
-		of_node_put(tn);
 	}
 	of_node_put(aliases);

diff -u -p a/arch/powerpc/platforms/52xx/mpc52xx_pci.c b/arch/powerpc/platforms/52xx/mpc52xx_pci.c
--- a/arch/powerpc/platforms/52xx/mpc52xx_pci.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_pci.c
@@ -408,12 +408,11 @@ mpc52xx_add_bridge(struct device_node *n

 void __init mpc52xx_setup_pci(void)
 {
-	struct device_node *pci;
+	struct device_node *pci __free(device_node);

 	pci = of_find_matching_node(NULL, mpc52xx_pci_ids);
 	if (!pci)
 		return;

 	mpc52xx_add_bridge(pci);
-	of_node_put(pci);
 }
diff -u -p a/arch/nios2/kernel/cpuinfo.c b/arch/nios2/kernel/cpuinfo.c
--- a/arch/nios2/kernel/cpuinfo.c
+++ b/arch/nios2/kernel/cpuinfo.c
@@ -30,7 +30,7 @@ static inline u32 fcpu(struct device_nod

 void __init setup_cpuinfo(void)
 {
-	struct device_node *cpu;
+	struct device_node *cpu __free(device_node);
 	const char *str;
 	int len;

@@ -107,8 +107,6 @@ void __init setup_cpuinfo(void)
 	cpuinfo.reset_addr = fcpu(cpu, "altr,reset-addr");
 	cpuinfo.exception_addr = fcpu(cpu, "altr,exception-addr");
 	cpuinfo.fast_tlb_miss_exc_addr = fcpu(cpu, "altr,fast-tlb-miss-addr");
-
-	of_node_put(cpu);
 }

 #ifdef CONFIG_PROC_FS
diff -u -p a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
--- a/drivers/clk/davinci/da8xx-cfgchip.c
+++ b/drivers/clk/davinci/da8xx-cfgchip.c
@@ -749,11 +749,10 @@ static int da8xx_cfgchip_probe(struct pl

 	clk_init = device_get_match_data(dev);
 	if (clk_init) {
-		struct device_node *parent;
+		struct device_node *parent __free(device_node);

 		parent = of_get_parent(dev->of_node);
 		regmap = syscon_node_to_regmap(parent);
-		of_node_put(parent);
 	} else if (pdev->id_entry && pdata) {
 		clk_init = (void *)pdev->id_entry->driver_data;
 		regmap = pdata->cfgchip;
diff -u -p a/arch/powerpc/platforms/powermac/pci.c b/arch/powerpc/platforms/powermac/pci.c
--- a/arch/powerpc/platforms/powermac/pci.c
+++ b/arch/powerpc/platforms/powermac/pci.c
@@ -490,7 +490,7 @@ static void __init init_bandit(struct pc
  */
 static void __init init_p2pbridge(void)
 {
-	struct device_node *p2pbridge;
+	struct device_node *p2pbridge __free(device_node);
 	struct pci_controller* hose;
 	u8 bus, devfn;
 	u16 val;
@@ -521,7 +521,6 @@ static void __init init_p2pbridge(void)
 	val &= ~PCI_BRIDGE_CTL_MASTER_ABORT;
 	early_write_config_word(hose, bus, devfn, PCI_BRIDGE_CONTROL, val);
 done:
-	of_node_put(p2pbridge);
 }

 static void __init init_second_ohare(void)
diff -u -p a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -116,7 +116,7 @@ static int mailbox_chan_validate(struct

 	/* Bail out if provided shmem descriptors do not refer distinct areas  */
 	if (num_sh > 1) {
-		struct device_node *np_tx, *np_rx;
+		struct device_node *np_tx, *np_rx __free(device_node);

 		np_tx = of_parse_phandle(np, "shmem", 0);
 		np_rx = of_parse_phandle(np, "shmem", 1);
@@ -127,7 +127,6 @@ static int mailbox_chan_validate(struct
 		}

 		of_node_put(np_tx);
-		of_node_put(np_rx);
 	}

 	/* Calculate channels IDs to use depending on mboxes/shmem layout */
diff -u -p a/drivers/sbus/char/envctrl.c b/drivers/sbus/char/envctrl.c
--- a/drivers/sbus/char/envctrl.c
+++ b/drivers/sbus/char/envctrl.c
@@ -899,7 +899,7 @@ static void envctrl_init_i2c_child(struc
 	 * 'NULL' monitor type.
 	 */
 	if (ENVCTRL_CPCI_IGNORED_NODE == pchild->addr) {
-		struct device_node *root_node;
+		struct device_node *root_node __free(device_node);
 		int len;

 		root_node = of_find_node_by_path("/");
@@ -907,10 +907,8 @@ static void envctrl_init_i2c_child(struc
 			for (len = 0; len < PCF8584_MAX_CHANNELS; ++len) {
 				pchild->mon_type[len] = ENVCTRL_NOMON;
 			}
-			of_node_put(root_node);
 			return;
 		}
-		of_node_put(root_node);
 	}

 	/* Get the monitor channels. */
diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1053,7 +1053,7 @@ static void __init of_unittest_pci_dma_r

 static void __init of_unittest_bus_ranges(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	struct of_range range;
 	struct of_range_parser parser;
 	struct resource res;
@@ -1118,13 +1118,11 @@ static void __init of_unittest_bus_range
 		}
 		i++;
 	}
-
-	of_node_put(np);
 }

 static void __init of_unittest_bus_3cell_ranges(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	struct of_range range;
 	struct of_range_parser parser;
 	int i = 0;
@@ -1173,13 +1171,11 @@ static void __init of_unittest_bus_3cell
 		}
 		i++;
 	}
-
-	of_node_put(np);
 }

 static void __init of_unittest_reg(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	int ret;
 	u64 addr, size;

@@ -1194,8 +1190,6 @@ static void __init of_unittest_reg(void)
 		np, ret);
 	unittest(addr == 0x1000, "of_property_read_reg(%pOF) untranslated address (%llx) incorrect\n",
 		np, addr);
-
-	of_node_put(np);
 }

 struct of_unittest_expected_res {
diff -u -p a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
--- a/drivers/clk/clk-qoriq.c
+++ b/drivers/clk/clk-qoriq.c
@@ -1065,11 +1065,10 @@ static void __init _clockgen_init(struct
 static void __init legacy_init_clockgen(struct device_node *np)
 {
 	if (!clockgen.node) {
-		struct device_node *parent_np;
+		struct device_node *parent_np __free(device_node);

 		parent_np = of_get_parent(np);
 		_clockgen_init(parent_np, true);
-		of_node_put(parent_np);
 	}
 }

diff -u -p a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
--- a/arch/powerpc/platforms/512x/mpc512x_shared.c
+++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
@@ -443,7 +443,7 @@ static void __init mpc512x_psc_fifo_init

 static void __init mpc512x_restart_init(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	const char *reset_compat;

 	reset_compat = mpc512x_select_reset_compat();
@@ -452,7 +452,6 @@ static void __init mpc512x_restart_init(
 		return;

 	reset_module_base = of_iomap(np, 0);
-	of_node_put(np);
 }

 void __init mpc512x_init_early(void)
diff -u -p a/drivers/pci/of.c b/drivers/pci/of.c
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -610,7 +610,7 @@ int devm_of_pci_bridge_init(struct devic

 void of_pci_remove_node(struct pci_dev *pdev)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	np = pci_device_to_OF_node(pdev);
 	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
@@ -619,7 +619,6 @@ void of_pci_remove_node(struct pci_dev *

 	of_changeset_revert(np->data);
 	of_changeset_destroy(np->data);
-	of_node_put(np);
 }

 void of_pci_make_dev_node(struct pci_dev *pdev)
diff -u -p a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1320,7 +1320,7 @@ static int ffa_sched_recv_irq_map(void)

 	if (acpi_disabled) {
 		struct of_phandle_args oirq = {};
-		struct device_node *gic;
+		struct device_node *gic __free(device_node);

 		/* Only GICv3 supported currently with the device tree */
 		gic = of_find_compatible_node(NULL, NULL, "arm,gic-v3");
@@ -1331,7 +1331,6 @@ static int ffa_sched_recv_irq_map(void)
 		oirq.args_count = 1;
 		oirq.args[0] = sr_intid;
 		irq = irq_create_of_mapping(&oirq);
-		of_node_put(gic);
 #ifdef CONFIG_ACPI
 	} else {
 		irq = acpi_register_gsi(NULL, sr_intid, ACPI_EDGE_SENSITIVE,
diff -u -p a/arch/powerpc/platforms/embedded6xx/flipper-pic.c b/arch/powerpc/platforms/embedded6xx/flipper-pic.c
--- a/arch/powerpc/platforms/embedded6xx/flipper-pic.c
+++ b/arch/powerpc/platforms/embedded6xx/flipper-pic.c
@@ -182,7 +182,7 @@ unsigned int flipper_pic_get_irq(void)

 void __init flipper_pic_probe(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	np = of_find_compatible_node(NULL, NULL, "nintendo,flipper-pic");
 	BUG_ON(!np);
@@ -191,8 +191,6 @@ void __init flipper_pic_probe(void)
 	BUG_ON(!flipper_irq_host);

 	irq_set_default_host(flipper_irq_host);
-
-	of_node_put(np);
 }

 /*
diff -u -p a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -6440,7 +6440,7 @@ static int kvmppc_book3s_init_hv(void)
 #ifdef CONFIG_SMP
 	if (!xics_on_xive() && !kvmhv_on_pseries() &&
 	    !local_paca->kvm_hstate.xics_phys) {
-		struct device_node *np;
+		struct device_node *np __free(device_node);

 		np = of_find_compatible_node(NULL, NULL, "ibm,opal-intc");
 		if (!np) {
@@ -6448,8 +6448,6 @@ static int kvmppc_book3s_init_hv(void)
 			r = -ENODEV;
 			goto err;
 		}
-		/* presence of intc confirmed - node can be dropped again */
-		of_node_put(np);
 	}
 #endif

diff -u -p a/arch/powerpc/platforms/pseries/rng.c b/arch/powerpc/platforms/pseries/rng.c
--- a/arch/powerpc/platforms/pseries/rng.c
+++ b/arch/powerpc/platforms/pseries/rng.c
@@ -27,11 +27,10 @@ static int pseries_get_random_long(unsig

 void __init pseries_rng_init(void)
 {
-	struct device_node *dn;
+	struct device_node *dn __free(device_node);

 	dn = of_find_compatible_node(NULL, NULL, "ibm,random");
 	if (!dn)
 		return;
 	ppc_md.get_random_seed = pseries_get_random_long;
-	of_node_put(dn);
 }
diff -u -p a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1795,12 +1795,11 @@ static struct extcon_dev *dwc3_get_extco
 	 */
 	np_phy = of_parse_phandle(dev->of_node, "phys", 0);
 	if (of_graph_is_present(np_phy)) {
-		struct device_node *np_conn;
+		struct device_node *np_conn __free(device_node);

 		np_conn = of_graph_get_remote_node(np_phy, -1, -1);
 		if (np_conn)
 			edev = extcon_find_edev_by_node(np_conn);
-		of_node_put(np_conn);
 	}
 	of_node_put(np_phy);

diff -u -p a/arch/arm/mach-bcm/platsmp-brcmstb.c b/arch/arm/mach-bcm/platsmp-brcmstb.c
--- a/arch/arm/mach-bcm/platsmp-brcmstb.c
+++ b/arch/arm/mach-bcm/platsmp-brcmstb.c
@@ -314,7 +314,7 @@ cleanup:
 static void __init brcmstb_cpu_ctrl_setup(unsigned int max_cpus)
 {
 	int rc;
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	char *name;

 	name = "brcm,brcmstb-smpboot";
@@ -333,7 +333,6 @@ static void __init brcmstb_cpu_ctrl_setu
 		goto out_put_node;

 out_put_node:
-	of_node_put(np);
 }

 static int brcmstb_boot_secondary(unsigned int cpu, struct task_struct *idle)
diff -u -p a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c
--- a/arch/mips/bmips/setup.c
+++ b/arch/mips/bmips/setup.c
@@ -201,7 +201,7 @@ void __init plat_mem_setup(void)

 void __init device_tree_init(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	unflatten_and_copy_device_tree();

@@ -209,7 +209,6 @@ void __init device_tree_init(void)
 	np = of_find_node_by_name(NULL, "cpus");
 	if (np && of_get_available_child_count(np) <= 1)
 		bmips_smp_enabled = 0;
-	of_node_put(np);
 }

 static int __init plat_dev_init(void)
diff -u -p a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
--- a/arch/powerpc/platforms/85xx/p1022_ds.c
+++ b/arch/powerpc/platforms/85xx/p1022_ds.c
@@ -163,7 +163,7 @@ static phys_addr_t lbc_br_to_phys(const
  */
 static void p1022ds_set_monitor_port(enum fsl_diu_monitor_port port)
 {
-	struct device_node *guts_node;
+	struct device_node *guts_node __free(device_node);
 	struct device_node *lbc_node = NULL;
 	struct device_node *law_node = NULL;
 	struct ccsr_guts __iomem *guts;
@@ -362,7 +362,6 @@ exit:

 	of_node_put(law_node);
 	of_node_put(lbc_node);
-	of_node_put(guts_node);
 }

 /**
diff -u -p a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -882,7 +882,7 @@ static void opal_add_exported_attrs(stru
  */
 static void opal_export_attrs(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	struct kobject *kobj;
 	int rc;

@@ -894,7 +894,6 @@ static void opal_export_attrs(void)
 	kobj = kobject_create_and_add("exports", opal_kobj);
 	if (!kobj) {
 		pr_warn("kobject_create_and_add() of exports failed\n");
-		of_node_put(np);
 		return;
 	}

@@ -908,8 +907,6 @@ static void opal_export_attrs(void)
 				 np->parent, "symbol-map");
 	if (rc)
 		pr_warn("Error %d creating OPAL symbols file\n", rc);
-
-	of_node_put(np);
 }

 static void __init opal_dump_region_init(void)
@@ -950,13 +947,11 @@ static void __init opal_pdev_init(const

 static void __init opal_imc_init_dev(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	np = of_find_compatible_node(NULL, NULL, IMC_DTB_COMPAT);
 	if (np)
 		of_platform_device_create(np, NULL, NULL);
-
-	of_node_put(np);
 }

 static int kopald(void *unused)
diff -u -p a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c
--- a/arch/arm/mach-rockchip/pm.c
+++ b/arch/arm/mach-rockchip/pm.c
@@ -304,7 +304,7 @@ void __init rockchip_suspend_init(void)
 {
 	const struct rockchip_pm_data *pm_data;
 	const struct of_device_id *match;
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	int ret;

 	np = of_find_matching_node_and_match(NULL, rockchip_pmu_of_device_ids,
@@ -327,5 +327,4 @@ void __init rockchip_suspend_init(void)
 	suspend_set_ops(pm_data->ops);

 out_put:
-	of_node_put(np);
 }
diff -u -p a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
--- a/drivers/soc/imx/soc-imx8m.c
+++ b/drivers/soc/imx/soc-imx8m.c
@@ -99,7 +99,7 @@ static u32 __init imx8mq_soc_revision(vo
 static void __init imx8mm_soc_uid(void)
 {
 	void __iomem *ocotp_base;
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	struct clk *clk;
 	u32 offset = of_machine_is_compatible("fsl,imx8mp") ?
 		     IMX8MP_OCOTP_UID_OFFSET : 0;
@@ -125,7 +125,6 @@ static void __init imx8mm_soc_uid(void)
 	clk_disable_unprepare(clk);
 	clk_put(clk);
 	iounmap(ocotp_base);
-	of_node_put(np);
 }

 static u32 __init imx8mm_soc_revision(void)
diff -u -p a/arch/powerpc/platforms/cell/setup.c b/arch/powerpc/platforms/cell/setup.c
--- a/arch/powerpc/platforms/cell/setup.c
+++ b/arch/powerpc/platforms/cell/setup.c
@@ -62,14 +62,13 @@

 static void cell_show_cpuinfo(struct seq_file *m)
 {
-	struct device_node *root;
+	struct device_node *root __free(device_node);
 	const char *model = "";

 	root = of_find_node_by_path("/");
 	if (root)
 		model = of_get_property(root, "model", NULL);
 	seq_printf(m, "machine\t\t: CHRP %s\n", model);
-	of_node_put(root);
 }

 static void cell_progress(char *s, unsigned short hex)
diff -u -p a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -1454,7 +1454,7 @@ static int ibmvfc_issue_fc_host_lip(stru
  **/
 static void ibmvfc_gather_partition_info(struct ibmvfc_host *vhost)
 {
-	struct device_node *rootdn;
+	struct device_node *rootdn __free(device_node);
 	const char *name;
 	const unsigned int *num;

@@ -1468,7 +1468,6 @@ static void ibmvfc_gather_partition_info
 	num = of_get_property(rootdn, "ibm,partition-no", NULL);
 	if (num)
 		vhost->partition_number = *num;
-	of_node_put(rootdn);
 }

 /**
diff -u -p a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2102,7 +2102,7 @@ static int __init gic_validate_dist_vers
 /* Create all possible partitions at boot time */
 static void __init gic_populate_ppi_partitions(struct device_node *gic_node)
 {
-	struct device_node *parts_node, *child_part;
+	struct device_node *parts_node __free(device_node), *child_part;
 	int part_idx = 0, i;
 	int nr_parts;
 	struct partition_affinity *parts;
@@ -2194,7 +2194,6 @@ static void __init gic_populate_ppi_part
 	}

 out_put_node:
-	of_node_put(parts_node);
 }

 static void __init gic_of_setup_kvm_info(struct device_node *node)
diff -u -p a/arch/powerpc/platforms/85xx/socrates.c b/arch/powerpc/platforms/85xx/socrates.c
--- a/arch/powerpc/platforms/85xx/socrates.c
+++ b/arch/powerpc/platforms/85xx/socrates.c
@@ -40,7 +40,7 @@

 static void __init socrates_pic_init(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	struct mpic *mpic = mpic_alloc(NULL, 0, MPIC_BIG_ENDIAN,
 			0, 256, " OpenPIC  ");
@@ -53,7 +53,6 @@ static void __init socrates_pic_init(voi
 		return;
 	}
 	socrates_fpga_pic_init(np);
-	of_node_put(np);
 }

 /*
diff -u -p a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -1654,7 +1654,7 @@ static void soctherm_init_hw_throt_cdev(
 {
 	struct device *dev = &pdev->dev;
 	struct tegra_soctherm *ts = dev_get_drvdata(dev);
-	struct device_node *np_stc, *np_stcc;
+	struct device_node *np_stc __free(device_node), *np_stcc;
 	const char *name;
 	int i;

@@ -1713,8 +1713,6 @@ static void soctherm_init_hw_throt_cdev(
 		}

 	}
-
-	of_node_put(np_stc);
 }

 /**
diff -u -p a/arch/arm/mach-imx/system.c b/arch/arm/mach-imx/system.c
--- a/arch/arm/mach-imx/system.c
+++ b/arch/arm/mach-imx/system.c
@@ -86,7 +86,7 @@ void __init imx1_reset_init(void __iomem
 void __init imx_init_l2cache(void)
 {
 	void __iomem *l2x0_base;
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	unsigned int val;

 	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
@@ -113,6 +113,5 @@ void __init imx_init_l2cache(void)

 	iounmap(l2x0_base);
 put_node:
-	of_node_put(np);
 }
 #endif
diff -u -p a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -758,7 +758,7 @@ static void sysrq_detect_reset_sequence(
 static void sysrq_of_get_keyreset_config(void)
 {
 	u32 key;
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	struct property *prop;
 	const __be32 *p;

@@ -781,8 +781,6 @@ static void sysrq_of_get_keyreset_config

 	/* Get reset timeout if any. */
 	of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
-
-	of_node_put(np);
 }
 #else
 static void sysrq_of_get_keyreset_config(void)
diff -u -p a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -269,7 +269,7 @@ IRQCHIP_DECLARE(imx_gpc, "fsl,imx6q-gpc"

 void __init imx_gpc_check_dt(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-gpc");
 	if (WARN_ON(!np))
@@ -281,5 +281,4 @@ void __init imx_gpc_check_dt(void)
 		/* map GPC, so that at least CPUidle and WARs keep working */
 		gpc_base = of_iomap(np, 0);
 	}
-	of_node_put(np);
 }
diff -u -p a/sound/soc/codecs/rk817_codec.c b/sound/soc/codecs/rk817_codec.c
--- a/sound/soc/codecs/rk817_codec.c
+++ b/sound/soc/codecs/rk817_codec.c
@@ -456,7 +456,7 @@ static const struct snd_soc_component_dr
 static void rk817_codec_parse_dt_property(struct device *dev,
 					 struct rk817_codec_priv *rk817)
 {
-	struct device_node *node;
+	struct device_node *node __free(device_node);

 	node = of_get_child_by_name(dev->parent->of_node, "codec");
 	if (!node) {
@@ -466,8 +466,6 @@ static void rk817_codec_parse_dt_propert

 	rk817->mic_in_differential =
 			of_property_read_bool(node, "rockchip,mic-in-differential");
-
-	of_node_put(node);
 }

 static int rk817_platform_probe(struct platform_device *pdev)
diff -u -p a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -627,7 +627,7 @@ static void __init smp_core99_setup_i2c_

 static void smp_core99_pfunc_tb_freeze(int freeze)
 {
-	struct device_node *cpus;
+	struct device_node *cpus __free(device_node);
 	struct pmf_args args;

 	cpus = of_find_node_by_path("/cpus");
@@ -635,7 +635,6 @@ static void smp_core99_pfunc_tb_freeze(i
 	args.count = 1;
 	args.u[0].v = !freeze;
 	pmf_call_function(cpus, "cpu-timebase", &args);
-	of_node_put(cpus);
 }

 #else /* CONFIG_PPC64 */
diff -u -p a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
--- a/arch/arm/mach-omap2/omap-secure.c
+++ b/arch/arm/mach-omap2/omap-secure.c
@@ -33,7 +33,7 @@ bool optee_available;

 static void __init omap_optee_init_check(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);

 	/*
 	 * We only check that the OP-TEE node is present and available. The
@@ -43,7 +43,6 @@ static void __init omap_optee_init_check
 	np = of_find_node_by_path("/firmware/optee");
 	if (np && of_device_is_available(np))
 		optee_available = true;
-	of_node_put(np);
 }

 /**
diff -u -p a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -78,7 +78,7 @@ static void __init imx6q_enet_phy_init(v

 static void __init imx6q_1588_init(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	struct clk *ptp_clk, *fec_enet_ref;
 	struct clk *enet_ref;
 	struct regmap *gpr;
@@ -130,7 +130,6 @@ static void __init imx6q_1588_init(void)
 put_ptp_clk:
 	clk_put(ptp_clk);
 put_node:
-	of_node_put(np);
 }

 static void __init imx6q_axi_init(void)
diff -u -p a/arch/powerpc/platforms/83xx/km83xx.c b/arch/powerpc/platforms/83xx/km83xx.c
--- a/arch/powerpc/platforms/83xx/km83xx.c
+++ b/arch/powerpc/platforms/83xx/km83xx.c
@@ -45,7 +45,7 @@ static void __init quirk_mpc8360e_qe_ene
 	 * RGMII AC values do not meet the specification
 	 */
 	uint svid = mfspr(SPRN_SVR);
-	struct	device_node *np_par;
+	struct	device_node *np_par __free(device_node);
 	struct	resource res;
 	void	__iomem *base;
 	int	ret;
@@ -113,7 +113,6 @@ static void __init quirk_mpc8360e_qe_ene
 	}
 	iounmap(base);
 out:
-	of_node_put(np_par);
 }

 /* ************************************************************************
diff -u -p a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -1113,7 +1113,7 @@ void rsnd_parse_connect_ssi(struct rsnd_
 {
 	struct rsnd_priv *priv = rsnd_rdai_to_priv(rdai);
 	struct device *dev = rsnd_priv_to_dev(priv);
-	struct device_node *node;
+	struct device_node *node __free(device_node);
 	struct device_node *np;
 	int i;

@@ -1139,8 +1139,6 @@ void rsnd_parse_connect_ssi(struct rsnd_
 			rsnd_ssi_connect(mod, &rdai->capture);
 		i++;
 	}
-
-	of_node_put(node);
 }

 struct rsnd_mod *rsnd_ssi_mod_get(struct rsnd_priv *priv, int id)
diff -u -p a/drivers/tty/serial/suncore.c b/drivers/tty/serial/suncore.c
--- a/drivers/tty/serial/suncore.c
+++ b/drivers/tty/serial/suncore.c
@@ -99,7 +99,7 @@ void sunserial_console_termios(struct co
 	} else if (of_node_name_eq(uart_dp, "lom-console")) {
 		mode = "9600,8,n,1,-";
 	} else {
-		struct device_node *dp;
+		struct device_node *dp __free(device_node);
 		char c;

 		c = 'a';
@@ -112,7 +112,6 @@ void sunserial_console_termios(struct co
 		mode = of_get_property(dp, mode_prop, NULL);
 		if (!mode)
 			mode = "9600,8,n,1,-";
-		of_node_put(dp);
 	}

 	cflag = CREAD | HUPCL | CLOCAL;
diff -u -p a/drivers/clk/clk-nomadik.c b/drivers/clk/clk-nomadik.c
--- a/drivers/clk/clk-nomadik.c
+++ b/drivers/clk/clk-nomadik.c
@@ -87,7 +87,7 @@ static const struct of_device_id nomadik

 static void __init nomadik_src_init(void)
 {
-	struct device_node *np;
+	struct device_node *np __free(device_node);
 	u32 val;

 	np = of_find_matching_node(NULL, nomadik_src_match);
@@ -134,7 +134,6 @@ static void __init nomadik_src_init(void
 	register_reboot_notifier(&nomadik_clk_reboot_notifier);

 out_put:
-	of_node_put(np);
 }

 /**
diff -u -p a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
--- a/arch/powerpc/platforms/chrp/setup.c
+++ b/arch/powerpc/platforms/chrp/setup.c
@@ -99,7 +99,7 @@ static void chrp_show_cpuinfo(struct seq
 {
 	int i, sdramen;
 	unsigned int t;
-	struct device_node *root;
+	struct device_node *root __free(device_node);
 	const char *model = "";

 	root = of_find_node_by_path("/");
@@ -152,7 +152,6 @@ static void chrp_show_cpuinfo(struct seq
 			   gg2_cachetypes[(t>>2) & 3],
 			   gg2_cachemodes[t & 3]);
 	}
-	of_node_put(root);
 }

 /*
@@ -195,7 +194,7 @@ static void __init sio_fixup_irq(const c

 static void __init sio_init(void)
 {
-	struct device_node *root;
+	struct device_node *root __free(device_node);
 	const char *model;

 	root = of_find_node_by_path("/");
@@ -209,8 +208,6 @@ static void __init sio_init(void)
 		/* select logical device 1 (KBC/Mouse) */
 		sio_fixup_irq("mouse", 1, 12, 2);
 	}
-
-	of_node_put(root);
 }


@@ -364,7 +361,7 @@ static void chrp_8259_cascade(struct irq
  */
 static void __init chrp_find_openpic(void)
 {
-	struct device_node *np, *root;
+	struct device_node *np __free(device_node), *root;
 	int len, i, j;
 	int isu_size;
 	const unsigned int *iranges, *opprop = NULL;
@@ -438,7 +435,6 @@ static void __init chrp_find_openpic(voi
 	ppc_md.get_irq = mpic_get_irq;
  bail:
 	of_node_put(root);
-	of_node_put(np);
 }

 static void __init chrp_find_8259(void)
diff -u -p a/arch/powerpc/platforms/52xx/efika.c b/arch/powerpc/platforms/52xx/efika.c
--- a/arch/powerpc/platforms/52xx/efika.c
+++ b/arch/powerpc/platforms/52xx/efika.c
@@ -140,7 +140,7 @@ static void __init efika_pcisetup(void)

 static void efika_show_cpuinfo(struct seq_file *m)
 {
-	struct device_node *root;
+	struct device_node *root __free(device_node);
 	const char *revision;
 	const char *codegendescription;
 	const char *codegenvendor;
@@ -163,8 +163,6 @@ static void efika_show_cpuinfo(struct se

 	if (codegenvendor)
 		seq_printf(m, "vendor\t\t: %s\n", codegenvendor);
-
-	of_node_put(root);
 }

 #ifdef CONFIG_PM
Jonathan Cameron Jan. 29, 2024, 7:52 p.m. UTC | #4 
On Mon, 29 Jan 2024 15:02:19 +0100 (CET)
Julia Lawall <julia.lawall@inria.fr> wrote:

> On Mon, 29 Jan 2024, Jonathan Cameron wrote:
> 
> > On Sun, 28 Jan 2024 19:06:53 +0100 (CET)
> > Julia Lawall <julia.lawall@inria.fr> wrote:
> >  
> > > On Sun, 28 Jan 2024, Jonathan Cameron wrote:
> > >  
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > +CC includes peopleinterested in property.h equivalents to minimize
> > > > duplication of discussion.  Outcome of this discussion will affect:
> > > > https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> > > > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> > > >
> > > > In discussion of previous approach with Rob Herring we talked about various
> > > > ways to avoid a disconnect between the declaration of the __free(device_node)
> > > > and the first non NULL assignment. Making this connection clear is useful for 2
> > > > reasons:
> > > > 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> > > > 2) Avoids disconnect between how cleanup is to be done and how the reference
> > > >    was acquired in the first place.
> > > >
> > > > https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> > > >
> > > > The options we discussed are:
> > > >
> > > > 1) Ignore this issue and merge original set.
> > > >
> > > > 2) Always put the declaration just before the for loop and don't set it NULL.
> > > >
> > > > {
> > > > 	int ret;
> > > >
> > > > 	ret = ... and other fun code.
> > > >
> > > > 	struct device_node *child __free(device_node);
> > > > 	for_each_child_of_node(np, child) {
> > > > 	}
> > > > }
> > > >
> > > > This works but careful review is needed to ensure that this unusual pattern is
> > > > followed.  We don't set it to NULL as the loop will do that anyway if there are
> > > > no child nodes, or the loop finishes without an early break or return.
> > > >
> > > > 3) Introduced the pointer to auto put device_node only within the
> > > >    for loop scope.
> > > >
> > > > +#define for_each_child_of_node_scoped(parent, child) \
> > > > +	for (struct device_node *child __free(device_node) =		\
> > > > +	     of_get_next_child(parent, NULL);				\
> > > > +	     child != NULL;						\
> > > > +	     child = of_get_next_available_child(parent, child))
> > > > +
> > > >
> > > > This series is presenting option 3.  I only implemented this loop out of
> > > > all the similar ones and it is only compile tested.
> > > >
> > > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> > > > a struct device_node *child.  I can't see a way around that other than option 2
> > > > above, but all suggestions welcome.  Note that if a conversion leaves an
> > > > 'external' struct device_node *child variable, in many cases the compiler
> > > > will catch that as an unused variable. We don't currently run shaddow
> > > > variable detection in normal kernel builds, but that could also be used
> > > > to catch such bugs.
> > > >
> > > > All comments welcome.  
> > >
> > > It looks promising to get rid of a lot of clunky and error-prone
> > > error-handling code.  
> >
> > Absolutely. I think I spotted 2 bugs whilst just looking for places this pattern
> > doesn't apply.  Will circle back to those once this discussion is resolved.
> > I think I've taken dozen's of fixes for cases where these were missed over the years
> > so hoping this means I'll never see another one!
> >  
> > >
> > > I guess that
> > >
> > > for_each_child_of_node_scoped(parent, struct device_node *, child)
> > >
> > > would seem too verbose?  
> >
> > Intent just to make the allocated internal type clear?  Sure we can do that if
> > it helps with making it clear something is being allocated.
> > I can't think of a way this could be used with anything other than
> > a struct device_node * as the second parameter but I guess it still helps
> > 'hint' at what is going on..

To touch back on this, I'm still not sure what your intent was in suggesting
having the explicit struct device_node *

> >  
> > >
> > > There are a lot of opportunities for device_node loops, but also for some
> > > more obscure loops over other types.  
> >  
> > > And there are a lot of of_node_puts
> > > that could be eliminated independent of loops.  
> >
> > The non loop cases should be handled via the __free(device_node) as provided
> > by patch 1.  We'll need to keep the declaration local and initial assignment
> > together but that is easy enough to do and similar to the many other cleanup.h
> > usecases that are surfacing.  
> 
> I tried the following semantic patch:
> 
> @@
> identifier x,f;
> attribute name __free;
> expression list es;
> expression e;
> statement S;
> @@
> 
> {
> ... when != S
> struct device_node *x
> + __free(device_node)
> ;
> ... when strict
> x = f(es);
> <... when any
>      when != x = e
> -of_node_put(x);
> ...>  
> -of_node_put(x);
> }
> 
Nice.  
> It is written defensively in various ways:
> 
> when != S means tha tthe device_node declaration has t be at the top of
> the block, perhaps with other declarations before it.
> 
> when strict means that all paths must lead from the declaration to the
> initialization.  So there is no need to intiialize the variable to NULL,
> as far as I understand.
> 
> when != x = e means that the declared variable is not reinitialized, which
> would require keeping the previous of_node_put.
> 
> There is an of_node_put at the end of the block, so the use of __free
> doesn't change the lifetime.
> 
> An unfortunate aspect of the last constraint is that some functions may
> declare multiple device_node variables, and only one of the of_not_puts
> can come at the very end of the block.  This can be solved by just running
> the semantic patch again.
> 
> An alternative would be to move the initialization up to the declaration,
> but the result was often a bit ugly, due to the various initialization
> function calls having long names and argument lists.

You have to do that in order to ensure there is no window for someone to
easily insert code that leaves them uninitialized.

Linus had some rather specific comments on that being the only right way to
do it.

> 
> The output is below.  I have looked quickly through all of the changes and
> they all look reasonable, but have not tried compiling anything (which I
> guess wouldn't currently work anyway).
> 
> julia
> 

I picked a couple from the end.  They show the sort of things that
would want cleaning up.  The script is great for finding low hanging
fruit but as you've identified there is often some stuff that isn't
easy to automate.

> diff -u -p a/drivers/clk/clk-nomadik.c b/drivers/clk/clk-nomadik.c
> --- a/drivers/clk/clk-nomadik.c
> +++ b/drivers/clk/clk-nomadik.c
> @@ -87,7 +87,7 @@ static const struct of_device_id nomadik
> 
>  static void __init nomadik_src_init(void)
>  {
> -	struct device_node *np;
> +	struct device_node *np __free(device_node);
>  	u32 val;
> 
>  	np = of_find_matching_node(NULL, nomadik_src_match);
> @@ -134,7 +134,6 @@ static void __init nomadik_src_init(void
>  	register_reboot_notifier(&nomadik_clk_reboot_notifier);
> 
>  out_put:
Can avoid the label given nothing to do any more. 
> -	of_node_put(np);
>  }
> 
>  /**
> diff -u -p a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
> --- a/arch/powerpc/platforms/chrp/setup.c
> +++ b/arch/powerpc/platforms/chrp/setup.c
> @@ -99,7 +99,7 @@ static void chrp_show_cpuinfo(struct seq
>  {
>  	int i, sdramen;
>  	unsigned int t;
> -	struct device_node *root;
> +	struct device_node *root __free(device_node);

Same as next 2.

>  	const char *model = "";
> 
>  	root = of_find_node_by_path("/");
> @@ -152,7 +152,6 @@ static void chrp_show_cpuinfo(struct seq
>  			   gg2_cachetypes[(t>>2) & 3],  
>  			   gg2_cachemodes[t & 3]);
>  	}
> -	of_node_put(root);
>  }
> 
>  /*
> @@ -195,7 +194,7 @@ static void __init sio_fixup_irq(const c
> 
>  static void __init sio_init(void)
>  {
> -	struct device_node *root;
> +	struct device_node *root __free(device_node);

As below needs to be 
	struct device_node *root __free(device_node) = of_find_node_by_path("/");
so there is no space for code to be added inbetween that might return before
this is set.

>  	const char *model;
> 
>  	root = of_find_node_by_path("/");
> @@ -209,8 +208,6 @@ static void __init sio_init(void)
>  		/* select logical device 1 (KBC/Mouse) */
>  		sio_fixup_irq("mouse", 1, 12, 2);
>  	}
> -
> -	of_node_put(root);
>  }
> 
> 
> @@ -364,7 +361,7 @@ static void chrp_8259_cascade(struct irq
>   */
>  static void __init chrp_find_openpic(void)
>  {
> -	struct device_node *np, *root;
> +	struct device_node *np __free(device_node), *root;

This one looks dangerous because of the chance of other code
getting added between here and the point where it is set.

Better to move that down so we have

struct device_node *np __free(device_node) = of_find_node_by_type(NULL, "open-pic");

Also there is a nicer use in:
https://elixir.bootlin.com/linux/v6.8-rc2/source/arch/powerpc/platforms/chrp/setup.c#L217

>  	int len, i, j;
>  	int isu_size;
>  	const unsigned int *iranges, *opprop = NULL;
> @@ -438,7 +435,6 @@ static void __init chrp_find_openpic(voi
>  	ppc_md.get_irq = mpic_get_irq;
>   bail:
>  	of_node_put(root);
With root covered this can return early.

> -	of_node_put(np);
>  }
>
Julia Lawall Jan. 29, 2024, 8:29 p.m. UTC | #5 
On Mon, 29 Jan 2024, Jonathan Cameron wrote:

> On Mon, 29 Jan 2024 15:02:19 +0100 (CET)
> Julia Lawall <julia.lawall@inria.fr> wrote:
>
> > On Mon, 29 Jan 2024, Jonathan Cameron wrote:
> >
> > > On Sun, 28 Jan 2024 19:06:53 +0100 (CET)
> > > Julia Lawall <julia.lawall@inria.fr> wrote:
> > >
> > > > On Sun, 28 Jan 2024, Jonathan Cameron wrote:
> > > >
> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > >
> > > > > +CC includes peopleinterested in property.h equivalents to minimize
> > > > > duplication of discussion.  Outcome of this discussion will affect:
> > > > > https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> > > > > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> > > > >
> > > > > In discussion of previous approach with Rob Herring we talked about various
> > > > > ways to avoid a disconnect between the declaration of the __free(device_node)
> > > > > and the first non NULL assignment. Making this connection clear is useful for 2
> > > > > reasons:
> > > > > 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> > > > > 2) Avoids disconnect between how cleanup is to be done and how the reference
> > > > >    was acquired in the first place.
> > > > >
> > > > > https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> > > > >
> > > > > The options we discussed are:
> > > > >
> > > > > 1) Ignore this issue and merge original set.
> > > > >
> > > > > 2) Always put the declaration just before the for loop and don't set it NULL.
> > > > >
> > > > > {
> > > > > 	int ret;
> > > > >
> > > > > 	ret = ... and other fun code.
> > > > >
> > > > > 	struct device_node *child __free(device_node);
> > > > > 	for_each_child_of_node(np, child) {
> > > > > 	}
> > > > > }
> > > > >
> > > > > This works but careful review is needed to ensure that this unusual pattern is
> > > > > followed.  We don't set it to NULL as the loop will do that anyway if there are
> > > > > no child nodes, or the loop finishes without an early break or return.
> > > > >
> > > > > 3) Introduced the pointer to auto put device_node only within the
> > > > >    for loop scope.
> > > > >
> > > > > +#define for_each_child_of_node_scoped(parent, child) \
> > > > > +	for (struct device_node *child __free(device_node) =		\
> > > > > +	     of_get_next_child(parent, NULL);				\
> > > > > +	     child != NULL;						\
> > > > > +	     child = of_get_next_available_child(parent, child))
> > > > > +
> > > > >
> > > > > This series is presenting option 3.  I only implemented this loop out of
> > > > > all the similar ones and it is only compile tested.
> > > > >
> > > > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> > > > > a struct device_node *child.  I can't see a way around that other than option 2
> > > > > above, but all suggestions welcome.  Note that if a conversion leaves an
> > > > > 'external' struct device_node *child variable, in many cases the compiler
> > > > > will catch that as an unused variable. We don't currently run shaddow
> > > > > variable detection in normal kernel builds, but that could also be used
> > > > > to catch such bugs.
> > > > >
> > > > > All comments welcome.
> > > >
> > > > It looks promising to get rid of a lot of clunky and error-prone
> > > > error-handling code.
> > >
> > > Absolutely. I think I spotted 2 bugs whilst just looking for places this pattern
> > > doesn't apply.  Will circle back to those once this discussion is resolved.
> > > I think I've taken dozen's of fixes for cases where these were missed over the years
> > > so hoping this means I'll never see another one!
> > >
> > > >
> > > > I guess that
> > > >
> > > > for_each_child_of_node_scoped(parent, struct device_node *, child)
> > > >
> > > > would seem too verbose?
> > >
> > > Intent just to make the allocated internal type clear?  Sure we can do that if
> > > it helps with making it clear something is being allocated.
> > > I can't think of a way this could be used with anything other than
> > > a struct device_node * as the second parameter but I guess it still helps
> > > 'hint' at what is going on..
>
> To touch back on this, I'm still not sure what your intent was in suggesting
> having the explicit struct device_node *

Well, I wanted struct device_node * child, but then there wouldn't be the
macro argument that would give the variable name.  So perhaps even with
the comma it would look a little more like a declaration.

It does seem problematic to have eg two freestanding occurrences of child
that are not in the same scope, if there is a child in the iterator
argument list and a use elsewhere in the function.

>
> > >
> > > >
> > > > There are a lot of opportunities for device_node loops, but also for some
> > > > more obscure loops over other types.
> > >
> > > > And there are a lot of of_node_puts
> > > > that could be eliminated independent of loops.
> > >
> > > The non loop cases should be handled via the __free(device_node) as provided
> > > by patch 1.  We'll need to keep the declaration local and initial assignment
> > > together but that is easy enough to do and similar to the many other cleanup.h
> > > usecases that are surfacing.
> >
> > I tried the following semantic patch:
> >
> > @@
> > identifier x,f;
> > attribute name __free;
> > expression list es;
> > expression e;
> > statement S;
> > @@
> >
> > {
> > ... when != S
> > struct device_node *x
> > + __free(device_node)
> > ;
> > ... when strict
> > x = f(es);
> > <... when any
> >      when != x = e
> > -of_node_put(x);
> > ...>
> > -of_node_put(x);
> > }
> >
> Nice.
> > It is written defensively in various ways:
> >
> > when != S means tha tthe device_node declaration has t be at the top of
> > the block, perhaps with other declarations before it.
> >
> > when strict means that all paths must lead from the declaration to the
> > initialization.  So there is no need to intiialize the variable to NULL,
> > as far as I understand.
> >
> > when != x = e means that the declared variable is not reinitialized, which
> > would require keeping the previous of_node_put.
> >
> > There is an of_node_put at the end of the block, so the use of __free
> > doesn't change the lifetime.
> >
> > An unfortunate aspect of the last constraint is that some functions may
> > declare multiple device_node variables, and only one of the of_not_puts
> > can come at the very end of the block.  This can be solved by just running
> > the semantic patch again.
> >
> > An alternative would be to move the initialization up to the declaration,
> > but the result was often a bit ugly, due to the various initialization
> > function calls having long names and argument lists.
>
> You have to do that in order to ensure there is no window for someone to
> easily insert code that leaves them uninitialized.
>
> Linus had some rather specific comments on that being the only right way to
> do it.

OK, the semantic patch can be changed to do that.  It would have to be a
bit more complex or a bit more defensive, to be sure that no variables are
used between the declaration and the current call site.

>
> >
> > The output is below.  I have looked quickly through all of the changes and
> > they all look reasonable, but have not tried compiling anything (which I
> > guess wouldn't currently work anyway).
> >
> > julia
> >
>
> I picked a couple from the end.  They show the sort of things that
> would want cleaning up.  The script is great for finding low hanging
> fruit but as you've identified there is often some stuff that isn't
> easy to automate.
>
> > diff -u -p a/drivers/clk/clk-nomadik.c b/drivers/clk/clk-nomadik.c
> > --- a/drivers/clk/clk-nomadik.c
> > +++ b/drivers/clk/clk-nomadik.c
> > @@ -87,7 +87,7 @@ static const struct of_device_id nomadik
> >
> >  static void __init nomadik_src_init(void)
> >  {
> > -	struct device_node *np;
> > +	struct device_node *np __free(device_node);
> >  	u32 val;
> >
> >  	np = of_find_matching_node(NULL, nomadik_src_match);
> > @@ -134,7 +134,6 @@ static void __init nomadik_src_init(void
> >  	register_reboot_notifier(&nomadik_clk_reboot_notifier);
> >
> >  out_put:
> Can avoid the label given nothing to do any more.

Indeed.

> > -	of_node_put(np);
> >  }
> >
> >  /**
> > diff -u -p a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
> > --- a/arch/powerpc/platforms/chrp/setup.c
> > +++ b/arch/powerpc/platforms/chrp/setup.c
> > @@ -99,7 +99,7 @@ static void chrp_show_cpuinfo(struct seq
> >  {
> >  	int i, sdramen;
> >  	unsigned int t;
> > -	struct device_node *root;
> > +	struct device_node *root __free(device_node);
>
> Same as next 2.
>
> >  	const char *model = "";
> >
> >  	root = of_find_node_by_path("/");
> > @@ -152,7 +152,6 @@ static void chrp_show_cpuinfo(struct seq
> >  			   gg2_cachetypes[(t>>2) & 3],
> >  			   gg2_cachemodes[t & 3]);
> >  	}
> > -	of_node_put(root);
> >  }
> >
> >  /*
> > @@ -195,7 +194,7 @@ static void __init sio_fixup_irq(const c
> >
> >  static void __init sio_init(void)
> >  {
> > -	struct device_node *root;
> > +	struct device_node *root __free(device_node);
>
> As below needs to be
> 	struct device_node *root __free(device_node) = of_find_node_by_path("/");
> so there is no space for code to be added inbetween that might return before
> this is set.

OK

> >  	const char *model;
> >
> >  	root = of_find_node_by_path("/");
> > @@ -209,8 +208,6 @@ static void __init sio_init(void)
> >  		/* select logical device 1 (KBC/Mouse) */
> >  		sio_fixup_irq("mouse", 1, 12, 2);
> >  	}
> > -
> > -	of_node_put(root);
> >  }
> >
> >
> > @@ -364,7 +361,7 @@ static void chrp_8259_cascade(struct irq
> >   */
> >  static void __init chrp_find_openpic(void)
> >  {
> > -	struct device_node *np, *root;
> > +	struct device_node *np __free(device_node), *root;
>
> This one looks dangerous because of the chance of other code
> getting added between here and the point where it is set.
>
> Better to move that down so we have
>
> struct device_node *np __free(device_node) = of_find_node_by_type(NULL, "open-pic");
>
> Also there is a nicer use in:
> https://elixir.bootlin.com/linux/v6.8-rc2/source/arch/powerpc/platforms/chrp/setup.c#L217

So the point is that we now allow declarations at random places, and not
only at the top of a function?

>
> >  	int len, i, j;
> >  	int isu_size;
> >  	const unsigned int *iranges, *opprop = NULL;
> > @@ -438,7 +435,6 @@ static void __init chrp_find_openpic(voi
> >  	ppc_md.get_irq = mpic_get_irq;
> >   bail:
> >  	of_node_put(root);
> With root covered this can return early.

Yes, this could be addressed by running the rule more than once.

Thanks for the feedback!

julia

>
> > -	of_node_put(np);
> >  }
> >
>
>
Jonathan Cameron Jan. 30, 2024, 9:38 a.m. UTC | #6 
On Mon, 29 Jan 2024 21:29:13 +0100 (CET)
Julia Lawall <julia.lawall@inria.fr> wrote:

> On Mon, 29 Jan 2024, Jonathan Cameron wrote:
> 
> > On Mon, 29 Jan 2024 15:02:19 +0100 (CET)
> > Julia Lawall <julia.lawall@inria.fr> wrote:
> >  
> > > On Mon, 29 Jan 2024, Jonathan Cameron wrote:
> > >  
> > > > On Sun, 28 Jan 2024 19:06:53 +0100 (CET)
> > > > Julia Lawall <julia.lawall@inria.fr> wrote:
> > > >  
> > > > > On Sun, 28 Jan 2024, Jonathan Cameron wrote:
> > > > >  
> > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > >
> > > > > > +CC includes peopleinterested in property.h equivalents to minimize
> > > > > > duplication of discussion.  Outcome of this discussion will affect:
> > > > > > https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> > > > > > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> > > > > >
> > > > > > In discussion of previous approach with Rob Herring we talked about various
> > > > > > ways to avoid a disconnect between the declaration of the __free(device_node)
> > > > > > and the first non NULL assignment. Making this connection clear is useful for 2
> > > > > > reasons:
> > > > > > 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> > > > > > 2) Avoids disconnect between how cleanup is to be done and how the reference
> > > > > >    was acquired in the first place.
> > > > > >
> > > > > > https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> > > > > >
> > > > > > The options we discussed are:
> > > > > >
> > > > > > 1) Ignore this issue and merge original set.
> > > > > >
> > > > > > 2) Always put the declaration just before the for loop and don't set it NULL.
> > > > > >
> > > > > > {
> > > > > > 	int ret;
> > > > > >
> > > > > > 	ret = ... and other fun code.
> > > > > >
> > > > > > 	struct device_node *child __free(device_node);
> > > > > > 	for_each_child_of_node(np, child) {
> > > > > > 	}
> > > > > > }
> > > > > >
> > > > > > This works but careful review is needed to ensure that this unusual pattern is
> > > > > > followed.  We don't set it to NULL as the loop will do that anyway if there are
> > > > > > no child nodes, or the loop finishes without an early break or return.
> > > > > >
> > > > > > 3) Introduced the pointer to auto put device_node only within the
> > > > > >    for loop scope.
> > > > > >
> > > > > > +#define for_each_child_of_node_scoped(parent, child) \
> > > > > > +	for (struct device_node *child __free(device_node) =		\
> > > > > > +	     of_get_next_child(parent, NULL);				\
> > > > > > +	     child != NULL;						\
> > > > > > +	     child = of_get_next_available_child(parent, child))
> > > > > > +
> > > > > >
> > > > > > This series is presenting option 3.  I only implemented this loop out of
> > > > > > all the similar ones and it is only compile tested.
> > > > > >
> > > > > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> > > > > > a struct device_node *child.  I can't see a way around that other than option 2
> > > > > > above, but all suggestions welcome.  Note that if a conversion leaves an
> > > > > > 'external' struct device_node *child variable, in many cases the compiler
> > > > > > will catch that as an unused variable. We don't currently run shaddow
> > > > > > variable detection in normal kernel builds, but that could also be used
> > > > > > to catch such bugs.
> > > > > >
> > > > > > All comments welcome.  
> > > > >
> > > > > It looks promising to get rid of a lot of clunky and error-prone
> > > > > error-handling code.  
> > > >
> > > > Absolutely. I think I spotted 2 bugs whilst just looking for places this pattern
> > > > doesn't apply.  Will circle back to those once this discussion is resolved.
> > > > I think I've taken dozen's of fixes for cases where these were missed over the years
> > > > so hoping this means I'll never see another one!
> > > >  
> > > > >
> > > > > I guess that
> > > > >
> > > > > for_each_child_of_node_scoped(parent, struct device_node *, child)
> > > > >
> > > > > would seem too verbose?  
> > > >
> > > > Intent just to make the allocated internal type clear?  Sure we can do that if
> > > > it helps with making it clear something is being allocated.
> > > > I can't think of a way this could be used with anything other than
> > > > a struct device_node * as the second parameter but I guess it still helps
> > > > 'hint' at what is going on..  
> >
> > To touch back on this, I'm still not sure what your intent was in suggesting
> > having the explicit struct device_node *  
> 
> Well, I wanted struct device_node * child, but then there wouldn't be the
> macro argument that would give the variable name.  So perhaps even with
> the comma it would look a little more like a declaration.
> 
> It does seem problematic to have eg two freestanding occurrences of child
> that are not in the same scope, if there is a child in the iterator
> argument list and a use elsewhere in the function.
Hi Julia,


Would be nice to have shadow variable detection enabled (QEMU did
that recently and it caught various bugs).

I'm not sure if this helps or not with readability and get the feeling
that people will perpetually send patches to remove the pointless
macro argument. I guess a comment on why it is there might avoid that
well enough though.


> 
> >  
> > > >  
> > > > >
> > > > > There are a lot of opportunities for device_node loops, but also for some
> > > > > more obscure loops over other types.  
> > > >  
> > > > > And there are a lot of of_node_puts
> > > > > that could be eliminated independent of loops.  
> > > >
> > > > The non loop cases should be handled via the __free(device_node) as provided
> > > > by patch 1.  We'll need to keep the declaration local and initial assignment
> > > > together but that is easy enough to do and similar to the many other cleanup.h
> > > > usecases that are surfacing.  
> > >
> > > I tried the following semantic patch:
> > >
> > > @@
> > > identifier x,f;
> > > attribute name __free;
> > > expression list es;
> > > expression e;
> > > statement S;
> > > @@
> > >
> > > {
> > > ... when != S
> > > struct device_node *x
> > > + __free(device_node)
> > > ;
> > > ... when strict
> > > x = f(es);
> > > <... when any
> > >      when != x = e
> > > -of_node_put(x);  
> > > ...>  
> > > -of_node_put(x);
> > > }
> > >  
> > Nice.  
> > > It is written defensively in various ways:
> > >
> > > when != S means tha tthe device_node declaration has t be at the top of
> > > the block, perhaps with other declarations before it.
> > >
> > > when strict means that all paths must lead from the declaration to the
> > > initialization.  So there is no need to intiialize the variable to NULL,
> > > as far as I understand.
> > >
> > > when != x = e means that the declared variable is not reinitialized, which
> > > would require keeping the previous of_node_put.
> > >
> > > There is an of_node_put at the end of the block, so the use of __free
> > > doesn't change the lifetime.
> > >
> > > An unfortunate aspect of the last constraint is that some functions may
> > > declare multiple device_node variables, and only one of the of_not_puts
> > > can come at the very end of the block.  This can be solved by just running
> > > the semantic patch again.
> > >
> > > An alternative would be to move the initialization up to the declaration,
> > > but the result was often a bit ugly, due to the various initialization
> > > function calls having long names and argument lists.  
> >
> > You have to do that in order to ensure there is no window for someone to
> > easily insert code that leaves them uninitialized.
> >
> > Linus had some rather specific comments on that being the only right way to
> > do it.  
> 
> OK, the semantic patch can be changed to do that.  It would have to be a
> bit more complex or a bit more defensive, to be sure that no variables are
> used between the declaration and the current call site.
> 
> >  
> > >
> > > The output is below.  I have looked quickly through all of the changes and
> > > they all look reasonable, but have not tried compiling anything (which I
> > > guess wouldn't currently work anyway).
> > >
> > > julia
> > >  
> >
> > I picked a couple from the end.  They show the sort of things that
> > would want cleaning up.  The script is great for finding low hanging
> > fruit but as you've identified there is often some stuff that isn't
> > easy to automate.
> >  
> > > diff -u -p a/drivers/clk/clk-nomadik.c b/drivers/clk/clk-nomadik.c
> > > --- a/drivers/clk/clk-nomadik.c
> > > +++ b/drivers/clk/clk-nomadik.c
> > > @@ -87,7 +87,7 @@ static const struct of_device_id nomadik
> > >
> > >  static void __init nomadik_src_init(void)
> > >  {
> > > -	struct device_node *np;
> > > +	struct device_node *np __free(device_node);
> > >  	u32 val;
> > >
> > >  	np = of_find_matching_node(NULL, nomadik_src_match);
> > > @@ -134,7 +134,6 @@ static void __init nomadik_src_init(void
> > >  	register_reboot_notifier(&nomadik_clk_reboot_notifier);
> > >
> > >  out_put:  
> > Can avoid the label given nothing to do any more.  
> 
> Indeed.
> 
> > > -	of_node_put(np);
> > >  }
> > >
> > >  /**
> > > diff -u -p a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
> > > --- a/arch/powerpc/platforms/chrp/setup.c
> > > +++ b/arch/powerpc/platforms/chrp/setup.c
> > > @@ -99,7 +99,7 @@ static void chrp_show_cpuinfo(struct seq
> > >  {
> > >  	int i, sdramen;
> > >  	unsigned int t;
> > > -	struct device_node *root;
> > > +	struct device_node *root __free(device_node);  
> >
> > Same as next 2.
> >  
> > >  	const char *model = "";
> > >
> > >  	root = of_find_node_by_path("/");
> > > @@ -152,7 +152,6 @@ static void chrp_show_cpuinfo(struct seq  
> > >  			   gg2_cachetypes[(t>>2) & 3],  
> > >  			   gg2_cachemodes[t & 3]);
> > >  	}
> > > -	of_node_put(root);
> > >  }
> > >
> > >  /*
> > > @@ -195,7 +194,7 @@ static void __init sio_fixup_irq(const c
> > >
> > >  static void __init sio_init(void)
> > >  {
> > > -	struct device_node *root;
> > > +	struct device_node *root __free(device_node);  
> >
> > As below needs to be
> > 	struct device_node *root __free(device_node) = of_find_node_by_path("/");
> > so there is no space for code to be added inbetween that might return before
> > this is set.  
> 
> OK
> 
> > >  	const char *model;
> > >
> > >  	root = of_find_node_by_path("/");
> > > @@ -209,8 +208,6 @@ static void __init sio_init(void)
> > >  		/* select logical device 1 (KBC/Mouse) */
> > >  		sio_fixup_irq("mouse", 1, 12, 2);
> > >  	}
> > > -
> > > -	of_node_put(root);
> > >  }
> > >
> > >
> > > @@ -364,7 +361,7 @@ static void chrp_8259_cascade(struct irq
> > >   */
> > >  static void __init chrp_find_openpic(void)
> > >  {
> > > -	struct device_node *np, *root;
> > > +	struct device_node *np __free(device_node), *root;  
> >
> > This one looks dangerous because of the chance of other code
> > getting added between here and the point where it is set.
> >
> > Better to move that down so we have
> >
> > struct device_node *np __free(device_node) = of_find_node_by_type(NULL, "open-pic");
> >
> > Also there is a nicer use in:
> > https://elixir.bootlin.com/linux/v6.8-rc2/source/arch/powerpc/platforms/chrp/setup.c#L217  
> 
> So the point is that we now allow declarations at random places, and not
> only at the top of a function?

yes.

https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/
And onwards in that thread.

Linus was pretty clear that constructor and destructor should come together
and gave an example at the end of that email 

+	lines = gpio_sim_count_lines(bank);
+	char **line_names __free(kfree) = kcalloc(lines, sizeof(*line_names), GFP_KERNEL);

Reality was that the cleanup.h macros were doing this anyway it just was
somewhat hidden in guard() and friends.

> 
> >  
> > >  	int len, i, j;
> > >  	int isu_size;
> > >  	const unsigned int *iranges, *opprop = NULL;
> > > @@ -438,7 +435,6 @@ static void __init chrp_find_openpic(voi
> > >  	ppc_md.get_irq = mpic_get_irq;
> > >   bail:
> > >  	of_node_put(root);  
> > With root covered this can return early.  
> 
> Yes, this could be addressed by running the rule more than once.
> 
> Thanks for the feedback!

Thanks for putting together a script so quickly.

Jonathan

> 
> julia
> 
> >  
> > > -	of_node_put(np);
> > >  }
> > >  
> >
> >
Julia Lawall Jan. 30, 2024, 10:26 a.m. UTC | #7 
On Tue, 30 Jan 2024, Jonathan Cameron wrote:

> On Mon, 29 Jan 2024 21:29:13 +0100 (CET)
> Julia Lawall <julia.lawall@inria.fr> wrote:
>
> > On Mon, 29 Jan 2024, Jonathan Cameron wrote:
> >
> > > On Mon, 29 Jan 2024 15:02:19 +0100 (CET)
> > > Julia Lawall <julia.lawall@inria.fr> wrote:
> > >
> > > > On Mon, 29 Jan 2024, Jonathan Cameron wrote:
> > > >
> > > > > On Sun, 28 Jan 2024 19:06:53 +0100 (CET)
> > > > > Julia Lawall <julia.lawall@inria.fr> wrote:
> > > > >
> > > > > > On Sun, 28 Jan 2024, Jonathan Cameron wrote:
> > > > > >
> > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > >
> > > > > > > +CC includes peopleinterested in property.h equivalents to minimize
> > > > > > > duplication of discussion.  Outcome of this discussion will affect:
> > > > > > > https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> > > > > > > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> > > > > > >
> > > > > > > In discussion of previous approach with Rob Herring we talked about various
> > > > > > > ways to avoid a disconnect between the declaration of the __free(device_node)
> > > > > > > and the first non NULL assignment. Making this connection clear is useful for 2
> > > > > > > reasons:
> > > > > > > 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> > > > > > > 2) Avoids disconnect between how cleanup is to be done and how the reference
> > > > > > >    was acquired in the first place.
> > > > > > >
> > > > > > > https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> > > > > > >
> > > > > > > The options we discussed are:
> > > > > > >
> > > > > > > 1) Ignore this issue and merge original set.
> > > > > > >
> > > > > > > 2) Always put the declaration just before the for loop and don't set it NULL.
> > > > > > >
> > > > > > > {
> > > > > > > 	int ret;
> > > > > > >
> > > > > > > 	ret = ... and other fun code.
> > > > > > >
> > > > > > > 	struct device_node *child __free(device_node);
> > > > > > > 	for_each_child_of_node(np, child) {
> > > > > > > 	}
> > > > > > > }
> > > > > > >
> > > > > > > This works but careful review is needed to ensure that this unusual pattern is
> > > > > > > followed.  We don't set it to NULL as the loop will do that anyway if there are
> > > > > > > no child nodes, or the loop finishes without an early break or return.
> > > > > > >
> > > > > > > 3) Introduced the pointer to auto put device_node only within the
> > > > > > >    for loop scope.
> > > > > > >
> > > > > > > +#define for_each_child_of_node_scoped(parent, child) \
> > > > > > > +	for (struct device_node *child __free(device_node) =		\
> > > > > > > +	     of_get_next_child(parent, NULL);				\
> > > > > > > +	     child != NULL;						\
> > > > > > > +	     child = of_get_next_available_child(parent, child))
> > > > > > > +
> > > > > > >
> > > > > > > This series is presenting option 3.  I only implemented this loop out of
> > > > > > > all the similar ones and it is only compile tested.
> > > > > > >
> > > > > > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> > > > > > > a struct device_node *child.  I can't see a way around that other than option 2
> > > > > > > above, but all suggestions welcome.  Note that if a conversion leaves an
> > > > > > > 'external' struct device_node *child variable, in many cases the compiler
> > > > > > > will catch that as an unused variable. We don't currently run shaddow
> > > > > > > variable detection in normal kernel builds, but that could also be used
> > > > > > > to catch such bugs.
> > > > > > >
> > > > > > > All comments welcome.
> > > > > >
> > > > > > It looks promising to get rid of a lot of clunky and error-prone
> > > > > > error-handling code.
> > > > >
> > > > > Absolutely. I think I spotted 2 bugs whilst just looking for places this pattern
> > > > > doesn't apply.  Will circle back to those once this discussion is resolved.
> > > > > I think I've taken dozen's of fixes for cases where these were missed over the years
> > > > > so hoping this means I'll never see another one!
> > > > >
> > > > > >
> > > > > > I guess that
> > > > > >
> > > > > > for_each_child_of_node_scoped(parent, struct device_node *, child)
> > > > > >
> > > > > > would seem too verbose?
> > > > >
> > > > > Intent just to make the allocated internal type clear?  Sure we can do that if
> > > > > it helps with making it clear something is being allocated.
> > > > > I can't think of a way this could be used with anything other than
> > > > > a struct device_node * as the second parameter but I guess it still helps
> > > > > 'hint' at what is going on..
> > >
> > > To touch back on this, I'm still not sure what your intent was in suggesting
> > > having the explicit struct device_node *
> >
> > Well, I wanted struct device_node * child, but then there wouldn't be the
> > macro argument that would give the variable name.  So perhaps even with
> > the comma it would look a little more like a declaration.
> >
> > It does seem problematic to have eg two freestanding occurrences of child
> > that are not in the same scope, if there is a child in the iterator
> > argument list and a use elsewhere in the function.
> Hi Julia,
>
>
> Would be nice to have shadow variable detection enabled (QEMU did
> that recently and it caught various bugs).
>
> I'm not sure if this helps or not with readability and get the feeling
> that people will perpetually send patches to remove the pointless
> macro argument. I guess a comment on why it is there might avoid that
> well enough though.

OK, it was a suggestion.  Even I was not convinced that it was a good
suggestion :)


> > > > > > There are a lot of opportunities for device_node loops, but also for some
> > > > > > more obscure loops over other types.
> > > > >
> > > > > > And there are a lot of of_node_puts
> > > > > > that could be eliminated independent of loops.
> > > > >
> > > > > The non loop cases should be handled via the __free(device_node) as provided
> > > > > by patch 1.  We'll need to keep the declaration local and initial assignment
> > > > > together but that is easy enough to do and similar to the many other cleanup.h
> > > > > usecases that are surfacing.
> > > >
> > > > I tried the following semantic patch:
> > > >
> > > > @@
> > > > identifier x,f;
> > > > attribute name __free;
> > > > expression list es;
> > > > expression e;
> > > > statement S;
> > > > @@
> > > >
> > > > {
> > > > ... when != S
> > > > struct device_node *x
> > > > + __free(device_node)
> > > > ;
> > > > ... when strict
> > > > x = f(es);
> > > > <... when any
> > > >      when != x = e
> > > > -of_node_put(x);
> > > > ...>
> > > > -of_node_put(x);
> > > > }
> > > >
> > > Nice.
> > > > It is written defensively in various ways:
> > > >
> > > > when != S means tha tthe device_node declaration has t be at the top of
> > > > the block, perhaps with other declarations before it.
> > > >
> > > > when strict means that all paths must lead from the declaration to the
> > > > initialization.  So there is no need to intiialize the variable to NULL,
> > > > as far as I understand.
> > > >
> > > > when != x = e means that the declared variable is not reinitialized, which
> > > > would require keeping the previous of_node_put.
> > > >
> > > > There is an of_node_put at the end of the block, so the use of __free
> > > > doesn't change the lifetime.
> > > >
> > > > An unfortunate aspect of the last constraint is that some functions may
> > > > declare multiple device_node variables, and only one of the of_not_puts
> > > > can come at the very end of the block.  This can be solved by just running
> > > > the semantic patch again.
> > > >
> > > > An alternative would be to move the initialization up to the declaration,
> > > > but the result was often a bit ugly, due to the various initialization
> > > > function calls having long names and argument lists.
> > >
> > > You have to do that in order to ensure there is no window for someone to
> > > easily insert code that leaves them uninitialized.
> > >
> > > Linus had some rather specific comments on that being the only right way to
> > > do it.
> >
> > OK, the semantic patch can be changed to do that.  It would have to be a
> > bit more complex or a bit more defensive, to be sure that no variables are
> > used between the declaration and the current call site.
> >
> > >
> > > >
> > > > The output is below.  I have looked quickly through all of the changes and
> > > > they all look reasonable, but have not tried compiling anything (which I
> > > > guess wouldn't currently work anyway).
> > > >
> > > > julia
> > > >
> > >
> > > I picked a couple from the end.  They show the sort of things that
> > > would want cleaning up.  The script is great for finding low hanging
> > > fruit but as you've identified there is often some stuff that isn't
> > > easy to automate.
> > >
> > > > diff -u -p a/drivers/clk/clk-nomadik.c b/drivers/clk/clk-nomadik.c
> > > > --- a/drivers/clk/clk-nomadik.c
> > > > +++ b/drivers/clk/clk-nomadik.c
> > > > @@ -87,7 +87,7 @@ static const struct of_device_id nomadik
> > > >
> > > >  static void __init nomadik_src_init(void)
> > > >  {
> > > > -	struct device_node *np;
> > > > +	struct device_node *np __free(device_node);
> > > >  	u32 val;
> > > >
> > > >  	np = of_find_matching_node(NULL, nomadik_src_match);
> > > > @@ -134,7 +134,6 @@ static void __init nomadik_src_init(void
> > > >  	register_reboot_notifier(&nomadik_clk_reboot_notifier);
> > > >
> > > >  out_put:
> > > Can avoid the label given nothing to do any more.
> >
> > Indeed.
> >
> > > > -	of_node_put(np);
> > > >  }
> > > >
> > > >  /**
> > > > diff -u -p a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
> > > > --- a/arch/powerpc/platforms/chrp/setup.c
> > > > +++ b/arch/powerpc/platforms/chrp/setup.c
> > > > @@ -99,7 +99,7 @@ static void chrp_show_cpuinfo(struct seq
> > > >  {
> > > >  	int i, sdramen;
> > > >  	unsigned int t;
> > > > -	struct device_node *root;
> > > > +	struct device_node *root __free(device_node);
> > >
> > > Same as next 2.
> > >
> > > >  	const char *model = "";
> > > >
> > > >  	root = of_find_node_by_path("/");
> > > > @@ -152,7 +152,6 @@ static void chrp_show_cpuinfo(struct seq
> > > >  			   gg2_cachetypes[(t>>2) & 3],
> > > >  			   gg2_cachemodes[t & 3]);
> > > >  	}
> > > > -	of_node_put(root);
> > > >  }
> > > >
> > > >  /*
> > > > @@ -195,7 +194,7 @@ static void __init sio_fixup_irq(const c
> > > >
> > > >  static void __init sio_init(void)
> > > >  {
> > > > -	struct device_node *root;
> > > > +	struct device_node *root __free(device_node);
> > >
> > > As below needs to be
> > > 	struct device_node *root __free(device_node) = of_find_node_by_path("/");
> > > so there is no space for code to be added inbetween that might return before
> > > this is set.
> >
> > OK
> >
> > > >  	const char *model;
> > > >
> > > >  	root = of_find_node_by_path("/");
> > > > @@ -209,8 +208,6 @@ static void __init sio_init(void)
> > > >  		/* select logical device 1 (KBC/Mouse) */
> > > >  		sio_fixup_irq("mouse", 1, 12, 2);
> > > >  	}
> > > > -
> > > > -	of_node_put(root);
> > > >  }
> > > >
> > > >
> > > > @@ -364,7 +361,7 @@ static void chrp_8259_cascade(struct irq
> > > >   */
> > > >  static void __init chrp_find_openpic(void)
> > > >  {
> > > > -	struct device_node *np, *root;
> > > > +	struct device_node *np __free(device_node), *root;
> > >
> > > This one looks dangerous because of the chance of other code
> > > getting added between here and the point where it is set.
> > >
> > > Better to move that down so we have
> > >
> > > struct device_node *np __free(device_node) = of_find_node_by_type(NULL, "open-pic");
> > >
> > > Also there is a nicer use in:
> > > https://elixir.bootlin.com/linux/v6.8-rc2/source/arch/powerpc/platforms/chrp/setup.c#L217
> >
> > So the point is that we now allow declarations at random places, and not
> > only at the top of a function?
>
> yes.
>
> https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/
> And onwards in that thread.
>
> Linus was pretty clear that constructor and destructor should come together
> and gave an example at the end of that email
>
> +	lines = gpio_sim_count_lines(bank);
> +	char **line_names __free(kfree) = kcalloc(lines, sizeof(*line_names), GFP_KERNEL);
>
> Reality was that the cleanup.h macros were doing this anyway it just was
> somewhat hidden in guard() and friends.
>
> >
> > >
> > > >  	int len, i, j;
> > > >  	int isu_size;
> > > >  	const unsigned int *iranges, *opprop = NULL;
> > > > @@ -438,7 +435,6 @@ static void __init chrp_find_openpic(voi
> > > >  	ppc_md.get_irq = mpic_get_irq;
> > > >   bail:
> > > >  	of_node_put(root);
> > > With root covered this can return early.
> >
> > Yes, this could be addressed by running the rule more than once.
> >
> > Thanks for the feedback!
>
> Thanks for putting together a script so quickly.

I have updated it to take into account most of the mentioned issues
(gotos, etc).

In my version, I move the call up to the original declaration site, when
that is safe to do.  A negative aspect is that the original call site may
have been preceeded by comments, which get removed.  It's hard to know
what should be done with comments, so that has to be addressed manually.
But if we can leave the call site where it is, the comments should
naturally stay there too.

There remains the problem of very long lines.  Coccinelle has some
heuristics for dealing with this, but they don't fit well with this case,
where something before the function name is already very long.

Anyway, I will send a new version later today to illustrate what can be
done.  No implication that the semantic patch should be sent off to Linus
for immediate application across the kernel.  This seems like something
that should be checked more carefully, especially since there is the
opportunity to fix bugs along the way (I saw one case with a missing
of_node_put on an error path).

julia


>
> Jonathan
>
> >
> > julia
> >
> > >
> > > > -	of_node_put(np);
> > > >  }
> > > >
> > >
> > >
>
>
Julia Lawall Jan. 31, 2024, 9:38 p.m. UTC | #8 
Here are some loop cases.  The semantic patch is as follows:

#spatch --allow-inconsistent-paths

@@
expression node;
identifier child;
symbol drop_me;
iterator name for_each_child_of_node;
@@

for_each_child_of_node(node,child) {
  ...
+ of_node_put(drop_me, child);
}

@@
expression node;
identifier child;
symbol drop_me;
iterator name for_each_child_of_node, for_each_child_of_node_scoped;
identifier L;
@@

- struct device_node *child;
 ... when != child
-for_each_child_of_node
+for_each_child_of_node_scoped
  (node,child) {
   ... when strict
(
-   {
-   of_node_put(child);
    return ...;
-   }
|
-   {
-   of_node_put(child);
    goto L;
-   }
|
-   {
-   of_node_put(child);
    break;
-   }
|
    continue;
|
-   of_node_put(child);
    return ...;
|
-   of_node_put(child);
    break;
|
-  of_node_put(drop_me, child);
)
}
 ... when != child

@@
expression child;
@@

- of_node_put(drop_me, child);

-------------------------------

This is quite conservative, in that it requires the only use of the child
variable to be in a single for_each_child_of_node loop at top level.

The drop_me thing is a hack to be able to refer to the bottom of the loop
in the same way as of_node_puts in front of returns etc are referenced.

This works fine when multiple device_node variables are declared at once.

The result is below.

julia

diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2789,7 +2789,7 @@ static int unittest_i2c_mux_probe(struct
 	int i, nchans;
 	struct device *dev = &client->dev;
 	struct i2c_adapter *adap = client->adapter;
-	struct device_node *np = client->dev.of_node, *child;
+	struct device_node *np = client->dev.of_node;
 	struct i2c_mux_core *muxc;
 	u32 reg, max_reg;

@@ -2801,7 +2801,7 @@ static int unittest_i2c_mux_probe(struct
 	}

 	max_reg = (u32)-1;
-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (of_property_read_u32(child, "reg", &reg))
 			continue;
 		if (max_reg == (u32)-1 || reg > max_reg)
diff -u -p a/drivers/mtd/devices/spear_smi.c b/drivers/mtd/devices/spear_smi.c
--- a/drivers/mtd/devices/spear_smi.c
+++ b/drivers/mtd/devices/spear_smi.c
@@ -793,7 +793,6 @@ static int spear_smi_probe_config_dt(str
 				     struct device_node *np)
 {
 	struct spear_smi_plat_data *pdata = dev_get_platdata(&pdev->dev);
-	struct device_node *pp;
 	const __be32 *addr;
 	u32 val;
 	int len;
@@ -812,7 +811,7 @@ static int spear_smi_probe_config_dt(str
 		return -ENOMEM;

 	/* Fill structs for each subnode (flash device) */
-	for_each_child_of_node(np, pp) {
+	for_each_child_of_node_scoped(np, pp) {
 		pdata->np[i] = pp;

 		/* Read base-addr and size from DT */
diff -u -p a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -137,7 +137,6 @@ static const struct of_device_id psci_of
 static int psci_cpuidle_domain_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *node;
 	bool use_osi = psci_has_osi_support();
 	int ret = 0, pd_count = 0;

@@ -148,15 +147,13 @@ static int psci_cpuidle_domain_probe(str
 	 * Parse child nodes for the "#power-domain-cells" property and
 	 * initialize a genpd/genpd-of-provider pair when it's found.
 	 */
-	for_each_child_of_node(np, node) {
+	for_each_child_of_node_scoped(np, node) {
 		if (!of_property_present(node, "#power-domain-cells"))
 			continue;

 		ret = psci_pd_init(node, use_osi);
-		if (ret) {
-			of_node_put(node);
+		if (ret)
 			goto exit;
-		}

 		pd_count++;
 	}
diff -u -p a/drivers/pinctrl/renesas/pinctrl.c b/drivers/pinctrl/renesas/pinctrl.c
--- a/drivers/pinctrl/renesas/pinctrl.c
+++ b/drivers/pinctrl/renesas/pinctrl.c
@@ -241,7 +241,6 @@ static int sh_pfc_dt_node_to_map(struct
 {
 	struct sh_pfc_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
 	struct device *dev = pmx->pfc->dev;
-	struct device_node *child;
 	unsigned int index;
 	int ret;

@@ -249,13 +248,11 @@ static int sh_pfc_dt_node_to_map(struct
 	*num_maps = 0;
 	index = 0;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = sh_pfc_dt_subnode_to_map(pctldev, child, map, num_maps,
 					       &index);
-		if (ret < 0) {
-			of_node_put(child);
+		if (ret < 0)
 			goto done;
-		}
 	}

 	/* If no mapping has been found in child nodes try the config node. */
diff -u -p a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -2715,19 +2715,16 @@ static int lm90_parse_dt_channel_info(st
 				      struct lm90_data *data)
 {
 	int err;
-	struct device_node *child;
 	struct device *dev = &client->dev;
 	const struct device_node *np = dev->of_node;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (strcmp(child->name, "channel"))
 			continue;

 		err = lm90_probe_channel_from_dt(client, child, data);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}

 	return 0;
diff -u -p a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -410,7 +410,6 @@ static int cmp_timings(const void *_a, c
 static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
 					  struct device_node *node)
 {
-	struct device_node *child;
 	struct emc_timing *timing;
 	int child_count;
 	int err;
@@ -428,15 +427,13 @@ static int tegra_emc_load_timings_from_d

 	timing = emc->timings;

-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		if (of_node_name_eq(child, "lpddr2"))
 			continue;

 		err = load_one_timing_from_dt(emc, timing++, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}

 		emc->num_timings++;
 	}
diff -u -p a/arch/powerpc/platforms/powermac/low_i2c.c b/arch/powerpc/platforms/powermac/low_i2c.c
--- a/arch/powerpc/platforms/powermac/low_i2c.c
+++ b/arch/powerpc/platforms/powermac/low_i2c.c
@@ -895,7 +895,7 @@ static int smu_i2c_xfer(struct pmac_i2c_

 static void __init smu_i2c_probe(void)
 {
-	struct device_node *controller, *busnode;
+	struct device_node *controller;
 	struct pmac_i2c_bus *bus;
 	const u32 *reg;
 	int sz;
@@ -915,7 +915,7 @@ static void __init smu_i2c_probe(void)
 	 * type as older device trees mix i2c busses and other things
 	 * at the same level
 	 */
-	for_each_child_of_node(controller, busnode) {
+	for_each_child_of_node_scoped(controller, busnode) {
 		if (!of_node_is_type(busnode, "i2c") &&
 		    !of_node_is_type(busnode, "i2c-bus"))
 			continue;
@@ -925,10 +925,8 @@ static void __init smu_i2c_probe(void)

 		sz = sizeof(struct pmac_i2c_bus) + sizeof(struct smu_i2c_cmd);
 		bus = kzalloc(sz, GFP_KERNEL);
-		if (bus == NULL) {
-			of_node_put(busnode);
+		if (bus == NULL)
 			return;
-		}

 		bus->controller = controller;
 		bus->busnode = of_node_get(busnode);
diff -u -p a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
--- a/drivers/regulator/scmi-regulator.c
+++ b/drivers/regulator/scmi-regulator.c
@@ -297,7 +297,7 @@ static int process_scmi_regulator_of_nod
 static int scmi_regulator_probe(struct scmi_device *sdev)
 {
 	int d, ret, num_doms;
-	struct device_node *np, *child;
+	struct device_node *np;
 	const struct scmi_handle *handle = sdev->handle;
 	struct scmi_regulator_info *rinfo;
 	struct scmi_protocol_handle *ph;
@@ -341,13 +341,11 @@ static int scmi_regulator_probe(struct s
 	 */
 	of_node_get(handle->dev->of_node);
 	np = of_find_node_by_name(handle->dev->of_node, "regulators");
-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo);
 		/* abort on any mem issue */
-		if (ret == -ENOMEM) {
-			of_node_put(child);
+		if (ret == -ENOMEM)
 			return ret;
-		}
 	}
 	of_node_put(np);
 	/*
diff -u -p a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -244,7 +244,7 @@ samsung_keypad_parse_dt(struct device *d
 	struct samsung_keypad_platdata *pdata;
 	struct matrix_keymap_data *keymap_data;
 	uint32_t *keymap, num_rows = 0, num_cols = 0;
-	struct device_node *np = dev->of_node, *key_np;
+	struct device_node *np = dev->of_node;
 	unsigned int key_count;

 	if (!np) {
@@ -283,7 +283,7 @@ samsung_keypad_parse_dt(struct device *d
 	}
 	keymap_data->keymap = keymap;

-	for_each_child_of_node(np, key_np) {
+	for_each_child_of_node_scoped(np, key_np) {
 		u32 row, col, key_code;
 		of_property_read_u32(key_np, "keypad,row", &row);
 		of_property_read_u32(key_np, "keypad,column", &col);
diff -u -p a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c
--- a/drivers/crypto/nx/nx-common-powernv.c
+++ b/drivers/crypto/nx/nx-common-powernv.c
@@ -907,7 +907,6 @@ static int __init nx_powernv_probe_vas(s
 {
 	int chip_id, vasid, ret = 0;
 	int ct_842 = 0, ct_gzip = 0;
-	struct device_node *dn;

 	chip_id = of_get_ibm_chip_id(pn);
 	if (chip_id < 0) {
@@ -921,7 +920,7 @@ static int __init nx_powernv_probe_vas(s
 		return -EINVAL;
 	}

-	for_each_child_of_node(pn, dn) {
+	for_each_child_of_node_scoped(pn, dn) {
 		ret = find_nx_device_tree(dn, chip_id, vasid, NX_CT_842,
 					"ibm,p9-nx-842", &ct_842);

@@ -929,10 +928,8 @@ static int __init nx_powernv_probe_vas(s
 			ret = find_nx_device_tree(dn, chip_id, vasid,
 				NX_CT_GZIP, "ibm,p9-nx-gzip", &ct_gzip);

-		if (ret) {
-			of_node_put(dn);
+		if (ret)
 			return ret;
-		}
 	}

 	if (!ct_842 || !ct_gzip) {
diff -u -p a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
--- a/drivers/pinctrl/pinctrl-st.c
+++ b/drivers/pinctrl/pinctrl-st.c
@@ -1271,7 +1271,6 @@ static int st_pctl_parse_functions(struc
 			struct st_pinctrl *info, u32 index, int *grp_index)
 {
 	struct device *dev = info->dev;
-	struct device_node *child;
 	struct st_pmx_func *func;
 	struct st_pctl_group *grp;
 	int ret, i;
@@ -1286,15 +1285,13 @@ static int st_pctl_parse_functions(struc
 		return -ENOMEM;

 	i = 0;
-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		func->groups[i] = child->name;
 		grp = &info->groups[*grp_index];
 		*grp_index += 1;
 		ret = st_pctl_dt_parse_groups(child, grp, info, i++);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			return ret;
-		}
 	}
 	dev_info(dev, "Function[%d\t name:%s,\tgroups:%d]\n", index, func->name, func->ngroups);

diff -u -p a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
--- a/drivers/slimbus/qcom-ngd-ctrl.c
+++ b/drivers/slimbus/qcom-ngd-ctrl.c
@@ -1230,9 +1230,8 @@ static int qcom_slim_ngd_power_up(struct
 static void qcom_slim_ngd_notify_slaves(struct qcom_slim_ngd_ctrl *ctrl)
 {
 	struct slim_device *sbdev;
-	struct device_node *node;

-	for_each_child_of_node(ctrl->ngd->pdev->dev.of_node, node) {
+	for_each_child_of_node_scoped(ctrl->ngd->pdev->dev.of_node, node) {
 		sbdev = of_slim_get_device(&ctrl->ctrl, node);
 		if (!sbdev)
 			continue;
diff -u -p a/arch/powerpc/platforms/cell/setup.c b/arch/powerpc/platforms/cell/setup.c
--- a/arch/powerpc/platforms/cell/setup.c
+++ b/arch/powerpc/platforms/cell/setup.c
@@ -154,7 +154,6 @@ static const struct of_device_id cell_bu
 static int __init cell_publish_devices(void)
 {
 	struct device_node *root = of_find_node_by_path("/");
-	struct device_node *np;
 	int node;

 	/* Publish OF platform devices for southbridge IOs */
@@ -163,7 +162,7 @@ static int __init cell_publish_devices(v
 	/* On spider based blades, we need to manually create the OF
 	 * platform devices for the PCI host bridges
 	 */
-	for_each_child_of_node(root, np) {
+	for_each_child_of_node_scoped(root, np) {
 		if (!of_node_is_type(np, "pci") && !of_node_is_type(np, "pciex"))
 			continue;
 		of_platform_device_create(np, NULL, NULL);
diff -u -p a/drivers/of/platform.c b/drivers/of/platform.c
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -336,7 +336,6 @@ static int of_platform_bus_create(struct
 				  struct device *parent, bool strict)
 {
 	const struct of_dev_auxdata *auxdata;
-	struct device_node *child;
 	struct platform_device *dev;
 	const char *bus_id = NULL;
 	void *platform_data = NULL;
@@ -380,13 +379,11 @@ static int of_platform_bus_create(struct
 	if (!dev || !of_match_node(matches, bus))
 		return 0;

-	for_each_child_of_node(bus, child) {
+	for_each_child_of_node_scoped(bus, child) {
 		pr_debug("   create child: %pOF\n", child);
 		rc = of_platform_bus_create(child, matches, lookup, &dev->dev, strict);
-		if (rc) {
-			of_node_put(child);
+		if (rc)
 			break;
-		}
 	}
 	of_node_set_flag(bus, OF_POPULATED_BUS);
 	return rc;
@@ -457,7 +454,6 @@ int of_platform_populate(struct device_n
 			const struct of_dev_auxdata *lookup,
 			struct device *parent)
 {
-	struct device_node *child;
 	int rc = 0;

 	root = root ? of_node_get(root) : of_find_node_by_path("/");
@@ -468,12 +464,10 @@ int of_platform_populate(struct device_n
 	pr_debug(" starting at: %pOF\n", root);

 	device_links_supplier_sync_state_pause();
-	for_each_child_of_node(root, child) {
+	for_each_child_of_node_scoped(root, child) {
 		rc = of_platform_bus_create(child, matches, lookup, parent, true);
-		if (rc) {
-			of_node_put(child);
+		if (rc)
 			break;
-		}
 	}
 	device_links_supplier_sync_state_resume();

diff -u -p a/drivers/cpuidle/dt_idle_genpd.c b/drivers/cpuidle/dt_idle_genpd.c
--- a/drivers/cpuidle/dt_idle_genpd.c
+++ b/drivers/cpuidle/dt_idle_genpd.c
@@ -130,11 +130,10 @@ out:

 int dt_idle_pd_init_topology(struct device_node *np)
 {
-	struct device_node *node;
 	struct of_phandle_args child, parent;
 	int ret;

-	for_each_child_of_node(np, node) {
+	for_each_child_of_node_scoped(np, node) {
 		if (of_parse_phandle_with_args(node, "power-domains",
 					"#power-domain-cells", 0, &parent))
 			continue;
@@ -143,10 +142,8 @@ int dt_idle_pd_init_topology(struct devi
 		child.args_count = 0;
 		ret = of_genpd_add_subdomain(&parent, &child);
 		of_node_put(parent.np);
-		if (ret) {
-			of_node_put(node);
+		if (ret)
 			return ret;
-		}
 	}

 	return 0;
@@ -154,11 +151,10 @@ int dt_idle_pd_init_topology(struct devi

 int dt_idle_pd_remove_topology(struct device_node *np)
 {
-	struct device_node *node;
 	struct of_phandle_args child, parent;
 	int ret;

-	for_each_child_of_node(np, node) {
+	for_each_child_of_node_scoped(np, node) {
 		if (of_parse_phandle_with_args(node, "power-domains",
 					"#power-domain-cells", 0, &parent))
 			continue;
@@ -167,10 +163,8 @@ int dt_idle_pd_remove_topology(struct de
 		child.args_count = 0;
 		ret = of_genpd_remove_subdomain(&parent, &child);
 		of_node_put(parent.np);
-		if (ret) {
-			of_node_put(node);
+		if (ret)
 			return ret;
-		}
 	}

 	return 0;
diff -u -p a/sound/soc/meson/axg-card.c b/sound/soc/meson/axg-card.c
--- a/sound/soc/meson/axg-card.c
+++ b/sound/soc/meson/axg-card.c
@@ -221,7 +221,6 @@ static int axg_card_parse_codecs_masks(s
 				       struct axg_dai_link_tdm_data *be)
 {
 	struct axg_dai_link_tdm_mask *codec_mask;
-	struct device_node *np;

 	codec_mask = devm_kcalloc(card->dev, link->num_codecs,
 				  sizeof(*codec_mask), GFP_KERNEL);
@@ -230,7 +229,7 @@ static int axg_card_parse_codecs_masks(s

 	be->codec_masks = codec_mask;

-	for_each_child_of_node(node, np) {
+	for_each_child_of_node_scoped(node, np) {
 		snd_soc_of_get_slot_mask(np, "dai-tdm-slot-rx-mask",
 					 &codec_mask->rx);
 		snd_soc_of_get_slot_mask(np, "dai-tdm-slot-tx-mask",
diff -u -p a/drivers/mtd/nand/raw/renesas-nand-controller.c b/drivers/mtd/nand/raw/renesas-nand-controller.c
--- a/drivers/mtd/nand/raw/renesas-nand-controller.c
+++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
@@ -1297,15 +1297,12 @@ static void rnandc_chips_cleanup(struct

 static int rnandc_chips_init(struct rnandc *rnandc)
 {
-	struct device_node *np;
 	int ret;

-	for_each_child_of_node(rnandc->dev->of_node, np) {
+	for_each_child_of_node_scoped(rnandc->dev->of_node, np) {
 		ret = rnandc_chip_init(rnandc, np);
-		if (ret) {
-			of_node_put(np);
+		if (ret)
 			goto cleanup_chips;
-		}
 	}

 	return 0;
diff -u -p a/drivers/mfd/tc3589x.c b/drivers/mfd/tc3589x.c
--- a/drivers/mfd/tc3589x.c
+++ b/drivers/mfd/tc3589x.c
@@ -330,7 +330,6 @@ tc3589x_of_probe(struct device *dev, enu
 {
 	struct device_node *np = dev->of_node;
 	struct tc3589x_platform_data *pdata;
-	struct device_node *child;
 	const struct of_device_id *of_id;

 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
@@ -342,7 +341,7 @@ tc3589x_of_probe(struct device *dev, enu
 		return ERR_PTR(-ENODEV);
 	*version = (uintptr_t) of_id->data;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (of_device_is_compatible(child, "toshiba,tc3589x-gpio"))
 			pdata->block |= TC3589x_BLOCK_GPIO;
 		if (of_device_is_compatible(child, "toshiba,tc3589x-keypad"))
diff -u -p a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
--- a/drivers/memory/tegra/tegra30-emc.c
+++ b/drivers/memory/tegra/tegra30-emc.c
@@ -979,7 +979,6 @@ static int emc_check_mc_timings(struct t
 static int emc_load_timings_from_dt(struct tegra_emc *emc,
 				    struct device_node *node)
 {
-	struct device_node *child;
 	struct emc_timing *timing;
 	int child_count;
 	int err;
@@ -998,12 +997,10 @@ static int emc_load_timings_from_dt(stru
 	emc->num_timings = child_count;
 	timing = emc->timings;

-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		err = load_one_timing_from_dt(emc, timing++, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}

 	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,
diff -u -p a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
--- a/drivers/iio/adc/stm32-adc-core.c
+++ b/drivers/iio/adc/stm32-adc-core.c
@@ -665,7 +665,6 @@ static int stm32_adc_probe_identificatio
 					  struct stm32_adc_priv *priv)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *child;
 	const char *compat;
 	int ret, count = 0;
 	u32 id, val;
@@ -680,7 +679,7 @@ static int stm32_adc_probe_identificatio
 		return -EINVAL;
 	}

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = of_property_read_string(child, "compatible", &compat);
 		if (ret)
 			continue;
diff -u -p a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -3054,7 +3054,7 @@ static int xilinx_dma_probe(struct platf
 					= axivdma_clk_init;
 	struct device_node *node = pdev->dev.of_node;
 	struct xilinx_dma_device *xdev;
-	struct device_node *child, *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node;
 	u32 num_frames, addr_width, len_width;
 	int i, err;

@@ -3189,12 +3189,10 @@ static int xilinx_dma_probe(struct platf
 	platform_set_drvdata(pdev, xdev);

 	/* Initialize the channels */
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		err = xilinx_dma_child_probe(xdev, child);
-		if (err < 0) {
-			of_node_put(child);
+		if (err < 0)
 			goto error;
-		}
 	}

 	if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {
diff -u -p a/sound/soc/sh/rcar/ssiu.c b/sound/soc/sh/rcar/ssiu.c
--- a/sound/soc/sh/rcar/ssiu.c
+++ b/sound/soc/sh/rcar/ssiu.c
@@ -478,17 +478,14 @@ void rsnd_parse_connect_ssiu(struct rsnd

 	/* use rcar_sound,ssiu if exist */
 	if (node) {
-		struct device_node *np;
 		int i = 0;

-		for_each_child_of_node(node, np) {
+		for_each_child_of_node_scoped(node, np) {
 			struct rsnd_mod *mod;

 			i = rsnd_node_fixed_index(dev, np, SSIU_NAME, i);
-			if (i < 0) {
-				of_node_put(np);
+			if (i < 0)
 				break;
-			}

 			mod = rsnd_ssiu_mod_get(priv, i);

diff -u -p a/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c b/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c
--- a/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c
@@ -238,20 +238,17 @@ static int tegra_xusb_padctl_dt_node_to_
 {
 	struct tegra_xusb_padctl *padctl = pinctrl_dev_get_drvdata(pinctrl);
 	unsigned int reserved_maps = 0;
-	struct device_node *np;
 	int err;

 	*num_maps = 0;
 	*maps = NULL;

-	for_each_child_of_node(parent, np) {
+	for_each_child_of_node_scoped(parent, np) {
 		err = tegra_xusb_padctl_parse_subnode(padctl, np, maps,
 						      &reserved_maps,
 						      num_maps);
-		if (err < 0) {
-			of_node_put(np);
+		if (err < 0)
 			return err;
-		}
 	}

 	return 0;
diff -u -p a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -76,7 +76,7 @@ struct rockchip_rgb *rockchip_rgb_init(s
 {
 	struct rockchip_rgb *rgb;
 	struct drm_encoder *encoder;
-	struct device_node *port, *endpoint;
+	struct device_node *port;
 	u32 endpoint_id;
 	int ret = 0, child_count = 0;
 	struct drm_panel *panel;
@@ -94,7 +94,7 @@ struct rockchip_rgb *rockchip_rgb_init(s
 	if (!port)
 		return ERR_PTR(-EINVAL);

-	for_each_child_of_node(port, endpoint) {
+	for_each_child_of_node_scoped(port, endpoint) {
 		if (of_property_read_u32(endpoint, "reg", &endpoint_id))
 			endpoint_id = 0;

@@ -105,10 +105,8 @@ struct rockchip_rgb *rockchip_rgb_init(s
 		child_count++;
 		ret = drm_of_find_panel_or_bridge(dev->of_node, video_port,
 						  endpoint_id, &panel, &bridge);
-		if (!ret) {
-			of_node_put(endpoint);
+		if (!ret)
 			break;
-		}
 	}

 	of_node_put(port);
diff -u -p a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -544,7 +544,6 @@ static void pci_of_scan_bus(struct pci_p
 			    struct device_node *node,
 			    struct pci_bus *bus)
 {
-	struct device_node *child;
 	const u32 *reg;
 	int reglen, devfn, prev_devfn;
 	struct pci_dev *dev;
@@ -554,7 +553,7 @@ static void pci_of_scan_bus(struct pci_p
 			 node, bus->number);

 	prev_devfn = -1;
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		if (ofpci_verbose)
 			pci_info(bus, "  * %pOF\n", child);
 		reg = of_get_property(child, "reg", &reglen);
diff -u -p a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1437,7 +1437,7 @@ static int tegra_powergate_init(struct t
 				struct device_node *parent)
 {
 	struct of_phandle_args child_args, parent_args;
-	struct device_node *np, *child;
+	struct device_node *np;
 	int err = 0;

 	/*
@@ -1456,12 +1456,10 @@ static int tegra_powergate_init(struct t
 	if (!np)
 		return 0;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		err = tegra_powergate_add(pmc, child);
-		if (err < 0) {
-			of_node_put(child);
+		if (err < 0)
 			break;
-		}

 		if (of_parse_phandle_with_args(child, "power-domains",
 					       "#power-domain-cells",
@@ -1473,10 +1471,8 @@ static int tegra_powergate_init(struct t

 		err = of_genpd_add_subdomain(&parent_args, &child_args);
 		of_node_put(parent_args.np);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			break;
-		}
 	}

 	of_node_put(np);
@@ -1503,13 +1499,13 @@ static void tegra_powergate_remove(struc
 static void tegra_powergate_remove_all(struct device_node *parent)
 {
 	struct generic_pm_domain *genpd;
-	struct device_node *np, *child;
+	struct device_node *np;

 	np = of_get_child_by_name(parent, "powergates");
 	if (!np)
 		return;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		of_genpd_del_provider(child);

 		genpd = of_genpd_remove_last(child);
diff -u -p a/drivers/i2c/muxes/i2c-mux-reg.c b/drivers/i2c/muxes/i2c-mux-reg.c
--- a/drivers/i2c/muxes/i2c-mux-reg.c
+++ b/drivers/i2c/muxes/i2c-mux-reg.c
@@ -80,7 +80,7 @@ static int i2c_mux_reg_probe_dt(struct r
 				struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *adapter_np, *child;
+	struct device_node *adapter_np;
 	struct i2c_adapter *adapter;
 	struct resource res;
 	unsigned *values;
@@ -126,7 +126,7 @@ static int i2c_mux_reg_probe_dt(struct r
 	if (!values)
 		return -ENOMEM;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		of_property_read_u32(child, "reg", values + i);
 		i++;
 	}
diff -u -p a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -410,7 +410,6 @@ static int ahci_platform_get_regulator(s
 static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
 				      struct device *dev)
 {
-	struct device_node *child;
 	u32 port;

 	if (!of_property_read_u32(dev->of_node, "hba-cap", &hpriv->saved_cap))
@@ -419,14 +418,12 @@ static int ahci_platform_get_firmware(st
 	of_property_read_u32(dev->of_node,
 			     "ports-implemented", &hpriv->saved_port_map);

-	for_each_child_of_node(dev->of_node, child) {
+	for_each_child_of_node_scoped(dev->of_node, child) {
 		if (!of_device_is_available(child))
 			continue;

-		if (of_property_read_u32(child, "reg", &port)) {
-			of_node_put(child);
+		if (of_property_read_u32(child, "reg", &port))
 			return -EINVAL;
-		}

 		if (!of_property_read_u32(child, "hba-port-cap", &hpriv->saved_port_cap[port]))
 			hpriv->saved_port_cap[port] &= PORT_CMD_CAP;
diff -u -p a/drivers/power/supply/lp8727_charger.c b/drivers/power/supply/lp8727_charger.c
--- a/drivers/power/supply/lp8727_charger.c
+++ b/drivers/power/supply/lp8727_charger.c
@@ -507,7 +507,6 @@ out:
 static struct lp8727_platform_data *lp8727_parse_dt(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
-	struct device_node *child;
 	struct lp8727_platform_data *pdata;
 	const char *type;

@@ -521,7 +520,7 @@ static struct lp8727_platform_data *lp87
 	if (of_get_child_count(np) == 0)
 		return pdata;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		of_property_read_string(child, "charger-type", &type);

 		if (!strcmp(type, "ac"))
diff -u -p a/drivers/pinctrl/freescale/pinctrl-imx1-core.c b/drivers/pinctrl/freescale/pinctrl-imx1-core.c
--- a/drivers/pinctrl/freescale/pinctrl-imx1-core.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx1-core.c
@@ -508,7 +508,6 @@ static int imx1_pinctrl_parse_functions(
 				       struct imx1_pinctrl_soc_info *info,
 				       u32 index)
 {
-	struct device_node *child;
 	struct imx1_pmx_func *func;
 	struct imx1_pin_group *grp;
 	int ret;
@@ -531,14 +530,12 @@ static int imx1_pinctrl_parse_functions(
 	if (!func->groups)
 		return -ENOMEM;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		func->groups[i] = child->name;
 		grp = &info->groups[grp_index++];
 		ret = imx1_pinctrl_parse_groups(child, grp, info, i++);
-		if (ret == -ENOMEM) {
-			of_node_put(child);
+		if (ret == -ENOMEM)
 			return ret;
-		}
 	}

 	return 0;
diff -u -p a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -133,9 +133,7 @@ EXPORT_SYMBOL_GPL(pnv_php_find_slot);
  */
 static void pnv_php_rmv_pdns(struct device_node *dn)
 {
-	struct device_node *child;
-
-	for_each_child_of_node(dn, child) {
+	for_each_child_of_node_scoped(dn, child) {
 		pnv_php_rmv_pdns(child);

 		pci_remove_device_node_info(child);
@@ -214,21 +212,16 @@ static void pnv_php_reverse_nodes(struct
 static int pnv_php_populate_changeset(struct of_changeset *ocs,
 				      struct device_node *dn)
 {
-	struct device_node *child;
 	int ret = 0;

-	for_each_child_of_node(dn, child) {
+	for_each_child_of_node_scoped(dn, child) {
 		ret = of_changeset_attach_node(ocs, child);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			break;
-		}

 		ret = pnv_php_populate_changeset(ocs, child);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			break;
-		}
 	}

 	return ret;
@@ -974,13 +967,11 @@ free_slot:

 static void pnv_php_register(struct device_node *dn)
 {
-	struct device_node *child;
-
 	/*
 	 * The parent slots should be registered before their
 	 * child slots.
 	 */
-	for_each_child_of_node(dn, child) {
+	for_each_child_of_node_scoped(dn, child) {
 		pnv_php_register_one(child);
 		pnv_php_register(child);
 	}
@@ -1002,10 +993,8 @@ static void pnv_php_unregister_one(struc

 static void pnv_php_unregister(struct device_node *dn)
 {
-	struct device_node *child;
-
 	/* The child slots should go before their parent slots */
-	for_each_child_of_node(dn, child) {
+	for_each_child_of_node_scoped(dn, child) {
 		pnv_php_unregister(child);
 		pnv_php_unregister_one(child);
 	}
diff -u -p a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -410,18 +410,15 @@ static int tmp421_probe_from_dt(struct i
 {
 	struct device *dev = &client->dev;
 	const struct device_node *np = dev->of_node;
-	struct device_node *child;
 	int err;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (strcmp(child->name, "channel"))
 			continue;

 		err = tmp421_probe_child_from_dt(client, child, data);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}

 	return 0;
diff -u -p a/drivers/media/platform/samsung/exynos4-is/media-dev.c b/drivers/media/platform/samsung/exynos4-is/media-dev.c
--- a/drivers/media/platform/samsung/exynos4-is/media-dev.c
+++ b/drivers/media/platform/samsung/exynos4-is/media-dev.c
@@ -482,15 +482,12 @@ static int fimc_md_parse_one_endpoint(st
 static int fimc_md_parse_port_node(struct fimc_md *fmd,
 				   struct device_node *port)
 {
-	struct device_node *ep;
 	int ret;

-	for_each_child_of_node(port, ep) {
+	for_each_child_of_node_scoped(port, ep) {
 		ret = fimc_md_parse_one_endpoint(fmd, ep);
-		if (ret < 0) {
-			of_node_put(ep);
+		if (ret < 0)
 			return ret;
-		}
 	}

 	return 0;
diff -u -p a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -2979,9 +2979,7 @@ static const struct of_device_id rockchi
 static void rockchip_pinctrl_child_count(struct rockchip_pinctrl *info,
 						struct device_node *np)
 {
-	struct device_node *child;
-
-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (of_match_node(rockchip_bank_match, child))
 			continue;

@@ -3057,7 +3055,6 @@ static int rockchip_pinctrl_parse_functi
 						u32 index)
 {
 	struct device *dev = info->dev;
-	struct device_node *child;
 	struct rockchip_pmx_func *func;
 	struct rockchip_pin_group *grp;
 	int ret;
@@ -3078,14 +3075,12 @@ static int rockchip_pinctrl_parse_functi
 	if (!func->groups)
 		return -ENOMEM;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		func->groups[i] = child->name;
 		grp = &info->groups[grp_index++];
 		ret = rockchip_pinctrl_parse_groups(child, grp, info, i++);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			return ret;
-		}
 	}

 	return 0;
diff -u -p a/drivers/of/overlay.c b/drivers/of/overlay.c
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -1083,16 +1083,12 @@ EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
  */
 static int find_node(struct device_node *tree, struct device_node *np)
 {
-	struct device_node *child;
-
 	if (tree == np)
 		return 1;

-	for_each_child_of_node(tree, child) {
-		if (find_node(child, np)) {
-			of_node_put(child);
+	for_each_child_of_node_scoped(tree, child) {
+		if (find_node(child, np))
 			return 1;
-		}
 	}

 	return 0;
diff -u -p a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -450,7 +450,6 @@ static int load_one_timing(struct tegra_

 static int load_timings(struct tegra_mc *mc, struct device_node *node)
 {
-	struct device_node *child;
 	struct tegra_mc_timing *timing;
 	int child_count = of_get_child_count(node);
 	int i = 0, err;
@@ -462,14 +461,12 @@ static int load_timings(struct tegra_mc

 	mc->num_timings = child_count;

-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		timing = &mc->timings[i++];

 		err = load_one_timing(mc, timing, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}

 	return 0;
diff -u -p a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -549,7 +549,7 @@ static int rockchip_lvds_bind(struct dev
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	struct device_node *remote = NULL;
-	struct device_node  *port, *endpoint;
+	struct device_node  *port;
 	int ret = 0, child_count = 0;
 	const char *name;
 	u32 endpoint_id = 0;
@@ -561,15 +561,13 @@ static int rockchip_lvds_bind(struct dev
 			      "can't found port point, please init lvds panel port!\n");
 		return -EINVAL;
 	}
-	for_each_child_of_node(port, endpoint) {
+	for_each_child_of_node_scoped(port, endpoint) {
 		child_count++;
 		of_property_read_u32(endpoint, "reg", &endpoint_id);
 		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
 						  &lvds->panel, &lvds->bridge);
-		if (!ret) {
-			of_node_put(endpoint);
+		if (!ret)
 			break;
-		}
 	}
 	if (!child_count) {
 		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
diff -u -p a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -763,14 +763,10 @@ static struct platform_driver qcom_iommu

 static bool qcom_iommu_has_secure_context(struct qcom_iommu_dev *qcom_iommu)
 {
-	struct device_node *child;
-
-	for_each_child_of_node(qcom_iommu->dev->of_node, child) {
+	for_each_child_of_node_scoped(qcom_iommu->dev->of_node, child) {
 		if (of_device_is_compatible(child, "qcom,msm-iommu-v1-sec") ||
-		    of_device_is_compatible(child, "qcom,msm-iommu-v2-sec")) {
-			of_node_put(child);
+		    of_device_is_compatible(child, "qcom,msm-iommu-v2-sec"))
 			return true;
-		}
 	}

 	return false;
diff -u -p a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -23,7 +23,6 @@
 static int of_find_trip_id(struct device_node *np, struct device_node *trip)
 {
 	struct device_node *trips;
-	struct device_node *t;
 	int i = 0;

 	trips = of_get_child_by_name(np, "trips");
@@ -35,12 +34,10 @@ static int of_find_trip_id(struct device
 	/*
 	 * Find the trip id point associated with the cooling device map
 	 */
-	for_each_child_of_node(trips, t) {
+	for_each_child_of_node_scoped(trips, t) {

-		if (t == trip) {
-			of_node_put(t);
+		if (t == trip)
 			goto out;
-		}
 		i++;
 	}

@@ -388,7 +385,7 @@ static int thermal_of_for_each_cooling_m
 					    int (*action)(struct device_node *, int, int,
 							  struct thermal_zone_device *, struct thermal_cooling_device *))
 {
-	struct device_node *tz_np, *cm_np, *child;
+	struct device_node *tz_np, *cm_np;
 	int ret = 0;

 	tz_np = thermal_of_zone_get_by_name(tz);
@@ -401,12 +398,10 @@ static int thermal_of_for_each_cooling_m
 	if (!cm_np)
 		goto out;

-	for_each_child_of_node(cm_np, child) {
+	for_each_child_of_node_scoped(cm_np, child) {
 		ret = thermal_of_for_each_cooling_device(tz_np, child, tz, cdev, action);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			break;
-		}
 	}

 	of_node_put(cm_np);
diff -u -p a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
--- a/drivers/memory/tegra/tegra124-emc.c
+++ b/drivers/memory/tegra/tegra124-emc.c
@@ -992,7 +992,6 @@ static int tegra_emc_load_timings_from_d
 					  struct device_node *node)
 {
 	int child_count = of_get_child_count(node);
-	struct device_node *child;
 	struct emc_timing *timing;
 	unsigned int i = 0;
 	int err;
@@ -1004,14 +1003,12 @@ static int tegra_emc_load_timings_from_d

 	emc->num_timings = child_count;

-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		timing = &emc->timings[i++];

 		err = load_one_timing_from_dt(emc, timing, child);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}

 	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,
diff -u -p a/drivers/i2c/i2c-core-slave.c b/drivers/i2c/i2c-core-slave.c
--- a/drivers/i2c/i2c-core-slave.c
+++ b/drivers/i2c/i2c-core-slave.c
@@ -109,15 +109,12 @@ EXPORT_SYMBOL_GPL(i2c_slave_event);
 bool i2c_detect_slave_mode(struct device *dev)
 {
 	if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
-		struct device_node *child;
 		u32 reg;

-		for_each_child_of_node(dev->of_node, child) {
+		for_each_child_of_node_scoped(dev->of_node, child) {
 			of_property_read_u32(child, "reg", &reg);
-			if (reg & I2C_OWN_SLAVE_ADDRESS) {
-				of_node_put(child);
+			if (reg & I2C_OWN_SLAVE_ADDRESS)
 				return true;
-			}
 		}
 	} else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
 		dev_dbg(dev, "ACPI slave is not supported yet\n");
diff -u -p a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -813,7 +813,6 @@ static int ina3221_probe_child_from_dt(s
 static int ina3221_probe_from_dt(struct device *dev, struct ina3221_data *ina)
 {
 	const struct device_node *np = dev->of_node;
-	struct device_node *child;
 	int ret;

 	/* Compatible with non-DT platforms */
@@ -822,12 +821,10 @@ static int ina3221_probe_from_dt(struct

 	ina->single_shot = of_property_read_bool(np, "ti,single-shot");

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = ina3221_probe_child_from_dt(dev, child, ina);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			return ret;
-		}
 	}

 	return 0;
diff -u -p a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -395,14 +395,13 @@ static struct pci_dev *of_scan_pci_dev(s
 static void __of_scan_bus(struct device_node *node, struct pci_bus *bus,
 			  int rescan_existing)
 {
-	struct device_node *child;
 	struct pci_dev *dev;

 	pr_debug("of_scan_bus(%pOF) bus no %d...\n",
 		 node, bus->number);

 	/* Scan direct children */
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		dev = of_scan_pci_dev(bus, child);
 		if (!dev)
 			continue;
diff -u -p a/arch/powerpc/platforms/pseries/vio.c b/arch/powerpc/platforms/pseries/vio.c
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -114,10 +114,9 @@ static int vio_cmo_num_OF_devs(void)
 	 */
 	node_vroot = of_find_node_by_name(NULL, "vdevice");
 	if (node_vroot) {
-		struct device_node *of_node;
 		struct property *prop;

-		for_each_child_of_node(node_vroot, of_node) {
+		for_each_child_of_node_scoped(node_vroot, of_node) {
 			prop = of_find_property(of_node, "ibm,my-dma-window",
 			                       NULL);
 			if (prop)
diff -u -p a/drivers/pinctrl/sprd/pinctrl-sprd.c b/drivers/pinctrl/sprd/pinctrl-sprd.c
--- a/drivers/pinctrl/sprd/pinctrl-sprd.c
+++ b/drivers/pinctrl/sprd/pinctrl-sprd.c
@@ -916,12 +916,11 @@ static int sprd_pinctrl_parse_groups(str

 static unsigned int sprd_pinctrl_get_groups(struct device_node *np)
 {
-	struct device_node *child;
 	unsigned int group_cnt, cnt;

 	group_cnt = of_get_child_count(np);

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		cnt = of_get_child_count(child);
 		if (cnt > 0)
 			group_cnt += cnt;
diff -u -p a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -786,7 +786,6 @@ static int s32_pinctrl_parse_functions(s
 					struct s32_pinctrl_soc_info *info,
 					u32 index)
 {
-	struct device_node *child;
 	struct pinfunction *func;
 	struct s32_pin_group *grp;
 	const char **groups;
@@ -810,14 +809,12 @@ static int s32_pinctrl_parse_functions(s
 	if (!groups)
 		return -ENOMEM;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		groups[i] = child->name;
 		grp = &info->groups[info->grp_index++];
 		ret = s32_pinctrl_parse_groups(child, grp, info);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			return ret;
-		}
 		i++;
 	}

diff -u -p a/drivers/of/resolver.c b/drivers/of/resolver.c
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -262,7 +262,7 @@ static int adjust_local_phandle_referenc
  */
 int of_resolve_phandles(struct device_node *overlay)
 {
-	struct device_node *child, *local_fixups, *refnode;
+	struct device_node *local_fixups, *refnode;
 	struct device_node *tree_symbols, *overlay_fixups;
 	struct property *prop;
 	const char *refpath;
@@ -296,7 +296,7 @@ int of_resolve_phandles(struct device_no

 	overlay_fixups = NULL;

-	for_each_child_of_node(overlay, child) {
+	for_each_child_of_node_scoped(overlay, child) {
 		if (of_node_name_eq(child, "__fixups__"))
 			overlay_fixups = child;
 	}
diff -u -p a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2774,7 +2774,6 @@ static void marvell_nand_chips_cleanup(s
 static int marvell_nand_chips_init(struct device *dev, struct marvell_nfc *nfc)
 {
 	struct device_node *np = dev->of_node;
-	struct device_node *nand_np;
 	int max_cs = nfc->caps->max_cs_nb;
 	int nchips;
 	int ret;
@@ -2801,12 +2800,10 @@ static int marvell_nand_chips_init(struc
 		return ret;
 	}

-	for_each_child_of_node(np, nand_np) {
+	for_each_child_of_node_scoped(np, nand_np) {
 		ret = marvell_nand_chip_init(dev, nfc, nand_np);
-		if (ret) {
-			of_node_put(nand_np);
+		if (ret)
 			goto cleanup_chips;
-		}
 	}

 	return 0;
diff -u -p a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -901,7 +901,7 @@ static void aspeed_pwm_tacho_remove(void
 static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np, *child;
+	struct device_node *np;
 	struct aspeed_pwm_tacho_data *priv;
 	void __iomem *regs;
 	struct device *hwmon;
@@ -944,12 +944,10 @@ static int aspeed_pwm_tacho_probe(struct

 	aspeed_create_type(priv);

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = aspeed_create_fan(dev, child, priv);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			return ret;
-		}
 	}

 	priv->groups[0] = &pwm_dev_group;
diff -u -p a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -343,14 +343,13 @@ static int lp855x_parse_dt(struct lp855x
 	rom_length = of_get_child_count(node);
 	if (rom_length > 0) {
 		struct lp855x_rom_data *rom;
-		struct device_node *child;
 		int i = 0;

 		rom = devm_kcalloc(dev, rom_length, sizeof(*rom), GFP_KERNEL);
 		if (!rom)
 			return -ENOMEM;

-		for_each_child_of_node(node, child) {
+		for_each_child_of_node_scoped(node, child) {
 			of_property_read_u8(child, "rom-addr", &rom[i].addr);
 			of_property_read_u8(child, "rom-val", &rom[i].val);
 			i++;
diff -u -p a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -433,7 +433,7 @@ static int populate_attr_groups(struct p
 {
 	struct platform_data *pdata = platform_get_drvdata(pdev);
 	const struct attribute_group **pgroups = pdata->attr_groups;
-	struct device_node *opal, *np;
+	struct device_node *opal;
 	enum sensors type;
 	int ret;

@@ -442,7 +442,7 @@ static int populate_attr_groups(struct p
 		return ret;

 	opal = of_find_node_by_path("/ibm,opal/sensors");
-	for_each_child_of_node(opal, np) {
+	for_each_child_of_node_scoped(opal, np) {
 		const char *label;

 		type = get_sensor_type(np);
diff -u -p a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -1823,7 +1823,6 @@ static int stm32_fmc2_nfc_parse_child(st
 static int stm32_fmc2_nfc_parse_dt(struct stm32_fmc2_nfc *nfc)
 {
 	struct device_node *dn = nfc->dev->of_node;
-	struct device_node *child;
 	int nchips = of_get_child_count(dn);
 	int ret = 0;

@@ -1837,12 +1836,10 @@ static int stm32_fmc2_nfc_parse_dt(struc
 		return -EINVAL;
 	}

-	for_each_child_of_node(dn, child) {
+	for_each_child_of_node_scoped(dn, child) {
 		ret = stm32_fmc2_nfc_parse_child(nfc, child);
-		if (ret < 0) {
-			of_node_put(child);
+		if (ret < 0)
 			return ret;
-		}
 	}

 	return ret;
diff -u -p a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
--- a/drivers/hwmon/tmp464.c
+++ b/drivers/hwmon/tmp464.c
@@ -565,18 +565,15 @@ static int tmp464_probe_child_from_dt(st
 static int tmp464_probe_from_dt(struct device *dev, struct tmp464_data *data)
 {
 	const struct device_node *np = dev->of_node;
-	struct device_node *child;
 	int err;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (strcmp(child->name, "channel"))
 			continue;

 		err = tmp464_probe_child_from_dt(dev, child, data);
-		if (err) {
-			of_node_put(child);
+		if (err)
 			return err;
-		}
 	}

 	return 0;
diff -u -p a/drivers/pinctrl/renesas/pinctrl-rzn1.c b/drivers/pinctrl/renesas/pinctrl-rzn1.c
--- a/drivers/pinctrl/renesas/pinctrl-rzn1.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c
@@ -404,7 +404,6 @@ static int rzn1_dt_node_to_map(struct pi
 			       struct pinctrl_map **map,
 			       unsigned int *num_maps)
 {
-	struct device_node *child;
 	int ret;

 	*map = NULL;
@@ -414,12 +413,10 @@ static int rzn1_dt_node_to_map(struct pi
 	if (ret < 0)
 		return ret;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = rzn1_dt_node_to_map_one(pctldev, child, map, num_maps);
-		if (ret < 0) {
-			of_node_put(child);
+		if (ret < 0)
 			return ret;
-		}
 	}

 	return 0;
@@ -740,13 +737,12 @@ static int rzn1_pinctrl_parse_groups(str

 static int rzn1_pinctrl_count_function_groups(struct device_node *np)
 {
-	struct device_node *child;
 	int count = 0;

 	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
 		count++;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (of_property_count_u32_elems(child, RZN1_PINS_PROP) > 0)
 			count++;
 	}
@@ -760,7 +756,6 @@ static int rzn1_pinctrl_parse_functions(
 {
 	struct rzn1_pmx_func *func;
 	struct rzn1_pin_group *grp;
-	struct device_node *child;
 	unsigned int i = 0;
 	int ret;

@@ -793,15 +788,13 @@ static int rzn1_pinctrl_parse_functions(
 		ipctl->ngroups++;
 	}

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		func->groups[i] = child->name;
 		grp = &ipctl->groups[ipctl->ngroups];
 		grp->func = func->name;
 		ret = rzn1_pinctrl_parse_groups(child, grp, ipctl);
-		if (ret < 0) {
-			of_node_put(child);
+		if (ret < 0)
 			return ret;
-		}
 		i++;
 		ipctl->ngroups++;
 	}
diff -u -p a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
--- a/sound/soc/meson/meson-card-utils.c
+++ b/sound/soc/meson/meson-card-utils.c
@@ -198,7 +198,6 @@ static int meson_card_add_links(struct s
 {
 	struct meson_card *priv = snd_soc_card_get_drvdata(card);
 	struct device_node *node = card->dev->of_node;
-	struct device_node *np;
 	int num, i, ret;

 	num = of_get_child_count(node);
@@ -212,12 +211,10 @@ static int meson_card_add_links(struct s
 		return ret;

 	i = 0;
-	for_each_child_of_node(node, np) {
+	for_each_child_of_node_scoped(node, np) {
 		ret = priv->match_data->add_link(card, np, &i);
-		if (ret) {
-			of_node_put(np);
+		if (ret)
 			return ret;
-		}

 		i++;
 	}
diff -u -p a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -434,7 +434,6 @@ static int miic_parse_dt(struct device *
 {
 	s8 dt_val[MIIC_MODCTRL_CONF_CONV_NUM];
 	struct device_node *np = dev->of_node;
-	struct device_node *conv;
 	u32 conf;
 	int port;

@@ -443,7 +442,7 @@ static int miic_parse_dt(struct device *
 	if (of_property_read_u32(np, "renesas,miic-switch-portin", &conf) == 0)
 		dt_val[0] = conf;

-	for_each_child_of_node(np, conv) {
+	for_each_child_of_node_scoped(np, conv) {
 		if (of_property_read_u32(conv, "reg", &port))
 			continue;

diff -u -p a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -115,7 +115,6 @@ static int denali_dt_probe(struct platfo
 	struct denali_dt *dt;
 	const struct denali_dt_data *data;
 	struct denali_controller *denali;
-	struct device_node *np;
 	int ret;

 	dt = devm_kzalloc(dev, sizeof(*dt), GFP_KERNEL);
@@ -204,12 +203,10 @@ static int denali_dt_probe(struct platfo
 	if (ret)
 		goto out_assert_rst;

-	for_each_child_of_node(dev->of_node, np) {
+	for_each_child_of_node_scoped(dev->of_node, np) {
 		ret = denali_dt_chip_init(denali, np);
-		if (ret) {
-			of_node_put(np);
+		if (ret)
 			goto out_remove_denali;
-		}
 	}

 	platform_set_drvdata(pdev, dt);
diff -u -p a/drivers/mtd/nand/raw/mtk_nand.c b/drivers/mtd/nand/raw/mtk_nand.c
--- a/drivers/mtd/nand/raw/mtk_nand.c
+++ b/drivers/mtd/nand/raw/mtk_nand.c
@@ -1432,15 +1432,12 @@ static int mtk_nfc_nand_chip_init(struct
 static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
 {
 	struct device_node *np = dev->of_node;
-	struct device_node *nand_np;
 	int ret;

-	for_each_child_of_node(np, nand_np) {
+	for_each_child_of_node_scoped(np, nand_np) {
 		ret = mtk_nfc_nand_chip_init(dev, nfc, nand_np);
-		if (ret) {
-			of_node_put(nand_np);
+		if (ret)
 			return ret;
-		}
 	}

 	return 0;
diff -u -p a/drivers/clk/at91/dt-compat.c b/drivers/clk/at91/dt-compat.c
--- a/drivers/clk/at91/dt-compat.c
+++ b/drivers/clk/at91/dt-compat.c
@@ -129,7 +129,7 @@ static void __init of_sama5d2_clk_genera
 	struct clk_hw *hw;
 	unsigned int num_parents;
 	const char *parent_names[GENERATED_SOURCE_MAX];
-	struct device_node *gcknp, *parent_np;
+	struct device_node *parent_np;
 	struct clk_range range = CLK_RANGE(0, 0);
 	struct regmap *regmap;

@@ -149,7 +149,7 @@ static void __init of_sama5d2_clk_genera
 	if (IS_ERR(regmap))
 		return;

-	for_each_child_of_node(np, gcknp) {
+	for_each_child_of_node_scoped(np, gcknp) {
 		int chg_pid = INT_MIN;

 		if (of_property_read_u32(gcknp, "reg", &id))
@@ -219,7 +219,6 @@ static void __init of_sama5d2_clk_i2s_mu
 	struct regmap *regmap_sfr;
 	u8 bus_id;
 	const char *parent_names[2];
-	struct device_node *i2s_mux_np;
 	struct clk_hw *hw;
 	int ret;

@@ -227,7 +226,7 @@ static void __init of_sama5d2_clk_i2s_mu
 	if (IS_ERR(regmap_sfr))
 		return;

-	for_each_child_of_node(np, i2s_mux_np) {
+	for_each_child_of_node_scoped(np, i2s_mux_np) {
 		if (of_property_read_u8(i2s_mux_np, "reg", &bus_id))
 			continue;

@@ -743,7 +742,7 @@ of_at91_clk_prog_setup(struct device_nod
 	unsigned int num_parents;
 	const char *parent_names[PROG_SOURCE_MAX];
 	const char *name;
-	struct device_node *progclknp, *parent_np;
+	struct device_node *parent_np;
 	struct regmap *regmap;

 	num_parents = of_clk_get_parent_count(np);
@@ -762,7 +761,7 @@ of_at91_clk_prog_setup(struct device_nod
 	if (IS_ERR(regmap))
 		return;

-	for_each_child_of_node(np, progclknp) {
+	for_each_child_of_node_scoped(np, progclknp) {
 		if (of_property_read_u32(progclknp, "reg", &id))
 			continue;

@@ -875,7 +874,7 @@ static void __init of_at91rm9200_clk_sys
 	u32 id;
 	struct clk_hw *hw;
 	const char *name;
-	struct device_node *sysclknp, *parent_np;
+	struct device_node *parent_np;
 	const char *parent_name;
 	struct regmap *regmap;

@@ -889,7 +888,7 @@ static void __init of_at91rm9200_clk_sys
 	if (IS_ERR(regmap))
 		return;

-	for_each_child_of_node(np, sysclknp) {
+	for_each_child_of_node_scoped(np, sysclknp) {
 		unsigned long flags = 0;

 		if (of_property_read_u32(sysclknp, "reg", &id))
diff -u -p a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -497,11 +497,10 @@ void __init pseries_little_endian_except

 static void __init pSeries_discover_phbs(void)
 {
-	struct device_node *node;
 	struct pci_controller *phb;
 	struct device_node *root = of_find_node_by_path("/");

-	for_each_child_of_node(root, node) {
+	for_each_child_of_node_scoped(root, node) {
 		if (!of_node_is_type(node, "pci") &&
 		    !of_node_is_type(node, "pciex"))
 			continue;
diff -u -p a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -1114,7 +1114,6 @@ void rsnd_parse_connect_ssi(struct rsnd_
 	struct rsnd_priv *priv = rsnd_rdai_to_priv(rdai);
 	struct device *dev = rsnd_priv_to_dev(priv);
 	struct device_node *node;
-	struct device_node *np;
 	int i;

 	node = rsnd_ssi_of_node(priv);
@@ -1122,14 +1121,12 @@ void rsnd_parse_connect_ssi(struct rsnd_
 		return;

 	i = 0;
-	for_each_child_of_node(node, np) {
+	for_each_child_of_node_scoped(node, np) {
 		struct rsnd_mod *mod;

 		i = rsnd_node_fixed_index(dev, np, SSI_NAME, i);
-		if (i < 0) {
-			of_node_put(np);
+		if (i < 0)
 			break;
-		}

 		mod = rsnd_ssi_mod_get(priv, i);

diff -u -p a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c
--- a/drivers/iio/adc/stm32-dfsdm-core.c
+++ b/drivers/iio/adc/stm32-dfsdm-core.c
@@ -308,7 +308,6 @@ static int stm32_dfsdm_probe_identificat
 					    const struct stm32_dfsdm_dev_data *dev_data)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *child;
 	struct stm32_dfsdm *dfsdm = &priv->dfsdm;
 	const char *compat;
 	int ret, count = 0;
@@ -329,7 +328,7 @@ static int stm32_dfsdm_probe_identificat
 		return -EINVAL;
 	}

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = of_property_read_string(child, "compatible", &compat);
 		if (ret)
 			continue;
diff -u -p a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1244,7 +1244,6 @@ static int at91_pinctrl_parse_groups(str
 static int at91_pinctrl_parse_functions(struct device_node *np,
 					struct at91_pinctrl *info, u32 index)
 {
-	struct device_node *child;
 	struct at91_pmx_func *func;
 	struct at91_pin_group *grp;
 	int ret;
@@ -1267,14 +1266,12 @@ static int at91_pinctrl_parse_functions(
 	if (!func->groups)
 		return -ENOMEM;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		func->groups[i] = child->name;
 		grp = &info->groups[grp_index++];
 		ret = at91_pinctrl_parse_groups(child, grp, info, i++);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			return ret;
-		}
 	}

 	return 0;
@@ -1296,7 +1293,6 @@ static int at91_pinctrl_probe_dt(struct
 	int i, j, ngpio_chips_enabled = 0;
 	uint32_t *tmp;
 	struct device_node *np = dev->of_node;
-	struct device_node *child;

 	if (!np)
 		return -ENODEV;
@@ -1349,14 +1345,12 @@ static int at91_pinctrl_probe_dt(struct

 	i = 0;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (of_device_is_compatible(child, gpio_compat))
 			continue;
 		ret = at91_pinctrl_parse_functions(child, info, i++);
-		if (ret) {
-			of_node_put(child);
+		if (ret)
 			return dev_err_probe(dev, ret, "failed to parse function\n");
-		}
 	}

 	return 0;
diff -u -p a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -521,7 +521,6 @@ static int rzg2l_dt_node_to_map(struct p
 				unsigned int *num_maps)
 {
 	struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	struct device_node *child;
 	unsigned int index;
 	int ret;

@@ -529,13 +528,11 @@ static int rzg2l_dt_node_to_map(struct p
 	*num_maps = 0;
 	index = 0;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = rzg2l_dt_subnode_to_map(pctldev, child, np, map,
 					      num_maps, &index);
-		if (ret < 0) {
-			of_node_put(child);
+		if (ret < 0)
 			goto done;
-		}
 	}

 	if (*num_maps == 0) {
diff -u -p a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
--- a/drivers/input/keyboard/cap11xx.c
+++ b/drivers/input/keyboard/cap11xx.c
@@ -436,7 +436,7 @@ static int cap11xx_led_set(struct led_cl
 static int cap11xx_init_leds(struct device *dev,
 			     struct cap11xx_priv *priv, int num_leds)
 {
-	struct device_node *node = dev->of_node, *child;
+	struct device_node *node = dev->of_node;
 	struct cap11xx_led *led;
 	int cnt = of_get_child_count(node);
 	int error;
@@ -465,7 +465,7 @@ static int cap11xx_init_leds(struct devi
 	if (error)
 		return error;

-	for_each_child_of_node(node, child) {
+	for_each_child_of_node_scoped(node, child) {
 		u32 reg;

 		led->cdev.name =
@@ -478,19 +478,15 @@ static int cap11xx_init_leds(struct devi
 		led->cdev.brightness = LED_OFF;

 		error = of_property_read_u32(child, "reg", &reg);
-		if (error != 0 || reg >= num_leds) {
-			of_node_put(child);
+		if (error != 0 || reg >= num_leds)
 			return -EINVAL;
-		}

 		led->reg = reg;
 		led->priv = priv;

 		error = devm_led_classdev_register(dev, &led->cdev);
-		if (error) {
-			of_node_put(child);
+		if (error)
 			return error;
-		}

 		priv->num_leds++;
 		led++;
diff -u -p a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
--- a/drivers/edac/xgene_edac.c
+++ b/drivers/edac/xgene_edac.c
@@ -1844,7 +1844,6 @@ static irqreturn_t xgene_edac_isr(int ir
 static int xgene_edac_probe(struct platform_device *pdev)
 {
 	struct xgene_edac *edac;
-	struct device_node *child;
 	struct resource *res;
 	int rc;

@@ -1935,7 +1934,7 @@ static int xgene_edac_probe(struct platf

 	edac->dfs = edac_debugfs_create_dir(pdev->dev.kobj.name);

-	for_each_child_of_node(pdev->dev.of_node, child) {
+	for_each_child_of_node_scoped(pdev->dev.of_node, child) {
 		if (!of_device_is_available(child))
 			continue;
 		if (of_device_is_compatible(child, "apm,xgene-edac-mc"))
diff -u -p a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -1171,21 +1171,18 @@ void rsnd_parse_connect_common(struct rs
 {
 	struct rsnd_priv *priv = rsnd_rdai_to_priv(rdai);
 	struct device *dev = rsnd_priv_to_dev(priv);
-	struct device_node *np;
 	int i;

 	if (!node)
 		return;

 	i = 0;
-	for_each_child_of_node(node, np) {
+	for_each_child_of_node_scoped(node, np) {
 		struct rsnd_mod *mod;

 		i = rsnd_node_fixed_index(dev, np, name, i);
-		if (i < 0) {
-			of_node_put(np);
+		if (i < 0)
 			break;
-		}

 		mod = mod_get(priv, i);

@@ -1234,16 +1231,13 @@ int rsnd_node_fixed_index(struct device
 int rsnd_node_count(struct rsnd_priv *priv, struct device_node *node, char *name)
 {
 	struct device *dev = rsnd_priv_to_dev(priv);
-	struct device_node *np;
 	int i;

 	i = 0;
-	for_each_child_of_node(node, np) {
+	for_each_child_of_node_scoped(node, np) {
 		i = rsnd_node_fixed_index(dev, np, name, i);
-		if (i < 0) {
-			of_node_put(np);
+		if (i < 0)
 			return 0;
-		}
 		i++;
 	}

diff -u -p a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -455,7 +455,6 @@ static int xvip_graph_dma_init_one(struc
 static int xvip_graph_dma_init(struct xvip_composite_device *xdev)
 {
 	struct device_node *ports;
-	struct device_node *port;
 	int ret = 0;

 	ports = of_get_child_by_name(xdev->dev->of_node, "ports");
@@ -464,12 +463,10 @@ static int xvip_graph_dma_init(struct xv
 		return -EINVAL;
 	}

-	for_each_child_of_node(ports, port) {
+	for_each_child_of_node_scoped(ports, port) {
 		ret = xvip_graph_dma_init_one(xdev, port);
-		if (ret) {
-			of_node_put(port);
+		if (ret)
 			break;
-		}
 	}

 	of_node_put(ports);
diff -u -p a/drivers/pinctrl/renesas/pinctrl-rza1.c b/drivers/pinctrl/renesas/pinctrl-rza1.c
--- a/drivers/pinctrl/renesas/pinctrl-rza1.c
+++ b/drivers/pinctrl/renesas/pinctrl-rza1.c
@@ -852,7 +852,6 @@ static const struct gpio_chip rza1_gpioc
  */
 static int rza1_dt_node_pin_count(struct device_node *np)
 {
-	struct device_node *child;
 	struct property *of_pins;
 	unsigned int npins;

@@ -861,12 +860,10 @@ static int rza1_dt_node_pin_count(struct
 		return of_pins->length / sizeof(u32);

 	npins = 0;
-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		of_pins = of_find_property(child, "pinmux", NULL);
-		if (!of_pins) {
-			of_node_put(child);
+		if (!of_pins)
 			return -EINVAL;
-		}

 		npins += of_pins->length / sizeof(u32);
 	}
diff -u -p a/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_encoder.c
@@ -25,14 +25,13 @@
 static unsigned int rcar_du_encoder_count_ports(struct device_node *node)
 {
 	struct device_node *ports;
-	struct device_node *port;
 	unsigned int num_ports = 0;

 	ports = of_get_child_by_name(node, "ports");
 	if (!ports)
 		ports = of_node_get(node);

-	for_each_child_of_node(ports, port) {
+	for_each_child_of_node_scoped(ports, port) {
 		if (of_node_name_eq(port, "port"))
 			num_ports++;
 	}
diff -u -p a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1027,7 +1027,7 @@ static const struct of_device_id altr_ed
 static int __init __maybe_unused altr_init_a10_ecc_device_type(char *compat)
 {
 	int irq;
-	struct device_node *child, *np;
+	struct device_node *np;

 	np = of_find_compatible_node(NULL, NULL,
 				     "altr,socfpga-a10-ecc-manager");
@@ -1036,7 +1036,7 @@ static int __init __maybe_unused altr_in
 		return -ENODEV;
 	}

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		const struct of_device_id *pdev_id;
 		const struct edac_device_prv_data *prv;

@@ -2109,7 +2109,6 @@ static int s10_edac_dberr_handler(struct
 static int altr_edac_a10_probe(struct platform_device *pdev)
 {
 	struct altr_arria10_edac *edac;
-	struct device_node *child;

 	edac = devm_kzalloc(&pdev->dev, sizeof(*edac), GFP_KERNEL);
 	if (!edac)
@@ -2180,7 +2179,7 @@ static int altr_edac_a10_probe(struct pl
 					 altr_edac_a10_irq_handler, edac);
 #endif

-	for_each_child_of_node(pdev->dev.of_node, child) {
+	for_each_child_of_node_scoped(pdev->dev.of_node, child) {
 		if (!of_device_is_available(child))
 			continue;

diff -u -p a/drivers/pinctrl/renesas/pinctrl-rzv2m.c b/drivers/pinctrl/renesas/pinctrl-rzv2m.c
--- a/drivers/pinctrl/renesas/pinctrl-rzv2m.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzv2m.c
@@ -388,7 +388,6 @@ static int rzv2m_dt_node_to_map(struct p
 				unsigned int *num_maps)
 {
 	struct rzv2m_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	struct device_node *child;
 	unsigned int index;
 	int ret;

@@ -396,13 +395,11 @@ static int rzv2m_dt_node_to_map(struct p
 	*num_maps = 0;
 	index = 0;

-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		ret = rzv2m_dt_subnode_to_map(pctldev, child, np, map,
 					      num_maps, &index);
-		if (ret < 0) {
-			of_node_put(child);
+		if (ret < 0)
 			goto done;
-		}
 	}

 	if (*num_maps == 0) {
diff -u -p a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4727,7 +4727,6 @@ static int mtk_sgmii_init(struct mtk_eth
 static int mtk_probe(struct platform_device *pdev)
 {
 	struct resource *res = NULL, *res_sram;
-	struct device_node *mac_np;
 	struct mtk_eth *eth;
 	int err, i;

@@ -4904,7 +4903,7 @@ static int mtk_probe(struct platform_dev

 	eth->hwlro = MTK_HAS_CAPS(eth->soc->caps, MTK_HWLRO);

-	for_each_child_of_node(pdev->dev.of_node, mac_np) {
+	for_each_child_of_node_scoped(pdev->dev.of_node, mac_np) {
 		if (!of_device_is_compatible(mac_np,
 					     "mediatek,eth-mac"))
 			continue;
@@ -4913,10 +4912,8 @@ static int mtk_probe(struct platform_dev
 			continue;

 		err = mtk_add_mac(eth, mac_np);
-		if (err) {
-			of_node_put(mac_np);
+		if (err)
 			goto err_deinit_hw;
-		}
 	}

 	if (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_INT)) {
Andy Shevchenko Feb. 1, 2024, 11:20 a.m. UTC | #9 
On Sun, Jan 28, 2024 at 04:05:37PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> +CC includes peopleinterested in property.h equivalents to minimize
> duplication of discussion.  Outcome of this discussion will affect:
> https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
> 
> In discussion of previous approach with Rob Herring we talked about various
> ways to avoid a disconnect between the declaration of the __free(device_node)
> and the first non NULL assignment. Making this connection clear is useful for 2
> reasons:
> 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> 2) Avoids disconnect between how cleanup is to be done and how the reference
>    was acquired in the first place.
> 
> https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
> 
> The options we discussed are:
> 
> 1) Ignore this issue and merge original set.
> 
> 2) Always put the declaration just before the for loop and don't set it NULL.
> 
> {
> 	int ret;
> 
> 	ret = ... and other fun code.
> 
> 	struct device_node *child __free(device_node);
> 	for_each_child_of_node(np, child) {
> 	}
> }
> 
> This works but careful review is needed to ensure that this unusual pattern is
> followed.  We don't set it to NULL as the loop will do that anyway if there are
> no child nodes, or the loop finishes without an early break or return.
> 
> 3) Introduced the pointer to auto put device_node only within the
>    for loop scope.
> 
> +#define for_each_child_of_node_scoped(parent, child) \
> +	for (struct device_node *child __free(device_node) =		\
> +	     of_get_next_child(parent, NULL);				\
> +	     child != NULL;						\

Just

	     child;							\

> +	     child = of_get_next_available_child(parent, child))
> +
> 
> This series is presenting option 3.  I only implemented this loop out of
> all the similar ones and it is only compile tested.
> 
> Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> a struct device_node *child.  I can't see a way around that other than option 2
> above, but all suggestions welcome.  Note that if a conversion leaves an
> 'external' struct device_node *child variable, in many cases the compiler
> will catch that as an unused variable. We don't currently run shaddow
> variable detection in normal kernel builds, but that could also be used
> to catch such bugs.
Jonathan Cameron Feb. 1, 2024, 3:21 p.m. UTC | #10 
> > 3) Introduced the pointer to auto put device_node only within the
> >    for loop scope.
> > 
> > +#define for_each_child_of_node_scoped(parent, child) \
> > +	for (struct device_node *child __free(device_node) =		\
> > +	     of_get_next_child(parent, NULL);				\
> > +	     child != NULL;						\  
> 
> Just
> 
> 	     child;

Agreed that's the same, but was thinking to follow local style.
I don't feel strongly though so fine with dropping the != NULL

> 
> > +	     child = of_get_next_available_child(parent, child))
> > +
> > 
> > This series is presenting option 3.  I only implemented this loop out of
> > all the similar ones and it is only compile tested.
> > 
> > Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> > a struct device_node *child.  I can't see a way around that other than option 2
> > above, but all suggestions welcome.  Note that if a conversion leaves an
> > 'external' struct device_node *child variable, in many cases the compiler
> > will catch that as an unused variable. We don't currently run shaddow
> > variable detection in normal kernel builds, but that could also be used
> > to catch such bugs.  
>
Jonathan Cameron Feb. 4, 2024, 9:08 p.m. UTC | #11 
On Wed, 31 Jan 2024 22:38:21 +0100 (CET)
Julia Lawall <julia.lawall@inria.fr> wrote:

> Here are some loop cases.  The semantic patch is as follows:
> 
> #spatch --allow-inconsistent-paths
> 
> @@
> expression node;
> identifier child;
> symbol drop_me;
> iterator name for_each_child_of_node;
> @@
> 
> for_each_child_of_node(node,child) {
>   ...
> + of_node_put(drop_me, child);
> }
> 
> @@
> expression node;
> identifier child;
> symbol drop_me;
> iterator name for_each_child_of_node, for_each_child_of_node_scoped;
> identifier L;
> @@
> 
> - struct device_node *child;
>  ... when != child
> -for_each_child_of_node
> +for_each_child_of_node_scoped
>   (node,child) {
>    ... when strict
> (
> -   {
> -   of_node_put(child);
>     return ...;
> -   }
> |
> -   {
> -   of_node_put(child);
>     goto L;
> -   }
> |
> -   {
> -   of_node_put(child);
>     break;
> -   }
> |
>     continue;
> |
> -   of_node_put(child);
>     return ...;
> |
> -   of_node_put(child);
>     break;
> |
> -  of_node_put(drop_me, child);
> )
> }
>  ... when != child
> 
> @@
> expression child;
> @@
> 
> - of_node_put(drop_me, child);
> 
> -------------------------------
> 
> This is quite conservative, in that it requires the only use of the child
> variable to be in a single for_each_child_of_node loop at top level.
> 
> The drop_me thing is a hack to be able to refer to the bottom of the loop
> in the same way as of_node_puts in front of returns etc are referenced.
> 
> This works fine when multiple device_node variables are declared at once.
> 
> The result is below.
> 
Very nice!

One issue is that Rob is keen that we also take this opportunity to
evaluate if the _available_ form is the more appropriate one.

Given that access either no defined "status" in the child node or
it being set to "okay" it is what should be used in the vast majority of
cases.

For reference, the property.h version only uses the available form.

So I think we'll need some hand checking of each case but for vast majority
it will be very straight forward.

One question is whether it is worth the scoped loops in cases
where there isn't a patch where we break out of or return from the loop
before it finishes.  Do we put them in as a defensive measure?

Sometimes we are going to want to combine this refactor with
some of the ones your previous script caught in a single patch given
it's roughly the same sort of change.


> julia
> 
> diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -2789,7 +2789,7 @@ static int unittest_i2c_mux_probe(struct
>  	int i, nchans;
>  	struct device *dev = &client->dev;
>  	struct i2c_adapter *adap = client->adapter;
> -	struct device_node *np = client->dev.of_node, *child;
> +	struct device_node *np = client->dev.of_node;
>  	struct i2c_mux_core *muxc;
>  	u32 reg, max_reg;
> 
> @@ -2801,7 +2801,7 @@ static int unittest_i2c_mux_probe(struct
>  	}
> 
>  	max_reg = (u32)-1;
> -	for_each_child_of_node(np, child) {
> +	for_each_child_of_node_scoped(np, child) {

This was a case I left alone in the original series because the auto
cleanup doesn't end up doing anything in any paths.

>  		if (of_property_read_u32(child, "reg", &reg))
>  			continue;
>  		if (max_reg == (u32)-1 || reg > max_reg)
>



> diff -u -p a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
> --- a/drivers/regulator/scmi-regulator.c
> +++ b/drivers/regulator/scmi-regulator.c
> @@ -297,7 +297,7 @@ static int process_scmi_regulator_of_nod
>  static int scmi_regulator_probe(struct scmi_device *sdev)
>  {
>  	int d, ret, num_doms;
> -	struct device_node *np, *child;
> +	struct device_node *np;
>  	const struct scmi_handle *handle = sdev->handle;
>  	struct scmi_regulator_info *rinfo;
>  	struct scmi_protocol_handle *ph;
> @@ -341,13 +341,11 @@ static int scmi_regulator_probe(struct s
>  	 */
>  	of_node_get(handle->dev->of_node);
>  	np = of_find_node_by_name(handle->dev->of_node, "regulators");
> -	for_each_child_of_node(np, child) {
> +	for_each_child_of_node_scoped(np, child) {
>  		ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo);
>  		/* abort on any mem issue */
> -		if (ret == -ENOMEM) {
> -			of_node_put(child);
> +		if (ret == -ENOMEM)
>  			return ret;
> -		}
Current code leaks np in this path :(

>  	}
>  	of_node_put(np);
>  	/*


> diff -u -p a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c
> --- a/drivers/crypto/nx/nx-common-powernv.c
> +++ b/drivers/crypto/nx/nx-common-powernv.c
> @@ -907,7 +907,6 @@ static int __init nx_powernv_probe_vas(s
>  {
>  	int chip_id, vasid, ret = 0;
>  	int ct_842 = 0, ct_gzip = 0;
> -	struct device_node *dn;
> 
>  	chip_id = of_get_ibm_chip_id(pn);
>  	if (chip_id < 0) {
> @@ -921,7 +920,7 @@ static int __init nx_powernv_probe_vas(s
>  		return -EINVAL;
>  	}
> 
> -	for_each_child_of_node(pn, dn) {
> +	for_each_child_of_node_scoped(pn, dn) {
>  		ret = find_nx_device_tree(dn, chip_id, vasid, NX_CT_842,
>  					"ibm,p9-nx-842", &ct_842);
> 
> @@ -929,10 +928,8 @@ static int __init nx_powernv_probe_vas(s
>  			ret = find_nx_device_tree(dn, chip_id, vasid,
>  				NX_CT_GZIP, "ibm,p9-nx-gzip", &ct_gzip);
The handling in here is odd (buggy?). There is an of_node_put()
in the failure path inside find_nx_device_tree() as well as out here.
> 
> -		if (ret) {
> -			of_node_put(dn);
> +		if (ret)
>  			return ret;
> -		}
>  	}
> 
>  	if (!ct_842 || !ct_gzip) {

I've glanced at a few of the others and some of them are hard.
This refactor is fine, but the other device_node handling often
is complex and I think fragile.  So definitely room for improvement!

Jonathan
Julia Lawall Feb. 4, 2024, 9:34 p.m. UTC | #12 
On Sun, 4 Feb 2024, Jonathan Cameron wrote:

> On Wed, 31 Jan 2024 22:38:21 +0100 (CET)
> Julia Lawall <julia.lawall@inria.fr> wrote:
>
> > Here are some loop cases.  The semantic patch is as follows:
> >
> > #spatch --allow-inconsistent-paths
> >
> > @@
> > expression node;
> > identifier child;
> > symbol drop_me;
> > iterator name for_each_child_of_node;
> > @@
> >
> > for_each_child_of_node(node,child) {
> >   ...
> > + of_node_put(drop_me, child);
> > }
> >
> > @@
> > expression node;
> > identifier child;
> > symbol drop_me;
> > iterator name for_each_child_of_node, for_each_child_of_node_scoped;
> > identifier L;
> > @@
> >
> > - struct device_node *child;
> >  ... when != child
> > -for_each_child_of_node
> > +for_each_child_of_node_scoped
> >   (node,child) {
> >    ... when strict
> > (
> > -   {
> > -   of_node_put(child);
> >     return ...;
> > -   }
> > |
> > -   {
> > -   of_node_put(child);
> >     goto L;
> > -   }
> > |
> > -   {
> > -   of_node_put(child);
> >     break;
> > -   }
> > |
> >     continue;
> > |
> > -   of_node_put(child);
> >     return ...;
> > |
> > -   of_node_put(child);
> >     break;
> > |
> > -  of_node_put(drop_me, child);
> > )
> > }
> >  ... when != child
> >
> > @@
> > expression child;
> > @@
> >
> > - of_node_put(drop_me, child);
> >
> > -------------------------------
> >
> > This is quite conservative, in that it requires the only use of the child
> > variable to be in a single for_each_child_of_node loop at top level.
> >
> > The drop_me thing is a hack to be able to refer to the bottom of the loop
> > in the same way as of_node_puts in front of returns etc are referenced.
> >
> > This works fine when multiple device_node variables are declared at once.
> >
> > The result is below.
> >
> Very nice!
>
> One issue is that Rob is keen that we also take this opportunity to
> evaluate if the _available_ form is the more appropriate one.
>
> Given that access either no defined "status" in the child node or
> it being set to "okay" it is what should be used in the vast majority of
> cases.
>
> For reference, the property.h version only uses the available form.
>
> So I think we'll need some hand checking of each case but for vast majority
> it will be very straight forward.

I'm not sure to follow this.  If the check is straightforward, perhaps it
can be integrated into the rule?  But I'm not sure what to check for.

> One question is whether it is worth the scoped loops in cases
> where there isn't a patch where we break out of or return from the loop
> before it finishes.  Do we put them in as a defensive measure?

I wondered about this also.  My thought was that it is better to be
uniform.  And maybe a break would be added later.

> Sometimes we are going to want to combine this refactor with
> some of the ones your previous script caught in a single patch given
> it's roughly the same sort of change.

Agreed.  Some blocks of code should indeed become much simpler.

>
>
> > julia
> >
> > diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > @@ -2789,7 +2789,7 @@ static int unittest_i2c_mux_probe(struct
> >  	int i, nchans;
> >  	struct device *dev = &client->dev;
> >  	struct i2c_adapter *adap = client->adapter;
> > -	struct device_node *np = client->dev.of_node, *child;
> > +	struct device_node *np = client->dev.of_node;
> >  	struct i2c_mux_core *muxc;
> >  	u32 reg, max_reg;
> >
> > @@ -2801,7 +2801,7 @@ static int unittest_i2c_mux_probe(struct
> >  	}
> >
> >  	max_reg = (u32)-1;
> > -	for_each_child_of_node(np, child) {
> > +	for_each_child_of_node_scoped(np, child) {
>
> This was a case I left alone in the original series because the auto
> cleanup doesn't end up doing anything in any paths.
>
> >  		if (of_property_read_u32(child, "reg", &reg))
> >  			continue;
> >  		if (max_reg == (u32)-1 || reg > max_reg)
> >
>
>
>
> > diff -u -p a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
> > --- a/drivers/regulator/scmi-regulator.c
> > +++ b/drivers/regulator/scmi-regulator.c
> > @@ -297,7 +297,7 @@ static int process_scmi_regulator_of_nod
> >  static int scmi_regulator_probe(struct scmi_device *sdev)
> >  {
> >  	int d, ret, num_doms;
> > -	struct device_node *np, *child;
> > +	struct device_node *np;
> >  	const struct scmi_handle *handle = sdev->handle;
> >  	struct scmi_regulator_info *rinfo;
> >  	struct scmi_protocol_handle *ph;
> > @@ -341,13 +341,11 @@ static int scmi_regulator_probe(struct s
> >  	 */
> >  	of_node_get(handle->dev->of_node);
> >  	np = of_find_node_by_name(handle->dev->of_node, "regulators");
> > -	for_each_child_of_node(np, child) {
> > +	for_each_child_of_node_scoped(np, child) {
> >  		ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo);
> >  		/* abort on any mem issue */
> > -		if (ret == -ENOMEM) {
> > -			of_node_put(child);
> > +		if (ret == -ENOMEM)
> >  			return ret;
> > -		}
> Current code leaks np in this path :(
>
> >  	}
> >  	of_node_put(np);
> >  	/*
>
>
> > diff -u -p a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c
> > --- a/drivers/crypto/nx/nx-common-powernv.c
> > +++ b/drivers/crypto/nx/nx-common-powernv.c
> > @@ -907,7 +907,6 @@ static int __init nx_powernv_probe_vas(s
> >  {
> >  	int chip_id, vasid, ret = 0;
> >  	int ct_842 = 0, ct_gzip = 0;
> > -	struct device_node *dn;
> >
> >  	chip_id = of_get_ibm_chip_id(pn);
> >  	if (chip_id < 0) {
> > @@ -921,7 +920,7 @@ static int __init nx_powernv_probe_vas(s
> >  		return -EINVAL;
> >  	}
> >
> > -	for_each_child_of_node(pn, dn) {
> > +	for_each_child_of_node_scoped(pn, dn) {
> >  		ret = find_nx_device_tree(dn, chip_id, vasid, NX_CT_842,
> >  					"ibm,p9-nx-842", &ct_842);
> >
> > @@ -929,10 +928,8 @@ static int __init nx_powernv_probe_vas(s
> >  			ret = find_nx_device_tree(dn, chip_id, vasid,
> >  				NX_CT_GZIP, "ibm,p9-nx-gzip", &ct_gzip);
> The handling in here is odd (buggy?). There is an of_node_put()
> in the failure path inside find_nx_device_tree() as well as out here.
> >
> > -		if (ret) {
> > -			of_node_put(dn);
> > +		if (ret)
> >  			return ret;
> > -		}
> >  	}
> >
> >  	if (!ct_842 || !ct_gzip) {
>
> I've glanced at a few of the others and some of them are hard.
> This refactor is fine, but the other device_node handling often
> is complex and I think fragile.  So definitely room for improvement!

I agree with all the above comments.

julia
Jonathan Cameron Feb. 5, 2024, 9:27 a.m. UTC | #13 
On Sun, 4 Feb 2024 22:34:25 +0100 (CET)
Julia Lawall <julia.lawall@inria.fr> wrote:

> On Sun, 4 Feb 2024, Jonathan Cameron wrote:
> 
> > On Wed, 31 Jan 2024 22:38:21 +0100 (CET)
> > Julia Lawall <julia.lawall@inria.fr> wrote:
> >  
> > > Here are some loop cases.  The semantic patch is as follows:
> > >
> > > #spatch --allow-inconsistent-paths
> > >
> > > @@
> > > expression node;
> > > identifier child;
> > > symbol drop_me;
> > > iterator name for_each_child_of_node;
> > > @@
> > >
> > > for_each_child_of_node(node,child) {
> > >   ...
> > > + of_node_put(drop_me, child);
> > > }
> > >
> > > @@
> > > expression node;
> > > identifier child;
> > > symbol drop_me;
> > > iterator name for_each_child_of_node, for_each_child_of_node_scoped;
> > > identifier L;
> > > @@
> > >
> > > - struct device_node *child;
> > >  ... when != child
> > > -for_each_child_of_node
> > > +for_each_child_of_node_scoped
> > >   (node,child) {
> > >    ... when strict
> > > (
> > > -   {
> > > -   of_node_put(child);
> > >     return ...;
> > > -   }
> > > |
> > > -   {
> > > -   of_node_put(child);
> > >     goto L;
> > > -   }
> > > |
> > > -   {
> > > -   of_node_put(child);
> > >     break;
> > > -   }
> > > |
> > >     continue;
> > > |
> > > -   of_node_put(child);
> > >     return ...;
> > > |
> > > -   of_node_put(child);
> > >     break;
> > > |
> > > -  of_node_put(drop_me, child);
> > > )
> > > }
> > >  ... when != child
> > >
> > > @@
> > > expression child;
> > > @@
> > >
> > > - of_node_put(drop_me, child);
> > >
> > > -------------------------------
> > >
> > > This is quite conservative, in that it requires the only use of the child
> > > variable to be in a single for_each_child_of_node loop at top level.
> > >
> > > The drop_me thing is a hack to be able to refer to the bottom of the loop
> > > in the same way as of_node_puts in front of returns etc are referenced.
> > >
> > > This works fine when multiple device_node variables are declared at once.
> > >
> > > The result is below.
> > >  
> > Very nice!
> >
> > One issue is that Rob is keen that we also take this opportunity to
> > evaluate if the _available_ form is the more appropriate one.
> >
> > Given that access either no defined "status" in the child node or
> > it being set to "okay" it is what should be used in the vast majority of
> > cases.
> >
> > For reference, the property.h version only uses the available form.
> >
> > So I think we'll need some hand checking of each case but for vast majority
> > it will be very straight forward.  
> 
> I'm not sure to follow this.  If the check is straightforward, perhaps it
> can be integrated into the rule?  But I'm not sure what to check for.

I don't think it will be easy.  Perhaps Rob can help on when the
_available_ case is the wrong one?  Is this ever a characteristic of
the binding. I guess in some cases it might be a characteristic of
the binding?

> 
> > One question is whether it is worth the scoped loops in cases
> > where there isn't a patch where we break out of or return from the loop
> > before it finishes.  Do we put them in as a defensive measure?  
> 
> I wondered about this also.  My thought was that it is better to be
> uniform.  And maybe a break would be added later.

Rob and other DT folk, what do you think on this?
Shall we convert cases where there isn't currently a path in which the
autocleanup receives anything other than a NULL.

i.e.

for_each_available_of_child_node_scoped(x, y) {
	no breaks or returns form in here.
}

on basis it's not 'wrong' and is defensive against future changes that
would require manual cleanup or the scoped for loop.

> 
> > Sometimes we are going to want to combine this refactor with
> > some of the ones your previous script caught in a single patch given
> > it's roughly the same sort of change.  
> 
> Agreed.  Some blocks of code should indeed become much simpler.
> 
> >
> >  
> > > julia
> > >
> > > diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > --- a/drivers/of/unittest.c
> > > +++ b/drivers/of/unittest.c
> > > @@ -2789,7 +2789,7 @@ static int unittest_i2c_mux_probe(struct
> > >  	int i, nchans;
> > >  	struct device *dev = &client->dev;
> > >  	struct i2c_adapter *adap = client->adapter;
> > > -	struct device_node *np = client->dev.of_node, *child;
> > > +	struct device_node *np = client->dev.of_node;
> > >  	struct i2c_mux_core *muxc;
> > >  	u32 reg, max_reg;
> > >
> > > @@ -2801,7 +2801,7 @@ static int unittest_i2c_mux_probe(struct
> > >  	}
> > >
> > >  	max_reg = (u32)-1;
> > > -	for_each_child_of_node(np, child) {
> > > +	for_each_child_of_node_scoped(np, child) {  
> >
> > This was a case I left alone in the original series because the auto
> > cleanup doesn't end up doing anything in any paths.
> >  
> > >  		if (of_property_read_u32(child, "reg", &reg))
> > >  			continue;
> > >  		if (max_reg == (u32)-1 || reg > max_reg)
> > >  
> >
> >
> >  
> > > diff -u -p a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
> > > --- a/drivers/regulator/scmi-regulator.c
> > > +++ b/drivers/regulator/scmi-regulator.c
> > > @@ -297,7 +297,7 @@ static int process_scmi_regulator_of_nod
> > >  static int scmi_regulator_probe(struct scmi_device *sdev)
> > >  {
> > >  	int d, ret, num_doms;
> > > -	struct device_node *np, *child;
> > > +	struct device_node *np;
> > >  	const struct scmi_handle *handle = sdev->handle;
> > >  	struct scmi_regulator_info *rinfo;
> > >  	struct scmi_protocol_handle *ph;
> > > @@ -341,13 +341,11 @@ static int scmi_regulator_probe(struct s
> > >  	 */
> > >  	of_node_get(handle->dev->of_node);
> > >  	np = of_find_node_by_name(handle->dev->of_node, "regulators");
> > > -	for_each_child_of_node(np, child) {
> > > +	for_each_child_of_node_scoped(np, child) {
> > >  		ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo);
> > >  		/* abort on any mem issue */
> > > -		if (ret == -ENOMEM) {
> > > -			of_node_put(child);
> > > +		if (ret == -ENOMEM)
> > >  			return ret;
> > > -		}  
> > Current code leaks np in this path :(
> >  
> > >  	}
> > >  	of_node_put(np);
> > >  	/*  
> >
> >  
> > > diff -u -p a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c
> > > --- a/drivers/crypto/nx/nx-common-powernv.c
> > > +++ b/drivers/crypto/nx/nx-common-powernv.c
> > > @@ -907,7 +907,6 @@ static int __init nx_powernv_probe_vas(s
> > >  {
> > >  	int chip_id, vasid, ret = 0;
> > >  	int ct_842 = 0, ct_gzip = 0;
> > > -	struct device_node *dn;
> > >
> > >  	chip_id = of_get_ibm_chip_id(pn);
> > >  	if (chip_id < 0) {
> > > @@ -921,7 +920,7 @@ static int __init nx_powernv_probe_vas(s
> > >  		return -EINVAL;
> > >  	}
> > >
> > > -	for_each_child_of_node(pn, dn) {
> > > +	for_each_child_of_node_scoped(pn, dn) {
> > >  		ret = find_nx_device_tree(dn, chip_id, vasid, NX_CT_842,
> > >  					"ibm,p9-nx-842", &ct_842);
> > >
> > > @@ -929,10 +928,8 @@ static int __init nx_powernv_probe_vas(s
> > >  			ret = find_nx_device_tree(dn, chip_id, vasid,
> > >  				NX_CT_GZIP, "ibm,p9-nx-gzip", &ct_gzip);  
> > The handling in here is odd (buggy?). There is an of_node_put()
> > in the failure path inside find_nx_device_tree() as well as out here.  
> > >
> > > -		if (ret) {
> > > -			of_node_put(dn);
> > > +		if (ret)
> > >  			return ret;
> > > -		}
> > >  	}
> > >
> > >  	if (!ct_842 || !ct_gzip) {  
> >
> > I've glanced at a few of the others and some of them are hard.
> > This refactor is fine, but the other device_node handling often
> > is complex and I think fragile.  So definitely room for improvement!  
> 
> I agree with all the above comments.
> 
> julia