| Message ID | 1551785646-46173-1-git-send-email-wen.yang99@zte.com.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/15] ARM: actions: fix a leaked reference by adding missing of_node_put | expand |
On Tue, Mar 05, 2019 at 07:34:03PM +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-vexpress/dcscb.c:150:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 145, but without a corresponding object release within this function. > ./arch/arm/mach-vexpress/dcscb.c:160:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 145, but without a corresponding object release within this function. > ./arch/arm/mach-vexpress/dcscb.c:171:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 145, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> I already Acked assuming you have plans to take this as series. There's no cover letter mentioning your plans to merge this or note to individual platform maintainers. Please let us know. -- Regards, Sudeep
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@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: "Andreas Färber" <afaerber@suse.de> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-arm-kernel@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 > >
On Tue, Mar 05, 2019 at 07:34:01PM +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-sunxi/platsmp.c:55:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 46, but without a corresponding object release within this function. > ./arch/arm/mach-sunxi/platsmp.c:138:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 129, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: Chen-Yu Tsai <wens@csie.org> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org Applied, thanks! Maxime
On 3/5/2019 3:33 AM, 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-bcm/board_bcm281xx.c:43:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 35, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Ray Jui <rjui@broadcom.com> > Cc: Scott Branden <sbranden@broadcom.com> > Cc: bcm-kernel-feedback-list@broadcom.com > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > v2->v1: add a missing space between "adding" and "missing" > > arch/arm/mach-bcm/board_bcm281xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/mach-bcm/board_bcm281xx.c b/arch/arm/mach-bcm/board_bcm281xx.c > index b81bb38..1238ac8 100644 > --- a/arch/arm/mach-bcm/board_bcm281xx.c > +++ b/arch/arm/mach-bcm/board_bcm281xx.c > @@ -38,6 +38,7 @@ static void bcm281xx_restart(enum reboot_mode mode, const char *cmd) > return; > } > base = of_iomap(np_wdog, 0); > + of_node_put(np_wdog); > if (!base) { > pr_emerg("Couldn't map brcm,kona-wdt\n"); > return; > Change looks good to me. Thanks! Acked-by: Ray Jui <ray.jui@broadcom.com>
On Tue, Mar 5, 2019 at 1:33 PM Wen Yang <wen.yang99@zte.com.cn> 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-npcm/platsmp.c:52:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 31, but without a corresponding object release within this function. > ./arch/arm/mach-npcm/platsmp.c:68:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 60, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Avi Fishman <avifishman70@gmail.com> > Cc: Tomer Maimon <tmaimon77@gmail.com> > Cc: Patrick Venture <venture@google.com> > Cc: Nancy Yuen <yuenn@google.com> > Cc: Brendan Higgins <brendanhiggins@google.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Cc: openbmc@lists.ozlabs.org > Cc: linux-kernel@vger.kernel.org > --- > v2->v1: add a missing space between "adding" and "missing" > > arch/arm/mach-npcm/platsmp.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm/mach-npcm/platsmp.c b/arch/arm/mach-npcm/platsmp.c > index 21633c7..fe63edc 100644 > --- a/arch/arm/mach-npcm/platsmp.c > +++ b/arch/arm/mach-npcm/platsmp.c > @@ -35,6 +35,7 @@ static int npcm7xx_smp_boot_secondary(unsigned int cpu, > goto out; > } > gcr_base = of_iomap(gcr_np, 0); > + of_node_put(gcr_np); > if (!gcr_base) { > pr_err("could not iomap gcr"); > ret = -ENOMEM; > @@ -63,6 +64,7 @@ static void __init npcm7xx_smp_prepare_cpus(unsigned int max_cpus) > return; > } > scu_base = of_iomap(scu_np, 0); > + of_node_put(scu_np); > if (!scu_base) { > pr_err("could not iomap scu"); > return; > -- > 2.9.5 > Reviewed-by: Avi Fishman <avifishman70@gmail.com>
On Tue, Mar 05, 2019 at 07:33:59PM +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-shmobile/pm-rcar-gen2.c:77:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 66, but without a corresponding object release within this function. > ./arch/arm/mach-shmobile/pm-rcar-gen2.c:85:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 66, but without a corresponding object release within this function. > ./arch/arm/mach-shmobile/pm-rcar-gen2.c:90:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 66, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Simon Horman <horms@verge.net.au> > Cc: Magnus Damm <magnus.damm@gmail.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-renesas-soc@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > v2->v1: add a missing space between "adding" and "missing" Thanks, I have this applied for inclusion in v5.2. > arch/arm/mach-shmobile/pm-rcar-gen2.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/mach-shmobile/pm-rcar-gen2.c b/arch/arm/mach-shmobile/pm-rcar-gen2.c > index 8c2a205..e84599d 100644 > --- a/arch/arm/mach-shmobile/pm-rcar-gen2.c > +++ b/arch/arm/mach-shmobile/pm-rcar-gen2.c > @@ -72,6 +72,7 @@ void __init rcar_gen2_pm_init(void) > } > > error = of_address_to_resource(np, 0, &res); > + of_node_put(np); > if (error) { > pr_err("Failed to get smp-sram address: %d\n", error); > return; > -- > 2.9.5 >
On 3/5/19 9:24 AM, Ray Jui wrote: > > > On 3/5/2019 3:33 AM, 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-bcm/board_bcm281xx.c:43:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 35, but without a corresponding object release within this function. >> >> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Ray Jui <rjui@broadcom.com> >> Cc: Scott Branden <sbranden@broadcom.com> >> Cc: bcm-kernel-feedback-list@broadcom.com >> Cc: Russell King <linux@armlinux.org.uk> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> --- >> v2->v1: add a missing space between "adding" and "missing" >> >> arch/arm/mach-bcm/board_bcm281xx.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/mach-bcm/board_bcm281xx.c b/arch/arm/mach-bcm/board_bcm281xx.c >> index b81bb38..1238ac8 100644 >> --- a/arch/arm/mach-bcm/board_bcm281xx.c >> +++ b/arch/arm/mach-bcm/board_bcm281xx.c >> @@ -38,6 +38,7 @@ static void bcm281xx_restart(enum reboot_mode mode, const char *cmd) >> return; >> } >> base = of_iomap(np_wdog, 0); >> + of_node_put(np_wdog); >> if (!base) { >> pr_emerg("Couldn't map brcm,kona-wdt\n"); >> return; >> > > Change looks good to me. Thanks! > > Acked-by: Ray Jui <ray.jui@broadcom.com> > Squashed this patch and "[PATCH 1/4] ARM: brcmstb: fix a leaked reference by adding missing of_node_put" into the same commit: https://github.com/Broadcom/stblinux/commit/ff98f8f6083a7f317463f538e9a21822e1128657 thanks Wen!
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@zte.com.cn> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Cc: "Andreas Färber" <afaerber@suse.de> > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: linux-arm-kernel@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
On Sat, Mar 09, 2019 at 07:47:42AM +0530, Manivannan Sadhasivam wrote: > 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. It doesn't, that's my point. In the case of normal drivers, there's an _extra_ refcount held by the device that is created - see the of_node_get() in of_device_alloc(). This refcount exists for the lifetime of the device structure. That refcount exists for the duration that the device exists, which bounds the lifetime of the availability of the device to the driver. In effect, while the device driver is bound, there is a refcount on the device node. So, the device node is guaranteed to be around for as long as the device driver is bound to the device. For the cases being addressed in these patches, there is no driver, so there is no bounding of the lifetime: the expectation is that the lifetime is the duration of the kernel. If such a device node were to be deleted, then there is no way to unbind the driver, and if we have dropped the refcount, the device node will be immediately freed. However, the device is still in use. These are a different "class" of driver.
On Tue, Mar 05, 2019 at 07:33:54PM +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-exynos/firmware.c:201:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 193, but without a corresponding object release within this function. > ./arch/arm/mach-exynos/firmware.c:204:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 193, but without a corresponding object release within this function. > ./arch/arm/mach-exynos/suspend.c:642:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 634, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Kukjin Kim <kgene@kernel.org> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-samsung-soc@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- Thanks, applied, with adjustments in commit msg and removal of Florian's reviewed-by tag (I cannot find his email with review). Best regards, Krzysztof
On Tue, Mar 05, 2019 at 07:34:05PM +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/arm64/kernel/cpu_ops.c:102:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 69, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > v2->v1: add a missing space between "adding" and "missing" > > arch/arm64/kernel/cpu_ops.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > index ea00124..00f8b86 100644 > --- a/arch/arm64/kernel/cpu_ops.c > +++ b/arch/arm64/kernel/cpu_ops.c > @@ -85,6 +85,7 @@ static const char *__init cpu_read_enable_method(int cpu) > pr_err("%pOF: missing enable-method property\n", > dn); > } > + of_node_put(dn); > } else { > enable_method = acpi_get_enable_method(cpu); > if (!enable_method) { Looks about right to me: Acked-by: Will Deacon <will.deacon@arm.com> How do you plan to get this upstream? Can we just pick this one up via arm64? Will
On Wed, Apr 03, 2019 at 01:56:09PM +0800, wen.yang99@zte.com.cn 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/arm64/kernel/cpu_ops.c:102:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 69, but without a corresponding object release within this function. > > > > > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > Cc: Will Deacon <will.deacon@arm.com> > > > Cc: linux-arm-kernel@lists.infradead.org > > > Cc: linux-kernel@vger.kernel.org > > > --- > > > v2->v1: add a missing space between "adding" and "missing" > > > > > > arch/arm64/kernel/cpu_ops.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > > > index ea00124..00f8b86 100644 > > > --- a/arch/arm64/kernel/cpu_ops.c > > > +++ b/arch/arm64/kernel/cpu_ops.c > > > @@ -85,6 +85,7 @@ static const char *__init cpu_read_enable_method(int cpu) > > > pr_err("%pOF: missing enable-method property\n", > > > dn); > > > } > > > + of_node_put(dn); > > > } else { > > > enable_method = acpi_get_enable_method(cpu); > > > if (!enable_method) { > > > > Looks about right to me: > > > > Acked-by: Will Deacon <will.deacon@arm.com> > > > > How do you plan to get this upstream? Can we just pick this one up via > > arm64? > > Ok, thank you. > We are very happy to know that it is useful. Ok, I picked it up. Will
Hi, sorry that this took so long to look at, but I think it needs a bit of rework, see below: Am Dienstag, 5. März 2019, 12:33:58 CEST schrieb Wen Yang: > 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-rockchip/pm.c:269:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 259, but without a corresponding object release within this function. > ./arch/arm/mach-rockchip/pm.c:275:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 259, but without a corresponding object release within this function > ./arch/arm/mach-rockchip/platsmp.c:280:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 271, but without a corresponding object release within this function. > ./arch/arm/mach-rockchip/platsmp.c:284:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 271, but without a corresponding object release within this function. > ./arch/arm/mach-rockchip/platsmp.c:288:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 271, but without a corresponding object release within this function. > ./arch/arm/mach-rockchip/platsmp.c:302:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 293, but without a corresponding object release within this function. > ./arch/arm/mach-rockchip/platsmp.c:250:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 241, but without a corresponding object release within this function. > ./arch/arm/mach-rockchip/platsmp.c:260:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 241, but without a corresponding object release within this function. > ./arch/arm/mach-rockchip/platsmp.c:263:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 241, but without a corresponding object release within this function. > > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-rockchip@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > v2->v1: add a missing space between "adding" and "missing" > > arch/arm/mach-rockchip/platsmp.c | 12 ++++++++---- > arch/arm/mach-rockchip/pm.c | 11 ++++++----- > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c > index 51984a4..f93d64e 100644 > --- a/arch/arm/mach-rockchip/platsmp.c > +++ b/arch/arm/mach-rockchip/platsmp.c > @@ -277,19 +277,20 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus) > sram_base_addr = of_iomap(node, 0); just do the of_node_put here and drop the whole error gotos? Because node in this case only holds the possible pointer to > if (!sram_base_addr) { > pr_err("%s: could not map sram registers\n", __func__); > - return; > + goto out_put_node; > } > > if (has_pmu && rockchip_smp_prepare_pmu()) > - return; > + goto out_put_node; > > if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { > if (rockchip_smp_prepare_sram(node)) > - return; > + goto out_put_node; > > /* enable the SCU power domain */ > pmu_set_power_domain(PMU_PWRDN_SCU, true); > > + of_node_put(node); > node = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); > if (!node) { > pr_err("%s: missing scu\n", __func__); > @@ -299,7 +300,7 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus) > scu_base_addr = of_iomap(node, 0); similarly just put the scu node here? > if (!scu_base_addr) { > pr_err("%s: could not map scu registers\n", __func__); > - return; > + goto out_put_node; > } > > /* > @@ -321,6 +322,9 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus) > /* Make sure that all cores except the first are really off */ > for (i = 1; i < ncores; i++) > pmu_set_power_domain(0 + i, false); > + > +out_put_node: > + of_node_put(node); > } > > static void __init rk3036_smp_prepare_cpus(unsigned int max_cpus) > diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c > index 0592534..43a16c9 100644 > --- a/arch/arm/mach-rockchip/pm.c > +++ b/arch/arm/mach-rockchip/pm.c > @@ -266,25 +266,26 @@ static int rk3288_suspend_init(struct device_node *np) > rk3288_bootram_base = of_iomap(sram_np, 0); > if (!rk3288_bootram_base) { > pr_err("%s: could not map bootram base\n", __func__); just add a regular of_node_put here? > - return -ENOMEM; > + ret = -ENOMEM; > + goto out_put_node; > } > > ret = of_address_to_resource(sram_np, 0, &res); > if (ret) { > pr_err("%s: could not get bootram phy addr\n", __func__); and here as well? Not having to follow gotos might improve readability especially as after here the node isn't used anymore as indicated by the already existing of_node_put below which should be kept. Heiko > - return ret; > + goto out_put_node; > } > rk3288_bootram_phy = res.start; > > - of_node_put(sram_np); > - > rk3288_config_bootdata(); > > /* copy resume code and data to bootsram */ > memcpy(rk3288_bootram_base, rockchip_slp_cpu_resume, > rk3288_bootram_sz); > > - return 0; > +out_put_node: > + of_node_put(sram_np); > + return ret; > } > > static const struct platform_suspend_ops rk3288_suspend_ops = { >
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;