[PATCH] rtc: ab8500: move device_init_wakeup() call

[PATCH] rtc: ab8500: move device_init_wakeup() call

[Linus Walleij]

commit 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
introduced the dev_pm_set_wake_irq() call to register a wake
IRQ for the AB8500 driver.

However this causes a regression since device_init_wakeup() must be
called *after* dev_pm_set_wake_irq() not *before* it. Before this
patch we get an error message like this during system resume from
sleep:

[  217.585571] PM: noirq resume of devices complete after 0.762 msecs
[  217.585815] ------------[ cut here ]------------
[  217.585845] WARNING: CPU: 0 PID: 200 at ../kernel/irq/manage.c:603 irq_set_irq_wake+0xc4/0xf8
[  217.585845] Unbalanced IRQ 396 wake disable
[  217.585845] Modules linked in:
[  217.585876] CPU: 0 PID: 200 Comm: sh Tainted: G        W       4.6.0-rc1-00010-gc6ade5a1da15-dirty #123
[  217.585876] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[  217.585906] [<c010e81c>] (unwind_backtrace) from [<c010b1b4>] (show_stack+0x10/0x14)
[  217.585937] [<c010b1b4>] (show_stack) from [<c034a150>] (dump_stack+0x8c/0xa0)
[  217.585937] [<c034a150>] (dump_stack) from [<c0121138>] (__warn+0xe8/0x100)
[  217.585937] [<c0121138>] (__warn) from [<c0121188>] (warn_slowpath_fmt+0x38/0x48)
[  217.585967] [<c0121188>] (warn_slowpath_fmt) from [<c0164f84>] (irq_set_irq_wake+0xc4/0xf8)
[  217.585967] [<c0164f84>] (irq_set_irq_wake) from [<c03c3648>] (device_wakeup_disarm_wake_irqs+0x30/0x48)
[  217.585998] [<c03c3648>] (device_wakeup_disarm_wake_irqs) from [<c03c152c>] (dpm_resume_noirq+0x208/0x21c)
[  217.585998] [<c03c152c>] (dpm_resume_noirq) from [<c0160248>] (suspend_devices_and_enter+0x1dc/0x444)
[  217.585998] [<c0160248>] (suspend_devices_and_enter) from [<c01606ac>] (pm_suspend+0x1fc/0x25c)
[  217.585998] [<c01606ac>] (pm_suspend) from [<c015f420>] (state_store+0x68/0xb8)
[  217.586029] [<c015f420>] (state_store) from [<c0266758>] (kernfs_fop_write+0xb8/0x1b4)
[  217.586029] [<c0266758>] (kernfs_fop_write) from [<c0204978>] (__vfs_write+0x1c/0xd8)
[  217.586059] [<c0204978>] (__vfs_write) from [<c02057fc>] (vfs_write+0x90/0x19c)
[  217.586059] [<c02057fc>] (vfs_write) from [<c02067a0>] (SyS_write+0x44/0x9c)
[  217.586059] [<c02067a0>] (SyS_write) from [<c0107740>] (ret_fast_syscall+0x0/0x3c)
[  217.586090] ---[ end trace 78c75240315212f4 ]---

This patch fixes it.

Cc: stable@vger.kernel.org
Fixes: 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/rtc/rtc-ab8500.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index 24a0af650a1b..5e9cb4132eb1 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -482,8 +482,6 @@ static int ab8500_rtc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	device_init_wakeup(&pdev->dev, true);
-
 	rtc = devm_rtc_device_register(&pdev->dev, "ab8500-rtc",
 				(struct rtc_class_ops *)platid->driver_data,
 				THIS_MODULE);
@@ -500,6 +498,8 @@ static int ab8500_rtc_probe(struct platform_device *pdev)
 		return err;
 
 	dev_pm_set_wake_irq(&pdev->dev, irq);
