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 <[email protected]>
> >
> > ---
> > 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);
> > }
> >
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
