Re: [PATCH 2/2] mfd: wm8994-core: Don't use managed regulator bulk get API
On Thu, Sep 15, 2016 at 08:38:44PM +0530, Viresh Kumar wrote: > On 15-09-16, 15:47, Mark Brown wrote: > > On Thu, Sep 15, 2016 at 04:47:01PM +0530, Viresh Kumar wrote: > > > Fix the kernel warnings and crashes by moving away from managed > > > regulator bulk get API by using regulator_bulk_get() and explicitly > > > calling regulator_put() for all the supplies in exit paths. > > Moving away from regulator bulk get to regulator bulk get? > Moving away from "managed" regulator bulk get to regulator bulk get? > Isn't this how we call the devm_* APIs as? That's really unclearly worded (especially since one uses the function name and the other doesn't). > > > - ret = devm_regulator_bulk_get(wm8994->dev, wm8994->num_supplies, > > > + ret = regulator_bulk_get(wm8994->dev, wm8994->num_supplies, > > >wm8994->supplies); > > > +err_regulator_put: > > > + for (i = wm8994->num_supplies - 1; i >= 0; i--) > > > + regulator_put(wm8994->supplies[i].consumer); > > Why are you unwinding regulator_bulk_get() with an open coded > > regulator_bulk_put()? > Because there is no regulator_bulk_put(). Should I add it? I mean regulator_bulk_free() there. signature.asc Description: PGP signature
Re: [PATCH 2/2] mfd: wm8994-core: Don't use managed regulator bulk get API
On 15-09-16, 15:47, Mark Brown wrote: > On Thu, Sep 15, 2016 at 04:47:01PM +0530, Viresh Kumar wrote: > > > Fix the kernel warnings and crashes by moving away from managed > > regulator bulk get API by using regulator_bulk_get() and explicitly > > calling regulator_put() for all the supplies in exit paths. > > Moving away from regulator bulk get to regulator bulk get? Moving away from "managed" regulator bulk get to regulator bulk get? Isn't this how we call the devm_* APIs as? > > - ret = devm_regulator_bulk_get(wm8994->dev, wm8994->num_supplies, > > + ret = regulator_bulk_get(wm8994->dev, wm8994->num_supplies, > > wm8994->supplies); > > > +err_regulator_put: > > + for (i = wm8994->num_supplies - 1; i >= 0; i--) > > + regulator_put(wm8994->supplies[i].consumer); > > Why are you unwinding regulator_bulk_get() with an open coded > regulator_bulk_put()? Because there is no regulator_bulk_put(). Should I add it? > Also please think hard before including complete backtraces in upstream > reports, they are very large and contain almost no useful information > relative to their size so often obscure the relevant content in your > message. If part of the backtrace is usefully illustrative then it's > usually better to pull out the relevant sections. Sure, will add only relevant bits in the next version. -- viresh
Re: [PATCH 2/2] mfd: wm8994-core: Don't use managed regulator bulk get API
On Thu, Sep 15, 2016 at 04:47:01PM +0530, Viresh Kumar wrote: > Fix the kernel warnings and crashes by moving away from managed > regulator bulk get API by using regulator_bulk_get() and explicitly > calling regulator_put() for all the supplies in exit paths. Moving away from regulator bulk get to regulator bulk get? > - ret = devm_regulator_bulk_get(wm8994->dev, wm8994->num_supplies, > + ret = regulator_bulk_get(wm8994->dev, wm8994->num_supplies, >wm8994->supplies); > +err_regulator_put: > + for (i = wm8994->num_supplies - 1; i >= 0; i--) > + regulator_put(wm8994->supplies[i].consumer); Why are you unwinding regulator_bulk_get() with an open coded regulator_bulk_put()? Also please think hard before including complete backtraces in upstream reports, they are very large and contain almost no useful information relative to their size so often obscure the relevant content in your message. If part of the backtrace is usefully illustrative then it's usually better to pull out the relevant sections. signature.asc Description: PGP signature
Re: [PATCH 2/2] mfd: wm8994-core: Don't use managed regulator bulk get API
On Thu, Sep 15, 2016 at 04:47:01PM +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 resources are freed isn't correct. > > The regulators are added as children of wm8994->dev. > 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) > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x88/0x9c) > [] (dump_stack) from [] (__warn+0xe8/0x100) > [] (__warn) from [] (warn_slowpath_null+0x20/0x28) > [] (warn_slowpath_null) from [] > (regulator_unregister+0xc8/0xd0) > [] (regulator_unregister) from [] > (release_nodes+0x16c/0x1dc) > [] (release_nodes) from [] > (__device_release_driver+0x8c/0x110) > [] (__device_release_driver) from [] > (device_release_driver+0x1c/0x28) > [] (device_release_driver) from [] > (bus_remove_device+0xd8/0x104) > [] (bus_remove_device) from [] (device_del+0x10c/0x218) > [] (device_del) from [] (platform_device_del+0x1c/0x88) > [] (platform_device_del) from [] > (platform_device_unregister+0xc/0x20) > [] (platform_device_unregister) from [] > (mfd_remove_devices_fn+0x5c/0x64) > [] (mfd_remove_devices_fn) from [] > (device_for_each_child_reverse+0x4c/0x78) > [] (device_for_each_child_reverse) from [] > (mfd_remove_devices+0x20/0x30) > [] (mfd_remove_devices) from [] > (wm8994_device_init+0x2ac/0x7f0) > [] (wm8994_device_init) from [] > (i2c_device_probe+0x178/0x1fc) > [] (i2c_device_probe) from [] > (driver_probe_device+0x214/0x2c0) > [] (driver_probe_device) from [] > (__driver_attach+0xac/0xb0) > [] (__driver_attach) from [] (bus_for_each_dev+0x68/0x9c) > [] (bus_for_each_dev) from [] > (bus_add_driver+0x1a0/0x218) > [] (bus_add_driver) from [] (driver_register+0x78/0xf8) > [] (driver_register) from [] > (i2c_register_driver+0x34/0x84) > [] (i2c_register_driver) from [] > (do_one_initcall+0x40/0x170) > [] (do_one_initcall) from [] > (kernel_init_freeable+0x15c/0x1fc) > [] (kernel_init_freeable) from [] (kernel_init+0x8/0x114) > [] (kernel_init) from [] (ret_from_fork+0x14/0x3c) > ---[ end trace 0919d3d0bc998260 ]--- > [ 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 Tainted: GW > 4.8.0-rc6-00154-g54fe84cbd50b #41 > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x88/0x9c) > [] (dump_stack) from [] (__warn+0xe8/0x100) > [] (__warn) from [] (warn_slowpath_null+0x20/0x28) > [] (warn_slowpath_null) from [] > (regulator_unregister+0xc8/0xd0) > [] (regulator_unregister) from [] > (release_nodes+0x16c/0x1dc) > [] (release_nodes) from [] > (__device_release_driver+0x8c/0x110) > [] (__device_release_driver) from [] > (device_release_driver+0x1c/0x28) > [] (device_release_driver) from [] > (bus_remove_device+0xd8/0x104) > [] (bus_remove_device) from [] (device_del+0x10c/0x218) > [] (device_del) from [] (platform_device_del+0x1c/0x88) > 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) > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x88/0x9c) > [] (dump_stack) from [] (__warn+0xe8/0x100) > [] (__warn) from [] (warn_slowpath_null+0x20/0x28) > [] (warn_slowpath_null) from [] > (regulator_unregister+0xc8/0xd0) > [] (regulator_unregister) from [] > (release_nodes+0x16c/0x1dc) > [] (release_nodes) from [] > (__device_release_driver+0x8c/0x110) > [] (__device_release_driver) from [] > (device_release_driver+0x1c/
[PATCH 2/2] mfd: wm8994-core: Don't use managed regulator bulk get API
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 resources are freed isn't correct. The regulators are added as children of wm8994->dev. 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) [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x88/0x9c) [] (dump_stack) from [] (__warn+0xe8/0x100) [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [] (warn_slowpath_null) from [] (regulator_unregister+0xc8/0xd0) [] (regulator_unregister) from [] (release_nodes+0x16c/0x1dc) [] (release_nodes) from [] (__device_release_driver+0x8c/0x110) [] (__device_release_driver) from [] (device_release_driver+0x1c/0x28) [] (device_release_driver) from [] (bus_remove_device+0xd8/0x104) [] (bus_remove_device) from [] (device_del+0x10c/0x218) [] (device_del) from [] (platform_device_del+0x1c/0x88) [] (platform_device_del) from [] (platform_device_unregister+0xc/0x20) [] (platform_device_unregister) from [] (mfd_remove_devices_fn+0x5c/0x64) [] (mfd_remove_devices_fn) from [] (device_for_each_child_reverse+0x4c/0x78) [] (device_for_each_child_reverse) from [] (mfd_remove_devices+0x20/0x30) [] (mfd_remove_devices) from [] (wm8994_device_init+0x2ac/0x7f0) [] (wm8994_device_init) from [] (i2c_device_probe+0x178/0x1fc) [] (i2c_device_probe) from [] (driver_probe_device+0x214/0x2c0) [] (driver_probe_device) from [] (__driver_attach+0xac/0xb0) [] (__driver_attach) from [] (bus_for_each_dev+0x68/0x9c) [] (bus_for_each_dev) from [] (bus_add_driver+0x1a0/0x218) [] (bus_add_driver) from [] (driver_register+0x78/0xf8) [] (driver_register) from [] (i2c_register_driver+0x34/0x84) [] (i2c_register_driver) from [] (do_one_initcall+0x40/0x170) [] (do_one_initcall) from [] (kernel_init_freeable+0x15c/0x1fc) [] (kernel_init_freeable) from [] (kernel_init+0x8/0x114) [] (kernel_init) from [] (ret_from_fork+0x14/0x3c) ---[ end trace 0919d3d0bc998260 ]--- [ 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 Tainted: GW 4.8.0-rc6-00154-g54fe84cbd50b #41 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x88/0x9c) [] (dump_stack) from [] (__warn+0xe8/0x100) [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [] (warn_slowpath_null) from [] (regulator_unregister+0xc8/0xd0) [] (regulator_unregister) from [] (release_nodes+0x16c/0x1dc) [] (release_nodes) from [] (__device_release_driver+0x8c/0x110) [] (__device_release_driver) from [] (device_release_driver+0x1c/0x28) [] (device_release_driver) from [] (bus_remove_device+0xd8/0x104) [] (bus_remove_device) from [] (device_del+0x10c/0x218) [] (device_del) from [] (platform_device_del+0x1c/0x88) 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) [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x88/0x9c) [] (dump_stack) from [] (__warn+0xe8/0x100) [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [] (warn_slowpath_null) from [] (regulator_unregister+0xc8/0xd0) [] (regulator_unregister) from [] (release_nodes+0x16c/0x1dc) [] (release_nodes) from [] (__device_release_driver+0x8c/0x110) [] (__device_release_driver) from [] (device_release_driver+0x1c/0x28) [] (device_release_driver) from [] (bus_remove_device+0xd8/0x104) [] (bus_remove_device) from [] (device_del+0x10c/0x218) [] (device_del) from [] (platform_device_del+0x1c/0x88) [] (platform_device_del) from [] (platform_device_unregister+0xc/0x20) [] (platform_device_unregister) fro