+	device_init_wakeup(&pdev->dev, true);
+
 	platform_set_drvdata(pdev, rtc);
 
 	err = ab8500_sysfs_rtc_register(&pdev->dev);
-- 
2.4.11

Re: [PATCH] rtc: ab8500: move device_init_wakeup() call

[Linus Walleij]

On Thu, Jun 2, 2016 at 3:53 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> commit 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
> introduced the dev_pm_set_wake_irq() call to register a wake
> IRQ for the AB8500 driver.

If this looks like a deja vu it is because I sent it before, then thought my
suspend/resume bugs were due to something else, fixed the other things,
and return to the conclusion that this patch is indeed needed too.

Sorry if it's confusing, please apply this patch.

Yours,
Linus Walleij

Re: [PATCH] rtc: ab8500: move device_init_wakeup() call

[Sudeep Holla]

On 02/06/16 14:53, Linus Walleij wrote:
> commit 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
> introduced the dev_pm_set_wake_irq() call to register a wake
> IRQ for the AB8500 driver.
>
> However this causes a regression since device_init_wakeup() must be
> called *after* dev_pm_set_wake_irq() not *before* it. Before this
> patch we get an error message like this during system resume from
> sleep:
>

I am unable to understand this. Because it's the exact same sequence in
rtc-pl031.c which works fine for me on ARM64 Juno platform. So I am
struggle to understand the linkage of the backtrace to this calling
sequence as I can't see any dependency. Let me know if you have already
figured out where exactly does it go wrong.

Prior to commit 93a6f9168f2f, set_irq_wake was never tested and hence
one possible reason I can think of is set_irq_wake_real may be failing
on suspend but the error value is ignored.

-- 
Regards,
Sudeep

Re: [PATCH] rtc: ab8500: move device_init_wakeup() call

[Ulf Hansson]

On 3 June 2016 at 11:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 02/06/16 14:53, Linus Walleij wrote:
>>
>> commit 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
>> introduced the dev_pm_set_wake_irq() call to register a wake
>> IRQ for the AB8500 driver.
>>
>> However this causes a regression since device_init_wakeup() must be
>> called *after* dev_pm_set_wake_irq() not *before* it. Before this
>> patch we get an error message like this during system resume from
>> sleep:

I think it's actually the opposite. device_init_wakeup() must be
called *before* dev_pm_set_wake_irq(), as otherwise
dev_pm_set_wake_irq() will fail.

Indeed this probably "solves" the problem for you, although only by
hiding it, as there is no error check of the return code from
dev_pm_set_wake_irq().

>>
>
> I am unable to understand this. Because it's the exact same sequence in
> rtc-pl031.c which works fine for me on ARM64 Juno platform. So I am
> struggle to understand the linkage of the backtrace to this calling
> sequence as I can't see any dependency. Let me know if you have already
> figured out where exactly does it go wrong.
>
> Prior to commit 93a6f9168f2f, set_irq_wake was never tested and hence
> one possible reason I can think of is set_irq_wake_real may be failing
> on suspend but the error value is ignored.

Yes, something else is wrong here!

Kind regards
Uffe

[PATCH] rtc: ab8500: move device_init_wakeup() call

[Linus Walleij]

commit 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
introduced the dev_pm_set_wake_irq() call to register a wake
IRQ for the AB8500 driver.

However this causes a regression since device_init_wakeup() must be
called *after* dev_pm_set_wake_irq() not *before* it. Before this
patch we get an error message like this during system resume from
sleep:

