[PATCH 1/2] pinctrl: single: Use a separate lockdep class

[PATCH 1/2] pinctrl: single: Use a separate lockdep class

[Sudeep Holla]

The single pinmux controller can be cascaded to the other interrupt
controllers. Hence when propagating wake-up settings to its parent
interrupt controller, there's possiblity of detecting possible recursive
locking and getting lockdep warning.

This patch avoids this false positive by using a separate lockdep class
for this single pinctrl interrupts.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-gpio@vger.kernel.org
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/pinctrl/pinctrl-single.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index ef04b962c3d5..945a7d0f0704 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -255,6 +255,13 @@ static enum pin_config_param pcs_bias[] = {
 };
 
 /*
+ * This lock class tells lockdep that irqchip core that this single
+ * pinctrl can be in a different category than its parents, so it won't
+ * report false recursion.
+ */
+static struct lock_class_key pcs_lock_class;
+
+/*
  * REVISIT: Reads and writes could eventually use regmap or something
  * generic. But at least on omaps, some mux registers are performance
  * critical as they may need to be remuxed every time before and after
@@ -1716,6 +1723,7 @@ static int pcs_irqdomain_map(struct irq_domain *d, unsigned int irq,
 	irq_set_chip_data(irq, pcs_soc);
 	irq_set_chip_and_handler(irq, &pcs->chip,
 				 handle_level_irq);
+	irq_set_lockdep_class(irq, &pcs_lock_class);
 	irq_set_noprobe(irq);
 
 	return 0;
-- 
1.9.1

[PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

From: Sudeep Holla <Sudeep.Holla@arm.com>

The IRQF_NO_SUSPEND flag is used to identify the interrupts that should
be left enabled so as to allow them to work as expected during the
suspend-resume cycle, but doesn't guarantee that it will wake the system
from a suspended state, enable_irq_wake is recommended to be used for
the wakeup.

This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
irq_set_irq_wake instead.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/pinctrl/pinctrl-single.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 945a7d0f0704..a1e32c6b554a 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1622,12 +1622,14 @@ static void pcs_irq_unmask(struct irq_data *d)
  */
 static int pcs_irq_set_wake(struct irq_data *d, unsigned int state)
 {
+	struct pcs_soc_data *pcs_soc = irq_data_get_irq_chip_data(d);
+
 	if (state)
 		pcs_irq_unmask(d);
 	else
 		pcs_irq_mask(d);
 
-	return 0;
+	return irq_set_irq_wake(pcs_soc->irq, state);
 }
 
 /**
@@ -1763,8 +1765,7 @@ static int pcs_irq_init_chained_handler(struct pcs_device *pcs,
 		int res;
 
 		res = request_irq(pcs_soc->irq, pcs_irq_handler,
-				  IRQF_SHARED | IRQF_NO_SUSPEND |
-				  IRQF_NO_THREAD,
+				  IRQF_SHARED | IRQF_NO_THREAD,
 				  name, pcs_soc);
 		if (res) {
 			pcs_soc->irq = -1;
-- 
1.9.1

Re: [PATCH 1/2] pinctrl: single: Use a separate lockdep class

[Linus Walleij]

On Fri, Nov 27, 2015 at 6:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> The single pinmux controller can be cascaded to the other interrupt
> controllers. Hence when propagating wake-up settings to its parent
> interrupt controller, there's possiblity of detecting possible recursive
> locking and getting lockdep warning.
>
> This patch avoids this false positive by using a separate lockdep class
> for this single pinctrl interrupts.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

I need Tony's ACK on this patch before applying.

Is it a regression that needs to go into fixes?

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Linus Walleij]

On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> From: Sudeep Holla <Sudeep.Holla@arm.com>
>
> The IRQF_NO_SUSPEND flag is used to identify the interrupts that should
> be left enabled so as to allow them to work as expected during the
> suspend-resume cycle, but doesn't guarantee that it will wake the system
> from a suspended state, enable_irq_wake is recommended to be used for
> the wakeup.
>
> This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
> irq_set_irq_wake instead.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

I need Tony's ACK on this as well.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: single: Use a separate lockdep class

[Sudeep Holla]

On 01/12/15 14:06, Linus Walleij wrote:
> On Fri, Nov 27, 2015 at 6:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>> The single pinmux controller can be cascaded to the other interrupt
>> controllers. Hence when propagating wake-up settings to its parent
>> interrupt controller, there's possiblity of detecting possible recursive
>> locking and getting lockdep warning.
>>
>> This patch avoids this false positive by using a separate lockdep class
>> for this single pinctrl interrupts.
>>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: linux-gpio@vger.kernel.org
>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> I need Tony's ACK on this patch before applying.
>
> Is it a regression that needs to go into fixes?
>

Not really, only needed by PATCH 2/2 to avoid recursive locking.

-- 
Regards,
Sudeep

Re: [PATCH 1/2] pinctrl: single: Use a separate lockdep class

[Tony Lindgren]

* Sudeep Holla <sudeep.holla@arm.com> [151201 06:10]:
> 
> 
> On 01/12/15 14:06, Linus Walleij wrote:
> >On Fri, Nov 27, 2015 at 6:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> >>The single pinmux controller can be cascaded to the other interrupt
> >>controllers. Hence when propagating wake-up settings to its parent
> >>interrupt controller, there's possiblity of detecting possible recursive
> >>locking and getting lockdep warning.
> >>
> >>This patch avoids this false positive by using a separate lockdep class
> >>for this single pinctrl interrupts.
> >>
> >>Cc: Linus Walleij <linus.walleij@linaro.org>
> >>Cc: linux-gpio@vger.kernel.org
> >>Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> >>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >
> >I need Tony's ACK on this patch before applying.
> >
> >Is it a regression that needs to go into fixes?
> >
> 
> Not really, only needed by PATCH 2/2 to avoid recursive locking.

No problem with this patch, so:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [151201 06:07]:
> On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> > From: Sudeep Holla <Sudeep.Holla@arm.com>
> >
> > The IRQF_NO_SUSPEND flag is used to identify the interrupts that should
> > be left enabled so as to allow them to work as expected during the
> > suspend-resume cycle, but doesn't guarantee that it will wake the system
> > from a suspended state, enable_irq_wake is recommended to be used for
> > the wakeup.
> >
> > This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
> > irq_set_irq_wake instead.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: linux-gpio@vger.kernel.org
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> I need Tony's ACK on this as well.

At least on omaps, this controller is always powered and we never want to
suspend it as it handles wake-up events for all the IO pins. And that
usecase sounds exactly like what you're describing above.

I don't quite follow what your suggested alternative for an interrupt
controller is?

At least we need to have the alternative patched in with this chage before
just removing IRQF_NO_SUSPEND.

The enable_irq_wake is naturally used for the consumer drivers of this
interrupt controller and actually mostly done automatically now with the
dev_pm_set_dedicated_wake_irq.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/03/2015 08:13 PM, Tony Lindgren wrote:
> * Linus Walleij <linus.walleij@linaro.org> [151201 06:07]:
>> On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>> From: Sudeep Holla <Sudeep.Holla@arm.com>
>>>
>>> The IRQF_NO_SUSPEND flag is used to identify the interrupts that should
>>> be left enabled so as to allow them to work as expected during the
>>> suspend-resume cycle, but doesn't guarantee that it will wake the system
>>> from a suspended state, enable_irq_wake is recommended to be used for
>>> the wakeup.
>>>
>>> This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>>> irq_set_irq_wake instead.
>>>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: linux-gpio@vger.kernel.org
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>
>> I need Tony's ACK on this as well.
> 
> At least on omaps, this controller is always powered and we never want to
> suspend it as it handles wake-up events for all the IO pins. And that
> usecase sounds exactly like what you're describing above.
> 
> I don't quite follow what your suggested alternative for an interrupt
> controller is?
> 
> At least we need to have the alternative patched in with this chage before
> just removing IRQF_NO_SUSPEND.
> 
> The enable_irq_wake is naturally used for the consumer drivers of this
> interrupt controller and actually mostly done automatically now with the
> dev_pm_set_dedicated_wake_irq.
> 

I think, this patch should not break our wake-up functionality.
It will just change the moment when pcs_irq_handler() will be called:

before this change:
- suspend_enter()
  ....
  - arch_suspend_enable_irqs();
    - ^ right here

after this change:
- suspend_enter()
  ....
  dpm_resume_noirq()
  - resume_device_irqs()
    ^ here

Correct? And as for me this is more safe.

-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/03/2015 08:13 PM, Tony Lindgren wrote:
> * Linus Walleij <linus.walleij@linaro.org> [151201 06:07]:
>> On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>> From: Sudeep Holla <Sudeep.Holla@arm.com>
>>>
>>> The IRQF_NO_SUSPEND flag is used to identify the interrupts that should
>>> be left enabled so as to allow them to work as expected during the
>>> suspend-resume cycle, but doesn't guarantee that it will wake the system
>>> from a suspended state, enable_irq_wake is recommended to be used for
>>> the wakeup.
>>>
>>> This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>>> irq_set_irq_wake instead.
>>>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: linux-gpio@vger.kernel.org
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>
>> I need Tony's ACK on this as well.
> 
> At least on omaps, this controller is always powered and we never want to
> suspend it as it handles wake-up events for all the IO pins. And that
> usecase sounds exactly like what you're describing above.
> 
> I don't quite follow what your suggested alternative for an interrupt
> controller is?
> 
> At least we need to have the alternative patched in with this chage before
> just removing IRQF_NO_SUSPEND.
> 
> The enable_irq_wake is naturally used for the consumer drivers of this
> interrupt controller and actually mostly done automatically now with the
> dev_pm_set_dedicated_wake_irq.
> 

I think, this patch should not break our wake-up functionality.
It will just change the moment when pcs_irq_handler() will be called:

before this change:
- suspend_enter()
  ....
  - arch_suspend_enable_irqs();
    - ^ right here

after this change:
- suspend_enter()
  ....
  dpm_resume_noirq()
  - resume_device_irqs()
    ^ here

Correct? And as for me this is more safe.

-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

On 03/12/15 18:13, Tony Lindgren wrote:
> * Linus Walleij <linus.walleij@linaro.org> [151201 06:07]:
>> On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>> From: Sudeep Holla <Sudeep.Holla@arm.com>
>>>
>>> The IRQF_NO_SUSPEND flag is used to identify the interrupts that should
>>> be left enabled so as to allow them to work as expected during the
>>> suspend-resume cycle, but doesn't guarantee that it will wake the system
>>> from a suspended state, enable_irq_wake is recommended to be used for
>>> the wakeup.
>>>
>>> This patch removes the use of IRQF_NO_SUSPEND flags replacing it with
>>> irq_set_irq_wake instead.
>>>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: linux-gpio@vger.kernel.org
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>
>> I need Tony's ACK on this as well.
>
> At least on omaps, this controller is always powered and we never want to
> suspend it as it handles wake-up events for all the IO pins. And that
> usecase sounds exactly like what you're describing above.
>

Understood, but I assume this is a generic driver that can be used by
any pinmux.

> I don't quite follow what your suggested alternative for an interrupt
> controller is?
>

Why can't we use enable_irq_wake even for parent/interrupt controller as
they can be considered as parent wakeup irq. I agree the interrupt
controller may not be powered down, but still it's part of wakeup and
the irq core needs to identify that. By just marking IRQF_NO_SUSPEND,
you are saying that you can handle interrupt in the suspend path but not
informing that it's a wakeup interrupt.

With this change, the wakeup handler (including the parent handler) is
called when it's safe as the irq core maintains the state machine.

> At least we need to have the alternative patched in with this chage before
> just removing IRQF_NO_SUSPEND.
>

I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
which ensures it's marked for wakeup.

> The enable_irq_wake is naturally used for the consumer drivers of this
> interrupt controller and actually mostly done automatically now with the
> dev_pm_set_dedicated_wake_irq.
>

Agreed, no doubt on that.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]:
> 
> I think, this patch should not break our wake-up functionality.
> It will just change the moment when pcs_irq_handler() will be called:
> 
> before this change:
> - suspend_enter()
>   ....
>   - arch_suspend_enable_irqs();
>     - ^ right here
> 
> after this change:
> - suspend_enter()
>   ....
>   dpm_resume_noirq()
>   - resume_device_irqs()
>     ^ here
> 
> Correct? And as for me this is more safe.

I think there's more to it though. With both applied, it produces this on
coming back up from suspend:

PM: noirq resume of devices complete after 18.127 msecs
------------[ cut here ]------------
WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc()
Unbalanced IRQ 375 wake disable
Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt
CPU: 0 PID: 123 Comm: bash Tainted: G        W       4.4.0-rc3-dirty #2682
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
<c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c)
[<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8)
[<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40)
[<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc)
[<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c)
[<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4)
[<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc)
[<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8)
[<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8)
[<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20)
[<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50)
[<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc)
[<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8)
[<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154)
[<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94)
[<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c)
---[ end trace 321b51565e161bee ]---

And these both need to be applied together when we have a fix for the above
as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
> On 03/12/15 18:13, Tony Lindgren wrote:
> >At least on omaps, this controller is always powered and we never want to
> >suspend it as it handles wake-up events for all the IO pins. And that
> >usecase sounds exactly like what you're describing above.
> >
> 
> Understood, but I assume this is a generic driver that can be used by
> any pinmux.

Right no question about that, but we need to keep things working. I just
pasted the output to this thread what happens coming back up from suspend.

> >I don't quite follow what your suggested alternative for an interrupt
> >controller is?
> 
> Why can't we use enable_irq_wake even for parent/interrupt controller as
> they can be considered as parent wakeup irq. I agree the interrupt
> controller may not be powered down, but still it's part of wakeup and
> the irq core needs to identify that. By just marking IRQF_NO_SUSPEND,
> you are saying that you can handle interrupt in the suspend path but not
> informing that it's a wakeup interrupt.
> 
> With this change, the wakeup handler (including the parent handler) is
> called when it's safe as the irq core maintains the state machine.

Maybe paste a suggested patch and I can try it. I guess you mean call
that from pinctrl-single.c.

> >At least we need to have the alternative patched in with this chage before
> >just removing IRQF_NO_SUSPEND.
> >
> 
> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
> which ensures it's marked for wakeup.

Hmm well see the error I pasted in this thread, maybe that provides
more clues.

Regards,

Tony

Re: [PATCH 1/2] pinctrl: single: Use a separate lockdep class

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [151203 10:07]:
> * Sudeep Holla <sudeep.holla@arm.com> [151201 06:10]:
> > 
> > 
> > On 01/12/15 14:06, Linus Walleij wrote:
> > >On Fri, Nov 27, 2015 at 6:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > >>The single pinmux controller can be cascaded to the other interrupt
> > >>controllers. Hence when propagating wake-up settings to its parent
> > >>interrupt controller, there's possiblity of detecting possible recursive
> > >>locking and getting lockdep warning.
> > >>
> > >>This patch avoids this false positive by using a separate lockdep class
> > >>for this single pinctrl interrupts.
> > >>
> > >>Cc: Linus Walleij <linus.walleij@linaro.org>
> > >>Cc: linux-gpio@vger.kernel.org
> > >>Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > >>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > >
> > >I need Tony's ACK on this patch before applying.
> > >
> > >Is it a regression that needs to go into fixes?
> > >
> > 
> > Not really, only needed by PATCH 2/2 to avoid recursive locking.
> 
> No problem with this patch, so:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>

Actually this needs to be merged together with 1/2 once the pending
issues are fixed as this will add a lockdep warning with 1/2.

So for now:

Un-Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]:
>>
>> I think, this patch should not break our wake-up functionality.
>> It will just change the moment when pcs_irq_handler() will be called:
>>
>> before this change:
>> - suspend_enter()
>>    ....
>>    - arch_suspend_enable_irqs();
>>      - ^ right here
>>
>> after this change:
>> - suspend_enter()
>>    ....
>>    dpm_resume_noirq()
>>    - resume_device_irqs()
>>      ^ here
>>
>> Correct? And as for me this is more safe.
>
> I think there's more to it though. With both applied, it produces this on
> coming back up from suspend:
>
> PM: noirq resume of devices complete after 18.127 msecs
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc()
> Unbalanced IRQ 375 wake disable
> Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt
> CPU: 0 PID: 123 Comm: bash Tainted: G        W       4.4.0-rc3-dirty #2682
> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
> <c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c)
> [<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8)
> [<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40)
> [<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc)
> [<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c)
> [<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4)
> [<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc)
> [<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8)
> [<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8)
> [<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20)
> [<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50)
> [<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc)
> [<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8)
> [<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154)
> [<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94)
> [<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c)
> ---[ end trace 321b51565e161bee ]---
>
> And these both need to be applied together when we have a fix for the above
> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>

Most probably below diff will fix above issue:

diff --git a/arch/arm/mach-omap2/prm_common.c 
b/arch/arm/mach-omap2/prm_common.c
index 3fc2cbe..69cde67 100644
--- a/arch/arm/mach-omap2/prm_common.c
+++ b/arch/arm/mach-omap2/prm_common.c
@@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct 
omap_prcm_irq_setup *irq_setup)
                 ct->chip.irq_ack = irq_gc_ack_set_bit;
                 ct->chip.irq_mask = irq_gc_mask_clr_bit;
                 ct->chip.irq_unmask = irq_gc_mask_set_bit;
+               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;

                 ct->regs.ack = irq_setup->ack + i * 4;
                 ct->regs.mask = irq_setup->mask + i * 4;


-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]:
>>
>> I think, this patch should not break our wake-up functionality.
>> It will just change the moment when pcs_irq_handler() will be called:
>>
>> before this change:
>> - suspend_enter()
>>    ....
>>    - arch_suspend_enable_irqs();
>>      - ^ right here
>>
>> after this change:
>> - suspend_enter()
>>    ....
>>    dpm_resume_noirq()
>>    - resume_device_irqs()
>>      ^ here
>>
>> Correct? And as for me this is more safe.
>
> I think there's more to it though. With both applied, it produces this on
> coming back up from suspend:
>
> PM: noirq resume of devices complete after 18.127 msecs
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc()
> Unbalanced IRQ 375 wake disable
> Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt
> CPU: 0 PID: 123 Comm: bash Tainted: G        W       4.4.0-rc3-dirty #2682
> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
> <c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c)
> [<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8)
> [<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40)
> [<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc)
> [<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c)
> [<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4)
> [<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc)
> [<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8)
> [<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8)
> [<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20)
> [<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50)
> [<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc)
> [<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8)
> [<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154)
> [<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94)
> [<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c)
> ---[ end trace 321b51565e161bee ]---
>
> And these both need to be applied together when we have a fix for the above
> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>

Most probably below diff will fix above issue:

diff --git a/arch/arm/mach-omap2/prm_common.c 
b/arch/arm/mach-omap2/prm_common.c
index 3fc2cbe..69cde67 100644
--- a/arch/arm/mach-omap2/prm_common.c
+++ b/arch/arm/mach-omap2/prm_common.c
@@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct 
omap_prcm_irq_setup *irq_setup)
                 ct->chip.irq_ack = irq_gc_ack_set_bit;
                 ct->chip.irq_mask = irq_gc_mask_clr_bit;
                 ct->chip.irq_unmask = irq_gc_mask_set_bit;
+               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;

                 ct->regs.ack = irq_setup->ack + i * 4;
                 ct->regs.mask = irq_setup->mask + i * 4;


-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

Hi Grygorii,

On 04/12/15 10:44, Grygorii Strashko wrote:
> On 12/03/2015 11:37 PM, Tony Lindgren wrote:

[...]

>> And these both need to be applied together when we have a fix for the
>> above
>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>>
>
> Most probably below diff will fix above issue:
>
> diff --git a/arch/arm/mach-omap2/prm_common.c
> b/arch/arm/mach-omap2/prm_common.c
> index 3fc2cbe..69cde67 100644
> --- a/arch/arm/mach-omap2/prm_common.c
> +++ b/arch/arm/mach-omap2/prm_common.c
> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
> omap_prcm_irq_setup *irq_setup)
>                  ct->chip.irq_ack = irq_gc_ack_set_bit;
>                  ct->chip.irq_mask = irq_gc_mask_clr_bit;
>                  ct->chip.irq_unmask = irq_gc_mask_set_bit;
> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;

Thanks for testing. In that case without this hunk, we should get error
from pcs_irq_set_wake in the suspend path. No ? May be driver is not
checking the error value and entering suspend.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 12:54 PM, Sudeep Holla wrote:
> Hi Grygorii,
> 
> On 04/12/15 10:44, Grygorii Strashko wrote:
>> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> 
> [...]
> 
>>> And these both need to be applied together when we have a fix for the
>>> above
>>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>>>
>>
>> Most probably below diff will fix above issue:
>>
>> diff --git a/arch/arm/mach-omap2/prm_common.c
>> b/arch/arm/mach-omap2/prm_common.c
>> index 3fc2cbe..69cde67 100644
>> --- a/arch/arm/mach-omap2/prm_common.c
>> +++ b/arch/arm/mach-omap2/prm_common.c
>> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
>> omap_prcm_irq_setup *irq_setup)
>>                  ct->chip.irq_ack = irq_gc_ack_set_bit;
>>                  ct->chip.irq_mask = irq_gc_mask_clr_bit;
>>                  ct->chip.irq_unmask = irq_gc_mask_set_bit;
>> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;
> 
> Thanks for testing. 

Sry, I've not tested it yet - it's just fast assumption :(

In that case without this hunk, we should get error
> from pcs_irq_set_wake in the suspend path. No ? May be driver is not
> checking the error value and entering suspend.
> 

Yep. Noone is checking return result from enable_irq_wake() in suspend path
(see dev_pm_arm_wake_irq()).

Actually, return result of  enable_irq_wake()  is checked only in ~30% of
cases in kernel now :)


-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 12:54 PM, Sudeep Holla wrote:
> Hi Grygorii,
> 
> On 04/12/15 10:44, Grygorii Strashko wrote:
>> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> 
> [...]
> 
>>> And these both need to be applied together when we have a fix for the
>>> above
>>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>>>
>>
>> Most probably below diff will fix above issue:
>>
>> diff --git a/arch/arm/mach-omap2/prm_common.c
>> b/arch/arm/mach-omap2/prm_common.c
>> index 3fc2cbe..69cde67 100644
>> --- a/arch/arm/mach-omap2/prm_common.c
>> +++ b/arch/arm/mach-omap2/prm_common.c
>> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
>> omap_prcm_irq_setup *irq_setup)
>>                  ct->chip.irq_ack = irq_gc_ack_set_bit;
>>                  ct->chip.irq_mask = irq_gc_mask_clr_bit;
>>                  ct->chip.irq_unmask = irq_gc_mask_set_bit;
>> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;
> 
> Thanks for testing. 

Sry, I've not tested it yet - it's just fast assumption :(

In that case without this hunk, we should get error
> from pcs_irq_set_wake in the suspend path. No ? May be driver is not
> checking the error value and entering suspend.
> 

Yep. Noone is checking return result from enable_irq_wake() in suspend path
(see dev_pm_arm_wake_irq()).

Actually, return result of  enable_irq_wake()  is checked only in ~30% of
cases in kernel now :)


-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

On 04/12/15 11:18, Grygorii Strashko wrote:
> On 12/04/2015 12:54 PM, Sudeep Holla wrote:
>> Hi Grygorii,
>>
>> On 04/12/15 10:44, Grygorii Strashko wrote:
>>> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
>>
>> [...]
>>
>>>> And these both need to be applied together when we have a fix for the
>>>> above
>>>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>>>>
>>>
>>> Most probably below diff will fix above issue:
>>>
>>> diff --git a/arch/arm/mach-omap2/prm_common.c
>>> b/arch/arm/mach-omap2/prm_common.c
>>> index 3fc2cbe..69cde67 100644
>>> --- a/arch/arm/mach-omap2/prm_common.c
>>> +++ b/arch/arm/mach-omap2/prm_common.c
>>> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
>>> omap_prcm_irq_setup *irq_setup)
>>>                   ct->chip.irq_ack = irq_gc_ack_set_bit;
>>>                   ct->chip.irq_mask = irq_gc_mask_clr_bit;
>>>                   ct->chip.irq_unmask = irq_gc_mask_set_bit;
>>> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;
>>
>> Thanks for testing.
>
> Sry, I've not tested it yet - it's just fast assumption :(
>

OK, no worries.

>> In that case without this hunk, we should get error
>> from pcs_irq_set_wake in the suspend path. No ? May be driver is not
>> checking the error value and entering suspend.
>>
>
> Yep. Noone is checking return result from enable_irq_wake() in suspend path
> (see dev_pm_arm_wake_irq()).
>

True, but one possible reason for the warning Tony posted.

> Actually, return result of  enable_irq_wake()  is checked only in ~30% of
> cases in kernel now :)
>

That's bad, but I admit that even I failed to add check in some of the
patches I posted earlier.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [151204 02:45]:
> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> >* Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]:
> >>
> >>I think, this patch should not break our wake-up functionality.
> >>It will just change the moment when pcs_irq_handler() will be called:
> >>
> >>before this change:
> >>- suspend_enter()
> >>   ....
> >>   - arch_suspend_enable_irqs();
> >>     - ^ right here
> >>
> >>after this change:
> >>- suspend_enter()
> >>   ....
> >>   dpm_resume_noirq()
> >>   - resume_device_irqs()
> >>     ^ here
> >>
> >>Correct? And as for me this is more safe.
> >
> >I think there's more to it though. With both applied, it produces this on
> >coming back up from suspend:
> >
> >PM: noirq resume of devices complete after 18.127 msecs
> >------------[ cut here ]------------
> >WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc()
> >Unbalanced IRQ 375 wake disable
> >Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt
> >CPU: 0 PID: 123 Comm: bash Tainted: G        W       4.4.0-rc3-dirty #2682
> >Hardware name: Generic OMAP36xx (Flattened Device Tree)
> >[<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
> ><c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c)
> >[<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8)
> >[<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40)
> >[<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc)
> >[<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c)
> >[<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4)
> >[<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc)
> >[<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8)
> >[<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8)
> >[<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20)
> >[<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50)
> >[<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc)
> >[<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8)
> >[<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154)
> >[<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94)
> >[<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c)
> >---[ end trace 321b51565e161bee ]---
> >
> >And these both need to be applied together when we have a fix for the above
> >as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
> >
> 
> Most probably below diff will fix above issue:
> 
> diff --git a/arch/arm/mach-omap2/prm_common.c
> b/arch/arm/mach-omap2/prm_common.c
> index 3fc2cbe..69cde67 100644
> --- a/arch/arm/mach-omap2/prm_common.c
> +++ b/arch/arm/mach-omap2/prm_common.c
> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
> omap_prcm_irq_setup *irq_setup)
>                 ct->chip.irq_ack = irq_gc_ack_set_bit;
>                 ct->chip.irq_mask = irq_gc_mask_clr_bit;
>                 ct->chip.irq_unmask = irq_gc_mask_set_bit;
> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;
> 
>                 ct->regs.ack = irq_setup->ack + i * 4;
>                 ct->regs.mask = irq_setup->mask + i * 4;
> 
> 

That fixes the warning on resume, but adds a new one during init:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at kernel/irq/pm.c:51 irq_pm_install_action+0x9c/0xec()
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc3-00001-g6a5e5ec #2694
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
[<c0014084>] (show_stack) from [<c03492f0>] (dump_stack+0x84/0x9c)
[<c03492f0>] (dump_stack) from [<c003ca34>] (warn_slowpath_common+0x7c/0xb8)
[<c003ca34>] (warn_slowpath_common) from [<c003cb0c>] (warn_slowpath_null+0x1c/0x24)
[<c003cb0c>] (warn_slowpath_null) from [<c00a27d8>] (irq_pm_install_action+0x9c/0xec)
[<c00a27d8>] (irq_pm_install_action) from [<c009ccb0>] (__setup_irq+0x434/0x5e0)
[<c009ccb0>] (__setup_irq) from [<c009cfb0>] (request_threaded_irq+0xc4/0x15c)
[<c009cfb0>] (request_threaded_irq) from [<c08c25e8>] (omap3_pm_init+0x10c/0x400)
[<c08c25e8>] (omap3_pm_init) from [<c08bc66c>] (omap3_init_late+0xc/0x14)
[<c08bc66c>] (omap3_init_late) from [<c08b5820>] (init_machine_late+0x1c/0x90)
[<c08b5820>] (init_machine_late) from [<c0009804>] (do_one_initcall+0x80/0x1e0)
[<c0009804>] (do_one_initcall) from [<c08b2ec4>] (kernel_init_freeable+0x218/0x2e8)
[<c08b2ec4>] (kernel_init_freeable) from [<c064584c>] (kernel_init+0x8/0xec)
[<c064584c>] (kernel_init) from [<c000f7f0>] (ret_from_fork+0x14/0x24)
---[ end trace 81093452bf564522 ]---

And then there's still at least one problem remaining with the original patch
where togging the parent irq in pin specific irq pcs_irq_set_wake does not
make sense. Will comment on that separately.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [151203 13:41]:
> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
> > 
> > I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
> > which ensures it's marked for wakeup.
> 
> Hmm well see the error I pasted in this thread, maybe that provides
> more clues.

The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
separately, not for the whole controller.

I think all that can be left out with the snipped from Grygorii, and maybe
also the lock_class_key changes.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

On 04/12/15 15:40, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [151203 13:41]:
>> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
>>>
>>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
>>> which ensures it's marked for wakeup.
>>
>> Hmm well see the error I pasted in this thread, maybe that provides
>> more clues.
>
> The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
> look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
> separately, not for the whole controller.
>

OK, my understanding was that this driver supports multiple single
pinmux with one main irq `pcs_soc->irq`. Hence I added the wakeup on
that irq. I now think that understand is wrong.

> I think all that can be left out with the snipped from Grygorii, and maybe
> also the lock_class_key changes.

OK

-- 
Regards,
Sudeep

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 05:35 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [151204 02:45]:
>> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
>>> * Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]:
>>>>
>>>> I think, this patch should not break our wake-up functionality.
>>>> It will just change the moment when pcs_irq_handler() will be called:
>>>>
>>>> before this change:
>>>> - suspend_enter()
>>>>    ....
>>>>    - arch_suspend_enable_irqs();
>>>>      - ^ right here
>>>>
>>>> after this change:
>>>> - suspend_enter()
>>>>    ....
>>>>    dpm_resume_noirq()
>>>>    - resume_device_irqs()
>>>>      ^ here
>>>>
>>>> Correct? And as for me this is more safe.
>>>
>>> I think there's more to it though. With both applied, it produces this on
>>> coming back up from suspend:
>>>
>>> PM: noirq resume of devices complete after 18.127 msecs
>>> ------------[ cut here ]------------
>>> WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc()
>>> Unbalanced IRQ 375 wake disable
>>> Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt
>>> CPU: 0 PID: 123 Comm: bash Tainted: G        W       4.4.0-rc3-dirty #2682
>>> Hardware name: Generic OMAP36xx (Flattened Device Tree)
>>> [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
>>> <c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c)
>>> [<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8)
>>> [<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40)
>>> [<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc)
>>> [<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c)
>>> [<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4)
>>> [<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc)
>>> [<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8)
>>> [<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8)
>>> [<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20)
>>> [<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50)
>>> [<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc)
>>> [<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8)
>>> [<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154)
>>> [<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94)
>>> [<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c)
>>> ---[ end trace 321b51565e161bee ]---
>>>
>>> And these both need to be applied together when we have a fix for the above
>>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>>>
>>
>> Most probably below diff will fix above issue:
>>
>> diff --git a/arch/arm/mach-omap2/prm_common.c
>> b/arch/arm/mach-omap2/prm_common.c
>> index 3fc2cbe..69cde67 100644
>> --- a/arch/arm/mach-omap2/prm_common.c
>> +++ b/arch/arm/mach-omap2/prm_common.c
>> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
>> omap_prcm_irq_setup *irq_setup)
>>                  ct->chip.irq_ack = irq_gc_ack_set_bit;
>>                  ct->chip.irq_mask = irq_gc_mask_clr_bit;
>>                  ct->chip.irq_unmask = irq_gc_mask_set_bit;
>> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;
>>
>>                  ct->regs.ack = irq_setup->ack + i * 4;
>>                  ct->regs.mask = irq_setup->mask + i * 4;
>>
>>
> 
> That fixes the warning on resume, but adds a new one during init:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at kernel/irq/pm.c:51 irq_pm_install_action+0x9c/0xec()
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc3-00001-g6a5e5ec #2694
> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
> [<c0014084>] (show_stack) from [<c03492f0>] (dump_stack+0x84/0x9c)
> [<c03492f0>] (dump_stack) from [<c003ca34>] (warn_slowpath_common+0x7c/0xb8)
> [<c003ca34>] (warn_slowpath_common) from [<c003cb0c>] (warn_slowpath_null+0x1c/0x24)
> [<c003cb0c>] (warn_slowpath_null) from [<c00a27d8>] (irq_pm_install_action+0x9c/0xec)
> [<c00a27d8>] (irq_pm_install_action) from [<c009ccb0>] (__setup_irq+0x434/0x5e0)
> [<c009ccb0>] (__setup_irq) from [<c009cfb0>] (request_threaded_irq+0xc4/0x15c)
> [<c009cfb0>] (request_threaded_irq) from [<c08c25e8>] (omap3_pm_init+0x10c/0x400)
> [<c08c25e8>] (omap3_pm_init) from [<c08bc66c>] (omap3_init_late+0xc/0x14)
> [<c08bc66c>] (omap3_init_late) from [<c08b5820>] (init_machine_late+0x1c/0x90)
> [<c08b5820>] (init_machine_late) from [<c0009804>] (do_one_initcall+0x80/0x1e0)
> [<c0009804>] (do_one_initcall) from [<c08b2ec4>] (kernel_init_freeable+0x218/0x2e8)
> [<c08b2ec4>] (kernel_init_freeable) from [<c064584c>] (kernel_init+0x8/0xec)
> [<c064584c>] (kernel_init) from [<c000f7f0>] (ret_from_fork+0x14/0x24)
> ---[ end trace 81093452bf564522 ]---
> 

Sorry, I can't test it right now :(
Potential fix below:
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 2dbd378..4e56fd9 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -481,7 +481,7 @@ int __init omap3_pm_init(void)
 
        /* IO interrupt is shared with mux code */
        ret = request_irq(omap_prcm_event_to_irq("io"),
-               _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io",
+               _prcm_int_handle_io, IRQF_SHARED, "pm_io",
                omap3_pm_init);
        enable_irq(omap_prcm_event_to_irq("io"));
 
