| Message ID | 20140223212737.214342433@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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 > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@gmail.com> wrote: > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> 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 > <u.kleine-koenig@pengutronix.de> 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 <xiechao.mail@gmail.com> wrote: > > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> 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 <tglx@linutronix.de> wrote: > On Mon, 24 Feb 2014, Haojian Zhuang wrote: > >> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@gmail.com> wrote: >> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König >> > <u.kleine-koenig@pengutronix.de> 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 <xiechao.mail@gmail.com> wrote: > On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner <tglx@linutronix.de> wrote: >> On Mon, 24 Feb 2014, Haojian Zhuang wrote: >> >>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@gmail.com> wrote: >>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König >>> > <u.kleine-koenig@pengutronix.de> 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
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) {
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 <tglx@linutronix.de> Cc: arm <linux-arm-kernel@lists.infradead.org> Cc: Eric Miao <eric.y.miao@gmail.com> Cc: Haojian Zhuang <haojian.zhuang@gmail.com> Cc: Russell King <linux@arm.linux.org.uk> --- arch/arm/mach-mmp/pm-mmp2.c | 16 +--------------- arch/arm/mach-mmp/pm-pxa910.c | 20 ++++---------------- 2 files changed, 5 insertions(+), 31 deletions(-)