linux-kernel - Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAN1soZzipHL=0i4gH3B7PwHaOJh6=_uwv5u0e7W2MsvARp_jLA@mail.gmail.com>
Date:	Thu, 27 Feb 2014 10:19:41 +0800
From:	Haojian Zhuang <haojian.zhuang@...il.com>
To:	Chao Xie <xiechao.mail@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Eric Miao <eric.y.miao@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Russell King <linux@....linux.org.uk>,
	Ingo Molnar <mingo@...e.hu>,
	arm <linux-arm-kernel@...ts.infradead.org>, fwu@...vell.com
Subject: Re: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

On Thu, Feb 27, 2014 at 9:37 AM, Chao Xie <xiechao.mail@...il.com> wrote:
> On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner <tglx@...utronix.de> wrote:
>> On Mon, 24 Feb 2014, Haojian Zhuang wrote:
>>
>>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@...il.com> wrote:
>>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-König
>>> > <u.kleine-koenig@...gutronix.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
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/