@@ -489,6 +489,7 @@ int __init omap3_pm_init(void)
                pr_err("pm: Failed to request pm_io irq\n");
                goto err2;
        }
+       enable_irq_wake(omap_prcm_event_to_irq("io"));
 
        ret = pwrdm_for_each(pwrdms_setup, NULL);
        if (ret) {



-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 05:35 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [151204 02:45]:
>> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
>>> * Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]:
>>>>
>>>> I think, this patch should not break our wake-up functionality.
>>>> It will just change the moment when pcs_irq_handler() will be called:
>>>>
>>>> before this change:
>>>> - suspend_enter()
>>>>    ....
>>>>    - arch_suspend_enable_irqs();
>>>>      - ^ right here
>>>>
>>>> after this change:
>>>> - suspend_enter()
>>>>    ....
>>>>    dpm_resume_noirq()
>>>>    - resume_device_irqs()
>>>>      ^ here
>>>>
>>>> Correct? And as for me this is more safe.
>>>
>>> I think there's more to it though. With both applied, it produces this on
>>> coming back up from suspend:
>>>
>>> PM: noirq resume of devices complete after 18.127 msecs
>>> ------------[ cut here ]------------
>>> WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc()
>>> Unbalanced IRQ 375 wake disable
>>> Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt
>>> CPU: 0 PID: 123 Comm: bash Tainted: G        W       4.4.0-rc3-dirty #2682
>>> Hardware name: Generic OMAP36xx (Flattened Device Tree)
>>> [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
>>> <c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c)
>>> [<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8)
>>> [<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40)
>>> [<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc)
>>> [<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c)
>>> [<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4)
>>> [<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc)
>>> [<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8)
>>> [<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8)
>>> [<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20)
>>> [<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50)
>>> [<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc)
>>> [<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8)
>>> [<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154)
>>> [<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94)
>>> [<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c)
>>> ---[ end trace 321b51565e161bee ]---
>>>
>>> And these both need to be applied together when we have a fix for the above
>>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
>>>
>>
>> Most probably below diff will fix above issue:
>>
>> diff --git a/arch/arm/mach-omap2/prm_common.c
>> b/arch/arm/mach-omap2/prm_common.c
>> index 3fc2cbe..69cde67 100644
>> --- a/arch/arm/mach-omap2/prm_common.c
>> +++ b/arch/arm/mach-omap2/prm_common.c
>> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
>> omap_prcm_irq_setup *irq_setup)
>>                  ct->chip.irq_ack = irq_gc_ack_set_bit;
>>                  ct->chip.irq_mask = irq_gc_mask_clr_bit;
>>                  ct->chip.irq_unmask = irq_gc_mask_set_bit;
>> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;
>>
>>                  ct->regs.ack = irq_setup->ack + i * 4;
>>                  ct->regs.mask = irq_setup->mask + i * 4;
>>
>>
> 
> That fixes the warning on resume, but adds a new one during init:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at kernel/irq/pm.c:51 irq_pm_install_action+0x9c/0xec()
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc3-00001-g6a5e5ec #2694
> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
> [<c0014084>] (show_stack) from [<c03492f0>] (dump_stack+0x84/0x9c)
> [<c03492f0>] (dump_stack) from [<c003ca34>] (warn_slowpath_common+0x7c/0xb8)
> [<c003ca34>] (warn_slowpath_common) from [<c003cb0c>] (warn_slowpath_null+0x1c/0x24)
> [<c003cb0c>] (warn_slowpath_null) from [<c00a27d8>] (irq_pm_install_action+0x9c/0xec)
> [<c00a27d8>] (irq_pm_install_action) from [<c009ccb0>] (__setup_irq+0x434/0x5e0)
> [<c009ccb0>] (__setup_irq) from [<c009cfb0>] (request_threaded_irq+0xc4/0x15c)
> [<c009cfb0>] (request_threaded_irq) from [<c08c25e8>] (omap3_pm_init+0x10c/0x400)
> [<c08c25e8>] (omap3_pm_init) from [<c08bc66c>] (omap3_init_late+0xc/0x14)
> [<c08bc66c>] (omap3_init_late) from [<c08b5820>] (init_machine_late+0x1c/0x90)
> [<c08b5820>] (init_machine_late) from [<c0009804>] (do_one_initcall+0x80/0x1e0)
> [<c0009804>] (do_one_initcall) from [<c08b2ec4>] (kernel_init_freeable+0x218/0x2e8)
> [<c08b2ec4>] (kernel_init_freeable) from [<c064584c>] (kernel_init+0x8/0xec)
> [<c064584c>] (kernel_init) from [<c000f7f0>] (ret_from_fork+0x14/0x24)
> ---[ end trace 81093452bf564522 ]---
> 

