Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Frederic Barrat]

Le 29/07/2016 à 13:38, Michael Ellerman a écrit :

But who does keep a reference on the device_node? I can't see it anywhere. Which
means in theory the device_node can be freed out from under you.

You have a reference for afu_np as part of for_each_child_of_node(), but it's
dropped as soon as you go around the loop.

The typical pattern would be that cxl_guest_init_afu() takes an additional
reference once it's done all its setup and can't fail.

That way at the end of the loop when the loop construct has dropped all
references, the nodes you actually init'ed have their reference count
incremented by 1.



We don't keep a reference on the AFU device_node. Once we've read the 
config of the AFU, the AFU device_node is never accessed again. So I 
guess it's possible (though unexpected) that it's freed from under us, 
but it should not affect the driver.


The AFU is really dependent on the adapter itself, which is one level up 
in the device tree, and for which we create a device through 
of_platform_device_create(). The properties under the AFU device node 
are read directly from the PCI config space in the bare-metal case, 
where the cxl adapter is a PCI device.


Do we really have a problem here?

  Fred

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Frederic Barrat]

Le 29/07/2016 à 13:38, Michael Ellerman a écrit :

But who does keep a reference on the device_node? I can't see it anywhere. Which
means in theory the device_node can be freed out from under you.

You have a reference for afu_np as part of for_each_child_of_node(), but it's
dropped as soon as you go around the loop.

The typical pattern would be that cxl_guest_init_afu() takes an additional
reference once it's done all its setup and can't fail.

That way at the end of the loop when the loop construct has dropped all
references, the nodes you actually init'ed have their reference count
incremented by 1.



We don't keep a reference on the AFU device_node. Once we've read the 
config of the AFU, the AFU device_node is never accessed again. So I 
guess it's possible (though unexpected) that it's freed from under us, 
but it should not affect the driver.


The AFU is really dependent on the adapter itself, which is one level up 
in the device tree, and for which we create a device through 
of_platform_device_create(). The properties under the AFU device node 
are read directly from the PCI config space in the bare-metal case, 
where the cxl adapter is a PCI device.


Do we really have a problem here?

  Fred

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Michael Ellerman]

Andrew Donnellan  writes:

> Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
> for_each_child_of_node() rather than a hand-coded for loop.
>
> Remove the useless of_node_put(afu_np) call after the loop, where it's
> guaranteed that afu_np == NULL.
>
> Reported-by: SF Markus Elfring 
> Reported-by: Julia Lawall 
> Signed-off-by: Andrew Donnellan 
>
> ---
>
> Checked the of_node_put() with Fred, he thinks it was probably just left
> over from an earlier private version of the code and we can just get rid of
> it.

But who does keep a reference on the device_node? I can't see it anywhere. Which
means in theory the device_node can be freed out from under you.

You have a reference for afu_np as part of for_each_child_of_node(), but it's
dropped as soon as you go around the loop.

The typical pattern would be that cxl_guest_init_afu() takes an additional
reference once it's done all its setup and can't fail.

That way at the end of the loop when the loop construct has dropped all
references, the nodes you actually init'ed have their reference count
incremented by 1.

cheers

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Michael Ellerman]

Andrew Donnellan  writes:

> Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
> for_each_child_of_node() rather than a hand-coded for loop.
>
> Remove the useless of_node_put(afu_np) call after the loop, where it's
> guaranteed that afu_np == NULL.
>
> Reported-by: SF Markus Elfring 
> Reported-by: Julia Lawall 
> Signed-off-by: Andrew Donnellan 
>
> ---
>
> Checked the of_node_put() with Fred, he thinks it was probably just left
> over from an earlier private version of the code and we can just get rid of
> it.

But who does keep a reference on the device_node? I can't see it anywhere. Which
means in theory the device_node can be freed out from under you.

You have a reference for afu_np as part of for_each_child_of_node(), but it's
dropped as soon as you go around the loop.

The typical pattern would be that cxl_guest_init_afu() takes an additional
reference once it's done all its setup and can't fail.

That way at the end of the loop when the loop construct has dropped all
references, the nodes you actually init'ed have their reference count
incremented by 1.

cheers

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Julia Lawall]

On Fri, 29 Jul 2016, walter harms wrote:

>
>
> Am 29.07.2016 05:55, schrieb Andrew Donnellan:
> > Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
> > for_each_child_of_node() rather than a hand-coded for loop.
> >
> > Remove the useless of_node_put(afu_np) call after the loop, where it's
> > guaranteed that afu_np == NULL.
> >
> > Reported-by: SF Markus Elfring 
> > Reported-by: Julia Lawall 
> > Signed-off-by: Andrew Donnellan 
> >
> > ---
> >
> > Checked the of_node_put() with Fred, he thinks it was probably just left
> > over from an earlier private version of the code and we can just get rid of
> > it.
> > ---
> >  drivers/misc/cxl/of.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
> > index edc4583..ec175ea 100644
> > --- a/drivers/misc/cxl/of.c
> > +++ b/drivers/misc/cxl/of.c
> > @@ -460,7 +460,7 @@ int cxl_of_probe(struct platform_device *pdev)
> > struct device_node *afu_np = NULL;
> > struct cxl *adapter = NULL;
> > int ret;
> > -   int slice, slice_ok;
> > +   int slice = 0, slice_ok = 0;
> >
> > pr_devel("in %s\n", __func__);
> >
> > @@ -476,13 +476,13 @@ int cxl_of_probe(struct platform_device *pdev)
> > }
> >
> > /* init afu */
> > -   slice_ok = 0;
> > -   for (afu_np = NULL, slice = 0; (afu_np = of_get_next_child(np, 
> > afu_np)); slice++) {
> > +   for_each_child_of_node(np, afu_np) {
> > if ((ret = cxl_guest_init_afu(adapter, slice, afu_np)))
> > dev_err(>dev, "AFU %i failed to initialise: %i\n",
> > slice, ret);
> > else
> > slice_ok++;
> > +   slice++;
> > }
>
> while you are here ..
> you could move the assign out of the condition..
>
> ret = cxl_guest_init_afu(adapter, slice, afu_np);
> if (ret) 

Yes, please.

julia


>
> just my 2 cents,
>
> re,
>  wh
>
> >
> > if (slice_ok == 0) {
> > @@ -490,8 +490,6 @@ int cxl_of_probe(struct platform_device *pdev)
> > adapter->slices = 0;
> > }
> >
> > -   if (afu_np)
> > -   of_node_put(afu_np);
> > return 0;
> >  }
> >
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Julia Lawall]

On Fri, 29 Jul 2016, walter harms wrote:

>
>
> Am 29.07.2016 05:55, schrieb Andrew Donnellan:
> > Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
> > for_each_child_of_node() rather than a hand-coded for loop.
> >
> > Remove the useless of_node_put(afu_np) call after the loop, where it's
> > guaranteed that afu_np == NULL.
> >
> > Reported-by: SF Markus Elfring 
> > Reported-by: Julia Lawall 
> > Signed-off-by: Andrew Donnellan 
> >
> > ---
> >
> > Checked the of_node_put() with Fred, he thinks it was probably just left
> > over from an earlier private version of the code and we can just get rid of
> > it.
> > ---
> >  drivers/misc/cxl/of.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
> > index edc4583..ec175ea 100644
> > --- a/drivers/misc/cxl/of.c
> > +++ b/drivers/misc/cxl/of.c
> > @@ -460,7 +460,7 @@ int cxl_of_probe(struct platform_device *pdev)
> > struct device_node *afu_np = NULL;
> > struct cxl *adapter = NULL;
> > int ret;
> > -   int slice, slice_ok;
> > +   int slice = 0, slice_ok = 0;
> >
> > pr_devel("in %s\n", __func__);
> >
> > @@ -476,13 +476,13 @@ int cxl_of_probe(struct platform_device *pdev)
> > }
> >
> > /* init afu */
> > -   slice_ok = 0;
> > -   for (afu_np = NULL, slice = 0; (afu_np = of_get_next_child(np, 
> > afu_np)); slice++) {
> > +   for_each_child_of_node(np, afu_np) {
> > if ((ret = cxl_guest_init_afu(adapter, slice, afu_np)))
> > dev_err(>dev, "AFU %i failed to initialise: %i\n",
> > slice, ret);
> > else
> > slice_ok++;
> > +   slice++;
> > }
>
> while you are here ..
> you could move the assign out of the condition..
>
> ret = cxl_guest_init_afu(adapter, slice, afu_np);
> if (ret) 

Yes, please.

julia


>
> just my 2 cents,
>
> re,
>  wh
>
> >
> > if (slice_ok == 0) {
> > @@ -490,8 +490,6 @@ int cxl_of_probe(struct platform_device *pdev)
> > adapter->slices = 0;
> > }
> >
> > -   if (afu_np)
> > -   of_node_put(afu_np);
> > return 0;
> >  }
> >
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[walter harms]

