[PATCH 0/4] add missing of_node_puts

[PATCH 0/4] add missing of_node_puts

[Julia Lawall]

The device node iterators perform an of_node_get on each iteration, so a
jump out of the loop requires an of_node_put.

The patches for drivers/gpu/drm/imx/imx-ldb.c and
drivers/gpu/drm/sun4i/sun4i_backend.c contain some added of_node_puts for
which the need was identified manually.  Details are in the patch logs.

---

 drivers/gpu/drm/imx/imx-ldb.c           |   25 +++++++++++++++++--------
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  |    5 ++++-
 drivers/gpu/drm/rockchip/rockchip_rgb.c |    4 +++-
 drivers/gpu/drm/sun4i/sun4i_backend.c   |    1 +
 4 files changed, 25 insertions(+), 10 deletions(-)

[PATCH 1/4] drm/mediatek: add missing of_node_puts

[Julia Lawall]

The device node iterators perform an of_node_get on each iteration, so a
jump out of the loop requires an of_node_put.

The semantic patch that fixes this problem is as follows
(http://coccinelle.lip6.fr):

// <smpl>
@@
local idexpression n;
expression e,e1;
identifier l;
@@

 for_each_child_of_node(e1,n) {
   ...
(
   of_node_put(n);
|
   e = n
|
+  of_node_put(n);
?  goto l;
)
   ...
 }
...
l: ... when != n
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 6422e99..b9205bf 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -522,12 +522,15 @@ static int mtk_drm_probe(struct platform_device *pdev)
 			comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
 			if (!comp) {
 				ret = -ENOMEM;
+				of_node_put(node);
 				goto err_node;
 			}
 
 			ret = mtk_ddp_comp_init(dev, node, comp, comp_id, NULL);
-			if (ret)
+			if (ret) {
+				of_node_put(node);
 				goto err_node;
+			}
 
 			private->ddp_comp[comp_id] = comp;
 		}

[PATCH 2/4] drm/imx: imx-ldb: add missing of_node_puts

[Julia Lawall]

The device node iterators perform an of_node_get on each
iteration, so a jump out of the loop requires an of_node_put.

Move the initialization channel->child = child; down to just
before the call to imx_ldb_register so that intervening failures
don't need to clear it.  Add a label at the end of the function to
do all the of_node_puts.

The semantic patch that finds part of this problem is as follows
(http://coccinelle.lip6.fr):

// <smpl>
@@
expression root,e;
local idexpression child;
iterator name for_each_child_of_node;
@@

 for_each_child_of_node(root, child) {
   ... when != of_node_put(child)
       when != e = child
(
   return child;
|
*  return ...;
)
   ...
 }
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
This changes the semantics, with respect to the availability of the child
field, and has not been tested.

 drivers/gpu/drm/imx/imx-ldb.c |   25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 2c5bbe3..e31e263 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -643,8 +643,10 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		int bus_format;
 
 		ret = of_property_read_u32(child, "reg", &i);
-		if (ret || i < 0 || i > 1)
-			return -EINVAL;
+		if (ret || i < 0 || i > 1) {
+			ret = -EINVAL;
+			goto free_child;
+		}
 
 		if (!of_device_is_available(child))
 			continue;
@@ -657,7 +659,6 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		channel = &imx_ldb->channel[i];
 		channel->ldb = imx_ldb;
 		channel->chno = i;
-		channel->child = child;
 
 		/*
 		 * The output port is port@4 with an external 4-port mux or
@@ -667,13 +668,13 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 						  imx_ldb->lvds_mux ? 4 : 2, 0,
 						  &channel->panel, &channel->bridge);
 		if (ret && ret != -ENODEV)
-			return ret;
+			goto free_child;
 
 		/* panel ddc only if there is no bridge */
 		if (!channel->bridge) {
 			ret = imx_ldb_panel_ddc(dev, channel, child);
 			if (ret)
-				return ret;
+				goto free_child;
 		}
 
 		bus_format = of_get_bus_format(dev, child);
@@ -689,18 +690,26 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		if (bus_format < 0) {
 			dev_err(dev, "could not determine data mapping: %d\n",
 				bus_format);
-			return bus_format;
+			ret = bus_format;
+			goto free_child;
 		}
 		channel->bus_format = bus_format;
+		channel->child = child;
 
 		ret = imx_ldb_register(drm, channel);
-		if (ret)
-			return ret;
+		if (ret) {
+			channel->child = NULL;
+			goto free_child;
+		}
 	}
 
 	dev_set_drvdata(dev, imx_ldb);
 
 	return 0;