Sorry, I can't test it right now :(
Potential fix below:
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 2dbd378..4e56fd9 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -481,7 +481,7 @@ int __init omap3_pm_init(void)
 
        /* IO interrupt is shared with mux code */
        ret = request_irq(omap_prcm_event_to_irq("io"),
-               _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io",
+               _prcm_int_handle_io, IRQF_SHARED, "pm_io",
                omap3_pm_init);
        enable_irq(omap_prcm_event_to_irq("io"));
 
@@ -489,6 +489,7 @@ int __init omap3_pm_init(void)
                pr_err("pm: Failed to request pm_io irq\n");
                goto err2;
        }
+       enable_irq_wake(omap_prcm_event_to_irq("io"));
 
        ret = pwrdm_for_each(pwrdms_setup, NULL);
        if (ret) {



-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

On 04/12/15 15:59, Grygorii Strashko wrote:
>
> Sorry, I can't test it right now :(
> Potential fix below:

I had posted similar patch a while ago which Tony rejected.
I might have made a mistake of not putting them together, though they
were part of the same series[1], patch 12 and 16

> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 2dbd378..4e56fd9 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -481,7 +481,7 @@ int __init omap3_pm_init(void)
>
>          /* IO interrupt is shared with mux code */
>          ret = request_irq(omap_prcm_event_to_irq("io"),
> -               _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io",
> +               _prcm_int_handle_io, IRQF_SHARED, "pm_io",
>                  omap3_pm_init);
>          enable_irq(omap_prcm_event_to_irq("io"));
>
> @@ -489,6 +489,7 @@ int __init omap3_pm_init(void)
>                  pr_err("pm: Failed to request pm_io irq\n");
>                  goto err2;
>          }
> +       enable_irq_wake(omap_prcm_event_to_irq("io"));
>
>          ret = pwrdm_for_each(pwrdms_setup, NULL);
>          if (ret) {
>
>
>

[1] http://lkml.iu.edu/hypermail/linux/kernel/1509.2/03937.html
-- 
Regards,
Sudeep

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

Hi Tony,

On 04/12/15 15:40, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [151203 13:41]:
>> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
>>>
>>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
>>> which ensures it's marked for wakeup.
>>
>> Hmm well see the error I pasted in this thread, maybe that provides
>> more clues.
>
> The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
> look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
> separately, not for the whole controller.
>

After thinking more about it we need some way to tell IRQ core that
pcs_soc->irq is wakeup capable. Is that going to happen automatically
via dev_pm_set_dedicated_wake_irq as you mentioned earlier ?

> I think all that can be left out with the snipped from Grygorii, and maybe
> also the lock_class_key changes.
>

If we not calling irq_set_irq_wake(pcs_soc->irq) in pcs_irq_set_wake, do
you see possibility of lockdep recursion in any other paths.

Otherwise we don't need this if we remove irq_set_irq_wake(pcs_soc->irq)
from pcs_irq_set_wake

-- 
Regards,
Sudeep

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 05:44 PM, Sudeep Holla wrote:
> 
> 
> On 04/12/15 15:40, Tony Lindgren wrote:
>> * Tony Lindgren <tony@atomide.com> [151203 13:41]:
>>> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
>>>>
>>>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
>>>> which ensures it's marked for wakeup.
>>>
>>> Hmm well see the error I pasted in this thread, maybe that provides
>>> more clues.
>>
>> The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
>> look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
>> separately, not for the whole controller.
>>
> 
> OK, my understanding was that this driver supports multiple single
> pinmux with one main irq `pcs_soc->irq`. Hence I added the wakeup on
> that irq. I now think that understand is wrong.
> 

With this change, PCS parent IRQ will be marked as wake up source as many
times as many pins were requested as wake up IRQs (protected by counter).
Most of all GPIO IRQ chips work this way.
Of course, if we will look on pinctrl-single.c from only OMAP point of view
then Prent IRQ can be marked as wake up source from probe only once.
But, since this driver expected to be generic - this patch is more correct,
because other HW may require to perform some real HW re-configuration to
enable/disable wake up capabilities for Parent IRQ in Parent IRQ controller.

Any way, in my opinion, it's right and more safe to manage all wakeup IRQs
through IRQ PM core and Device wakeirq framework. And this patch should just
go together with platform changes and not alone.

-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 05:44 PM, Sudeep Holla wrote:
> 
> 
> On 04/12/15 15:40, Tony Lindgren wrote:
>> * Tony Lindgren <tony@atomide.com> [151203 13:41]:
>>> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
>>>>
>>>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
>>>> which ensures it's marked for wakeup.
>>>
>>> Hmm well see the error I pasted in this thread, maybe that provides
>>> more clues.
>>
>> The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
>> look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
>> separately, not for the whole controller.
>>
> 
> OK, my understanding was that this driver supports multiple single
> pinmux with one main irq `pcs_soc->irq`. Hence I added the wakeup on
> that irq. I now think that understand is wrong.
> 

With this change, PCS parent IRQ will be marked as wake up source as many
times as many pins were requested as wake up IRQs (protected by counter).
Most of all GPIO IRQ chips work this way.
Of course, if we will look on pinctrl-single.c from only OMAP point of view
then Prent IRQ can be marked as wake up source from probe only once.
But, since this driver expected to be generic - this patch is more correct,
because other HW may require to perform some real HW re-configuration to
enable/disable wake up capabilities for Parent IRQ in Parent IRQ controller.

Any way, in my opinion, it's right and more safe to manage all wakeup IRQs
through IRQ PM core and Device wakeirq framework. And this patch should just
go together with platform changes and not alone.

-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Sudeep Holla]

