OMAP/pinmux: remove misuse of IRQF_NO_SUSPEND flag
Subsystems (8)
cover OMAP/pinmux: remove misuse of IRQF_NO_SUSPEND flag
Hi Tony/Grygorii,
Just revieving the old threads [1][2]. I have put all the necessary patches together so that it can tested. Can you give it a go ?
Regards,
Sudeep
[1] https://www.spinics.net/lists/linux-gpio/msg09879.html
[2] http://lkml.iu.edu/hypermail/linux/kernel/1509.2/03937.html
Sudeep Holla (3):
pinctrl: single: Use a separate lockdep class
pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag
ARM: OMAP2+: remove misuse of IRQF_NO_SUSPEND flag
arch/arm/mach-omap2/mux.c | 4 ++-- arch/arm/mach-omap2/pm34xx.c | 9 ++++----- arch/arm/mach-omap2/prm_common.c | 1 + drivers/pinctrl/pinctrl-single.c | 15 ++++++++++++--- 4 files changed, 19 insertions(+), 10 deletions(-)
Signature · 1 line
1.9.1
PATCH 1/3 pinctrl: single: Use a separate lockdep class
The single pinmux controller can be cascaded to the other interrupt controllers. Hence when propagating wake-up settings to its parent interrupt controller, there's possiblity of detecting possible recursive locking and getting lockdep warning.
This patch avoids this false positive by using a separate lockdep class for this single pinctrl interrupts.
--- drivers/pinctrl/pinctrl-single.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/pinctrl/pinctrl-single.c +8 -0
@@ -255,6 +255,13 @@ static enum pin_config_param pcs_bias[] = { };/* +* This lock class tells lockdep that irqchip core that this single +* pinctrl can be in a different category than its parents, so it won't +* report false recursion. +*/ +static struct lock_class_key pcs_lock_class; + +/* * REVISIT: Reads and writes could eventually use regmap or something * generic. But at least on omaps, some mux registers are performance * critical as they may need to be remuxed every time before and after
@@ -1713,6 +1720,7 @@ static int pcs_irqdomain_map(struct irq_domain *d, unsigned int irq,irq_set_chip_data (irq, pcs_soc);irq_set_chip_and_handler (irq, &pcs->chip, handle_level_irq); +irq_set_lockdep_class (irq, &pcs_lock_class);irq_set_noprobe (irq);return 0 ;
Signature · 1 line
1.9.1
PATCH 1/3 pinctrl: single: Use a separate lockdep class
Quoted 36 lines · Sudeep Holla
The single pinmux controller can be cascaded to the other interrupt controllers. Hence when propagating wake-up settings to its parent interrupt controller, there's possiblity of detecting possible recursive locking and getting lockdep warning.
This patch avoids this false positive by using a separate lockdep class for this single pinctrl interrupts.
Cc Linus Walleij <[email protected]>Cc linux-gpio at vger.kernel.orgSuggested-by Thomas Gleixner <[email protected]>Signed-off-by Sudeep Holla <[email protected]>--- drivers/pinctrl/pinctrl-single.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pinctrl/pinctrl-single.c +7
-0@@ -255,6 +255,13 @@ static enum pin_config_param pcs_bias[] = { };/* +* This lock class tells lockdep that irqchip core that this single +* pinctrl can be in a different category than its parents, so it won't +* report false recursion. +*/ +static struct lock_class_key pcs_lock_class; + +/* * REVISIT: Reads and writes could eventually use regmap or something * generic. But at least on omaps, some mux registers are performance * critical as they may need to be remuxed every time before and after
drivers/pinctrl/pinctrl-single.c static int pcs_irqdomain_map(struct irq_domain *d, unsigned int irq, · line 1726
Quoted 7 lines
drivers/pinctrl/pinctrl-single.c +1
-0@@ -1713,6 +1720,7 @@ static int pcs_irqdomain_map(struct irq_domain *d, unsigned int irq,irq_set_chip_data (irq, pcs_soc);irq_set_chip_and_handler (irq, &pcs->chip, handle_level_irq); +irq_set_lockdep_class (irq, &pcs_lock_class);irq_set_noprobe (irq);return 0 ;
I have no objection to this patch and it's independent from other patches in this series.
Signature · 2 lines
regards, -grygorii
PATCH 1/3 pinctrl: single: Use a separate lockdep class
In reply to Sudeep HollaQuoted 5 lines · Sudeep Holla
The single pinmux controller can be cascaded to the other interrupt controllers. Hence when propagating wake-up settings to its parent interrupt controller, there's possiblity of detecting possible recursive locking and getting lockdep warning.
Quoted 7 lines
This patch avoids this false positive by using a separate lockdep class for this single pinctrl interrupts.
Cc Linus Walleij <[email protected]>Cc linux-gpio at vger.kernel.orgSuggested-by Thomas Gleixner <[email protected]>Signed-off-by Sudeep Holla <[email protected]>
Patch applied with Grygorii's review tag.
Yours,
Linus Walleij
PATCH 2/3 pinctrl: single: remove misuse of IRQF_NO_SUSPEND flag
In reply to Sudeep HollaFrom: Sudeep Holla <[email protected]>
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.
--- drivers/pinctrl/pinctrl-single.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/pinctrl/pinctrl-single.c +4 -3
@@ -1619,12 +1619,14 @@ static void pcs_irq_unmask(struct irq_data *d)*/ static int pcs_irq_set_wake (struct irq_data *d,unsigned int state) { +struct pcs_soc_data *pcs_soc =irq_data_get_irq_chip_data (d); +if (state)pcs_irq_unmask (d);else pcs_irq_mask (d); -return 0 ; +return irq_set_irq_wake (pcs_soc->irq, state); }/**
@@ -1760,8 +1762,7 @@ static int pcs_irq_init_chained_handler(struct pcs_device *pcs,int res; res =request_irq (pcs_soc->irq, pcs_irq_handler, -IRQF_SHARED |IRQF_NO_SUSPEND | -IRQF_NO_THREAD , +IRQF_SHARED |IRQF_NO_THREAD , name, pcs_soc);if (res) { pcs_soc->irq =-1 ;
Signature · 1 line
1.9.1
PATCH 3/3 ARM: OMAP2+: remove misuse of IRQF_NO_SUSPEND flag
In reply to Sudeep HollaThe 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 enable_irq_wake instead.
--- arch/arm/mach-omap2/mux.c | 4 ++-- arch/arm/mach-omap2/pm34xx.c | 9 ++++----- arch/arm/mach-omap2/prm_common.c | 1 + 3 files changed, 7 insertions(+), 7 deletions(-)
arch/arm/mach-omap2/mux.c +2 -2
@@ -810,13 +810,13 @@ int __init omap_mux_late_init(void)return 0 ; ret =request_irq (omap_prcm_event_to_irq ("io" ), - omap_hwmod_mux_handle_irq,IRQF_SHARED |IRQF_NO_SUSPEND , + omap_hwmod_mux_handle_irq,IRQF_SHARED ,"hwmod_io" , omap_mux_late_init);if (ret)pr_warn ("mux: Failed to setup hwmod io irq %d\n" , ret); -return 0 ; +return enable_irq_wake (omap_prcm_event_to_irq ("io" )); } static void __init omap_mux_package_fixup(struct omap_mux *p,
arch/arm/mach-omap2/pm34xx.c +4 -5
@@ -472,23 +472,22 @@ int __init omap3_pm_init(void)prcm_setup_regs (); ret =request_irq (omap_prcm_event_to_irq ("wkup" ), - _prcm_int_handle_wakeup,IRQF_NO_SUSPEND ,"pm_wkup" ,NULL ); + _prcm_int_handle_wakeup,0 ,"pm_wkup" ,NULL );if (ret) {pr_err ("pm: Failed to request pm_wkup irq\n" ); goto err1; } +enable_irq_wake (omap_prcm_event_to_irq ("wkup" ));/* 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" , - omap3_pm_init); -enable_irq (omap_prcm_event_to_irq ("io" )); - + _prcm_int_handle_io,IRQF_SHARED ,"pm_io" , omap3_pm_init);if (ret) {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) {
arch/arm/mach-omap2/prm_common.c +1 -0
@@ -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 ;
Signature · 1 line
1.9.1
PATCH 3/3 ARM: OMAP2+: remove misuse of IRQF_NO_SUSPEND flag
Hi Sudeep,
Quoted 8 lines · Sudeep Holla
The
IRQF_NO_SUSPENDflag 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_wakeis recommended to be used for the wakeup.This patch removes the use of
IRQF_NO_SUSPENDflags replacing it withenable_irq_wakeinstead.
And sorry for delayed reply - I've spent some time investigating it, but It was during Christmas holidays and finally I lost track of it :)
So, what we have:
1) bad news: it will not work :(
The PRCM wake up is handled in the following way on OMAP3+ -- PRCM IRQ omap_prcm_irq_handler()
|- (a) "wkup" event -> _prcm_int_handle_wakeup() |- "io" event (shared)
|- (b) _prcm_int_handle_io()
|- (c) pcs_irq_handler()(mux is for legacy platform - can be ignored for now)
All "wkup"/"io" events must be handled in PRCM registers before PRCM IRQ status bits can be acked and reset.
It means that all IRQs (a), (b) (c) have to be enabled at the moment when PRCM IRQ is fired. Unfortunately this can't be guaranteed by IR PM core and omap_prcm_irq_handler() will stuck forever on Resume :(.
Resume example:
omap_prcm_irq_handler()
[1] -> prcm_irq_setup->read_pending_irqs(pending);
-> is there pending irqs?
-> io event
-> (c) pcs_irq (enabled | wakeup src)
-> irq_pm_check_wakeup()
-> (disabled | suspended| pending)
pcs_irq_handler() not run-> goto [1] ----- OOps dead-loop
2) Potential good news: It could be fixed the same way as it's done omap34xx (and as it was before :
arch/arm/mach-omap2/pm.c +2 -4
@@ -221,8 +221,7 @@ static int omap_pm_enter(suspend_state_t suspend_state)static int omap_pm_begin (suspend_state_t state) {cpu_idle_poll_ctrl (true); -if (cpu_is_omap34xx ()) -omap_prcm_irq_prepare (); +omap_prcm_irq_prepare ();return 0 ; }
@@ -233,8 +232,7 @@ static void omap_pm_end(void)static void omap_pm_finish (void ) { -if (cpu_is_omap34xx ()) -omap_prcm_irq_complete (); +omap_prcm_irq_complete (); }
Another option is to convert omap_prcm_irq_handler to the generic handler (now chained) and, probably, make it threaded and all cascaded IRQs as nested threaded (this is just a theory).
I'll be on business trip next two weeks and will not be able to help more with it Sorry.
Quoted 77 lines
Cc Grygorii Strashko <[email protected]>Cc Tony Lindgren <[email protected]>Cc linux-omap at vger.kernel.orgCc linux-arm-kernel at lists.infradead.orgSigned-off-by Sudeep Holla <[email protected]>--- arch/arm/mach-omap2/mux.c | 4 ++-- arch/arm/mach-omap2/pm34xx.c | 9 ++++----- arch/arm/mach-omap2/prm_common.c | 1 + 3 files changed, 7 insertions(+), 7 deletions(-)arch/arm/mach-omap2/mux.c +2
-2@@ -810,13 +810,13 @@ int __init omap_mux_late_init(void)return 0 ; ret =request_irq (omap_prcm_event_to_irq ("io" ), - omap_hwmod_mux_handle_irq,IRQF_SHARED |IRQF_NO_SUSPEND , + omap_hwmod_mux_handle_irq,IRQF_SHARED ,"hwmod_io" , omap_mux_late_init);if (ret)pr_warn ("mux: Failed to setup hwmod io irq %d\n" , ret); -return 0 ; +return enable_irq_wake (omap_prcm_event_to_irq ("io" )); } static void __init omap_mux_package_fixup(struct omap_mux *p,arch/arm/mach-omap2/pm34xx.c +4
-5@@ -472,23 +472,22 @@ int __init omap3_pm_init(void)prcm_setup_regs (); ret =request_irq (omap_prcm_event_to_irq ("wkup" ), - _prcm_int_handle_wakeup,IRQF_NO_SUSPEND ,"pm_wkup" ,NULL ); + _prcm_int_handle_wakeup,0 ,"pm_wkup" ,NULL );if (ret) {pr_err ("pm: Failed to request pm_wkup irq\n" ); goto err1; } +enable_irq_wake (omap_prcm_event_to_irq ("wkup" ));/* 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" , - omap3_pm_init); -enable_irq (omap_prcm_event_to_irq ("io" )); - + _prcm_int_handle_io,IRQF_SHARED ,"pm_io" , omap3_pm_init);if (ret) {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) {arch/arm/mach-omap2/prm_common.c +1
-0@@ -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 ;
Signature · 2 lines
regards, -grygorii
PATCH 3/3 ARM: OMAP2+: remove misuse of IRQF_NO_SUSPEND flag
Quoted 9 lines · Grygorii Strashko
Hi Sudeep,
Quoted 6 lines · Sudeep Holla
The
IRQF_NO_SUSPENDflag 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_wakeis recommended to be used for the wakeup.
Quoted 6 lines
Quoted 2 lines
This patch removes the use of
IRQF_NO_SUSPENDflags replacing it withenable_irq_wakeinstead.And sorry for delayed reply - I've spent some time investigating it, but It was during Christmas holidays and finally I lost track of it :)
That's fine, usually that's the case in general will all of us :)
[..]
Quoted 5 lines
Another option is to convert
omap_prcm_irq_handlerto the generic handler (now chained) and, probably, make it threaded and all cascaded IRQs as nested threaded (this is just a theory).
Since I don't have any knowledge of OMAP, I will completely depend on your for anything OMAP specific.
Quoted 3 lines
I'll be on business trip next two weeks and will not be able to help more with it Sorry.
No problem, let me know once you get a chance to try out things at your end.
Signature · 2 lines
Regards, Sudeep
cover OMAP/pinmux: remove misuse of IRQF_NO_SUSPEND flag
In reply to Sudeep HollaQuoted 8 lines · Sudeep Holla
pinctrl: single: Use a separate lockdep class
pinctrl: single: remove misuse ofIRQF_NO_SUSPENDflag
ARM: OMAP2+: remove misuse ofIRQF_NO_SUSPENDflagarch/arm/mach-omap2/mux.c | 4 ++-- arch/arm/mach-omap2/pm34xx.c | 9 ++++----- arch/arm/mach-omap2/prm_common.c | 1 + drivers/pinctrl/pinctrl-single.c | 15 ++++++++++++---
Are these patches orthogonal so I can merge the pinctrl patch independently of the OMAP patches if Tony ACKs this?
If not: should it be merged by me or through ARM SoC/Tony?
Anyways waiting for maintainer feedback, but the above is good to know.
Yours,
Linus Walleij
cover OMAP/pinmux: remove misuse of IRQF_NO_SUSPEND flag
Hi Linus,
Quoted 6 lines · Linus Walleij
Quoted 4 lines · Sudeep Holla
pinctrl: single: Use a separate lockdep class
pinctrl: single: remove misuse ofIRQF_NO_SUSPENDflag
ARM: OMAP2+: remove misuse ofIRQF_NO_SUSPENDflag
Quoted 8 lines
Quoted 4 lines
arch/arm/mach-omap2/mux.c | 4 ++-- arch/arm/mach-omap2/pm34xx.c | 9 ++++----- arch/arm/mach-omap2/prm_common.c | 1 + drivers/pinctrl/pinctrl-single.c | 15 ++++++++++++---Are these patches orthogonal so I can merge the pinctrl patch independently of the OMAP patches if Tony ACKs this?
Thanks for the follow up.
IIUC they are dependent and remember Tony/Grygorii wanted them to be merged together(at-least for bisectibility)
Quoted 2 lines
If not: should it be merged by me or through ARM SoC/Tony?
I think so.
Quoted 2 lines
Anyways waiting for maintainer feedback, but the above is good to know.
Grygorii was still not sure if this works and he couldn't give it a test. He was traveling and may need more time to get back on this.
Signature · 2 lines
Regards, Sudeep
Tony Lindgren
maintainercover OMAP/pinmux: remove misuse of IRQF_NO_SUSPEND flag
Quoted 14 lines · Sudeep Holla
Hi Linus,
Quoted 11 lines · Linus Walleij
Quoted 8 lines · Sudeep Holla
pinctrl: single: Use a separate lockdep class
pinctrl: single: remove misuse ofIRQF_NO_SUSPENDflag
ARM: OMAP2+: remove misuse ofIRQF_NO_SUSPENDflagarch/arm/mach-omap2/mux.c | 4 ++-- arch/arm/mach-omap2/pm34xx.c | 9 ++++----- arch/arm/mach-omap2/prm_common.c | 1 + drivers/pinctrl/pinctrl-single.c | 15 ++++++++++++---
Quoted 7 lines
Quoted 3 lines
Are these patches orthogonal so I can merge the pinctrl patch independently of the OMAP patches if Tony ACKs this?
Thanks for the follow up.
IIUC they are dependent and remember Tony/Grygorii wanted them to be merged together(at-least for bisectibility)
AFAIK this series needs to be kept together to keep things working.
Quoted 10 lines
Quoted 2 lines
If not: should it be merged by me or through ARM SoC/Tony?
I think so.
Quoted 2 lines
Anyways waiting for maintainer feedback, but the above is good to know.
Grygorii was still not sure if this works and he couldn't give it a test. He was traveling and may need more time to get back on this.
He said it does not work so clearly more work is needed.
Regards,
Tony
cover OMAP/pinmux: remove misuse of IRQF_NO_SUSPEND flag
Quoted 31 lines · Tony Lindgren
Quoted 14 lines · Sudeep Holla
Hi Linus,
Quoted 11 lines · Linus Walleij
Quoted 8 lines · Sudeep Holla
pinctrl: single: Use a separate lockdep class
pinctrl: single: remove misuse ofIRQF_NO_SUSPENDflag
ARM: OMAP2+: remove misuse ofIRQF_NO_SUSPENDflagarch/arm/mach-omap2/mux.c | 4 ++-- arch/arm/mach-omap2/pm34xx.c | 9 ++++----- arch/arm/mach-omap2/prm_common.c | 1 + drivers/pinctrl/pinctrl-single.c | 15 ++++++++++++---Quoted 7 lines
Quoted 3 lines
Are these patches orthogonal so I can merge the pinctrl patch independently of the OMAP patches if Tony ACKs this?
Thanks for the follow up.
IIUC they are dependent and remember Tony/Grygorii wanted them to be merged together(at-least for bisectibility)AFAIK this series needs to be kept together to keep things working.
Quoted 5 lines
Quoted 2 lines
If not: should it be merged by me or through ARM SoC/Tony?
I think so.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Anyways waiting for maintainer feedback, but the above is good to know.
Grygorii was still not sure if this works and he couldn't give it a test. He was traveling and may need more time to get back on this.
He said it does not work so clearly more work is needed.
Yeah. As I mentioned before patches 2/3 will not work.
I've tried to test suspend with diff I posted [1], but I did it only on dra7 and am43 where suspend i more simple than on omap3. Also, I worry that with such approach some irq can be missed (especially edge ones), because PRCM irqs will be re-enabled at very late resume stage (right before thaw).
Second option, I see, is to rework PRCM to be threaded IRQ, but in this case all children IRQs will need to be threaded/nested threaded
(including PCS, legacy OMAP mux, OMAP3 _prcm_int_handle_wakeup/_prcm_int_handle_io).
Are there any objection to convert PCS to threaded IRQ, for example? ;)
[1] https://patchwork.ozlabs.org/patch/578915/
Signature · 2 lines
regards, -grygorii
Tony Lindgren
maintainercover OMAP/pinmux: remove misuse of IRQF_NO_SUSPEND flag
Quoted 7 lines · Grygorii Strashko
Yeah. As I mentioned before patches 2/3 will not work.
I've tried to test suspend with diff I posted [1], but I did it only on dra7 and am43 where suspend i more simple than on omap3. Also, I worry that with such approach some irq can be missed (especially edge ones), because PRCM irqs will be re-enabled at very late resume stage (right before thaw).
Yeah any tests not done with PM on omap3 with the mainline kernel are clearly incomplete in this case :)
Quoted 5 lines
Second option, I see, is to rework PRCM to be threaded IRQ, but in this case all children IRQs will need to be threaded/nested threaded
(including PCS, legacy OMAP mux, OMAP3 _prcm_int_handle_wakeup/_prcm_int_handle_io).Are there any objection to convert PCS to threaded IRQ, for example? ;)
For PRCM interrupt, I don't think that can be threaded IRQ as that thing is firing all the time. The consumers for the chained PRCM interrupt consumers like PCS could probably use threaded IRQ. When the PCS interrupts fire, we're already in some sleeper idle state so the extra latency thre does not matter.
Regards,
Tony