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

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

[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

[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

[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

[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

[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]

* 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

[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]

* 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 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: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

[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

[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

[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