linux-kernel - Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

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: <20120827085312.75957354@notabene.brown>
Date:	Mon, 27 Aug 2012 08:53:12 +1000
From:	NeilBrown <neilb@...e.de>
To:	"Shilimkar, Santosh" <santosh.shilimkar@...com>
Cc:	Tarun Kanti DebBarma <tarun.kanti@...com>,
	Kevin Hilman <khilman@...com>,
	Tony Lindgren <tony@...mide.com>, Cousson@...e.de,
	Benoit <b-cousson@...com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Felipe Balbi <balbi@...com>, linux-omap@...r.kernel.org,
	lkml <linux-kernel@...r.kernel.org>,
	Jon Hunter <jon-hunter@...com>
Subject: Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

On Sun, 26 Aug 2012 09:47:50 +0530 "Shilimkar, Santosh"
<santosh.shilimkar@...com> wrote:

> + Jon,
> 
> On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown <neilb@...e.de> wrote:
> >
> >
> >
> > Current kernel will wake from suspend on an event on any active
> > GPIO even if enable_irq_wake() wasn't called.
> >
> > There are two reasons that the hardware wake-enable bit should be set:
> >
> > 1/ while non-suspended the CPU might go into a deep sleep (off_mode)
> >   in which the wake-enable bit is needed for an interrupt to be
> >   recognised.
> > 2/ while suspended the GPIO interrupt should wake from suspend if and
> >    only if irq_wake as been enabled.
> >
> > The code currently doesn't keep these two reasons separate so they get
> > confused and sometimes the wakeup flags is set incorrectly.
> >
> > This patch reverts:
> >  commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc
> >     gpio/omap: remove suspend/resume callbacks
> > and
> >  commit 0aa2727399c0b78225021413022c164cb99fbc5e
> >     gpio/omap: remove suspend_wakeup field from struct gpio_bank
> >
> > and makes some minor changes so that we have separate flags for "GPIO
> > should wake from deep idle" and "GPIO should wake from suspend".
> >
> > With this patch, the GPIO from my touch screen doesn't wake my device
> > any more, which is what I want.
> >
> > Cc: Kevin Hilman <khilman@...com>
> > Cc: Tony Lindgren <tony@...mide.com>
> > Cc: Santosh Shilimkar <santosh.shilimkar@...com>
> > Cc: Cousson, Benoit <b-cousson@...com>
> > Cc: Grant Likely <grant.likely@...retlab.ca>
> > Cc: Tarun Kanti DebBarma <tarun.kanti@...com>
> > Cc: Felipe Balbi <balbi@...com>
> > Cc: Govindraj.R <govindraj.raja@...com>
> >
> > Signed-off-by: NeilBrown <neilb@...e.de>
> >
> The patch doesn't seems to be correct. At least the 2/ gets
> fixed with a proper IRQCHIP flag. Can you try the patch at
> end of the email and see if it helps ? Am attaching it in case
> mailer damages it.
> 
> Regards
> Santosh
> 
> >From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
> From: Santosh Shilimkar <santosh.shilimkar@...com>
> Date: Sun, 26 Aug 2012 09:39:51 +0530
> Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
>  non-wakeup gpio wakeups.
> 
> Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
> to mask all non-wake gpios in suspend, which will ensure the wakeup enable
> bit is not set on non-wake gpios.
> 
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@...com>
> ---
>  drivers/gpio/gpio-omap.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index e6efd77..50b4c18 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
>  	.irq_unmask	= gpio_unmask_irq,
>  	.irq_set_type	= gpio_irq_type,
>  	.irq_set_wake	= gpio_wake_enable,
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND;
>  };
> 
>  /*---------------------------------------------------------------------*/


No obvious damage, unless the mailer is responsible or the ';' at the end of
the line, rather than ',' :-)

The approach makes sense, but does actually work.  Should be fixable though.

When I try this I get:



[  158.114440] Checking wakeup interrupts
[  158.118408] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb054040
[  158.126403] Internal error: : 1028 [#1] PREEMPT ARM
[  158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas cfg80211
[  158.139190] CPU: 0    Not tainted  (3.5.0-gta04-debug+ #2)
[  158.144927] PC is at _set_gpio_triggering+0x38/0x258
[  158.150115] LR is at gpio_mask_irq+0xac/0xc0
[  158.154602] pc : [<c01d24a0>]    lr : [<c01d2f68>]    psr: 60000193
[  158.154602] sp : db521e90  ip : 00000011  fp : beeecc2c
[  158.166595] r10: c05c8ebc  r9 : daa5a858  r8 : 00000003
[  158.172027] r7 : a0000193  r6 : 00000000  r5 : fb054000  r4 : ded44e18
[  158.178863] r3 : 00000001  r2 : 00000000  r1 : ded30340  r0 : 00000040
[  158.185668] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment use

so it looks like runtime PM has turned off the iclk to the GPIO module so that
when we try to tell it to change settings, it is no longer listening to us.
The "Checking wakeup interrupts" function happens very late in the suspend
cycle, after all the suspend_late and suspend_noirq functions have run.
Maybe it needs to be moved earlier...

Thanks,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)