From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754990AbcJZMWL (ORCPT ); Wed, 26 Oct 2016 08:22:11 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:36614 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754308AbcJZMWJ (ORCPT ); Wed, 26 Oct 2016 08:22:09 -0400 Date: Wed, 26 Oct 2016 13:24:39 +0100 From: Lee Jones To: Viresh Kumar Cc: linaro-kernel@lists.linaro.org, Mark Brown , Krzysztof =?utf-8?Q?Koz=C5=82owski?= , Charles Keepax , patches@opensource.wolfsonmicro.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API Message-ID: <20161026122439.GY8574@dell> References: <7d68a9e5dd338081342dd8b06310c2f3e61fccc0.1473996370.git.viresh.kumar@linaro.org> <00005c8c0b1294412c543604cb537622d6257416.1473996370.git.viresh.kumar@linaro.org> <20161026122228.GW8574@dell> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20161026122228.GW8574@dell> User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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) > > [] (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 ]--- > > > > [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..] > > > > [] (down_write) from [] (debugfs_remove_recursive+0x30/0x150) > > [] (debugfs_remove_recursive) from [] (_regulator_put+0x24/0xac) > > [] (_regulator_put) from [] (regulator_put+0x1c/0x2c) > > [] (regulator_put) from [] (release_nodes+0x16c/0x1dc) > > [] (release_nodes) from [] (driver_probe_device+0xec/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) > > 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 > > > > --- > > 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