The pm-mmp2 and pm-pxa910 power management related irq_set_wake
callbacks fiddle pointlessly with the irq actions for no reason except
for lack of understanding how the wakeup mechanism works.
On supsend the core disables all interrupts lazily, i.e. it does not
mask them at the irq controller level. So any interrupt which is
firing during supsend will mark the corresponding interrupt line as
pending. Just before the core powers down it checks whether there are
interrupts pending from interrupt lines which are marked as wakeup
sources and if so it aborts the resume and resends the interrupts.
The IRQF_NO_SUSPEND flag for interrupt actions is a totally different
mechanism. That allows the device driver to prevent the core from
disabling the interrupt despite the fact that it is not marked as a
wakeup source. This has nothing to do with the case at hand. It was
introduced for special cases where lazy disable is not possible.
Remove the nonsense along with the braindamaged boundary check. The
core code does NOT call these functions out of boundary.
Add a FIXME comment to an unhandled error path which merily printks
some useless blurb instead of returning a proper error code.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: arm <[email protected]>
Cc: Eric Miao <[email protected]>
Cc: Haojian Zhuang <[email protected]>
Cc: Russell King <[email protected]>
---
arch/arm/mach-mmp/pm-mmp2.c | 16 +---------------
arch/arm/mach-mmp/pm-pxa910.c | 20 ++++----------------
2 files changed, 5 insertions(+), 31 deletions(-)
Index: tip/arch/arm/mach-mmp/pm-mmp2.c
===================================================================
--- tip.orig/arch/arm/mach-mmp/pm-mmp2.c
+++ tip/arch/arm/mach-mmp/pm-mmp2.c
@@ -27,22 +27,8 @@
int mmp2_set_wake(struct irq_data *d, unsigned int on)
{
- int irq = d->irq;
- struct irq_desc *desc = irq_to_desc(irq);
unsigned long data = 0;
-
- if (unlikely(irq >= nr_irqs)) {
- pr_err("IRQ nubmers are out of boundary!\n");
- return -EINVAL;
- }
-
- if (on) {
- if (desc->action)
- desc->action->flags |= IRQF_NO_SUSPEND;
- } else {
- if (desc->action)
- desc->action->flags &= ~IRQF_NO_SUSPEND;
- }
+ int irq = d->irq;
/* enable wakeup sources */
switch (irq) {
Index: tip/arch/arm/mach-mmp/pm-pxa910.c
===================================================================
--- tip.orig/arch/arm/mach-mmp/pm-pxa910.c
+++ tip/arch/arm/mach-mmp/pm-pxa910.c
@@ -27,22 +27,8 @@
int pxa910_set_wake(struct irq_data *data, unsigned int on)
{
- int irq = data->irq;
- struct irq_desc *desc = irq_to_desc(data->irq);
uint32_t awucrm = 0, apcr = 0;
-
- if (unlikely(irq >= nr_irqs)) {
- pr_err("IRQ nubmers are out of boundary!\n");
- return -EINVAL;
- }
-
- if (on) {
- if (desc->action)
- desc->action->flags |= IRQF_NO_SUSPEND;
- } else {
- if (desc->action)
- desc->action->flags &= ~IRQF_NO_SUSPEND;
- }
+ int irq = data->irq;
/* setting wakeup sources */
switch (irq) {
@@ -115,9 +101,11 @@ int pxa910_set_wake(struct irq_data *dat
if (irq >= IRQ_GPIO_START && irq < IRQ_BOARD_START) {
awucrm = MPMU_AWUCRM_WAKEUP(2);
apcr |= MPMU_APCR_SLPWP2;
- } else
+ } else {
+ /* FIXME: This should return a proper error code ! */
printk(KERN_ERR "Error: no defined wake up source irq: %d\n",
irq);
+ }
}
if (on) {
Hi Thomas,
On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
> callbacks fiddle pointlessly with the irq actions for no reason except
> for lack of understanding how the wakeup mechanism works.
>
> On supsend the core disables all interrupts lazily, i.e. it does not
> mask them at the irq controller level. So any interrupt which is
> firing during supsend will mark the corresponding interrupt line as
s/supsend/suspend/ twice
> pending. Just before the core powers down it checks whether there are
> interrupts pending from interrupt lines which are marked as wakeup
> sources and if so it aborts the resume and resends the interrupts.
It's the suspend that is aborted, not the resume.
Other than that your change looks fine.
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
<[email protected]> wrote:
> Hi Thomas,
>
> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>> callbacks fiddle pointlessly with the irq actions for no reason except
>> for lack of understanding how the wakeup mechanism works.
>>
>> On supsend the core disables all interrupts lazily, i.e. it does not
>> mask them at the irq controller level. So any interrupt which is
>> firing during supsend will mark the corresponding interrupt line as
> s/supsend/suspend/ twice
>> pending. Just before the core powers down it checks whether there are
>> interrupts pending from interrupt lines which are marked as wakeup
>> sources and if so it aborts the resume and resends the interrupts.
> It's the suspend that is aborted, not the resume.
>
> Other than that your change looks fine.
>
For pxa910 and MMP2, wake up source only wake up the AP subsystem.
The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
Now the core is still powered down. APMU will check the interrupt
lines, and find
that there are interrupt pending, it will power on the cores.
So if the irq is disabled, even wake up source can wake up AP subsystem, but the
core is still powered down. It will not be powered up by APMU.
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <[email protected]> wrote:
> On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
> <[email protected]> wrote:
>> Hi Thomas,
>>
>> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>>> callbacks fiddle pointlessly with the irq actions for no reason except
>>> for lack of understanding how the wakeup mechanism works.
>>>
>>> On supsend the core disables all interrupts lazily, i.e. it does not
>>> mask them at the irq controller level. So any interrupt which is
>>> firing during supsend will mark the corresponding interrupt line as
>> s/supsend/suspend/ twice
>>> pending. Just before the core powers down it checks whether there are
>>> interrupts pending from interrupt lines which are marked as wakeup
>>> sources and if so it aborts the resume and resends the interrupts.
>> It's the suspend that is aborted, not the resume.
>>
>> Other than that your change looks fine.
>>
> For pxa910 and MMP2, wake up source only wake up the AP subsystem.
> The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
> Now the core is still powered down. APMU will check the interrupt
> lines, and find
> that there are interrupt pending, it will power on the cores.
> So if the irq is disabled, even wake up source can wake up AP subsystem, but the
> core is still powered down. It will not be powered up by APMU.
>
Yes, suspend/resume can't work if the above code is removed.
Interrupt source (logic AND with interrupt mask register) is connected
to MPMU as
wakeup source. If the interrupt is disabled, there's no wakeup source event.
And APMU is waken up by MPMU.
So please don't remove the above code. We must keep these interrupt lines active
to wake up the whole system.
Regards
Haojian
On Mon, 24 Feb 2014, Chao Xie wrote:
> On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
> <[email protected]> wrote:
> > Hi Thomas,
> >
> > On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
> >> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
> >> callbacks fiddle pointlessly with the irq actions for no reason except
> >> for lack of understanding how the wakeup mechanism works.
> >>
> >> On supsend the core disables all interrupts lazily, i.e. it does not
> >> mask them at the irq controller level. So any interrupt which is
> >> firing during supsend will mark the corresponding interrupt line as
> > s/supsend/suspend/ twice
> >> pending. Just before the core powers down it checks whether there are
> >> interrupts pending from interrupt lines which are marked as wakeup
> >> sources and if so it aborts the resume and resends the interrupts.
> > It's the suspend that is aborted, not the resume.
> >
> > Other than that your change looks fine.
> >
> For pxa910 and MMP2, wake up source only wake up the AP subsystem.
> The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
> Now the core is still powered down. APMU will check the interrupt
> lines, and find
> that there are interrupt pending, it will power on the cores.
> So if the irq is disabled, even wake up source can wake up AP subsystem, but the
> core is still powered down. It will not be powered up by APMU.
The interrupt is NOT disabled at the interrupt chip level. The core does:
suspend_device_irqs() {
__disable_irq() {
if (!desc->depth++)
irq_disable(desc) {
irq_state_set_disabled(desc);
if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
irq_state_set_masked(desc);
}
Your chip does not have a chip->irq_disable() callback installed, so
the interrupt is not masked at the controller level. So APMU sees the
interrupt enabled.
APMU does not care about the irq_desc->depth counter and the irq_data
IRQD_DISABLED state bit.
So this hackery is completely pointless.
Thanks,
tglx
On Mon, 24 Feb 2014, Haojian Zhuang wrote:
> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <[email protected]> wrote:
> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
> > <[email protected]> wrote:
> >> Hi Thomas,
> >>
> >> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
> >>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
> >>> callbacks fiddle pointlessly with the irq actions for no reason except
> >>> for lack of understanding how the wakeup mechanism works.
> >>>
> >>> On supsend the core disables all interrupts lazily, i.e. it does not
> >>> mask them at the irq controller level. So any interrupt which is
> >>> firing during supsend will mark the corresponding interrupt line as
> >> s/supsend/suspend/ twice
> >>> pending. Just before the core powers down it checks whether there are
> >>> interrupts pending from interrupt lines which are marked as wakeup
> >>> sources and if so it aborts the resume and resends the interrupts.
> >> It's the suspend that is aborted, not the resume.
> >>
> >> Other than that your change looks fine.
> >>
> > For pxa910 and MMP2, wake up source only wake up the AP subsystem.
> > The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
> > Now the core is still powered down. APMU will check the interrupt
> > lines, and find
> > that there are interrupt pending, it will power on the cores.
> > So if the irq is disabled, even wake up source can wake up AP subsystem, but the
> > core is still powered down. It will not be powered up by APMU.
> >
>
> Yes, suspend/resume can't work if the above code is removed.
>
> Interrupt source (logic AND with interrupt mask register) is connected
> to MPMU as
> wakeup source. If the interrupt is disabled, there's no wakeup source event.
>
> And APMU is waken up by MPMU.
>
> So please don't remove the above code. We must keep these interrupt lines active
> to wake up the whole system.
They are kept active at the interrupt controller level. You just
refuse to understand how the internals of the interrupt subsystem
work.
And even if you would need this flag, then fiddling with the irq desc
internals is a big NONO. There is a proper way to hand that in.
Thanks,
tglx
On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner <[email protected]> wrote:
> On Mon, 24 Feb 2014, Haojian Zhuang wrote:
>
>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <[email protected]> wrote:
>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
>> > <[email protected]> wrote:
>> >> Hi Thomas,
>> >>
>> >> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>> >>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>> >>> callbacks fiddle pointlessly with the irq actions for no reason except
>> >>> for lack of understanding how the wakeup mechanism works.
>> >>>
>> >>> On supsend the core disables all interrupts lazily, i.e. it does not
>> >>> mask them at the irq controller level. So any interrupt which is
>> >>> firing during supsend will mark the corresponding interrupt line as
>> >> s/supsend/suspend/ twice
>> >>> pending. Just before the core powers down it checks whether there are
>> >>> interrupts pending from interrupt lines which are marked as wakeup
>> >>> sources and if so it aborts the resume and resends the interrupts.
>> >> It's the suspend that is aborted, not the resume.
>> >>
>> >> Other than that your change looks fine.
>> >>
>> > For pxa910 and MMP2, wake up source only wake up the AP subsystem.
>> > The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
>> > Now the core is still powered down. APMU will check the interrupt
>> > lines, and find
>> > that there are interrupt pending, it will power on the cores.
>> > So if the irq is disabled, even wake up source can wake up AP subsystem, but the
>> > core is still powered down. It will not be powered up by APMU.
>> >
>>
>> Yes, suspend/resume can't work if the above code is removed.
>>
>> Interrupt source (logic AND with interrupt mask register) is connected
>> to MPMU as
>> wakeup source. If the interrupt is disabled, there's no wakeup source event.
>>
>> And APMU is waken up by MPMU.
>>
>> So please don't remove the above code. We must keep these interrupt lines active
>> to wake up the whole system.
>
> They are kept active at the interrupt controller level. You just
> refuse to understand how the internals of the interrupt subsystem
> work.
>
If no irq_disable callback is hooked, when do irq_disable, it will not
actually disable
the interrupt, it will depend on next time when the irq happens, the
handler will first mask
the interrupt as this interrupt never happens.
So after system suspended, the interrupt happens, but the device
driver will not recieve this interrupt
because it is masked.
It results in that the device driver miss a important interrupt which
related to something need to be
handled. If user application for example android has power managment
daemon. It will find that nothing
to handle, it will make the system enter suspend again.
> And even if you would need this flag, then fiddling with the irq desc
> internals is a big NONO. There is a proper way to hand that in.
>
So can you suggest the proper way to handle it? Thanks.
> Thanks,
>
> tglx
>
On Thu, Feb 27, 2014 at 9:37 AM, Chao Xie <[email protected]> wrote:
> On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner <[email protected]> wrote:
>> On Mon, 24 Feb 2014, Haojian Zhuang wrote:
>>
>>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <[email protected]> wrote:
>>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
>>> > <[email protected]> wrote:
>>> >> Hi Thomas,
>>> >>
>>> >> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>>> >>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>>> >>> callbacks fiddle pointlessly with the irq actions for no reason except
>>> >>> for lack of understanding how the wakeup mechanism works.
>>> >>>
>>> >>> On supsend the core disables all interrupts lazily, i.e. it does not
>>> >>> mask them at the irq controller level. So any interrupt which is
>>> >>> firing during supsend will mark the corresponding interrupt line as
>>> >> s/supsend/suspend/ twice
>>> >>> pending. Just before the core powers down it checks whether there are
>>> >>> interrupts pending from interrupt lines which are marked as wakeup
>>> >>> sources and if so it aborts the resume and resends the interrupts.
>>> >> It's the suspend that is aborted, not the resume.
>>> >>
>>> >> Other than that your change looks fine.
>>> >>
>>> > For pxa910 and MMP2, wake up source only wake up the AP subsystem.
>>> > The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
>>> > Now the core is still powered down. APMU will check the interrupt
>>> > lines, and find
>>> > that there are interrupt pending, it will power on the cores.
>>> > So if the irq is disabled, even wake up source can wake up AP subsystem, but the
>>> > core is still powered down. It will not be powered up by APMU.
>>> >
>>>
>>> Yes, suspend/resume can't work if the above code is removed.
>>>
>>> Interrupt source (logic AND with interrupt mask register) is connected
>>> to MPMU as
>>> wakeup source. If the interrupt is disabled, there's no wakeup source event.
>>>
>>> And APMU is waken up by MPMU.
>>>
>>> So please don't remove the above code. We must keep these interrupt lines active
>>> to wake up the whole system.
>>
>> They are kept active at the interrupt controller level. You just
>> refuse to understand how the internals of the interrupt subsystem
>> work.
>>
> If no irq_disable callback is hooked, when do irq_disable, it will not
> actually disable
> the interrupt, it will depend on next time when the irq happens, the
> handler will first mask
> the interrupt as this interrupt never happens.
> So after system suspended, the interrupt happens, but the device
> driver will not recieve this interrupt
> because it is masked.
> It results in that the device driver miss a important interrupt which
> related to something need to be
> handled. If user application for example android has power managment
> daemon. It will find that nothing
> to handle, it will make the system enter suspend again.
>
Let me list the logic to make it easier to understand.
suspend_enter()
--> dpm_suspend_end()
--> dpm_suspend_noirq()
--> suspend_device_irqs()
--> __disable_irq()
--> set IRQS_SUSPENDED && call
irq_disable() if necessary
--> syscore_suspend()
--> check_wakeup_irqs()
If there's no pending irq in suspend process &&
IRQS_SUSPENDED is set,
then mask the irq.
Yes, we didn't implement disable_irq(). But we must implement mask_irq().
So system suspends. Then system will never be waken up by this irq any
more since
it's masked.
>> And even if you would need this flag, then fiddling with the irq desc
>> internals is a big NONO. There is a proper way to hand that in.
>>
>
> So can you suggest the proper way to handle it? Thanks.
>
>> Thanks,
>>
>> tglx
>>
On Thu, 27 Feb 2014, Haojian Zhuang wrote:
> Let me list the logic to make it easier to understand.
>
> suspend_enter()
> --> dpm_suspend_end()
> --> dpm_suspend_noirq()
> --> suspend_device_irqs()
> --> __disable_irq()
> --> set IRQS_SUSPENDED && call
> irq_disable() if necessary
> --> syscore_suspend()
> --> check_wakeup_irqs()
> If there's no pending irq in suspend process &&
> IRQS_SUSPENDED is set,
> then mask the irq.
>
> Yes, we didn't implement disable_irq(). But we must implement mask_irq().
>
> So system suspends. Then system will never be waken up by this irq any
> more since
> it's masked.
This is so wrong, it's not even funny anymore.
check_wakeup_irqs()
{
for_each_irq_desc(irq, desc) {
if (irqd_is_wakeup_set(&desc->irq_data)) {
if (desc->depth == 1 && desc->istate & IRQS_PENDING)
return -EBUSY;
continue;
}
So all interrupt lines which have been marked as wakeup sources are
not masked. And we only mask the other lines if the irq chip has the
IRQCHIP_MASK_ON_SUSPEND flag set.
if (desc->istate & IRQS_SUSPENDED &&
irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
mask_irq(desc);
}
The interrupts which can wake up your system fall into the
irqd_is_wakeup_set() clause. So nothing masks the interrupts at these
interrupt controller level. Your chip does not have the
IRQCHIP_MASK_ON_SUSPEND flag set either, so not a single interrupt
line gets masked.
The only thing you do with your hackery is to avoid that the interrupt
is marked disabled. And that means that you violate the rules of the
suspend logic.
We lazy disable the interrupts when we go into suspend in order to
abort the suspend when a wakeup interrupt happens between the
suspend_device_irqs() and check_wakeup_irqs(). Your hackery allows the
system to handle the interrupt, so we have no indication that the
suspend should be aborted.
You insist, that the interrupt line is masked at the irq chip level on
suspend, but you completely fail to explain how this should
happen. All you came up with so far is handwaving and a proper proof
of incompetence.
I'm really start to get grumpy about this utter waste of time.
Thanks,
tglx