+
+free_child:
+	of_node_put(child);
+	return ret;
 }
 
 static void imx_ldb_unbind(struct device *dev, struct device *master,

[PATCH 3/4] drm/rockchip: add missing of_node_put

[Julia Lawall]

The device node iterators perform an of_node_get on each iteration, so a
jump out of the loop requires an of_node_put.

The semantic patch that fixes this problem is as follows
(http://coccinelle.lip6.fr):

// <smpl>
@@
expression root,e;
local idexpression child;
iterator name for_each_child_of_node;
@@

 for_each_child_of_node(root, child) {
   ... when != of_node_put(child)
       when != e = child
+  of_node_put(child);
?  break;
   ...
}
... when != child
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/gpu/drm/rockchip/rockchip_rgb.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 96ac145..37f9302 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -113,8 +113,10 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 		child_count++;
 		ret = drm_of_find_panel_or_bridge(dev->of_node, 0, endpoint_id,
 						  &panel, &bridge);
-		if (!ret)
+		if (!ret) {
+			of_node_put(endpoint);
 			break;
+		}
 	}
 
 	of_node_put(port);

[PATCH 4/4] drm/sun4i: backend: add missing of_node_puts

[Julia Lawall]

The device node iterators perform an of_node_get on each
iteration, so a jump out of the loop requires an of_node_put.

Remote and port also have augmented reference counts, so drop them
on each iteration and at the end of the function, respectively.
Remote is only used for the address it contains, not for the
contents of that address, so the reference count can be dropped
immediately.

The semantic patch that fixes the first part of this problem is
as follows (http://coccinelle.lip6.fr):

// <smpl>
@@
expression root,e;
local idexpression child;
iterator name for_each_child_of_node;
@@

 for_each_available_child_of_node(root, child) {
   ... when != of_node_put(child)
       when != e = child
+  of_node_put(child);
?  break;
   ...
}
... when != child
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not tested.

 drivers/gpu/drm/sun4i/sun4i_backend.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 9e9255e..a021bab 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -786,17 +786,18 @@ static struct sun4i_frontend *sun4i_backend_find_frontend(struct sun4i_drv *drv,
 		remote = of_graph_get_remote_port_parent(ep);
 		if (!remote)
 			continue;
+		of_node_put(remote);
 
 		/* does this node match any registered engines? */
 		list_for_each_entry(frontend, &drv->frontend_list, list) {
 			if (remote == frontend->node) {
-				of_node_put(remote);
 				of_node_put(port);
+				of_node_put(ep);
 				return frontend;
 			}
 		}
 	}
-
+	of_node_put(port);
 	return ERR_PTR(-EINVAL);
 }
 

Re: [PATCH 3/4] drm/rockchip: add missing of_node_put

[Heiko Stuebner]

Am Sonntag, 13. Januar 2019, 09:47:43 CET schrieb Julia Lawall:
> The device node iterators perform an of_node_get on each iteration, so a
> jump out of the loop requires an of_node_put.
> 
> The semantic patch that fixes this problem is as follows
> (http://coccinelle.lip6.fr):
> 
> // <smpl>
> @@
> expression root,e;
> local idexpression child;
> iterator name for_each_child_of_node;
> @@
> 
>  for_each_child_of_node(root, child) {
>    ... when != of_node_put(child)
>        when != e = child
> +  of_node_put(child);
> ?  break;
>    ...
> }
> ... when != child
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

I've added a fixes+stable tag and applied it to drm-misc-fixes

Thanks for catching that
Heiko

Re: [PATCH 3/4] drm/rockchip: add missing of_node_put

[Daniel Vetter]

On Sun, Jan 13, 2019 at 07:48:49PM +0100, Heiko Stuebner wrote:
> Am Sonntag, 13. Januar 2019, 09:47:43 CET schrieb Julia Lawall:
> > The device node iterators perform an of_node_get on each iteration, so a
> > jump out of the loop requires an of_node_put.
> > 
> > The semantic patch that fixes this problem is as follows
> > (http://coccinelle.lip6.fr):
> > 
> > // <smpl>
> > @@
> > expression root,e;
> > local idexpression child;
> > iterator name for_each_child_of_node;
> > @@
> > 
> >  for_each_child_of_node(root, child) {
> >    ... when != of_node_put(child)
> >        when != e = child
> > +  of_node_put(child);
> > ?  break;
> >    ...
> > }
> > ... when != child
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> I've added a fixes+stable tag and applied it to drm-misc-fixes

All of them or just this one here? These cleanup patches have a high
chance of falling through cracks, so taking them all usually works out
better ...
-Daniel

> 
> Thanks for catching that
> Heiko
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/4] drm/rockchip: add missing of_node_put

[Heiko Stuebner]

Am Montag, 14. Januar 2019, 11:05:56 CET schrieb Daniel Vetter:
> On Sun, Jan 13, 2019 at 07:48:49PM +0100, Heiko Stuebner wrote:
> > Am Sonntag, 13. Januar 2019, 09:47:43 CET schrieb Julia Lawall:
> > > The device node iterators perform an of_node_get on each iteration, so a
> > > jump out of the loop requires an of_node_put.
> > > 
> > > The semantic patch that fixes this problem is as follows
> > > (http://coccinelle.lip6.fr):
> > > 
> > > // <smpl>
> > > @@
> > > expression root,e;
> > > local idexpression child;
> > > iterator name for_each_child_of_node;
> > > @@
> > > 
> > >  for_each_child_of_node(root, child) {
> > >    ... when != of_node_put(child)
> > >        when != e = child
> > > +  of_node_put(child);
> > > ?  break;
> > >    ...
> > > }
> > > ... when != child
> > > // </smpl>
> > > 
> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > I've added a fixes+stable tag and applied it to drm-misc-fixes
> 
> All of them or just this one here? These cleanup patches have a high
> chance of falling through cracks, so taking them all usually works out
> better ...

That is the only one I got, so right now I only applied this one in my inbox.
Weekend and such resulted in me not going looking for other ones.

Heiko

Re: [PATCH 4/4] drm/sun4i: backend: add missing of_node_puts

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

On Sun, Jan 13, 2019 at 09:47:44AM +0100, Julia Lawall wrote:
> The device node iterators perform an of_node_get on each
> iteration, so a jump out of the loop requires an of_node_put.
> 
> Remote and port also have augmented reference counts, so drop them
> on each iteration and at the end of the function, respectively.
> Remote is only used for the address it contains, not for the
> contents of that address, so the reference count can be dropped
> immediately.
> 
> The semantic patch that fixes the first part of this problem is
> as follows (http://coccinelle.lip6.fr):
> 
> // <smpl>
> @@
> expression root,e;
> local idexpression child;
> iterator name for_each_child_of_node;
> @@
> 
>  for_each_available_child_of_node(root, child) {
>    ... when != of_node_put(child)
>        when != e = child
> +  of_node_put(child);
> ?  break;
>    ...
> }
> ... when != child
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] drm/imx: imx-ldb: add missing of_node_puts

[Philipp Zabel]

Hi Julia,

On Sun, 2019-01-13 at 09:47 +0100, Julia Lawall wrote:
> The device node iterators perform an of_node_get on each
> iteration, so a jump out of the loop requires an of_node_put.
> 
> Move the initialization channel->child = child; down to just
> before the call to imx_ldb_register so that intervening failures
> don't need to clear it.  Add a label at the end of the function to
> do all the of_node_puts.

Thank you, I've applied the patch to the imx-drm/fixes branch.

regards
Philipp