diff mbox series

PCI/hotplug: Remove unneeded of_node_put() in pnv_php

Message ID 20211020094604.2106-1-wanjiabing@vivo.com
State New
Headers show
Series PCI/hotplug: Remove unneeded of_node_put() in pnv_php | expand

Commit Message

Jiabing Wan Oct. 20, 2021, 9:46 a.m. UTC 
Fix following coccicheck warning:
./drivers/pci/hotplug/pnv_php.c:161:2-13: ERROR: probable double put.

Device node iterators put the previous value of the index variable, so
an explicit put causes a double put.

Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
 drivers/pci/hotplug/pnv_php.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Nathan Lynch Oct. 20, 2021, 11:39 a.m. UTC | #1 
Wan Jiabing <wanjiabing@vivo.com> writes:
> Fix following coccicheck warning:
> ./drivers/pci/hotplug/pnv_php.c:161:2-13: ERROR: probable double put.
>
> Device node iterators put the previous value of the index variable, so
> an explicit put causes a double put.

I suppose Coccinelle doesn't take into account that this code is
detaching and freeing the nodes.


> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index f4c2e6e01be0..f3da4f95d73f 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -158,7 +158,6 @@ static void pnv_php_detach_device_nodes(struct device_node *parent)
>  	for_each_child_of_node(parent, dn) {
>  		pnv_php_detach_device_nodes(dn);
>  
> -		of_node_put(dn);
>  		of_detach_node(dn);
>  	}

The code might be improved by comments explaining how the bare
of_node_put() corresponds to a "get" somewhere else in the driver, and
how it doesn't render the ongoing traversal unsafe. It looks suspicious
on first review, but I believe it's intentional and probably correct as
written.
Tyrel Datwyler Oct. 20, 2021, 4:53 p.m. UTC | #2 
On 10/20/21 2:46 AM, Wan Jiabing wrote:
> Fix following coccicheck warning:
> ./drivers/pci/hotplug/pnv_php.c:161:2-13: ERROR: probable double put.
> 
> Device node iterators put the previous value of the index variable, so
> an explicit put causes a double put.
> 
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>

NACK

This is a false positive from coccicheck. This is a case were a node is being
dynamically removed and the long reference needs to be dropped. Otherwise, the
reference count doesn't go to zero and trigger cleanup. This would result in us
ending up in a leaked device node.

-Tyrel

> ---
>  drivers/pci/hotplug/pnv_php.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index f4c2e6e01be0..f3da4f95d73f 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -158,7 +158,6 @@ static void pnv_php_detach_device_nodes(struct device_node *parent)
>  	for_each_child_of_node(parent, dn) {
>  		pnv_php_detach_device_nodes(dn);
> 
> -		of_node_put(dn);
>  		of_detach_node(dn);
>  	}
>  }
>
Tyrel Datwyler Oct. 20, 2021, 4:57 p.m. UTC | #3 
On 10/20/21 4:39 AM, Nathan Lynch wrote:
> Wan Jiabing <wanjiabing@vivo.com> writes:
>> Fix following coccicheck warning:
>> ./drivers/pci/hotplug/pnv_php.c:161:2-13: ERROR: probable double put.
>>
>> Device node iterators put the previous value of the index variable, so
>> an explicit put causes a double put.
> 
> I suppose Coccinelle doesn't take into account that this code is
> detaching and freeing the nodes.
> 
> 
>> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
>> index f4c2e6e01be0..f3da4f95d73f 100644
>> --- a/drivers/pci/hotplug/pnv_php.c
>> +++ b/drivers/pci/hotplug/pnv_php.c
>> @@ -158,7 +158,6 @@ static void pnv_php_detach_device_nodes(struct device_node *parent)
>>  	for_each_child_of_node(parent, dn) {
>>  		pnv_php_detach_device_nodes(dn);
>>  
>> -		of_node_put(dn);
>>  		of_detach_node(dn);
>>  	}
> 
> The code might be improved by comments explaining how the bare
> of_node_put() corresponds to a "get" somewhere else in the driver, and
> how it doesn't render the ongoing traversal unsafe. It looks suspicious
> on first review, but I believe it's intentional and probably correct as
> written.
> 

This is a common usage pattern which if we put a comment about the pattern here
we need to do it every where. I suppose a better solution is to wrap this put in
a more descriptive function name like of_node_long_put() or something of the
sort the makes it obvious we are dropping a long held global scope reference.

-Tyrel
diff mbox series

Patch

diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index f4c2e6e01be0..f3da4f95d73f 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -158,7 +158,6 @@  static void pnv_php_detach_device_nodes(struct device_node *parent)
 	for_each_child_of_node(parent, dn) {
 		pnv_php_detach_device_nodes(dn);
 
-		of_node_put(dn);
 		of_detach_node(dn);
 	}
 }