* [PATCH V2 2/3] mfd: wm8994-core: disable regulators before removing them
2016-09-16 3:26 [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Viresh Kumar
@ 2016-09-16 3:26 ` Viresh Kumar
2016-10-26 12:22 ` Lee Jones
2016-09-16 3:27 ` [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API Viresh Kumar
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2016-09-16 3:26 UTC (permalink / raw)
To: Lee Jones
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, Viresh Kumar, patches, linux-kernel
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[] = {
--
2.7.1.410.g6faf27b
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V2 2/3] mfd: wm8994-core: disable regulators before removing them
2016-09-16 3:26 ` [PATCH V2 2/3] mfd: wm8994-core: disable regulators before removing them Viresh Kumar
@ 2016-10-26 12:22 ` Lee Jones
0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2016-10-26 12:22 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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[] = {
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API
2016-09-16 3:26 [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Viresh Kumar
2016-09-16 3:26 ` [PATCH V2 2/3] mfd: wm8994-core: disable regulators before removing them Viresh Kumar
@ 2016-09-16 3:27 ` Viresh Kumar
2016-09-16 7:38 ` Charles Keepax
2016-10-26 12:22 ` Lee Jones
2016-09-16 7:37 ` [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Charles Keepax
` (2 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Viresh Kumar @ 2016-09-16 3:27 UTC (permalink / raw)
To: Lee Jones
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, Viresh Kumar, patches, linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API
2016-09-16 3:27 ` [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API Viresh Kumar
@ 2016-09-16 7:38 ` Charles Keepax
2016-10-26 12:22 ` Lee Jones
1 sibling, 0 replies; 15+ messages in thread
From: Charles Keepax @ 2016-09-16 7:38 UTC (permalink / raw)
To: Viresh Kumar
Cc: Lee Jones, linaro-kernel, Mark Brown, Krzysztof Kozłowski,
patches, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API
2016-09-16 3:27 ` [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API Viresh Kumar
2016-09-16 7:38 ` Charles Keepax
@ 2016-10-26 12:22 ` Lee Jones
2016-10-26 12:24 ` Lee Jones
1 sibling, 1 reply; 15+ messages in thread
From: Lee Jones @ 2016-10-26 12:22 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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);
> }
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API
2016-10-26 12:22 ` Lee Jones
@ 2016-10-26 12:24 ` Lee Jones
0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2016-10-26 12:24 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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);
> > }
> >
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-09-16 3:26 [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Viresh Kumar
2016-09-16 3:26 ` [PATCH V2 2/3] mfd: wm8994-core: disable regulators before removing them Viresh Kumar
2016-09-16 3:27 ` [PATCH V2 3/3] mfd: wm8994-core: Don't use managed regulator bulk get API Viresh Kumar
@ 2016-09-16 7:37 ` Charles Keepax
2016-10-03 10:56 ` Viresh Kumar
2016-10-26 12:20 ` Lee Jones
4 siblings, 0 replies; 15+ messages in thread
From: Charles Keepax @ 2016-09-16 7:37 UTC (permalink / raw)
To: Viresh Kumar
Cc: Lee Jones, linaro-kernel, Mark Brown, Krzysztof Kozłowski,
patches, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-09-16 3:26 [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Viresh Kumar
` (2 preceding siblings ...)
2016-09-16 7:37 ` [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Charles Keepax
@ 2016-10-03 10:56 ` Viresh Kumar
2016-10-04 14:41 ` Lee Jones
2016-10-26 12:20 ` Lee Jones
4 siblings, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2016-10-03 10:56 UTC (permalink / raw)
To: Lee Jones
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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!!
--
viresh
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-10-03 10:56 ` Viresh Kumar
@ 2016-10-04 14:41 ` Lee Jones
2016-10-05 0:39 ` Viresh Kumar
0 siblings, 1 reply; 15+ messages in thread
From: Lee Jones @ 2016-10-04 14:41 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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!
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-10-04 14:41 ` Lee Jones
@ 2016-10-05 0:39 ` Viresh Kumar
2016-10-05 8:49 ` Lee Jones
0 siblings, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2016-10-05 0:39 UTC (permalink / raw)
To: Lee Jones
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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?
--
viresh
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-10-05 0:39 ` Viresh Kumar
@ 2016-10-05 8:49 ` Lee Jones
2016-10-05 8:55 ` Viresh Kumar
0 siblings, 1 reply; 15+ messages in thread
From: Lee Jones @ 2016-10-05 8:49 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-10-05 8:49 ` Lee Jones
@ 2016-10-05 8:55 ` Viresh Kumar
2016-10-05 9:10 ` Lee Jones
0 siblings, 1 reply; 15+ messages in thread
From: Viresh Kumar @ 2016-10-05 8:55 UTC (permalink / raw)
To: Lee Jones
Cc: linaro-kernel, Mark Brown, Charles Keepax, patches, linux-kernel
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.
--
viresh
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-10-05 8:55 ` Viresh Kumar
@ 2016-10-05 9:10 ` Lee Jones
0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2016-10-05 9:10 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, Mark Brown, Charles Keepax, patches, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily
2016-09-16 3:26 [PATCH V2 1/3] mfd: wm8994-core: don't split lines unnecessarily Viresh Kumar
` (3 preceding siblings ...)
2016-10-03 10:56 ` Viresh Kumar
@ 2016-10-26 12:20 ` Lee Jones
4 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2016-10-26 12:20 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, Mark Brown, Krzysztof Kozłowski,
Charles Keepax, patches, linux-kernel
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[] = {
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread