firmware: arm_sdei: fix wrong of_node_put() in init function
diff mbox series

Message ID 20181126121536.28739-1-nsaenzjulienne@suse.de
State In Next
Commit c3790b3799f8d75d93d26f6fd7bb569fc8c8b0cb
Headers show
Series
  • firmware: arm_sdei: fix wrong of_node_put() in init function
Related show

Commit Message

Nicolas Saenz Julienne Nov. 26, 2018, 12:15 p.m. UTC
After finding a "firmware" dt node arm_sdei tries to match it's
compatible string with it. To do so it's calling of_find_matching_node()
which already takes care of decreasing the refcount on the "firmware"
node. We are then incorrectly decreasing the refcount on that node
again.

This patch removes the unwarranted call to of_node_put().

Fixes: ad6eb31ef903 ("firmware: arm_sdei: Add driver for Software Delegated Exceptions")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/firmware/arm_sdei.c | 1 -
 1 file changed, 1 deletion(-)

Comments

James Morse Nov. 30, 2018, 6:31 p.m. UTC | #1
Hi Nicolas,

On 26/11/2018 12:15, Nicolas Saenz Julienne wrote:
> After finding a "firmware" dt node arm_sdei tries to match it's
> compatible string with it. To do so it's calling of_find_matching_node()
> which already takes care of decreasing the refcount on the "firmware"
> node. We are then incorrectly decreasing the refcount on that node
> again.
> 
> This patch removes the unwarranted call to of_node_put().
> 
> Fixes: ad6eb31ef903 ("firmware: arm_sdei: Add driver for Software Delegated Exceptions")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>


Thanks!, I agree this is unwarranted.
Is there a tool that picks these up? I remember sparse giving me a headache, but
I don't remember this one... I probably cargo-culted it from somewhere else.

Acked-by: James Morse <james.morse@arm.com>


Thanks,

James


> ---
>  drivers/firmware/arm_sdei.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 1ea71640fdc2..dffb47c6b480 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -1017,7 +1017,6 @@ static bool __init sdei_present_dt(void)
>  		return false;
>  
>  	np = of_find_matching_node(fw_np, sdei_of_match);
> -	of_node_put(fw_np);
>  	if (!np)
>  		return false;
>  
>
Nicolas Saenz Julienne Dec. 3, 2018, 12:25 p.m. UTC | #2
Hi James, thanks for the review! 

On Fri, 2018-11-30 at 18:31 +0000, James Morse wrote:
> Hi Nicolas,
> 
> On 26/11/2018 12:15, Nicolas Saenz Julienne wrote:
> > After finding a "firmware" dt node arm_sdei tries to match it's
> > compatible string with it. To do so it's calling
> > of_find_matching_node()
> > which already takes care of decreasing the refcount on the
> > "firmware"
> > node. We are then incorrectly decreasing the refcount on that node
> > again.
> > 
> > This patch removes the unwarranted call to of_node_put().
> > 
> > Fixes: ad6eb31ef903 ("firmware: arm_sdei: Add driver for Software
> > Delegated Exceptions")
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> 
> Thanks!, I agree this is unwarranted.
> Is there a tool that picks these up? I remember sparse giving me a
> headache, but
> I don't remember this one... I probably cargo-culted it from
> somewhere else.

We stumbled upon this one on a test system. TBH I don't really know
much about these tools so I can't tell. That said, I sent 4 more fixes
on this bug (one more in drivers/firmware) so there definitively was
some cargo-culting happening.

Regards,
Nicolas

> 
> Acked-by: James Morse <james.morse@arm.com>
> 
> 
> Thanks,
> 
> James
> 
> 
> > ---
> >  drivers/firmware/arm_sdei.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_sdei.c
> > b/drivers/firmware/arm_sdei.c
> > index 1ea71640fdc2..dffb47c6b480 100644
> > --- a/drivers/firmware/arm_sdei.c
> > +++ b/drivers/firmware/arm_sdei.c
> > @@ -1017,7 +1017,6 @@ static bool __init sdei_present_dt(void)
> >  		return false;
> >  
> >  	np = of_find_matching_node(fw_np, sdei_of_match);
> > -	of_node_put(fw_np);
> >  	if (!np)
> >  		return false;
> >  
> >
James Morse Dec. 21, 2018, 7:19 p.m. UTC | #3
Hi Nicolas,

On 03/12/2018 12:25, Nicolas Saenz Julienne wrote:
> On Fri, 2018-11-30 at 18:31 +0000, James Morse wrote:
>> On 26/11/2018 12:15, Nicolas Saenz Julienne wrote:
>>> After finding a "firmware" dt node arm_sdei tries to match it's
>>> compatible string with it. To do so it's calling
>>> of_find_matching_node()
>>> which already takes care of decreasing the refcount on the
>>> "firmware"
>>> node. We are then incorrectly decreasing the refcount on that node
>>> again.
>>>
>>> This patch removes the unwarranted call to of_node_put().
>>>
>>> Fixes: ad6eb31ef903 ("firmware: arm_sdei: Add driver for Software
>>> Delegated Exceptions")
>>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>
>> Thanks!, I agree this is unwarranted.
>> Is there a tool that picks these up? I remember sparse giving me a
>> headache, but
>> I don't remember this one... I probably cargo-culted it from
>> somewhere else.
> 
> We stumbled upon this one on a test system. TBH I don't really know
> much about these tools so I can't tell. That said, I sent 4 more fixes
> on this bug (one more in drivers/firmware) so there definitively was
> some cargo-culting happening.

Well this is embarrassing. I was trying to test this before re-posting it, to
discover dt-probing hasn't worked properly since it was merged, so I evidently
didn't test it properly after the merge-window. (at the same time the OF core
code took over creating platform devices from the firmware node, meaning my
attempt here fails, and the driver never gets registered).

I'll post the additional patch, and drop the fixes tag as this has never worked.


Thanks!

James

Patch
diff mbox series

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 1ea71640fdc2..dffb47c6b480 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -1017,7 +1017,6 @@  static bool __init sdei_present_dt(void)
 		return false;
 
 	np = of_find_matching_node(fw_np, sdei_of_match);
-	of_node_put(fw_np);
 	if (!np)
 		return false;