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

2015-12-04 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Grygorii Strashko 

On 12/03/2015 11:37 PM, Tony Lindgren wrote:

* Grygorii Strashko  [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: GW   4.4.0-rc3-dirty #2682
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
] (show_stack) from [] (dump_stack+0x84/0x9c)
[] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
[] (warn_slowpath_common) from [] 
(warn_slowpath_fmt+0x30/0x40)
[] (warn_slowpath_fmt) from [] (irq_set_irq_wake+0xbc/0xfc)
[] (irq_set_irq_wake) from [] 
(device_wakeup_disarm_wake_irqs+0x70/0x12c)
[] (device_wakeup_disarm_wake_irqs) from [] 
(dpm_resume_noirq+0x20c/0x2e4)
[] (dpm_resume_noirq) from [] 
(suspend_devices_and_enter+0x1e4/0x6bc)
[] (suspend_devices_and_enter) from [] 
(pm_suspend+0x358/0x4b8)
[] (pm_suspend) from [] (state_store+0x64/0xb8)
[] (state_store) from [] (kobj_attr_store+0x14/0x20)
[] (kobj_attr_store) from [] (sysfs_kf_write+0x4c/0x50)
[] (sysfs_kf_write) from [] (kernfs_fop_write+0xbc/0x1cc)
[] (kernfs_fop_write) from [] (__vfs_write+0x24/0xd8)
[] (__vfs_write) from [] (vfs_write+0x94/0x154)
[] (vfs_write) from [] (SyS_write+0x40/0x94)
[] (SyS_write) from [] (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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Tony Lindgren 
* Grygorii Strashko  [151204 02:45]:
> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> >* Grygorii Strashko  [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: GW   4.4.0-rc3-dirty #2682
> >Hardware name: Generic OMAP36xx (Flattened Device Tree)
> >[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> >] (show_stack) from [] (dump_stack+0x84/0x9c)
> >[] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
> >[] (warn_slowpath_common) from [] 
> >(warn_slowpath_fmt+0x30/0x40)
> >[] (warn_slowpath_fmt) from [] 
> >(irq_set_irq_wake+0xbc/0xfc)
> >[] (irq_set_irq_wake) from [] 
> >(device_wakeup_disarm_wake_irqs+0x70/0x12c)
> >[] (device_wakeup_disarm_wake_irqs) from [] 
> >(dpm_resume_noirq+0x20c/0x2e4)
> >[] (dpm_resume_noirq) from [] 
> >(suspend_devices_and_enter+0x1e4/0x6bc)
> >[] (suspend_devices_and_enter) from [] 
> >(pm_suspend+0x358/0x4b8)
> >[] (pm_suspend) from [] (state_store+0x64/0xb8)
> >[] (state_store) from [] (kobj_attr_store+0x14/0x20)
> >[] (kobj_attr_store) from [] (sysfs_kf_write+0x4c/0x50)
> >[] (sysfs_kf_write) from [] (kernfs_fop_write+0xbc/0x1cc)
> >[] (kernfs_fop_write) from [] (__vfs_write+0x24/0xd8)
> >[] (__vfs_write) from [] (vfs_write+0x94/0x154)
> >[] (vfs_write) from [] (SyS_write+0x40/0x94)
> >[] (SyS_write) from [] (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-1-g6a5e5ec #2694
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x84/0x9c)
[] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
[] (warn_slowpath_common) from [] 
(warn_slowpath_null+0x1c/0x24)
[] (warn_slowpath_null) from [] 
(irq_pm_install_action+0x9c/0xec)
[] (irq_pm_install_action) from [] (__setup_irq+0x434/0x5e0)
[] (__setup_irq) from [] (request_threaded_irq+0xc4/0x15c)
[] (request_threaded_irq) from [] 
(omap3_pm_init+0x10c/0x400)
[] (omap3_pm_init) from [] (omap3_init_late+0xc/0x14)
[] (omap3_init_late) from [] (init_machine_late+0x1c/0x90)
[] (init_machine_late) from [] (do_one_initcall+0x80/0x1e0)
[] (do_one_initcall) from [] 
(kernel_init_freeable+0x218/0x2e8)
[] (kernel_init_freeable) from [] (kernel_init+0x8/0xec)
[] (kernel_init) from [] (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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Tony Lindgren 
* Tony Lindgren  [151203 13:41]:
> * Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Sudeep Holla 



On 04/12/15 15:40, Tony Lindgren wrote:

* Tony Lindgren  [151203 13:41]:

* Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Grygorii Strashko 
On 12/04/2015 05:35 PM, Tony Lindgren wrote:
> * Grygorii Strashko  [151204 02:45]:
>> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
>>> * Grygorii Strashko  [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: GW   4.4.0-rc3-dirty #2682
>>> Hardware name: Generic OMAP36xx (Flattened Device Tree)
>>> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
>>> ] (show_stack) from [] (dump_stack+0x84/0x9c)
>>> [] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
>>> [] (warn_slowpath_common) from [] 
>>> (warn_slowpath_fmt+0x30/0x40)
>>> [] (warn_slowpath_fmt) from [] 
>>> (irq_set_irq_wake+0xbc/0xfc)
>>> [] (irq_set_irq_wake) from [] 
>>> (device_wakeup_disarm_wake_irqs+0x70/0x12c)
>>> [] (device_wakeup_disarm_wake_irqs) from [] 
>>> (dpm_resume_noirq+0x20c/0x2e4)
>>> [] (dpm_resume_noirq) from [] 
>>> (suspend_devices_and_enter+0x1e4/0x6bc)
>>> [] (suspend_devices_and_enter) from [] 
>>> (pm_suspend+0x358/0x4b8)
>>> [] (pm_suspend) from [] (state_store+0x64/0xb8)
>>> [] (state_store) from [] (kobj_attr_store+0x14/0x20)
>>> [] (kobj_attr_store) from [] (sysfs_kf_write+0x4c/0x50)
>>> [] (sysfs_kf_write) from [] 
>>> (kernfs_fop_write+0xbc/0x1cc)
>>> [] (kernfs_fop_write) from [] (__vfs_write+0x24/0xd8)
>>> [] (__vfs_write) from [] (vfs_write+0x94/0x154)
>>> [] (vfs_write) from [] (SyS_write+0x40/0x94)
>>> [] (SyS_write) from [] (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-1-g6a5e5ec #2694
> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x84/0x9c)
> [] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
> [] (warn_slowpath_common) from [] 
> (warn_slowpath_null+0x1c/0x24)
> [] (warn_slowpath_null) from [] 
> (irq_pm_install_action+0x9c/0xec)
> [] (irq_pm_install_action) from [] 
> (__setup_irq+0x434/0x5e0)
> [] (__setup_irq) from [] (request_threaded_irq+0xc4/0x15c)
> [] (request_threaded_irq) from [] 
> (omap3_pm_init+0x10c/0x400)
> [] (omap3_pm_init) from [] (omap3_init_late+0xc/0x14)
> [] (omap3_init_late) from [] (init_machine_late+0x1c/0x90)
> [] (init_machine_late) from [] 
> (do_one_initcall+0x80/0x1e0)
> [] (do_one_initcall) from [] 
> (kernel_init_freeable+0x218/0x2e8)
> [] (kernel_init_freeable) from [] (kernel_init+0x8/0xec)
> [] (kernel_init) from [] (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,

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

2015-12-04 Thread Tony Lindgren 
* Grygorii Strashko  [151204 08:00]:
> On 12/04/2015 05:35 PM, Tony Lindgren wrote:
> > * Grygorii Strashko  [151204 02:45]:
> >> On 12/03/2015 11:37 PM, Tony Lindgren wrote:
> >>> * Grygorii Strashko  [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: GW   4.4.0-rc3-dirty #2682
> >>> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> >>> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> >>> ] (show_stack) from [] (dump_stack+0x84/0x9c)
> >>> [] (dump_stack) from [] 
> >>> (warn_slowpath_common+0x7c/0xb8)
> >>> [] (warn_slowpath_common) from [] 
> >>> (warn_slowpath_fmt+0x30/0x40)
> >>> [] (warn_slowpath_fmt) from [] 
> >>> (irq_set_irq_wake+0xbc/0xfc)
> >>> [] (irq_set_irq_wake) from [] 
> >>> (device_wakeup_disarm_wake_irqs+0x70/0x12c)
> >>> [] (device_wakeup_disarm_wake_irqs) from [] 
> >>> (dpm_resume_noirq+0x20c/0x2e4)
> >>> [] (dpm_resume_noirq) from [] 
> >>> (suspend_devices_and_enter+0x1e4/0x6bc)
> >>> [] (suspend_devices_and_enter) from [] 
> >>> (pm_suspend+0x358/0x4b8)
> >>> [] (pm_suspend) from [] (state_store+0x64/0xb8)
> >>> [] (state_store) from [] (kobj_attr_store+0x14/0x20)
> >>> [] (kobj_attr_store) from [] 
> >>> (sysfs_kf_write+0x4c/0x50)
> >>> [] (sysfs_kf_write) from [] 
> >>> (kernfs_fop_write+0xbc/0x1cc)
> >>> [] (kernfs_fop_write) from [] (__vfs_write+0x24/0xd8)
> >>> [] (__vfs_write) from [] (vfs_write+0x94/0x154)
> >>> [] (vfs_write) from [] (SyS_write+0x40/0x94)
> >>> [] (SyS_write) from [] (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-1-g6a5e5ec #2694
> > Hardware name: Generic OMAP36xx (Flattened Device Tree)
> > [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> > [] (show_stack) from [] (dump_stack+0x84/0x9c)
> > [] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
> > [] (warn_slowpath_common) from [] 
> > (warn_slowpath_null+0x1c/0x24)
> > [] (warn_slowpath_null) from [] 
> > (irq_pm_install_action+0x9c/0xec)
> > [] (irq_pm_install_action) from [] 
> > (__setup_irq+0x434/0x5e0)
> > [] (__setup_irq) from [] 
> > (request_threaded_irq+0xc4/0x15c)
> > [] (request_threaded_irq) from [] 
> > (omap3_pm_init+0x10c/0x400)
> > [] (omap3_pm_init) from [] (omap3_init_late+0xc/0x14)
> > [] (omap3_init_late) from [] 
> > (init_machine_late+0x1c/0x90)
> > [] (init_machine_late) from [] 
> > (do_one_initcall+0x80/0x1e0)
> > [] (do_one_initcall) from [] 
> > (kernel_init_freeable+0x218/0x2e8)
> > [] (kernel_init_freeable) from [] (kernel_init+0x8/0xec)
> > [] (kernel_init) from [] (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

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

2015-12-04 Thread Tony Lindgren 
* Sudeep Holla  [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  [151203 13:41]:
> * Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Tony Lindgren 
* Grygorii Strashko  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Tony Lindgren 
* Sudeep Holla  [151204 08:16]:
> Hi Tony,
> 
> On 04/12/15 15:40, Tony Lindgren wrote:
> >* Tony Lindgren  [151203 13:41]:
> >>* Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread 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  [151203 13:41]:

* Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Sudeep Holla 

Hi Tony,

On 04/12/15 15:40, Tony Lindgren wrote:

* Tony Lindgren  [151203 13:41]:

* Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread Grygorii Strashko 
On 12/04/2015 05:44 PM, Sudeep Holla wrote:
> 
> 
> On 04/12/15 15:40, Tony Lindgren wrote:
>> * Tony Lindgren  [151203 13:41]:
>>> * Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-04 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-03 Thread Sudeep Holla 



On 03/12/15 18:13, Tony Lindgren wrote:

* Linus Walleij  [151201 06:07]:

On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla  wrote:


From: Sudeep Holla 

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 
Cc: linux-g...@vger.kernel.org
Signed-off-by: Sudeep Holla 


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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-03 Thread Tony Lindgren 
* Sudeep Holla  [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-03 Thread Tony Lindgren 
* Grygorii Strashko  [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: GW   4.4.0-rc3-dirty #2682
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
] (show_stack) from [] (dump_stack+0x84/0x9c)
[] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8)
[] (warn_slowpath_common) from [] 
(warn_slowpath_fmt+0x30/0x40)
[] (warn_slowpath_fmt) from [] (irq_set_irq_wake+0xbc/0xfc)
[] (irq_set_irq_wake) from [] 
(device_wakeup_disarm_wake_irqs+0x70/0x12c)
[] (device_wakeup_disarm_wake_irqs) from [] 
(dpm_resume_noirq+0x20c/0x2e4)
[] (dpm_resume_noirq) from [] 
(suspend_devices_and_enter+0x1e4/0x6bc)
[] (suspend_devices_and_enter) from [] 
(pm_suspend+0x358/0x4b8)
[] (pm_suspend) from [] (state_store+0x64/0xb8)
[] (state_store) from [] (kobj_attr_store+0x14/0x20)
[] (kobj_attr_store) from [] (sysfs_kf_write+0x4c/0x50)
[] (sysfs_kf_write) from [] (kernfs_fop_write+0xbc/0x1cc)
[] (kernfs_fop_write) from [] (__vfs_write+0x24/0xd8)
[] (__vfs_write) from [] (vfs_write+0x94/0x154)
[] (vfs_write) from [] (SyS_write+0x40/0x94)
[] (SyS_write) from [] (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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-03 Thread Tony Lindgren 
* Linus Walleij  [151201 06:07]:
> On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla  wrote:
> 
> > From: Sudeep Holla 
> >
> > 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 
> > Cc: linux-g...@vger.kernel.org
> > Signed-off-by: Sudeep Holla 
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-03 Thread Grygorii Strashko 
On 12/03/2015 08:13 PM, Tony Lindgren wrote:
> * Linus Walleij  [151201 06:07]:
>> On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla  wrote:
>>
>>> From: Sudeep Holla 
>>>
>>> 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 
>>> Cc: linux-g...@vger.kernel.org
>>> Signed-off-by: Sudeep Holla 
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html