[  217.585571] PM: noirq resume of devices complete after 0.762 msecs
[  217.585815] ------------[ cut here ]------------
[  217.585845] WARNING: CPU: 0 PID: 200 at ../kernel/irq/manage.c:603 irq_set_irq_wake+0xc4/0xf8
[  217.585845] Unbalanced IRQ 396 wake disable
[  217.585845] Modules linked in:
[  217.585876] CPU: 0 PID: 200 Comm: sh Tainted: G        W       4.6.0-rc1-00010-gc6ade5a1da15-dirty #123
[  217.585876] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[  217.585906] [<c010e81c>] (unwind_backtrace) from [<c010b1b4>] (show_stack+0x10/0x14)
[  217.585937] [<c010b1b4>] (show_stack) from [<c034a150>] (dump_stack+0x8c/0xa0)
[  217.585937] [<c034a150>] (dump_stack) from [<c0121138>] (__warn+0xe8/0x100)
[  217.585937] [<c0121138>] (__warn) from [<c0121188>] (warn_slowpath_fmt+0x38/0x48)
[  217.585967] [<c0121188>] (warn_slowpath_fmt) from [<c0164f84>] (irq_set_irq_wake+0xc4/0xf8)
[  217.585967] [<c0164f84>] (irq_set_irq_wake) from [<c03c3648>] (device_wakeup_disarm_wake_irqs+0x30/0x48)
[  217.585998] [<c03c3648>] (device_wakeup_disarm_wake_irqs) from [<c03c152c>] (dpm_resume_noirq+0x208/0x21c)
[  217.585998] [<c03c152c>] (dpm_resume_noirq) from [<c0160248>] (suspend_devices_and_enter+0x1dc/0x444)
[  217.585998] [<c0160248>] (suspend_devices_and_enter) from [<c01606ac>] (pm_suspend+0x1fc/0x25c)
[  217.585998] [<c01606ac>] (pm_suspend) from [<c015f420>] (state_store+0x68/0xb8)
[  217.586029] [<c015f420>] (state_store) from [<c0266758>] (kernfs_fop_write+0xb8/0x1b4)
[  217.586029] [<c0266758>] (kernfs_fop_write) from [<c0204978>] (__vfs_write+0x1c/0xd8)
[  217.586059] [<c0204978>] (__vfs_write) from [<c02057fc>] (vfs_write+0x90/0x19c)
[  217.586059] [<c02057fc>] (vfs_write) from [<c02067a0>] (SyS_write+0x44/0x9c)
[  217.586059] [<c02067a0>] (SyS_write) from [<c0107740>] (ret_fast_syscall+0x0/0x3c)
[  217.586090] ---[ end trace 78c75240315212f4 ]---

This patch fixes it.

Cc: stable@vger.kernel.org
Fixes: 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/rtc/rtc-ab8500.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index 24a0af650a1b..5e9cb4132eb1 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -482,8 +482,6 @@ static int ab8500_rtc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	device_init_wakeup(&pdev->dev, true);
-
 	rtc = devm_rtc_device_register(&pdev->dev, "ab8500-rtc",
 				(struct rtc_class_ops *)platid->driver_data,
 				THIS_MODULE);
@@ -500,6 +498,8 @@ static int ab8500_rtc_probe(struct platform_device *pdev)
 		return err;
 
 	dev_pm_set_wake_irq(&pdev->dev, irq);
+	device_init_wakeup(&pdev->dev, true);
+
 	platform_set_drvdata(pdev, rtc);
 
 	err = ab8500_sysfs_rtc_register(&pdev->dev);
-- 
2.4.3

Re: [PATCH] rtc: ab8500: move device_init_wakeup() call

[Linus Walleij]

On Mon, Apr 11, 2016 at 9:54 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

> commit 93a6f9168f2f ("rtc: ab8500: remove misuse of IRQF_NO_SUSPEND flag")
> introduced the dev_pm_set_wake_irq() call to register a wake
> IRQ for the AB8500 driver.
>
> However this causes a regression since device_init_wakeup() must be
> called *after* dev_pm_set_wake_irq() not *before* it. Before this
> patch we get an error message like this during system resume from
> sleep:

Bah it seems this is not the real problem :(

We end up with unbalanced wake disable for some totally
different reason, maybe in the core.

Drop this patch for now.

Yours,
Linus Walleij