[PATCH 0/5] add missing of_node_put

[PATCH 0/5] add missing of_node_put

[Julia Lawall]

The various for_each device_node iterators performs an of_node_get on each
iteration, so a break out of the loop requires an of_node_put.

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

// <smpl>
@r@
local idexpression n;
expression e1,e2;
iterator name for_each_node_by_name, for_each_node_by_type,
for_each_compatible_node, for_each_matching_node,
for_each_matching_node_and_match, for_each_child_of_node,
for_each_available_child_of_node, for_each_node_with_property;
iterator i;
statement S;
expression list [n1] es;
@@

(
(
for_each_node_by_name(n,e1) S
|
for_each_node_by_type(n,e1) S
|
for_each_compatible_node(n,e1,e2) S
|
for_each_matching_node(n,e1) S
|
for_each_matching_node_and_match(n,e1,e2) S
|
for_each_child_of_node(e1,n) S
|
for_each_available_child_of_node(e1,n) S
|
for_each_node_with_property(n,e1) S
)
&
i(es,n,...) S
)

@@
local idexpression r.n;
iterator r.i;
expression e;
expression list [r.n1] es;
@@

 i(es,n,...) {
   ...
(
   of_node_put(n);
|
   e = n
|
   return n;
|
+  of_node_put(n);
?  return ...;
)
   ...
 }

@@
local idexpression r.n;
iterator r.i;
expression e;
expression list [r.n1] es;
@@

 i(es,n,...) {
   ...
(
   of_node_put(n);
|
   e = n
|
+  of_node_put(n);
?  break;
)
   ...
 }
... when != n

@@
local idexpression r.n;
iterator r.i;
expression e;
identifier l;
expression list [r.n1] es;
@@

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

---

 drivers/clk/clk-scpi.c      |    1 +
 drivers/clk/clk-si5351.c    |   17 ++++++++++-------
 drivers/clk/clk.c           |    4 ++++
 drivers/clk/imx/clk-imx27.c |    4 +++-
 drivers/clk/imx/clk-imx31.c |    4 +++-
 5 files changed, 21 insertions(+), 9 deletions(-)

[PATCH 1/5] clk: add missing of_node_put

[Julia Lawall]

for_each_matching_node_and_match performs an of_node_get on each iteration,
so a break out of the loop requires an of_node_put.

A simplified version of the semantic patch that fixes this problem is as
follows (http://coccinelle.lip6.fr):

// <smpl>
@@
expression e1,e2,e;
local idexpression np;
@@

 for_each_matching_node_and_match(np, e1, e2) {
   ... when != of_node_put(np)
       when != e = np
(
   return np;
|
+  of_node_put(np);
?  return ...;
)
   ...
 }
// </smpl>

Besides the problem identified by the semantic patch, this patch adds an
of_node_get in front of saving np in a field of parent, to account for the
fact that this value will be put on going on to the next element in the
iteration, and then adds of_node_puts in the two loops where the parent
pointer can be freed.

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

---
 drivers/clk/clk.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e735eab..11babd1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3200,12 +3200,15 @@ void __init of_clk_init(const struct of_device_id *matches)
 			list_for_each_entry_safe(clk_provider, next,
 						 &clk_provider_list, node) {
 				list_del(&clk_provider->node);
+				of_node_put(clk_provider->np);
 				kfree(clk_provider);
 			}
+			of_node_put(np);
 			return;
 		}
 
 		parent->clk_init_cb = match->data;
+		of_node_get(np);
 		parent->np = np;
 		list_add_tail(&parent->node, &clk_provider_list);
 	}
@@ -3220,6 +3223,7 @@ void __init of_clk_init(const struct of_device_id *matches)
 				of_clk_set_defaults(clk_provider->np, true);
 
 				list_del(&clk_provider->node);
+				of_node_put(clk_provider->np);
 				kfree(clk_provider);
 				is_init_done = true;
 			}

[PATCH 2/5] clk: si5351: add missing of_node_put

[Julia Lawall]

for_each_child_of_node performs an of_node_get on each iteration, so
a break out of the loop requires an of_node_put.

A simplified version of the semantic patch that fixes this problem is as
follows (http://coccinelle.lip6.fr):

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

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

The resulting puts were manually moved to the end of the function for
conciseness.

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

---
 drivers/clk/clk-si5351.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 5596c0a..e346b22 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -1183,13 +1183,13 @@ static int si5351_dt_parse(struct i2c_client *client,
 		if (of_property_read_u32(child, "reg", &num)) {
 			dev_err(&client->dev, "missing reg property of %s\n",
 				child->name);
-			return -EINVAL;
+			goto put_child;
 		}
 
 		if (num >= 8 ||
 		    (variant == SI5351_VARIANT_A3 && num >= 3)) {
 			dev_err(&client->dev, "invalid clkout %d\n", num);
-			return -EINVAL;
+			goto put_child;
 		}
 
 		if (!of_property_read_u32(child, "silabs,multisynth-source",
@@ -1207,7 +1207,7 @@ static int si5351_dt_parse(struct i2c_client *client,
 				dev_err(&client->dev,
 					"invalid parent %d for multisynth %d\n",
 					val, num);
-				return -EINVAL;
+				goto put_child;
 			}
 		}
 
@@ -1230,7 +1230,7 @@ static int si5351_dt_parse(struct i2c_client *client,
 					dev_err(&client->dev,
 						"invalid parent %d for clkout %d\n",
 						val, num);
-					return -EINVAL;
+					goto put_child;
 				}
 				pdata->clkout[num].clkout_src =
 					SI5351_CLKOUT_SRC_CLKIN;
@@ -1239,7 +1239,7 @@ static int si5351_dt_parse(struct i2c_client *client,
 				dev_err(&client->dev,
 					"invalid parent %d for clkout %d\n",
 					val, num);
-				return -EINVAL;
+				goto put_child;
 			}
 		}
 
@@ -1256,7 +1256,7 @@ static int si5351_dt_parse(struct i2c_client *client,
 				dev_err(&client->dev,
 					"invalid drive strength %d for clkout %d\n",
 					val, num);
-				return -EINVAL;
+				goto put_child;
 			}
 		}
 
@@ -1283,7 +1283,7 @@ static int si5351_dt_parse(struct i2c_client *client,
 				dev_err(&client->dev,
 					"invalid disable state %d for clkout %d\n",
 					val, num);
-				return -EINVAL;
+				goto put_child;
 			}
 		}
 
@@ -1296,6 +1296,9 @@ static int si5351_dt_parse(struct i2c_client *client,
 	client->dev.platform_data = pdata;
 
 	return 0;
+put_child:
+	of_node_put(child);
+	return -EINVAL;
 }
 #else
 static int si5351_dt_parse(struct i2c_client *client, enum si5351_variant variant)

[PATCH 3/5] clk: imx27: add missing of_node_put

[Julia Lawall]

for_each_compatible_node performs an of_node_get on each iteration, so a
break 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;
@@

 for_each_compatible_node(n,...) {
   ...
(
   of_node_put(n);
|
   e = n
|
+  of_node_put(n);
?  break;
)
   ...
 }
... when != n
// </smpl>

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

---
 drivers/clk/imx/clk-imx27.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx27.c b/drivers/clk/imx/clk-imx27.c
index 0d7b8df..cf5cf75 100644
--- a/drivers/clk/imx/clk-imx27.c
+++ b/drivers/clk/imx/clk-imx27.c
@@ -261,8 +261,10 @@ static void __init mx27_clocks_init_dt(struct device_node *np)
 		if (!of_device_is_compatible(refnp, "fsl,imx-osc26m"))
 			continue;
 
-		if (!of_property_read_u32(refnp, "clock-frequency", &fref))
+		if (!of_property_read_u32(refnp, "clock-frequency", &fref)) {
+			of_node_put(refnp);
 			break;
+		}
 	}
 
 	ccm = of_iomap(np, 0);

[PATCH 4/5] clk: imx31: add missing of_node_put

[Julia Lawall]

for_each_compatible_node performs an of_node_get on each iteration, so a
break 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;
@@

 for_each_compatible_node(n,...) {
   ...
(
   of_node_put(n);
|
   e = n
|
+  of_node_put(n);
?  break;
)
   ...
 }
... when != n
// </smpl>

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

---
 drivers/clk/imx/clk-imx31.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx31.c b/drivers/clk/imx/clk-imx31.c