On 04/12/15 16:19, Grygorii Strashko wrote:
> On 12/04/2015 05:44 PM, Sudeep Holla wrote:
>>
>>
>> On 04/12/15 15:40, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [151203 13:41]:
>>>> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
>>>>>
>>>>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
>>>>> which ensures it's marked for wakeup.
>>>>
>>>> Hmm well see the error I pasted in this thread, maybe that provides
>>>> more clues.
>>>
>>> The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
>>> look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
>>> separately, not for the whole controller.
>>>
>>
>> OK, my understanding was that this driver supports multiple single
>> pinmux with one main irq `pcs_soc->irq`. Hence I added the wakeup on
>> that irq. I now think that understand is wrong.
>>
>
> With this change, PCS parent IRQ will be marked as wake up source as many
> times as many pins were requested as wake up IRQs (protected by counter).
> Most of all GPIO IRQ chips work this way.
> Of course, if we will look on pinctrl-single.c from only OMAP point of view
> then Prent IRQ can be marked as wake up source from probe only once.
> But, since this driver expected to be generic - this patch is more correct,
> because other HW may require to perform some real HW re-configuration to
> enable/disable wake up capabilities for Parent IRQ in Parent IRQ controller.
>

Thanks for the detailed explanation. I was bit confused if my
understanding is correct or not.