Am 29.07.2016 05:55, schrieb Andrew Donnellan:
> Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
> for_each_child_of_node() rather than a hand-coded for loop.
> 
> Remove the useless of_node_put(afu_np) call after the loop, where it's
> guaranteed that afu_np == NULL.
> 
> Reported-by: SF Markus Elfring 
> Reported-by: Julia Lawall 
> Signed-off-by: Andrew Donnellan 
> 
> ---
> 
> Checked the of_node_put() with Fred, he thinks it was probably just left
> over from an earlier private version of the code and we can just get rid of
> it.
> ---
>  drivers/misc/cxl/of.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
> index edc4583..ec175ea 100644
> --- a/drivers/misc/cxl/of.c
> +++ b/drivers/misc/cxl/of.c
> @@ -460,7 +460,7 @@ int cxl_of_probe(struct platform_device *pdev)
>   struct device_node *afu_np = NULL;
>   struct cxl *adapter = NULL;
>   int ret;
> - int slice, slice_ok;
> + int slice = 0, slice_ok = 0;
>  
>   pr_devel("in %s\n", __func__);
>  
> @@ -476,13 +476,13 @@ int cxl_of_probe(struct platform_device *pdev)
>   }
>  
>   /* init afu */
> - slice_ok = 0;
> - for (afu_np = NULL, slice = 0; (afu_np = of_get_next_child(np, 
> afu_np)); slice++) {
> + for_each_child_of_node(np, afu_np) {
>   if ((ret = cxl_guest_init_afu(adapter, slice, afu_np)))
>   dev_err(>dev, "AFU %i failed to initialise: %i\n",
>   slice, ret);
>   else
>   slice_ok++;
> + slice++;
>   }

while you are here ..
you could move the assign out of the condition..

ret = cxl_guest_init_afu(adapter, slice, afu_np);
if (ret) 

just my 2 cents,

re,
 wh

>  
>   if (slice_ok == 0) {
> @@ -490,8 +490,6 @@ int cxl_of_probe(struct platform_device *pdev)
>   adapter->slices = 0;
>   }
>  
> - if (afu_np)
> - of_node_put(afu_np);
>   return 0;
>  }
>  

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[walter harms]

Am 29.07.2016 05:55, schrieb Andrew Donnellan:
> Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
> for_each_child_of_node() rather than a hand-coded for loop.
> 
> Remove the useless of_node_put(afu_np) call after the loop, where it's
> guaranteed that afu_np == NULL.
> 
> Reported-by: SF Markus Elfring 
> Reported-by: Julia Lawall 
> Signed-off-by: Andrew Donnellan 
> 
> ---
> 
> Checked the of_node_put() with Fred, he thinks it was probably just left
> over from an earlier private version of the code and we can just get rid of
> it.
> ---
>  drivers/misc/cxl/of.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
> index edc4583..ec175ea 100644
> --- a/drivers/misc/cxl/of.c
> +++ b/drivers/misc/cxl/of.c
> @@ -460,7 +460,7 @@ int cxl_of_probe(struct platform_device *pdev)
>   struct device_node *afu_np = NULL;
>   struct cxl *adapter = NULL;
>   int ret;
> - int slice, slice_ok;
> + int slice = 0, slice_ok = 0;
>  
>   pr_devel("in %s\n", __func__);
>  
> @@ -476,13 +476,13 @@ int cxl_of_probe(struct platform_device *pdev)
>   }
>  
>   /* init afu */
> - slice_ok = 0;
> - for (afu_np = NULL, slice = 0; (afu_np = of_get_next_child(np, 
> afu_np)); slice++) {
> + for_each_child_of_node(np, afu_np) {
>   if ((ret = cxl_guest_init_afu(adapter, slice, afu_np)))
>   dev_err(>dev, "AFU %i failed to initialise: %i\n",
>   slice, ret);
>   else
>   slice_ok++;
> + slice++;
>   }

while you are here ..
you could move the assign out of the condition..

ret = cxl_guest_init_afu(adapter, slice, afu_np);
if (ret) 

just my 2 cents,

re,
 wh

>  
>   if (slice_ok == 0) {
> @@ -490,8 +490,6 @@ int cxl_of_probe(struct platform_device *pdev)
>   adapter->slices = 0;
>   }
>  
> - if (afu_np)
> - of_node_put(afu_np);
>   return 0;
>  }
>  

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Frederic Barrat]