index f65b8b1..6a96414 100644
--- a/drivers/clk/imx/clk-imx31.c
+++ b/drivers/clk/imx/clk-imx31.c
@@ -233,8 +233,10 @@ int __init mx31_clocks_init_dt(void)
 		if (!of_device_is_compatible(np, "fsl,imx-osc26m"))
 			continue;
 
-		if (!of_property_read_u32(np, "clock-frequency", &fref))
+		if (!of_property_read_u32(np, "clock-frequency", &fref)) {
+			of_node_put(np);
 			break;
+		}
 	}
 
 	_mx31_clocks_init(fref);

[PATCH 5/5] clk: scpi: add missing of_node_put

[Julia Lawall]

for_each_available_child_of_node performs an of_node_get on each iteration,
so a break 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;
@@

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

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

---
 drivers/clk/clk-scpi.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
index 0b501a9..cd0f272 100644
--- a/drivers/clk/clk-scpi.c
+++ b/drivers/clk/clk-scpi.c
@@ -292,6 +292,7 @@ static int scpi_clocks_probe(struct platform_device *pdev)
 		ret = scpi_clk_add(dev, child, match);
 		if (ret) {
 			scpi_clocks_remove(pdev);
+			of_node_put(child);
 			return ret;
 		}
 	}

Re: [PATCH 1/5] clk: add missing of_node_put

[Stephen Boyd]

On 10/21, Julia Lawall wrote:
> for_each_matching_node_and_match performs an of_node_get on each iteration,
> so a break out of the loop requires an of_node_put.
> 
> A simplified version of the semantic patch that fixes this problem is as
> follows (http://coccinelle.lip6.fr):
> 
> // <smpl>
> @@
> expression e1,e2,e;
> local idexpression np;
> @@
> 
>  for_each_matching_node_and_match(np, e1, e2) {
>    ... when != of_node_put(np)
>        when != e = np
> (
>    return np;
> |
> +  of_node_put(np);
> ?  return ...;
> )
>    ...
>  }
> // </smpl>
> 
> Besides the problem identified by the semantic patch, this patch adds an
> of_node_get in front of saving np in a field of parent, to account for the
> fact that this value will be put on going on to the next element in the
> iteration, and then adds of_node_puts in the two loops where the parent
> pointer can be freed.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---

Applied to clk-next, except I collapsed the of_node_get() into
the assignment.

---8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d366bfb66c58..2eae76f21d6f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3205,8 +3205,7 @@ void __init of_clk_init(const struct of_device_id *matches)
 		}
 
 		parent->clk_init_cb = match->data;
-		of_node_get(np);
-		parent->np = np;
+		parent->np = of_node_get(np);
 		list_add_tail(&parent->node, &clk_provider_list);
 	}
 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/5] clk: si5351: add missing of_node_put

[Stephen Boyd]

On 10/21, Julia Lawall wrote:
> for_each_child_of_node performs an of_node_get on each iteration, so
> a break out of the loop requires an of_node_put.
> 
> A simplified version of the semantic patch that fixes this problem is as
> follows (http://coccinelle.lip6.fr):
> 
> // <smpl>
> @@
> expression root,e;
> local idexpression child;
> @@
> 
>  for_each_child_of_node(root, child) {
>    ... when != of_node_put(child)
>        when != e = child
> (
>    return child;
> |
> +  of_node_put(child);
> ?  return ...;
> )
>    ...
>  }
> // </smpl>
> 
> The resulting puts were manually moved to the end of the function for
> conciseness.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/5] clk: imx27: add missing of_node_put

[Stephen Boyd]

On 10/21, Julia Lawall wrote:
> for_each_compatible_node performs an of_node_get on each iteration, so a
> break 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;
> @@
> 
>  for_each_compatible_node(n,...) {
>    ...
> (
>    of_node_put(n);
> |
>    e = n
> |
> +  of_node_put(n);
> ?  break;
> )
>    ...
>  }
> ... when != n
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/5] clk: imx31: add missing of_node_put

[Stephen Boyd]

On 10/21, Julia Lawall wrote:
> for_each_compatible_node performs an of_node_get on each iteration, so a
> break 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;
> @@
> 
>  for_each_compatible_node(n,...) {
>    ...
> (
>    of_node_put(n);
> |
>    e = n
> |
> +  of_node_put(n);
> ?  break;
> )
>    ...
>  }
> ... when != n
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 5/5] clk: scpi: add missing of_node_put