> Any way, in my opinion, it's right and more safe to manage all wakeup IRQs
> through IRQ PM core and Device wakeirq framework. And this patch should just
> go together with platform changes and not alone.
>

Agreed, since I don't have platform to test, I will leave it you guys to
pick up these patches when ready and with any changes if required.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 06:11 PM, Sudeep Holla wrote:
> 
> 
> On 04/12/15 15:59, Grygorii Strashko wrote:
>>
>> Sorry, I can't test it right now :(
>> Potential fix below:
> 
> I had posted similar patch a while ago which Tony rejected.
> I might have made a mistake of not putting them together, though they
> were part of the same series[1], patch 12 and 16

True. I've remembered that I saw smth. like this, but I was not able to find it :(


-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Grygorii Strashko]

On 12/04/2015 06:11 PM, Sudeep Holla wrote:
> 
> 
> On 04/12/15 15:59, Grygorii Strashko wrote:
>>
>> Sorry, I can't test it right now :(
>> Potential fix below:
> 
> I had posted similar patch a while ago which Tony rejected.
> I might have made a mistake of not putting them together, though they
> were part of the same series[1], patch 12 and 16

True. I've remembered that I saw smth. like this, but I was not able to find it :(


-- 
regards,
-grygorii

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Sudeep Holla <sudeep.holla@arm.com> [151204 08:27]:
> 
> 
> On 04/12/15 16:19, Grygorii Strashko wrote:
> >On 12/04/2015 05:44 PM, Sudeep Holla wrote:
> >>
> >>
> >>On 04/12/15 15:40, Tony Lindgren wrote:
> >>>* Tony Lindgren <tony@atomide.com> [151203 13:41]:
> >>>>* Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
> >>>>>
> >>>>>I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
> >>>>>which ensures it's marked for wakeup.
> >>>>
> >>>>Hmm well see the error I pasted in this thread, maybe that provides
> >>>>more clues.
> >>>
> >>>The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
> >>>look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
> >>>separately, not for the whole controller.
> >>>
> >>
> >>OK, my understanding was that this driver supports multiple single
> >>pinmux with one main irq `pcs_soc->irq`. Hence I added the wakeup on
> >>that irq. I now think that understand is wrong.
> >>
> >
> >With this change, PCS parent IRQ will be marked as wake up source as many
> >times as many pins were requested as wake up IRQs (protected by counter).
> >Most of all GPIO IRQ chips work this way.
> >Of course, if we will look on pinctrl-single.c from only OMAP point of view
> >then Prent IRQ can be marked as wake up source from probe only once.
> >But, since this driver expected to be generic - this patch is more correct,
> >because other HW may require to perform some real HW re-configuration to
> >enable/disable wake up capabilities for Parent IRQ in Parent IRQ controller.
> >
> 
> Thanks for the detailed explanation. I was bit confused if my
> understanding is correct or not.
> 
> >Any way, in my opinion, it's right and more safe to manage all wakeup IRQs
> >through IRQ PM core and Device wakeirq framework. And this patch should just
> >go together with platform changes and not alone.

