These can fit in a single line (80 columns), don't split lines unnecessarily.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
--- V1->V2: New patch --- drivers/mfd/wm8994-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 7eec619a6023..1990b2c90732 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -401,8 +401,7 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) goto err; }
- ret = regulator_bulk_enable(wm8994->num_supplies, - wm8994->supplies); + ret = regulator_bulk_enable(wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to enable supplies: %d\n", ret); goto err; @@ -606,8 +605,7 @@ static void wm8994_device_exit(struct wm8994 *wm8994) pm_runtime_disable(wm8994->dev); mfd_remove_devices(wm8994->dev); wm8994_irq_exit(wm8994); - regulator_bulk_disable(wm8994->num_supplies, - wm8994->supplies); + regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies); }
static const struct of_device_id wm8994_of_match[] = {
The order in which resources were freed in wm8994_device_exit() isn't correct. The regulators are removed before they are disabled.
Fix it by reordering code a bit, which makes it exact opposite of wm8994_device_init() as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
--- V1->V2: - Added Ack from Charles. --- drivers/mfd/wm8994-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 1990b2c90732..95e6bc55adbb 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -603,9 +603,9 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) static void wm8994_device_exit(struct wm8994 *wm8994) { pm_runtime_disable(wm8994->dev); - mfd_remove_devices(wm8994->dev); wm8994_irq_exit(wm8994); regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies); + mfd_remove_devices(wm8994->dev); }
static const struct of_device_id wm8994_of_match[] = {
On Fri, 16 Sep 2016, Viresh Kumar wrote:
The order in which resources were freed in wm8994_device_exit() isn't correct. The regulators are removed before they are disabled.
Fix it by reordering code a bit, which makes it exact opposite of wm8994_device_init() as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
V1->V2:
- Added Ack from Charles.
drivers/mfd/wm8994-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 1990b2c90732..95e6bc55adbb 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -603,9 +603,9 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) static void wm8994_device_exit(struct wm8994 *wm8994) { pm_runtime_disable(wm8994->dev);
- mfd_remove_devices(wm8994->dev); wm8994_irq_exit(wm8994); regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies);
- mfd_remove_devices(wm8994->dev);
} static const struct of_device_id wm8994_of_match[] = {
The kernel WARNs and then crashes today if wm8994_device_init() fails after calling devm_regulator_bulk_get().
That happens because there are multiple devices involved here and the order in which managed resources are freed isn't correct.
The regulators are added as children of wm8994->dev. Whereas, devm_regulator_bulk_get() receives wm8994->dev as the device, though it gets the same regulators which were added as children of wm8994->dev earlier.
During failures, the children are removed first and the core eventually calls regulator_unregister() for them. As regulator_put() was never done for them (opposite of devm_regulator_bulk_get()), the kernel WARNs at
WARN_ON(rdev->open_count);
And eventually it crashes from debugfs_remove_recursive().
--------x------------------x----------------
wm8994 3-001a: Device is not a WM8994, ID is 0 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /mnt/ssd/all/work/repos/devel/linux/drivers/regulator/core.c:4072 regulator_unregister+0xc8/0xd0 Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [<c010e24c>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14) [<c010af38>] (show_stack) from [<c032a1c4>] (dump_stack+0x88/0x9c) [<c032a1c4>] (dump_stack) from [<c011a98c>] (__warn+0xe8/0x100) [<c011a98c>] (__warn) from [<c011aa54>] (warn_slowpath_null+0x20/0x28) [<c011aa54>] (warn_slowpath_null) from [<c0384a0c>] (regulator_unregister+0xc8/0xd0) [<c0384a0c>] (regulator_unregister) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04039c4>] (__device_release_driver+0x8c/0x110) [<c04039c4>] (__device_release_driver) from [<c0403a64>] (device_release_driver+0x1c/0x28) [<c0403a64>] (device_release_driver) from [<c0402b24>] (bus_remove_device+0xd8/0x104) [<c0402b24>] (bus_remove_device) from [<c03ffcd8>] (device_del+0x10c/0x218) [<c03ffcd8>] (device_del) from [<c0404e4c>] (platform_device_del+0x1c/0x88) [<c0404e4c>] (platform_device_del) from [<c0404ec4>] (platform_device_unregister+0xc/0x20) [<c0404ec4>] (platform_device_unregister) from [<c0428bc0>] (mfd_remove_devices_fn+0x5c/0x64) [<c0428bc0>] (mfd_remove_devices_fn) from [<c03ff9d8>] (device_for_each_child_reverse+0x4c/0x78) [<c03ff9d8>] (device_for_each_child_reverse) from [<c04288c4>] (mfd_remove_devices+0x20/0x30) [<c04288c4>] (mfd_remove_devices) from [<c042758c>] (wm8994_device_init+0x2ac/0x7f0) [<c042758c>] (wm8994_device_init) from [<c04f14a8>] (i2c_device_probe+0x178/0x1fc) [<c04f14a8>] (i2c_device_probe) from [<c04036fc>] (driver_probe_device+0x214/0x2c0) [<c04036fc>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) ---[ end trace 0919d3d0bc998260 ]---
[snip..]
Unable to handle kernel NULL pointer dereference at virtual address 00000078 pgd = c0004000 [00000078] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) task: ee874000 task.stack: ee878000 PC is at down_write+0x14/0x54 LR is at debugfs_remove_recursive+0x30/0x150
[snip..]
[<c06e489c>] (down_write) from [<c02e9954>] (debugfs_remove_recursive+0x30/0x150) [<c02e9954>] (debugfs_remove_recursive) from [<c0382b78>] (_regulator_put+0x24/0xac) [<c0382b78>] (_regulator_put) from [<c0382c1c>] (regulator_put+0x1c/0x2c) [<c0382c1c>] (regulator_put) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04035d4>] (driver_probe_device+0xec/0x2c0) [<c04035d4>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) Code: e1a04000 f590f000 e3a03001 e34f3fff (e1902f9f) ---[ end trace 0919d3d0bc998262 ]---
--------x------------------x----------------
Fix the kernel warnings and crashes by using regulator_bulk_get() instead of devm_regulator_bulk_get() and explicitly freeing the supplies in exit paths.
Tested on Exynos 5250, dual core ARM A15 machine.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
--- V1->V2: - Use regulator_bulk_free() instead of open coding it. - Shorter backtrace - Reworded the last paragraph to make it more clear - Added a comment in code --- drivers/mfd/wm8994-core.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 95e6bc55adbb..953d0790ffd5 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -393,8 +393,13 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) BUG(); goto err; } - - ret = devm_regulator_bulk_get(wm8994->dev, wm8994->num_supplies, + + /* + * Can't use devres helper here as some of the supplies are provided by + * wm8994->dev's children (regulators) and those regulators are + * unregistered by the devres core before the supplies are freed. + */ + ret = regulator_bulk_get(wm8994->dev, wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to get supplies: %d\n", ret); @@ -404,7 +409,7 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) ret = regulator_bulk_enable(wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to enable supplies: %d\n", ret); - goto err; + goto err_regulator_free; }
ret = wm8994_reg_read(wm8994, WM8994_SOFTWARE_RESET); @@ -595,6 +600,8 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) err_enable: regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies); +err_regulator_free: + regulator_bulk_free(wm8994->num_supplies, wm8994->supplies); err: mfd_remove_devices(wm8994->dev); return ret; @@ -605,6 +612,7 @@ static void wm8994_device_exit(struct wm8994 *wm8994) pm_runtime_disable(wm8994->dev); wm8994_irq_exit(wm8994); regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies); + regulator_bulk_free(wm8994->num_supplies, wm8994->supplies); mfd_remove_devices(wm8994->dev); }
On Fri, Sep 16, 2016 at 08:57:00AM +0530, Viresh Kumar wrote:
The kernel WARNs and then crashes today if wm8994_device_init() fails after calling devm_regulator_bulk_get().
That happens because there are multiple devices involved here and the order in which managed resources are freed isn't correct.
The regulators are added as children of wm8994->dev. Whereas, devm_regulator_bulk_get() receives wm8994->dev as the device, though it gets the same regulators which were added as children of wm8994->dev earlier.
During failures, the children are removed first and the core eventually calls regulator_unregister() for them. As regulator_put() was never done for them (opposite of devm_regulator_bulk_get()), the kernel WARNs at
WARN_ON(rdev->open_count);
And eventually it crashes from debugfs_remove_recursive().
--------x------------------x----------------
wm8994 3-001a: Device is not a WM8994, ID is 0 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /mnt/ssd/all/work/repos/devel/linux/drivers/regulator/core.c:4072 regulator_unregister+0xc8/0xd0 Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [<c010e24c>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14) [<c010af38>] (show_stack) from [<c032a1c4>] (dump_stack+0x88/0x9c) [<c032a1c4>] (dump_stack) from [<c011a98c>] (__warn+0xe8/0x100) [<c011a98c>] (__warn) from [<c011aa54>] (warn_slowpath_null+0x20/0x28) [<c011aa54>] (warn_slowpath_null) from [<c0384a0c>] (regulator_unregister+0xc8/0xd0) [<c0384a0c>] (regulator_unregister) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04039c4>] (__device_release_driver+0x8c/0x110) [<c04039c4>] (__device_release_driver) from [<c0403a64>] (device_release_driver+0x1c/0x28) [<c0403a64>] (device_release_driver) from [<c0402b24>] (bus_remove_device+0xd8/0x104) [<c0402b24>] (bus_remove_device) from [<c03ffcd8>] (device_del+0x10c/0x218) [<c03ffcd8>] (device_del) from [<c0404e4c>] (platform_device_del+0x1c/0x88) [<c0404e4c>] (platform_device_del) from [<c0404ec4>] (platform_device_unregister+0xc/0x20) [<c0404ec4>] (platform_device_unregister) from [<c0428bc0>] (mfd_remove_devices_fn+0x5c/0x64) [<c0428bc0>] (mfd_remove_devices_fn) from [<c03ff9d8>] (device_for_each_child_reverse+0x4c/0x78) [<c03ff9d8>] (device_for_each_child_reverse) from [<c04288c4>] (mfd_remove_devices+0x20/0x30) [<c04288c4>] (mfd_remove_devices) from [<c042758c>] (wm8994_device_init+0x2ac/0x7f0) [<c042758c>] (wm8994_device_init) from [<c04f14a8>] (i2c_device_probe+0x178/0x1fc) [<c04f14a8>] (i2c_device_probe) from [<c04036fc>] (driver_probe_device+0x214/0x2c0) [<c04036fc>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) ---[ end trace 0919d3d0bc998260 ]---
[snip..]
Unable to handle kernel NULL pointer dereference at virtual address 00000078 pgd = c0004000 [00000078] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) task: ee874000 task.stack: ee878000 PC is at down_write+0x14/0x54 LR is at debugfs_remove_recursive+0x30/0x150
[snip..]
[<c06e489c>] (down_write) from [<c02e9954>] (debugfs_remove_recursive+0x30/0x150) [<c02e9954>] (debugfs_remove_recursive) from [<c0382b78>] (_regulator_put+0x24/0xac) [<c0382b78>] (_regulator_put) from [<c0382c1c>] (regulator_put+0x1c/0x2c) [<c0382c1c>] (regulator_put) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04035d4>] (driver_probe_device+0xec/0x2c0) [<c04035d4>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) Code: e1a04000 f590f000 e3a03001 e34f3fff (e1902f9f) ---[ end trace 0919d3d0bc998262 ]---
--------x------------------x----------------
Fix the kernel warnings and crashes by using regulator_bulk_get() instead of devm_regulator_bulk_get() and explicitly freeing the supplies in exit paths.
Tested on Exynos 5250, dual core ARM A15 machine.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
Thanks, Charles
On Fri, 16 Sep 2016, Viresh Kumar wrote:
The kernel WARNs and then crashes today if wm8994_device_init() fails after calling devm_regulator_bulk_get().
That happens because there are multiple devices involved here and the order in which managed resources are freed isn't correct.
The regulators are added as children of wm8994->dev. Whereas, devm_regulator_bulk_get() receives wm8994->dev as the device, though it gets the same regulators which were added as children of wm8994->dev earlier.
During failures, the children are removed first and the core eventually calls regulator_unregister() for them. As regulator_put() was never done for them (opposite of devm_regulator_bulk_get()), the kernel WARNs at
WARN_ON(rdev->open_count);
And eventually it crashes from debugfs_remove_recursive().
--------x------------------x----------------
wm8994 3-001a: Device is not a WM8994, ID is 0 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /mnt/ssd/all/work/repos/devel/linux/drivers/regulator/core.c:4072 regulator_unregister+0xc8/0xd0 Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [<c010e24c>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14) [<c010af38>] (show_stack) from [<c032a1c4>] (dump_stack+0x88/0x9c) [<c032a1c4>] (dump_stack) from [<c011a98c>] (__warn+0xe8/0x100) [<c011a98c>] (__warn) from [<c011aa54>] (warn_slowpath_null+0x20/0x28) [<c011aa54>] (warn_slowpath_null) from [<c0384a0c>] (regulator_unregister+0xc8/0xd0) [<c0384a0c>] (regulator_unregister) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04039c4>] (__device_release_driver+0x8c/0x110) [<c04039c4>] (__device_release_driver) from [<c0403a64>] (device_release_driver+0x1c/0x28) [<c0403a64>] (device_release_driver) from [<c0402b24>] (bus_remove_device+0xd8/0x104) [<c0402b24>] (bus_remove_device) from [<c03ffcd8>] (device_del+0x10c/0x218) [<c03ffcd8>] (device_del) from [<c0404e4c>] (platform_device_del+0x1c/0x88) [<c0404e4c>] (platform_device_del) from [<c0404ec4>] (platform_device_unregister+0xc/0x20) [<c0404ec4>] (platform_device_unregister) from [<c0428bc0>] (mfd_remove_devices_fn+0x5c/0x64) [<c0428bc0>] (mfd_remove_devices_fn) from [<c03ff9d8>] (device_for_each_child_reverse+0x4c/0x78) [<c03ff9d8>] (device_for_each_child_reverse) from [<c04288c4>] (mfd_remove_devices+0x20/0x30) [<c04288c4>] (mfd_remove_devices) from [<c042758c>] (wm8994_device_init+0x2ac/0x7f0) [<c042758c>] (wm8994_device_init) from [<c04f14a8>] (i2c_device_probe+0x178/0x1fc) [<c04f14a8>] (i2c_device_probe) from [<c04036fc>] (driver_probe_device+0x214/0x2c0) [<c04036fc>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) ---[ end trace 0919d3d0bc998260 ]---
[snip..]
Unable to handle kernel NULL pointer dereference at virtual address 00000078 pgd = c0004000 [00000078] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) task: ee874000 task.stack: ee878000 PC is at down_write+0x14/0x54 LR is at debugfs_remove_recursive+0x30/0x150
[snip..]
[<c06e489c>] (down_write) from [<c02e9954>] (debugfs_remove_recursive+0x30/0x150) [<c02e9954>] (debugfs_remove_recursive) from [<c0382b78>] (_regulator_put+0x24/0xac) [<c0382b78>] (_regulator_put) from [<c0382c1c>] (regulator_put+0x1c/0x2c) [<c0382c1c>] (regulator_put) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04035d4>] (driver_probe_device+0xec/0x2c0) [<c04035d4>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) Code: e1a04000 f590f000 e3a03001 e34f3fff (e1902f9f) ---[ end trace 0919d3d0bc998262 ]---
--------x------------------x----------------
Fix the kernel warnings and crashes by using regulator_bulk_get() instead of devm_regulator_bulk_get() and explicitly freeing the supplies in exit paths.
Tested on Exynos 5250, dual core ARM A15 machine.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V1->V2:
- Use regulator_bulk_free() instead of open coding it.
- Shorter backtrace
- Reworded the last paragraph to make it more clear
- Added a comment in code
drivers/mfd/wm8994-core.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
Applied, thanks.
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 95e6bc55adbb..953d0790ffd5 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -393,8 +393,13 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) BUG(); goto err; }
- ret = devm_regulator_bulk_get(wm8994->dev, wm8994->num_supplies,
- /*
* Can't use devres helper here as some of the supplies are provided by* wm8994->dev's children (regulators) and those regulators are* unregistered by the devres core before the supplies are freed.*/- ret = regulator_bulk_get(wm8994->dev, wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to get supplies: %d\n", ret);
@@ -404,7 +409,7 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) ret = regulator_bulk_enable(wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to enable supplies: %d\n", ret);
goto err;
goto err_regulator_free;ret = wm8994_reg_read(wm8994, WM8994_SOFTWARE_RESET); @@ -595,6 +600,8 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) err_enable: regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies); +err_regulator_free:
- regulator_bulk_free(wm8994->num_supplies, wm8994->supplies);
err: mfd_remove_devices(wm8994->dev); return ret; @@ -605,6 +612,7 @@ static void wm8994_device_exit(struct wm8994 *wm8994) pm_runtime_disable(wm8994->dev); wm8994_irq_exit(wm8994); regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies);
- regulator_bulk_free(wm8994->num_supplies, wm8994->supplies); mfd_remove_devices(wm8994->dev);
}
On Wed, 26 Oct 2016, Lee Jones wrote:
On Fri, 16 Sep 2016, Viresh Kumar wrote:
The kernel WARNs and then crashes today if wm8994_device_init() fails after calling devm_regulator_bulk_get().
That happens because there are multiple devices involved here and the order in which managed resources are freed isn't correct.
The regulators are added as children of wm8994->dev. Whereas, devm_regulator_bulk_get() receives wm8994->dev as the device, though it gets the same regulators which were added as children of wm8994->dev earlier.
During failures, the children are removed first and the core eventually calls regulator_unregister() for them. As regulator_put() was never done for them (opposite of devm_regulator_bulk_get()), the kernel WARNs at
WARN_ON(rdev->open_count);
And eventually it crashes from debugfs_remove_recursive().
--------x------------------x----------------
wm8994 3-001a: Device is not a WM8994, ID is 0 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /mnt/ssd/all/work/repos/devel/linux/drivers/regulator/core.c:4072 regulator_unregister+0xc8/0xd0 Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [<c010e24c>] (unwind_backtrace) from [<c010af38>] (show_stack+0x10/0x14) [<c010af38>] (show_stack) from [<c032a1c4>] (dump_stack+0x88/0x9c) [<c032a1c4>] (dump_stack) from [<c011a98c>] (__warn+0xe8/0x100) [<c011a98c>] (__warn) from [<c011aa54>] (warn_slowpath_null+0x20/0x28) [<c011aa54>] (warn_slowpath_null) from [<c0384a0c>] (regulator_unregister+0xc8/0xd0) [<c0384a0c>] (regulator_unregister) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04039c4>] (__device_release_driver+0x8c/0x110) [<c04039c4>] (__device_release_driver) from [<c0403a64>] (device_release_driver+0x1c/0x28) [<c0403a64>] (device_release_driver) from [<c0402b24>] (bus_remove_device+0xd8/0x104) [<c0402b24>] (bus_remove_device) from [<c03ffcd8>] (device_del+0x10c/0x218) [<c03ffcd8>] (device_del) from [<c0404e4c>] (platform_device_del+0x1c/0x88) [<c0404e4c>] (platform_device_del) from [<c0404ec4>] (platform_device_unregister+0xc/0x20) [<c0404ec4>] (platform_device_unregister) from [<c0428bc0>] (mfd_remove_devices_fn+0x5c/0x64) [<c0428bc0>] (mfd_remove_devices_fn) from [<c03ff9d8>] (device_for_each_child_reverse+0x4c/0x78) [<c03ff9d8>] (device_for_each_child_reverse) from [<c04288c4>] (mfd_remove_devices+0x20/0x30) [<c04288c4>] (mfd_remove_devices) from [<c042758c>] (wm8994_device_init+0x2ac/0x7f0) [<c042758c>] (wm8994_device_init) from [<c04f14a8>] (i2c_device_probe+0x178/0x1fc) [<c04f14a8>] (i2c_device_probe) from [<c04036fc>] (driver_probe_device+0x214/0x2c0) [<c04036fc>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) ---[ end trace 0919d3d0bc998260 ]---
[snip..]
Unable to handle kernel NULL pointer dereference at virtual address 00000078 pgd = c0004000 [00000078] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) task: ee874000 task.stack: ee878000 PC is at down_write+0x14/0x54 LR is at debugfs_remove_recursive+0x30/0x150
[snip..]
[<c06e489c>] (down_write) from [<c02e9954>] (debugfs_remove_recursive+0x30/0x150) [<c02e9954>] (debugfs_remove_recursive) from [<c0382b78>] (_regulator_put+0x24/0xac) [<c0382b78>] (_regulator_put) from [<c0382c1c>] (regulator_put+0x1c/0x2c) [<c0382c1c>] (regulator_put) from [<c0406434>] (release_nodes+0x16c/0x1dc) [<c0406434>] (release_nodes) from [<c04035d4>] (driver_probe_device+0xec/0x2c0) [<c04035d4>] (driver_probe_device) from [<c0403854>] (__driver_attach+0xac/0xb0) [<c0403854>] (__driver_attach) from [<c0401a74>] (bus_for_each_dev+0x68/0x9c) [<c0401a74>] (bus_for_each_dev) from [<c0402cf0>] (bus_add_driver+0x1a0/0x218) [<c0402cf0>] (bus_add_driver) from [<c040406c>] (driver_register+0x78/0xf8) [<c040406c>] (driver_register) from [<c04f20a0>] (i2c_register_driver+0x34/0x84) [<c04f20a0>] (i2c_register_driver) from [<c01017d0>] (do_one_initcall+0x40/0x170) [<c01017d0>] (do_one_initcall) from [<c0a00dbc>] (kernel_init_freeable+0x15c/0x1fc) [<c0a00dbc>] (kernel_init_freeable) from [<c06e07b0>] (kernel_init+0x8/0x114) [<c06e07b0>] (kernel_init) from [<c0107978>] (ret_from_fork+0x14/0x3c) Code: e1a04000 f590f000 e3a03001 e34f3fff (e1902f9f) ---[ end trace 0919d3d0bc998262 ]---
--------x------------------x----------------
Fix the kernel warnings and crashes by using regulator_bulk_get() instead of devm_regulator_bulk_get() and explicitly freeing the supplies in exit paths.
Tested on Exynos 5250, dual core ARM A15 machine.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V1->V2:
- Use regulator_bulk_free() instead of open coding it.
- Shorter backtrace
- Reworded the last paragraph to make it more clear
- Added a comment in code
drivers/mfd/wm8994-core.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
Applied, thanks.
Change of plan, since it does not apply.
Please rebase and re-send, complete with Charles' Ack.
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 95e6bc55adbb..953d0790ffd5 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -393,8 +393,13 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) BUG(); goto err; }
- ret = devm_regulator_bulk_get(wm8994->dev, wm8994->num_supplies,
- /*
* Can't use devres helper here as some of the supplies are provided by* wm8994->dev's children (regulators) and those regulators are* unregistered by the devres core before the supplies are freed.*/- ret = regulator_bulk_get(wm8994->dev, wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to get supplies: %d\n", ret);
@@ -404,7 +409,7 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) ret = regulator_bulk_enable(wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to enable supplies: %d\n", ret);
goto err;
goto err_regulator_free;ret = wm8994_reg_read(wm8994, WM8994_SOFTWARE_RESET); @@ -595,6 +600,8 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) err_enable: regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies); +err_regulator_free:
- regulator_bulk_free(wm8994->num_supplies, wm8994->supplies);
err: mfd_remove_devices(wm8994->dev); return ret; @@ -605,6 +612,7 @@ static void wm8994_device_exit(struct wm8994 *wm8994) pm_runtime_disable(wm8994->dev); wm8994_irq_exit(wm8994); regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies);
- regulator_bulk_free(wm8994->num_supplies, wm8994->supplies); mfd_remove_devices(wm8994->dev);
}
On Fri, Sep 16, 2016 at 08:56:58AM +0530, Viresh Kumar wrote:
These can fit in a single line (80 columns), don't split lines unnecessarily.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com
Thanks, Charles
On 16-09-16, 08:56, Viresh Kumar wrote:
These can fit in a single line (80 columns), don't split lines unnecessarily.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V1->V2: New patch
drivers/mfd/wm8994-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Ping!!
On Mon, 03 Oct 2016, Viresh Kumar wrote:
On 16-09-16, 08:56, Viresh Kumar wrote:
These can fit in a single line (80 columns), don't split lines unnecessarily.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V1->V2: New patch
drivers/mfd/wm8994-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Ping!!
Don't do that!
On 04-10-16, 15:41, Lee Jones wrote:
On Mon, 03 Oct 2016, Viresh Kumar wrote:
On 16-09-16, 08:56, Viresh Kumar wrote:
These can fit in a single line (80 columns), don't split lines unnecessarily.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V1->V2: New patch
drivers/mfd/wm8994-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Ping!!
Don't do that!
Okay, but what's exactly wrong with that? Its been 20 days that I have heard anything from you on this. Pinging the maintainers is the only option other people have, right?
On Wed, 05 Oct 2016, Viresh Kumar wrote:
On 04-10-16, 15:41, Lee Jones wrote:
On Mon, 03 Oct 2016, Viresh Kumar wrote:
On 16-09-16, 08:56, Viresh Kumar wrote:
These can fit in a single line (80 columns), don't split lines unnecessarily.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V1->V2: New patch
drivers/mfd/wm8994-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Ping!!
Don't do that!
Okay, but what's exactly wrong with that? Its been 20 days that I have heard anything from you on this. Pinging the maintainers is the only option other people have, right?
You are experienced enough to know better than this.
a) Contentless pings have never been acceptable. If you genuinely think a patch has been forgotten you should resubmit with a [RESEND]. That is their entire purpose.
b) You submitted this patch right at the end of the release cycle, and your ping was sent during the merge-window. Most Maintainers, myself included, like to have patches soak tested in -next for at least a couple of weeks prior to acceptance.
c) The merge-window is open. We are likely conducting final tests and formatting pull-requests during this time. As an experienced submitter, I would have expected you to follow the release cycle and know that sending patches late is cause for delay.
d) Maintainers take vacations and attend conferences, so some delays are to be expected.
FYI: Your patch has not slipped through the net. It is in the pile to be reviewed. Please be more patient, especially around merge time.
On 05-10-16, 09:49, Lee Jones wrote:
You are experienced enough to know better than this.
:)
a) Contentless pings have never been acceptable. If you genuinely think a patch has been forgotten you should resubmit with a [RESEND]. That is their entire purpose.
Sure, but I really believe a light *ping* is much better than a complete resend to start with. It generates far less noise.
b) You submitted this patch right at the end of the release cycle, and your ping was sent during the merge-window. Most Maintainers, myself included, like to have patches soak tested in -next for at least a couple of weeks prior to acceptance.
I agree to that, I sent it after rc6. I wasn't looking to get this merged during this cycle, but was wondering if it got missed or something like that.
I still don't think that a simple Ping was that bad of an option, but its fine. Take your time to review this, no issues.
Cheers.
On Wed, 05 Oct 2016, Viresh Kumar wrote:
On 05-10-16, 09:49, Lee Jones wrote:
You are experienced enough to know better than this.
:)
a) Contentless pings have never been acceptable. If you genuinely think a patch has been forgotten you should resubmit with a [RESEND]. That is their entire purpose.
Sure, but I really believe a light *ping* is much better than a complete resend to start with. It generates far less noise.
Contentless pings are generally not accepted.
It also has the unfortunate side-effect of placing your patches at the top of the pile, and since I process patches in reverse chronological order, you just put yourself at the back of the list. ;)
-- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Fri, 16 Sep 2016, Viresh Kumar wrote:
These can fit in a single line (80 columns), don't split lines unnecessarily.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V1->V2: New patch
drivers/mfd/wm8994-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Applied, thanks.
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 7eec619a6023..1990b2c90732 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -401,8 +401,7 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) goto err; }
- ret = regulator_bulk_enable(wm8994->num_supplies,
wm8994->supplies);
- ret = regulator_bulk_enable(wm8994->num_supplies, wm8994->supplies); if (ret != 0) { dev_err(wm8994->dev, "Failed to enable supplies: %d\n", ret); goto err;
@@ -606,8 +605,7 @@ static void wm8994_device_exit(struct wm8994 *wm8994) pm_runtime_disable(wm8994->dev); mfd_remove_devices(wm8994->dev); wm8994_irq_exit(wm8994);
- regulator_bulk_disable(wm8994->num_supplies,
wm8994->supplies);
- regulator_bulk_disable(wm8994->num_supplies, wm8994->supplies);
} static const struct of_device_id wm8994_of_match[] = {
linaro-kernel@lists.linaro.org
-
Charles Keepax -
Lee Jones -
Viresh Kumar