Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1319068 > unrolled thread
| Started by | Amitoj Kaur Chawla <amitoj1606@gmail.com> |
|---|---|
| First post | 2016-01-27 16:30 +0100 |
| Last post | 2016-01-27 20:50 +0100 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] of: resolver: Add missing of_node_put Amitoj Kaur Chawla <amitoj1606@gmail.com> - 2016-01-27 16:30 +0100
Re: [PATCH] of: resolver: Add missing of_node_put Mark Rutland <mark.rutland@arm.com> - 2016-01-27 17:10 +0100
Re: [PATCH] of: resolver: Add missing of_node_put Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-01-27 17:20 +0100
Re: [PATCH] of: resolver: Add missing of_node_put Mark Rutland <mark.rutland@arm.com> - 2016-01-27 17:30 +0100
Re: [PATCH] of: resolver: Add missing of_node_put Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-01-27 19:10 +0100
Re: [PATCH] of: resolver: Add missing of_node_put Julia Lawall <julia.lawall@lip6.fr> - 2016-01-27 20:50 +0100
| From | Amitoj Kaur Chawla <amitoj1606@gmail.com> |
|---|---|
| Date | 2016-01-27 16:30 +0100 |
| Subject | [PATCH] of: resolver: Add missing of_node_put |
| Message-ID | <qVAcG-5tw-21@gated-at.bofh.it> |
for_each_child_of_node performs an of_node_get on each iteration, so
to break out of the loop an of_node_put is required.
Found using Coccinelle. The semantic patch used for this is as follows:
// <smpl>
@@
expression e;
local idexpression n;
@@
for_each_child_of_node(..., n) {
... when != of_node_put(n)
when != e = n
(
return n;
|
+ of_node_put(n);
? return ...;
)
...
}
// </smpl
Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
drivers/of/resolver.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 640eb4c..e2a0143 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
for_each_child_of_node(node, child) {
found = __of_find_node_by_full_name(child, full_name);
- if (found != NULL)
+ if (found != NULL) {
+ of_node_put(child);
return found;
+ }
}
return NULL;
--
1.9.1
[toc] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-01-27 17:10 +0100 |
| Message-ID | <qVAPo-65V-19@gated-at.bofh.it> |
| In reply to | #1319068 |
On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> for_each_child_of_node performs an of_node_get on each iteration, so
> to break out of the loop an of_node_put is required.
>
> Found using Coccinelle. The semantic patch used for this is as follows:
>
> // <smpl>
> @@
> expression e;
> local idexpression n;
> @@
>
> for_each_child_of_node(..., n) {
> ... when != of_node_put(n)
> when != e = n
> (
> return n;
> |
> + of_node_put(n);
> ? return ...;
> )
> ...
> }
> // </smpl
>
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
> drivers/of/resolver.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 640eb4c..e2a0143 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>
> for_each_child_of_node(node, child) {
> found = __of_find_node_by_full_name(child, full_name);
> - if (found != NULL)
> + if (found != NULL) {
> + of_node_put(child);
> return found;
> + }
> }
>
> return NULL;
I don't think this is quite right. When child == found, this change will
leave it decremented.
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Pantelis Antoniou <pantelis.antoniou@konsulko.com> |
|---|---|
| Date | 2016-01-27 17:20 +0100 |
| Message-ID | <qVAZ4-6ax-15@gated-at.bofh.it> |
| In reply to | #1319108 |
Hi Mark,
> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
>> for_each_child_of_node performs an of_node_get on each iteration, so
>> to break out of the loop an of_node_put is required.
>>
>> Found using Coccinelle. The semantic patch used for this is as follows:
>>
>> // <smpl>
>> @@
>> expression e;
>> local idexpression n;
>> @@
>>
>> for_each_child_of_node(..., n) {
>> ... when != of_node_put(n)
>> when != e = n
>> (
>> return n;
>> |
>> + of_node_put(n);
>> ? return ...;
>> )
>> ...
>> }
>> // </smpl
>>
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> ---
>> drivers/of/resolver.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 640eb4c..e2a0143 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>>
>> for_each_child_of_node(node, child) {
>> found = __of_find_node_by_full_name(child, full_name);
>> - if (found != NULL)
>> + if (found != NULL) {
>> + of_node_put(child);
>> return found;
>> + }
>> }
>>
>> return NULL;
>
> I don't think this is quite right. When child == found, this change will
> leave it decremented.
>
This patch is bogus.
__of_find_node_by_full_name() is not taking a reference on the node if found.
This method relies on keeping the reference taken by the loop.
Taking this into account all of these conccinelle tests are bogus.
The DT internal method are not using the object model in an obvious manner
and applying these patches without vetting each and everyone is bound to
break things.
Regards
— Pantelis
> Thanks,
> Mark.
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-01-27 17:30 +0100 |
| Message-ID | <qVB8L-6ft-23@gated-at.bofh.it> |
| In reply to | #1319119 |
On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> Hi Mark,
>
> > On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> >> for_each_child_of_node performs an of_node_get on each iteration, so
> >> to break out of the loop an of_node_put is required.
> >>
> >> Found using Coccinelle. The semantic patch used for this is as follows:
> >>
> >> // <smpl>
> >> @@
> >> expression e;
> >> local idexpression n;
> >> @@
> >>
> >> for_each_child_of_node(..., n) {
> >> ... when != of_node_put(n)
> >> when != e = n
> >> (
> >> return n;
> >> |
> >> + of_node_put(n);
> >> ? return ...;
> >> )
> >> ...
> >> }
> >> // </smpl
> >>
> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> ---
> >> drivers/of/resolver.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> >> index 640eb4c..e2a0143 100644
> >> --- a/drivers/of/resolver.c
> >> +++ b/drivers/of/resolver.c
> >> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >>
> >> for_each_child_of_node(node, child) {
> >> found = __of_find_node_by_full_name(child, full_name);
> >> - if (found != NULL)
> >> + if (found != NULL) {
> >> + of_node_put(child);
> >> return found;
> >> + }
> >> }
> >>
> >> return NULL;
> >
> > I don't think this is quite right. When child == found, this change will
> > leave it decremented.
> >
>
>
> This patch is bogus.
>
> __of_find_node_by_full_name() is not taking a reference on the node if found.
> This method relies on keeping the reference taken by the loop.
Sure. For the found node, that makes sense.
However, it also increments the refcount of all the parents, which does
not seem correct to me, given they're not put on the return path, and a
put of the found node won't decrement its parents refcounts, unless I
have missed something.
So I believe we are missing some of_node_put logic here.
> Taking this into account all of these conccinelle tests are bogus.
>
> The DT internal method are not using the object model in an obvious manner
> and applying these patches without vetting each and everyone is bound to
> break things.
Agreed.
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Pantelis Antoniou <pantelis.antoniou@konsulko.com> |
|---|---|
| Date | 2016-01-27 19:10 +0100 |
| Message-ID | <qVCHv-7xR-1@gated-at.bofh.it> |
| In reply to | #1319139 |
Hi Mark,
> On Jan 27, 2016, at 18:21 , Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
>> Hi Mark,
>>
>>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
>>>
>>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
>>>> for_each_child_of_node performs an of_node_get on each iteration, so
>>>> to break out of the loop an of_node_put is required.
>>>>
>>>> Found using Coccinelle. The semantic patch used for this is as follows:
>>>>
>>>> // <smpl>
>>>> @@
>>>> expression e;
>>>> local idexpression n;
>>>> @@
>>>>
>>>> for_each_child_of_node(..., n) {
>>>> ... when != of_node_put(n)
>>>> when != e = n
>>>> (
>>>> return n;
>>>> |
>>>> + of_node_put(n);
>>>> ? return ...;
>>>> )
>>>> ...
>>>> }
>>>> // </smpl
>>>>
>>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>>>> ---
>>>> drivers/of/resolver.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>> index 640eb4c..e2a0143 100644
>>>> --- a/drivers/of/resolver.c
>>>> +++ b/drivers/of/resolver.c
>>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>>>>
>>>> for_each_child_of_node(node, child) {
>>>> found = __of_find_node_by_full_name(child, full_name);
>>>> - if (found != NULL)
>>>> + if (found != NULL) {
>>>> + of_node_put(child);
>>>> return found;
>>>> + }
>>>> }
>>>>
>>>> return NULL;
>>>
>>> I don't think this is quite right. When child == found, this change will
>>> leave it decremented.
>>>
>>
>>
>> This patch is bogus.
>>
>> __of_find_node_by_full_name() is not taking a reference on the node if found.
>> This method relies on keeping the reference taken by the loop.
>
> Sure. For the found node, that makes sense.
>
> However, it also increments the refcount of all the parents, which does
> not seem correct to me, given they're not put on the return path, and a
> put of the found node won't decrement its parents refcounts, unless I
> have missed something.
>
Hmm, yes. The parent refcounts must be decremented.
> So I believe we are missing some of_node_put logic here.
>
>> Taking this into account all of these conccinelle tests are bogus.
>>
>> The DT internal method are not using the object model in an obvious manner
>> and applying these patches without vetting each and everyone is bound to
>> break things.
>
> Agreed.
>
> Thanks,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2016-01-27 20:50 +0100 |
| Message-ID | <qVEgl-6M-79@gated-at.bofh.it> |
| In reply to | #1319198 |
On Wed, 27 Jan 2016, Pantelis Antoniou wrote:
> Hi Mark,
>
> > On Jan 27, 2016, at 18:21 , Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote:
> >> Hi Mark,
> >>
> >>> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@arm.com> wrote:
> >>>
> >>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
> >>>> for_each_child_of_node performs an of_node_get on each iteration, so
> >>>> to break out of the loop an of_node_put is required.
> >>>>
> >>>> Found using Coccinelle. The semantic patch used for this is as follows:
> >>>>
> >>>> // <smpl>
> >>>> @@
> >>>> expression e;
> >>>> local idexpression n;
> >>>> @@
> >>>>
> >>>> for_each_child_of_node(..., n) {
> >>>> ... when != of_node_put(n)
> >>>> when != e = n
> >>>> (
> >>>> return n;
> >>>> |
> >>>> + of_node_put(n);
> >>>> ? return ...;
> >>>> )
> >>>> ...
> >>>> }
> >>>> // </smpl
> >>>>
> >>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >>>> ---
> >>>> drivers/of/resolver.c | 4 +++-
> >>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> >>>> index 640eb4c..e2a0143 100644
> >>>> --- a/drivers/of/resolver.c
> >>>> +++ b/drivers/of/resolver.c
> >>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >>>>
> >>>> for_each_child_of_node(node, child) {
> >>>> found = __of_find_node_by_full_name(child, full_name);
> >>>> - if (found != NULL)
> >>>> + if (found != NULL) {
> >>>> + of_node_put(child);
> >>>> return found;
> >>>> + }
> >>>> }
> >>>>
> >>>> return NULL;
> >>>
> >>> I don't think this is quite right. When child == found, this change will
> >>> leave it decremented.
> >>>
> >>
> >>
> >> This patch is bogus.
> >>
> >> __of_find_node_by_full_name() is not taking a reference on the node if found.
> >> This method relies on keeping the reference taken by the loop.
> >
> > Sure. For the found node, that makes sense.
> >
> > However, it also increments the refcount of all the parents, which does
> > not seem correct to me, given they're not put on the return path, and a
> > put of the found node won't decrement its parents refcounts, unless I
> > have missed something.
> >
>
> Hmm, yes. The parent refcounts must be decremented.
So there should be if (found != child) of_node_put(child); ? Another
option would be to duplicate the test and avoid the recursive call.
julia
> > So I believe we are missing some of_node_put logic here.
> >
> >> Taking this into account all of these conccinelle tests are bogus.
> >>
> >> The DT internal method are not using the object model in an obvious manner
> >> and applying these patches without vetting each and everyone is bound to
> >> break things.
> >
> > Agreed.
> >
> > Thanks,
> > Mark.
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web