Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1484659 > unrolled thread
| Started by | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| First post | 2016-09-16 05:30 +0200 |
| Last post | 2016-09-16 09:40 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-16 05:30 +0200
[PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-16 05:30 +0200
Re: [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2016-09-16 09:40 +0200
Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2016-09-16 09:40 +0200
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-09-16 05:30 +0200 |
| Subject | [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily |
| Message-ID | <shS0F-7tw-3@gated-at.bofh.it> |
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[] = {
--
2.7.1.410.g6faf27b
[toc] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-09-16 05:30 +0200 |
| Subject | [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API |
| Message-ID | <shS0F-7tw-5@gated-at.bofh.it> |
| In reply to | #1484659 |
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);
}
--
2.7.1.410.g6faf27b
[toc] | [prev] | [next] | [standalone]
| From | Charles Keepax <ckeepax@opensource.wolfsonmicro.com> |
|---|---|
| Date | 2016-09-16 09:40 +0200 |
| Subject | Re: [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API |
| Message-ID | <shVUB-1AV-1@gated-at.bofh.it> |
| In reply to | #1484660 |
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
[toc] | [prev] | [next] | [standalone]
| From | Charles Keepax <ckeepax@opensource.wolfsonmicro.com> |
|---|---|
| Date | 2016-09-16 09:40 +0200 |
| Message-ID | <shVUB-1AV-21@gated-at.bofh.it> |
| In reply to | #1484659 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web