Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Thu, Feb 27, 2014 at 9:37 AM, Chao Xie wrote: > On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner wrote: >> On Mon, 24 Feb 2014, Haojian Zhuang wrote: >> >>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie wrote: >>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König >>> > wrote: >>> >> Hi Thomas, >>> >> >>> >> On Sun, Feb 23, 2014 at 09:40:13PM -, 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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner wrote: > On Mon, 24 Feb 2014, Haojian Zhuang wrote: > >> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie wrote: >> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König >> > wrote: >> >> Hi Thomas, >> >> >> >> On Sun, Feb 23, 2014 at 09:40:13PM -, 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner t...@linutronix.de wrote: On Mon, 24 Feb 2014, Haojian Zhuang wrote: On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie xiechao.m...@gmail.com wrote: On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: Hi Thomas, On Sun, Feb 23, 2014 at 09:40:13PM -, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Thu, Feb 27, 2014 at 9:37 AM, Chao Xie xiechao.m...@gmail.com wrote: On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner t...@linutronix.de wrote: On Mon, 24 Feb 2014, Haojian Zhuang wrote: On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie xiechao.m...@gmail.com wrote: On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: Hi Thomas, On Sun, Feb 23, 2014 at 09:40:13PM -, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, 24 Feb 2014, Haojian Zhuang wrote: > On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie wrote: > > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König > > wrote: > >> Hi Thomas, > >> > >> On Sun, Feb 23, 2014 at 09:40:13PM -, 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
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, 24 Feb 2014, Chao Xie wrote:
> On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König
> wrote:
> > Hi Thomas,
> >
> > On Sun, Feb 23, 2014 at 09:40:13PM -, 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(>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
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, 24 Feb 2014, Chao Xie wrote:
On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König
u.kleine-koe...@pengutronix.de wrote:
Hi Thomas,
On Sun, Feb 23, 2014 at 09:40:13PM -, 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
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, 24 Feb 2014, Haojian Zhuang wrote: On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie xiechao.m...@gmail.com wrote: On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: Hi Thomas, On Sun, Feb 23, 2014 at 09:40:13PM -, 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
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie wrote: > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König > wrote: >> Hi Thomas, >> >> On Sun, Feb 23, 2014 at 09:40:13PM -, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König wrote: > Hi Thomas, > > On Sun, Feb 23, 2014 at 09:40:13PM -, 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-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
Hi Thomas, On Sun, Feb 23, 2014 at 09:40:13PM -, 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/ | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
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
Cc: arm
Cc: Eric Miao
Cc: Haojian Zhuang
Cc: Russell King
---
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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
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 t...@linutronix.de
Cc: arm linux-arm-ker...@lists.infradead.org
Cc: Eric Miao eric.y.m...@gmail.com
Cc: Haojian Zhuang haojian.zhu...@gmail.com
Cc: Russell King li...@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(-)
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) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
Hi Thomas, On Sun, Feb 23, 2014 at 09:40:13PM -, 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/ | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: Hi Thomas, On Sun, Feb 23, 2014 at 09:40:13PM -, 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-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals
On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie xiechao.m...@gmail.com wrote: On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König u.kleine-koe...@pengutronix.de wrote: Hi Thomas, On Sun, Feb 23, 2014 at 09:40:13PM -, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
