Re: [PATCH v2 01/15] ARM: actions: fix a leaked reference by adding missing of_node_put

[Russell King - ARM Linux admin]

On Tue, Mar 05, 2019 at 07:33:52PM +0800, Wen Yang wrote:
> The call to of_get_next_child returns a node pointer with refcount
> incremented thus it must be explicitly decremented after the last
> usage.
> 
> Detected by coccinelle with the following warnings:
> ./arch/arm/mach-actions/platsmp.c:112:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 103, but without a corresponding object release within this function.
> ./arch/arm/mach-actions/platsmp.c:124:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 115, but without a corresponding object release within this function.
> ./arch/arm/mach-actions/platsmp.c:137:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 128, but without a corresponding object release within this function.

I question this.  Your reasoning is that the node is no longer used
so the reference count needs to be put.

However, in all these cases, data is read from the nodes properties
and the device remains in-use for the life of the kernel.  There is
a big difference here.

With normal drivers, each device is bound to their associated device
node associated with the device.  When the device node goes away, then
the corresponding device goes away too, which causes the driver to be
unbound from the device.

However, there is another class of "driver" which are the ones below,
where they are "permanent" devices.  These can never go away, even if
the device node refcount hits zero and the device node is freed - the
device is still present and in-use in the system.  So, having the
device node refcount hit zero is actually a bug: what that's saying
is the system device (eg, SCU) has gone away.  If you somehow were to
remove the SCU from the system, you'd end up severing the connection
between the CPU cores and the rest of the system - obviously resulting
in a dead system!

So, what is the point of dropping these refcounts for devices that can
never go away - and thus their associated device_node should also never
go away?

> 
> Signed-off-by: Wen Yang <wen.yang99@xxxxxxxxxx>
> Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Cc: "Andreas Färber" <afaerber@xxxxxxx>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> v2->v1: add a missing space between "adding" and "missing"
> 
>  arch/arm/mach-actions/platsmp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/mach-actions/platsmp.c b/arch/arm/mach-actions/platsmp.c
> index 4fd479c..1a8e078 100644
> --- a/arch/arm/mach-actions/platsmp.c
> +++ b/arch/arm/mach-actions/platsmp.c
> @@ -107,6 +107,7 @@ static void __init s500_smp_prepare_cpus(unsigned int max_cpus)
>  	}
>  
>  	timer_base_addr = of_iomap(node, 0);
> +	of_node_put(node);
>  	if (!timer_base_addr) {
>  		pr_err("%s: could not map timer registers\n", __func__);
>  		return;
> @@ -119,6 +120,7 @@ static void __init s500_smp_prepare_cpus(unsigned int max_cpus)
>  	}
>  
>  	sps_base_addr = of_iomap(node, 0);
> +	of_node_put(node);
>  	if (!sps_base_addr) {
>  		pr_err("%s: could not map sps registers\n", __func__);
>  		return;
> @@ -132,6 +134,7 @@ static void __init s500_smp_prepare_cpus(unsigned int max_cpus)
>  		}
>  
>  		scu_base_addr = of_iomap(node, 0);
> +		of_node_put(node);
>  		if (!scu_base_addr) {
>  			pr_err("%s: could not map scu registers\n", __func__);
>  			return;
> -- 
> 2.9.5
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up