linux-kernel - RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system

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]

Date:   Tue, 21 May 2019 06:36:17 +0000
From:   Anson Huang <anson.huang@....com>
To:     "dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>
CC:     "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake
 up system

Hi, Dmitry

> -----Original Message-----
> From: dmitry.torokhov@...il.com [mailto:dmitry.torokhov@...il.com]
> Sent: Tuesday, May 21, 2019 1:31 PM
> To: Anson Huang <anson.huang@....com>
> Cc: shawnguo@...nel.org; s.hauer@...gutronix.de;
> kernel@...gutronix.de; festevam@...il.com; linux-input@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; dl-linux-
> imx <linux-imx@....com>
> Subject: Re: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
> 
> Hi Anson,
> On Thu, Apr 04, 2019 at 01:40:16AM +0000, Anson Huang wrote:
> > There are several scenarios that keyboard can NOT wake up system from
> > suspend, e.g., if a keyboard is depressed between system device
> > suspend phase and device noirq suspend phase, the keyboard ISR will be
> > called and both keyboard depress and release interrupts will be
> > disabled, then keyboard will no longer be able to wake up system.
> > Another scenario would be, if a keyboard is kept depressed, and then
> > system goes into suspend, the expected behavior would be when keyboard
> > is released, system will be waked up, but current implementation can
> > NOT achieve that, because both depress and release interrupts are
> > disabled in ISR, and the event check is still in progress.
> >
> > To fix these issues, need to make sure keyboard's depress or release
> > interrupt is enabled after noirq device suspend phase, this patch
> > moves the suspend/resume callback to noirq suspend/resume phase, and
> > enable the corresponding interrupt according to current keyboard status.
> 
> I believe it is possible for IRQ to be disabled and still  being enabled as
> wakeup source. What happens if you call disable_irq() before disabling the
> clock?

Doing below does NOT fix the scenario/issue 100%, if the keypad's IRQ arrived during suspend
phase but before disabling its IRQ in its suspend callback, then issue is still there, as the issue is
that when system suspend, keypad's irq arrived during suspend and noirq suspend phase, then
keypad's hardware interrupt detection will be disabled in the ISR handler, and the timer event
setup by ISR handler is NOT fired, imx_keypad_check_for_events() is NOT executed and hardware
keypad's depress/release interrupt is NOT re-enabled yet, so it can NOT wake up system anymore...

So I think the solid fix is to make sure keypad can generate IRQ (either depress or release) at any time
during system suspend flow.

+++ b/drivers/input/keyboard/imx_keypad.c
@@ -533,6 +533,8 @@ static int __maybe_unused imx_kbd_suspend(struct device *dev)
        /* imx kbd can wake up system even clock is disabled */
        mutex_lock(&input_dev->mutex);

+       disable_irq(kbd->irq);
+
        if (input_dev->users)
                clk_disable_unprepare(kbd->clk);


@@ -562,6 +569,8 @@ static int __maybe_unused imx_kbd_resume(struct device *dev)
                        goto err_clk;
        }

+       enable_irq(kbd->irq);
+
 err_clk:

Anson.

> 
> Thanks.
> 
> --
> Dmitry