Le 29/07/2016 à 05:55, Andrew Donnellan a écrit :

Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
for_each_child_of_node() rather than a hand-coded for loop.

Remove the useless of_node_put(afu_np) call after the loop, where it's
guaranteed that afu_np == NULL.

Reported-by: SF Markus Elfring 
Reported-by: Julia Lawall 
Signed-off-by: Andrew Donnellan 



Thanks!

Reviewed-by: Frederic Barrat 

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Frederic Barrat]

Le 29/07/2016 à 05:55, Andrew Donnellan a écrit :

Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
for_each_child_of_node() rather than a hand-coded for loop.

Remove the useless of_node_put(afu_np) call after the loop, where it's
guaranteed that afu_np == NULL.

Reported-by: SF Markus Elfring 
Reported-by: Julia Lawall 
Signed-off-by: Andrew Donnellan 



Thanks!

Reviewed-by: Frederic Barrat 

[PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Andrew Donnellan]

Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
for_each_child_of_node() rather than a hand-coded for loop.

Remove the useless of_node_put(afu_np) call after the loop, where it's
guaranteed that afu_np == NULL.

Reported-by: SF Markus Elfring 
Reported-by: Julia Lawall 
Signed-off-by: Andrew Donnellan 

---

Checked the of_node_put() with Fred, he thinks it was probably just left
over from an earlier private version of the code and we can just get rid of
it.
---
 drivers/misc/cxl/of.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
index edc4583..ec175ea 100644
--- a/drivers/misc/cxl/of.c
+++ b/drivers/misc/cxl/of.c
@@ -460,7 +460,7 @@ int cxl_of_probe(struct platform_device *pdev)
struct device_node *afu_np = NULL;
struct cxl *adapter = NULL;
int ret;
-   int slice, slice_ok;
+   int slice = 0, slice_ok = 0;
 
pr_devel("in %s\n", __func__);
 
@@ -476,13 +476,13 @@ int cxl_of_probe(struct platform_device *pdev)
}
 
/* init afu */
-   slice_ok = 0;
-   for (afu_np = NULL, slice = 0; (afu_np = of_get_next_child(np, 
afu_np)); slice++) {
+   for_each_child_of_node(np, afu_np) {
if ((ret = cxl_guest_init_afu(adapter, slice, afu_np)))
dev_err(>dev, "AFU %i failed to initialise: %i\n",
slice, ret);
else
slice_ok++;
+   slice++;
}
 
if (slice_ok == 0) {
@@ -490,8 +490,6 @@ int cxl_of_probe(struct platform_device *pdev)
adapter->slices = 0;
}
 
-   if (afu_np)
-   of_node_put(afu_np);
return 0;
 }
 
-- 
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

[PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[Andrew Donnellan]

Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
for_each_child_of_node() rather than a hand-coded for loop.

Remove the useless of_node_put(afu_np) call after the loop, where it's
guaranteed that afu_np == NULL.

Reported-by: SF Markus Elfring 
Reported-by: Julia Lawall 
Signed-off-by: Andrew Donnellan 

---

Checked the of_node_put() with Fred, he thinks it was probably just left
over from an earlier private version of the code and we can just get rid of
it.
---
 drivers/misc/cxl/of.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
index edc4583..ec175ea 100644
--- a/drivers/misc/cxl/of.c
+++ b/drivers/misc/cxl/of.c
@@ -460,7 +460,7 @@ int cxl_of_probe(struct platform_device *pdev)
struct device_node *afu_np = NULL;
struct cxl *adapter = NULL;
int ret;
-   int slice, slice_ok;
+   int slice = 0, slice_ok = 0;
 
pr_devel("in %s\n", __func__);
 
@@ -476,13 +476,13 @@ int cxl_of_probe(struct platform_device *pdev)
}
 
/* init afu */
-   slice_ok = 0;
-   for (afu_np = NULL, slice = 0; (afu_np = of_get_next_child(np, 
afu_np)); slice++) {
+   for_each_child_of_node(np, afu_np) {
if ((ret = cxl_guest_init_afu(adapter, slice, afu_np)))
dev_err(>dev, "AFU %i failed to initialise: %i\n",
slice, ret);
else
slice_ok++;
+   slice++;
}
 
if (slice_ok == 0) {
@@ -490,8 +490,6 @@ int cxl_of_probe(struct platform_device *pdev)
adapter->slices = 0;
}
 
-   if (afu_np)
-   of_node_put(afu_np);
return 0;
 }
 
-- 
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited