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

Anson Huang
2019-06-10T06:44:08Z

Hi, Dmitry
Any feedback for this patch?

Thanks,
Anson

> -----Original Message-----
> From: Anson Huang
> Sent: Tuesday, May 21, 2019 2:36 PM
> To: dmitry.torokhov@gmail.com
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; linux-input@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-
> imx <linux-imx@nxp.com>
> Subject: RE: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
>
> Hi, Dmitry
>
> > -----Original Message-----
> > From: dmitry.torokhov@gmail.com [mailto:dmitry.torokhov@gmail.com]
> > Sent: Tuesday, May 21, 2019 1:31 PM
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; festevam@gmail.com;
> > linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; dl-linux- imx <linux-imx@nxp.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