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.yan...@zte.com.cn> > > Reviewed-by: Florian Fainelli <f.faine...@gmail.com> > > Cc: "Andreas Färber" <afaer...@suse.de> > > Cc: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org> > > Cc: Russell King <li...@armlinux.org.uk> > > Cc: linux-arm-ker...@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > --- > > 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