| Message ID | 37728512.316309.1502619006183@email.1und1.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, > Stefan Wahren <stefan.wahren@i2se.com> hat am 13. August 2017 um 12:10 geschrieben: > > > Hi Leo, > > > Leo Yan <leo.yan@linaro.org> hat am 13. August 2017 um 05:50 geschrieben: > > > > > > Hi Stefan, > > > > On Sat, Aug 12, 2017 at 03:19:33PM +0200, Stefan Wahren wrote: > > > Hi, > > > > > > if i additionally enable kmemleak (on top of multi_v7_defconfig) on RPi 3 (4 cores) with 4.13-rc4, i get the following output from kmemleak: > > > > > > unreferenced object 0xede0dc00 (size 1024): > > > comm "swapper/0", pid 1, jiffies 4294937431 (age 744.510s) > > > hex dump (first 32 bytes): > > > 94 9e 0b c1 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > 57 46 49 00 00 00 00 00 00 00 00 00 00 00 00 00 WFI............. > > > backtrace: > > > [<c1295f04>] arm_idle_init+0x44/0x1ac > > > [<c0301e6c>] do_one_initcall+0x3c/0x16c > > > [<c1200d70>] kernel_init_freeable+0x110/0x1d0 > > > [<c0cb3624>] kernel_init+0x8/0x114 > > > [<c0307a98>] ret_from_fork+0x14/0x3c > > > [<ffffffff>] 0xffffffff > > > > > > If i revert the last commit in cpuidle-arm d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition") then kmemleak stays calm. > > > > I cannot reproduce the error at my side, I also tried to disable idle > > states but cannot trigger the failure. So first thing is to know the > > reason for registeration failure and finally introduce the memory > > leaking. Could you point out the dts you are using for idle states > > binding? > > thanks for you quick response. Sorry, i forgot to mention that arch/arm64/boot/dts/broadcom/bcm2837.dtsi doesn't contain any idle states. So the error path of dt_init_idle_driver() is expected. If i get it right, the clean up loop after out_fail only handles registered drivers (better label name or a comment would be helpful). So combined with your fix the following fixes the memleak for me (didn't test the clean up loop): > > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c > index 7080c38..827ab25 100644 > --- a/drivers/cpuidle/cpuidle-arm.c > +++ b/drivers/cpuidle/cpuidle-arm.c > @@ -104,12 +104,14 @@ static int __init arm_idle_init(void) > ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); > if (ret <= 0) { > ret = ret ? : -ENODEV; > + kfree(drv); > goto out_fail; > } > > ret = cpuidle_register_driver(drv); > if (ret) { > pr_err("Failed to register cpuidle driver\n"); > + kfree(drv); > goto out_fail; > } > > @@ -152,11 +154,13 @@ static int __init arm_idle_init(void) > out_fail: > while (--cpu >= 0) { > dev = per_cpu(cpuidle_devices, cpu); > - cpuidle_unregister_device(dev); > - kfree(dev); > - drv = cpuidle_get_driver(); > + drv = cpuidle_get_cpu_driver(dev); > + > cpuidle_unregister_driver(drv); > kfree(drv); > + > + cpuidle_unregister_device(dev); > + kfree(dev); > } > > return ret; should i split this patch (fix for "my" memleak and cleanup fix)?
On 26/08/2017 13:46, Stefan Wahren wrote: [ ... ] >> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c >> index 7080c38..827ab25 100644 >> --- a/drivers/cpuidle/cpuidle-arm.c >> +++ b/drivers/cpuidle/cpuidle-arm.c >> @@ -104,12 +104,14 @@ static int __init arm_idle_init(void) >> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); >> if (ret <= 0) { >> ret = ret ? : -ENODEV; >> + kfree(drv); >> goto out_fail; >> } >> >> ret = cpuidle_register_driver(drv); >> if (ret) { >> pr_err("Failed to register cpuidle driver\n"); >> + kfree(drv); >> goto out_fail; >> } I would prefer if you create another label on top of out_fail to free 'drv' and 'dev'. So you can remove the kfree in this loop (also take the opportunity to remove the pr_err("Failed to allocate cpuidle device") message. > should i split this patch (fix for "my" memleak and cleanup fix)? Yes, please. Thanks. -- Daniel
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c index 7080c38..827ab25 100644 --- a/drivers/cpuidle/cpuidle-arm.c +++ b/drivers/cpuidle/cpuidle-arm.c @@ -104,12 +104,14 @@ static int __init arm_idle_init(void) ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); if (ret <= 0) { ret = ret ? : -ENODEV; + kfree(drv); goto out_fail; } ret = cpuidle_register_driver(drv); if (ret) { pr_err("Failed to register cpuidle driver\n"); + kfree(drv); goto out_fail; } @@ -152,11 +154,13 @@ static int __init arm_idle_init(void) out_fail: while (--cpu >= 0) { dev = per_cpu(cpuidle_devices, cpu); - cpuidle_unregister_device(dev); - kfree(dev); - drv = cpuidle_get_driver(); + drv = cpuidle_get_cpu_driver(dev); + cpuidle_unregister_driver(drv); kfree(drv); + + cpuidle_unregister_device(dev); + kfree(dev); } return ret;