[Stephen Boyd]

On 10/21, Julia Lawall wrote:
> for_each_available_child_of_node performs an of_node_get on each iteration,
> so a break 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;
> @@
> 
>  for_each_available_child_of_node(root, child) {
>    ... when != of_node_put(child)
>        when != e = child
> (
>    return child;
> |
> +  of_node_put(child);
> ?  return ...;
> )
>    ...
>  }
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---

Acked-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/5] clk: add missing of_node_put

[Julia Lawall]

On Wed, 21 Oct 2015, Stephen Boyd wrote:

> On 10/21, Julia Lawall wrote:
> > for_each_matching_node_and_match performs an of_node_get on each iteration,
> > so a break out of the loop requires an of_node_put.
> > 
> > A simplified version of the semantic patch that fixes this problem is as
> > follows (http://coccinelle.lip6.fr):
> > 
> > // <smpl>
> > @@
> > expression e1,e2,e;
> > local idexpression np;
> > @@
> > 
> >  for_each_matching_node_and_match(np, e1, e2) {
> >    ... when != of_node_put(np)
> >        when != e = np
> > (
> >    return np;
> > |
> > +  of_node_put(np);
> > ?  return ...;
> > )
> >    ...
> >  }
> > // </smpl>
> > 
> > Besides the problem identified by the semantic patch, this patch adds an
> > of_node_get in front of saving np in a field of parent, to account for the
> > fact that this value will be put on going on to the next element in the
> > iteration, and then adds of_node_puts in the two loops where the parent
> > pointer can be freed.
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> 
> Applied to clk-next, except I collapsed the of_node_get() into
> the assignment.
> 
> ---8<---
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d366bfb66c58..2eae76f21d6f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3205,8 +3205,7 @@ void __init of_clk_init(const struct of_device_id *matches)
>  		}
>  
>  		parent->clk_init_cb = match->data;
> -		of_node_get(np);
> -		parent->np = np;
> +		parent->np = of_node_get(np);

Thanks!

julia

>  		list_add_tail(&parent->node, &clk_provider_list);
>  	}
>  
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/5] clk: scpi: add missing of_node_put

[Sudeep Holla]

On 21/10/15 21:41, Julia Lawall wrote:
> for_each_available_child_of_node performs an of_node_get on each iteration,
> so a break 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;
> @@
>
>   for_each_available_child_of_node(root, child) {
>     ... when != of_node_put(child)
>         when != e = child
> (
>     return child;
> |
> +  of_node_put(child);
> ?  return ...;
> )
>     ...
>   }
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Thanks for the fix.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

PS: Since this driver is queued via arm-soc, it needs to go via them or
wait for v4.4-rc1 and then queue via clk tree.

-- 
Regards,
Sudeep

Re: [PATCH 5/5] clk: scpi: add missing of_node_put

[Sudeep Holla]

Hi Mike/Stephen,

On Thu, Oct 22, 2015 at 10:21 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> Thanks for the fix.
>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>
> PS: Since this driver is queued via arm-soc, it needs to go via them or
> wait for v4.4-rc1 and then queue via clk tree.
>

Now that the driver is in mainline, can you take this fix via clk tree for
you next -rc fixes ?

Regards,
Sudeep

Re: [PATCH 5/5] clk: scpi: add missing of_node_put

[Stephen Boyd]

On 11/26, Sudeep Holla wrote:
> Hi Mike/Stephen,
> 
> On Thu, Oct 22, 2015 at 10:21 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> >
> > Thanks for the fix.
> >
> > Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> >
> > PS: Since this driver is queued via arm-soc, it needs to go via them or
> > wait for v4.4-rc1 and then queue via clk tree.
> >
> 
> Now that the driver is in mainline, can you take this fix via clk tree for
> you next -rc fixes ?
> 

Sure.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 5/5] clk: scpi: add missing of_node_put

[Stephen Boyd]

On 10/21, Julia Lawall wrote:
> for_each_available_child_of_node performs an of_node_get on each iteration,
> so a break 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;
> @@
> 
>  for_each_available_child_of_node(root, child) {
>    ... when != of_node_put(child)
>        when != e = child
> (
>    return child;
> |
> +  of_node_put(child);
> ?  return ...;
> )
>    ...
>  }
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---

Applied to clk-fixes

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project