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

[Manivannan Sadhasivam]

Hi Russel,

On Tue, Mar 05, 2019 at 11:40:48AM +0000, Russell King - ARM Linux admin wrote:
> 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?
> 

Yes, practically we would never hit this case but theoretically we should
decrement the refcount for nodes/properties whenever we are done with it.
As you know, there are 'n' number of places in kernel where we can see the
refcount not being put after use. So I would welcome these kind of patches
to set an example for someone who tries to use the of_* calls in future.

IMO, DT should've handled the refcount internally without exposing the
pointers to external world.

Thanks,
Mani

> > 
> > 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