OK yeah if it's a counter then it makes sense to me.

> Agreed, since I don't have platform to test, I will leave it you guys to
> pick up these patches when ready and with any changes if required.

Yeah probably best that Grygorii tries to sort it out :)

Regards,

Tony

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [151204 08:31]:
> On 12/04/2015 06:11 PM, Sudeep Holla wrote:
> > 
> > 
> > On 04/12/15 15:59, Grygorii Strashko wrote:
> >>
> >> Sorry, I can't test it right now :(
> >> Potential fix below:
> > 
> > I had posted similar patch a while ago which Tony rejected.
> > I might have made a mistake of not putting them together, though they
> > were part of the same series[1], patch 12 and 16
> 
> True. I've remembered that I saw smth. like this, but I was not able to find it :(

Just tried it and applying that makes the wake-up interrupts not work at all.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [151204 08:00]:
> On 12/04/2015 05:35 PM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.strashko@ti.com> [151204 02:45]:
> >> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> >>> * Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]:
> >>>>
> >>>> I think, this patch should not break our wake-up functionality.
> >>>> It will just change the moment when pcs_irq_handler() will be called:
> >>>>
> >>>> before this change:
> >>>> - suspend_enter()
> >>>>    ....
> >>>>    - arch_suspend_enable_irqs();
> >>>>      - ^ right here
> >>>>
> >>>> after this change:
> >>>> - suspend_enter()
> >>>>    ....
> >>>>    dpm_resume_noirq()
> >>>>    - resume_device_irqs()
> >>>>      ^ here
> >>>>
> >>>> Correct? And as for me this is more safe.
> >>>
> >>> I think there's more to it though. With both applied, it produces this on
> >>> coming back up from suspend:
> >>>
> >>> PM: noirq resume of devices complete after 18.127 msecs
> >>> ------------[ cut here ]------------
> >>> WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc()
> >>> Unbalanced IRQ 375 wake disable
> >>> Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt
> >>> CPU: 0 PID: 123 Comm: bash Tainted: G        W       4.4.0-rc3-dirty #2682
> >>> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> >>> [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
> >>> <c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c)
> >>> [<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8)
> >>> [<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40)
> >>> [<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc)
> >>> [<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c)
> >>> [<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4)
> >>> [<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc)
> >>> [<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8)
> >>> [<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8)
> >>> [<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20)
> >>> [<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50)
> >>> [<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc)
> >>> [<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8)
> >>> [<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154)
> >>> [<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94)
> >>> [<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c)
> >>> ---[ end trace 321b51565e161bee ]---
> >>>
> >>> And these both need to be applied together when we have a fix for the above
> >>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2.
> >>>
> >>
> >> Most probably below diff will fix above issue:
> >>
> >> diff --git a/arch/arm/mach-omap2/prm_common.c
> >> b/arch/arm/mach-omap2/prm_common.c
> >> index 3fc2cbe..69cde67 100644
> >> --- a/arch/arm/mach-omap2/prm_common.c
> >> +++ b/arch/arm/mach-omap2/prm_common.c
> >> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct
> >> omap_prcm_irq_setup *irq_setup)
> >>                  ct->chip.irq_ack = irq_gc_ack_set_bit;
> >>                  ct->chip.irq_mask = irq_gc_mask_clr_bit;
> >>                  ct->chip.irq_unmask = irq_gc_mask_set_bit;
> >> +               ct->chip.flags = IRQCHIP_SKIP_SET_WAKE;
> >>
> >>                  ct->regs.ack = irq_setup->ack + i * 4;
> >>                  ct->regs.mask = irq_setup->mask + i * 4;
> >>
> >>
> > 
> > That fixes the warning on resume, but adds a new one during init:
> > 
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 1 at kernel/irq/pm.c:51 irq_pm_install_action+0x9c/0xec()
> > Modules linked in:
> > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc3-00001-g6a5e5ec #2694
> > Hardware name: Generic OMAP36xx (Flattened Device Tree)
> > [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14)
> > [<c0014084>] (show_stack) from [<c03492f0>] (dump_stack+0x84/0x9c)
> > [<c03492f0>] (dump_stack) from [<c003ca34>] (warn_slowpath_common+0x7c/0xb8)
> > [<c003ca34>] (warn_slowpath_common) from [<c003cb0c>] (warn_slowpath_null+0x1c/0x24)
> > [<c003cb0c>] (warn_slowpath_null) from [<c00a27d8>] (irq_pm_install_action+0x9c/0xec)
> > [<c00a27d8>] (irq_pm_install_action) from [<c009ccb0>] (__setup_irq+0x434/0x5e0)
> > [<c009ccb0>] (__setup_irq) from [<c009cfb0>] (request_threaded_irq+0xc4/0x15c)
> > [<c009cfb0>] (request_threaded_irq) from [<c08c25e8>] (omap3_pm_init+0x10c/0x400)
> > [<c08c25e8>] (omap3_pm_init) from [<c08bc66c>] (omap3_init_late+0xc/0x14)
> > [<c08bc66c>] (omap3_init_late) from [<c08b5820>] (init_machine_late+0x1c/0x90)
> > [<c08b5820>] (init_machine_late) from [<c0009804>] (do_one_initcall+0x80/0x1e0)
> > [<c0009804>] (do_one_initcall) from [<c08b2ec4>] (kernel_init_freeable+0x218/0x2e8)
> > [<c08b2ec4>] (kernel_init_freeable) from [<c064584c>] (kernel_init+0x8/0xec)
> > [<c064584c>] (kernel_init) from [<c000f7f0>] (ret_from_fork+0x14/0x24)
> > ---[ end trace 81093452bf564522 ]---
> > 
> 
> Sorry, I can't test it right now :(
> Potential fix below:
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 2dbd378..4e56fd9 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -481,7 +481,7 @@ int __init omap3_pm_init(void)
>  
>         /* IO interrupt is shared with mux code */
>         ret = request_irq(omap_prcm_event_to_irq("io"),
> -               _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io",
> +               _prcm_int_handle_io, IRQF_SHARED, "pm_io",
>                 omap3_pm_init);
>         enable_irq(omap_prcm_event_to_irq("io"));
>  
> @@ -489,6 +489,7 @@ int __init omap3_pm_init(void)
>                 pr_err("pm: Failed to request pm_io irq\n");
>                 goto err2;
>         }
> +       enable_irq_wake(omap_prcm_event_to_irq("io"));
>  
>         ret = pwrdm_for_each(pwrdms_setup, NULL);
>         if (ret) {

OK probably best that you take a look at it when you have a chance as
you've already spent some time on it.

Regards,

Tony

Re: [PATCH 2/2] pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag

[Tony Lindgren]

* Sudeep Holla <sudeep.holla@arm.com> [151204 08:16]:
> Hi Tony,
> 
> On 04/12/15 15:40, Tony Lindgren wrote:
> >* Tony Lindgren <tony@atomide.com> [151203 13:41]:
> >>* Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]:
> >>>
> >>>I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake
> >>>which ensures it's marked for wakeup.
> >>
> >>Hmm well see the error I pasted in this thread, maybe that provides
> >>more clues.
> >
> >The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not
> >look right to me as pcs_irq_set_wake toggles the irq_wake for each pin
> >separately, not for the whole controller.
> >
> 
> After thinking more about it we need some way to tell IRQ core that
> pcs_soc->irq is wakeup capable. Is that going to happen automatically
> via dev_pm_set_dedicated_wake_irq as you mentioned earlier ?
> 
> >I think all that can be left out with the snipped from Grygorii, and maybe
> >also the lock_class_key changes.
> >
> 
> If we not calling irq_set_irq_wake(pcs_soc->irq) in pcs_irq_set_wake, do
> you see possibility of lockdep recursion in any other paths.
> 
> Otherwise we don't need this if we remove irq_set_irq_wake(pcs_soc->irq)
> from pcs_irq_set_wake

I think Grygorii is right here and this is correct as it's a counter
once the other issues are sorted out and we have figured out what all
needs to be patched together.

Regards,

Tony