Suspending with keypad bug (RFC)

Suspending with keypad bug (RFC)

[andrzej zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

Hi
  When the keypad driver is compiled in, the kernel won't resume from
a sleep state if any key on the keypad was pressed during suspending.
This is because the keypad driver masks the keyboard interrupt when
the irq is received and doesn't unmask it until after keys are
released. I don't know if it's true for all CPUs, it happens on my
board which is 15xx.

This may often be a problem when one of the keys is a power key (like
on PDAs) and you're in a console. If your device doesn't have UART2 it
effectively means a lock-up.

I unmasked the keyboard interrupt just before suspending, in pm.c and
this prevents the lock-ups but I don't know if it's a correct fix.
This results in that the device immediately resumes if any key is
still pressed after entering idle.

I came up with a different fix, in the keypad driver but it's a bit
more intrusive. With this the device sleeps peacefully until another
key-press is detected. What should be the correct behavior?
I'm attaching both changes.

Another approach could be to mask the irq in the MPU interrupt handler
registers instead of with keypad registers, maybe.
Regards,
Andrzej

[-- Attachment #2: linux-keypad-suspend0.patch --]
[-- Type: application/octet-stream, Size: 827 bytes --]

--- linux-omap-2.6-git/arch/arm/mach-omap1/pm.c	2006-07-06 02:47:56.000000000 +0000
+++ linux-omap-2.6-newbuild/arch/arm/mach-omap1/pm.c	2006-08-05 00:16:38.000000000 +0000
@@ -60,6 +60,7 @@
 #include <asm/arch/dma.h>
 #include <asm/arch/dsp_common.h>
 #include <asm/arch/dmtimer.h>
+#include <asm/arch/gpio.h>
 
 static unsigned int arm_sleep_save[ARM_SLEEP_SAVE_SIZE];
 static unsigned short dsp_sleep_save[DSP_SLEEP_SAVE_SIZE];
@@ -222,6 +221,9 @@ static void omap_pm_wakeup_setup(void)
 				OMAP_IRQ_BIT(INT_730_MPUIO_KEYPAD)),
 				OMAP_IH2_1_MIR);
 	} else if (cpu_is_omap15xx()) {
+		/* Make sure INT_KEYBOARD is enabled in MPUIO */
+		omap_writew(0, OMAP_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
+
 		level2_wake |= OMAP_IRQ_BIT(INT_KEYBOARD);
 		omap_writel(~level2_wake,  OMAP_IH2_MIR);
 	} else if (cpu_is_omap16xx()) {

[-- Attachment #3: linux-keypad-suspend1.patch --]
[-- Type: application/octet-stream, Size: 1540 bytes --]

--- linux-omap-2.6-git/drivers/input/keyboard/omap-keypad.c	2006-07-06 02:48:09.000000000 +0000
+++ linux-omap-2.6-newbuild/drivers/input/keyboard/omap-keypad.c	2006-08-05 00:40:54.000000000 +0000
@@ -59,6 +59,7 @@ struct omap_kp {
 	int irq;
 	unsigned int rows;
 	unsigned int cols;
+	int suspended;
 };
 
 DECLARE_TASKLET_DISABLED(kp_tasklet, omap_kp_tasklet, 0);
@@ -100,6 +101,9 @@ static irqreturn_t omap_kp_interrupt(int
 {
 	struct omap_kp *omap_kp = dev_id;
 
+	if (omap_kp->suspended)
+		return IRQ_HANDLED;
+
 	/* disable keyboard interrupt and schedule for handling */
 	if (cpu_is_omap24xx()) {
 		int i;
@@ -269,15 +273,24 @@ static DEVICE_ATTR(enable, S_IRUGO | S_I
 #ifdef CONFIG_PM
 static int omap_kp_suspend(struct platform_device *dev, pm_message_t state)
 {
-	/* Nothing yet */
+	struct omap_kp *omap_kp = platform_get_drvdata(dev);
+
+	omap_kp->suspended = 1;
+	/*
+	 * Re-enable the interrupt in case it has been masked by the
+	 * handler and a key is still pressed.  We need the interrupt
+	 * to wake us up from suspended.
+	 */
+	omap_writew(0, OMAP_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
 
 	return 0;
 }
 
 static int omap_kp_resume(struct platform_device *dev)
 {
-	/* Nothing yet */
+	struct omap_kp *omap_kp = platform_get_drvdata(dev);
 
+	omap_kp->suspended = 0;
 	return 0;
 }
 #else
@@ -349,6 +362,8 @@ static int __init omap_kp_probe(struct p
 		}
 	}
 
+	omap_kp->suspended = 0;
+
 	init_timer(&omap_kp->timer);
 	omap_kp->timer.function = omap_kp_timer;
 	omap_kp->timer.data = (unsigned long) omap_kp;

[-- Attachment #4: Type: text/plain, Size: 0 bytes --]

Re: Suspending with keypad bug (RFC)

[Juha Yrjölä]

andrzej zaborowski wrote:

> I came up with a different fix, in the keypad driver but it's a bit
> more intrusive. With this the device sleeps peacefully until another
> key-press is detected. What should be the correct behavior?
> I'm attaching both changes.

This approach is better; the generic PM code should not have to care 
that much about different wakeup sources.

I'm not exactly sure in which state the system is when the suspend 
function is called.  Can there still be timer activity, for example? 
Nevertheless, you're safer doing a spin_lock_irqsave() in the suspend 
function first, and releasing the lock only after setting the suspended 
variable and re-enabling the interrupt.

You should also check whether you're actually running on OMAP1 before 
you write to the register.

In the IRQ handler, you should also do a spin_lock_irqsave() before 
accessing the suspended variable.

Cheers,
Juha

Re: Suspending with keypad bug (RFC)

[andrzej zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

On 06/08/06, Juha Yrjölä <juha.yrjola@solidboot.com> wrote:
> andrzej zaborowski wrote:
>
> > I came up with a different fix, in the keypad driver but it's a bit
> > more intrusive. With this the device sleeps peacefully until another
> > key-press is detected. What should be the correct behavior?
> > I'm attaching both changes.
>
> This approach is better; the generic PM code should not have to care
> that much about different wakeup sources.
>
> I'm not exactly sure in which state the system is when the suspend
> function is called.  Can there still be timer activity, for example?

I think yes, some suspend functions may rely on timers because they
are not obliged to return immdiately.

> Nevertheless, you're safer doing a spin_lock_irqsave() in the suspend
> function first, and releasing the lock only after setting the suspended
> variable and re-enabling the interrupt.
>
> You should also check whether you're actually running on OMAP1 before
> you write to the register.
>
> In the IRQ handler, you should also do a spin_lock_irqsave() before
> accessing the suspended variable.

Thanks for help! I'm attaching the corrected version, it works nicely.

In the irq handler I release the lock just after accessing the
"suspended" variable because as I understand an irq handler will never
be interrupted by something that calls the suspend function.

>
> Cheers,
> Juha
>


-- 
balrog 2oo6

[-- Attachment #2: linux-keypad-suspend2.patch --]
[-- Type: application/octet-stream, Size: 2112 bytes --]

--- a/drivers/input/keyboard/omap-keypad.c	2006-08-05 05:18:01.000000000 +0000
+++ b/drivers/input/keyboard/omap-keypad.c	2006-08-10 06:19:11.000000000 +0000
@@ -33,6 +33,7 @@
 #include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <asm/arch/irqs.h>
 #include <asm/arch/gpio.h>
 #include <asm/arch/hardware.h>
@@ -59,6 +60,8 @@ struct omap_kp {
 	int irq;
 	unsigned int rows;
 	unsigned int cols;
+	int suspended;
+	spinlock_t suspend_lock;
 };
 
 DECLARE_TASKLET_DISABLED(kp_tasklet, omap_kp_tasklet, 0);
@@ -99,6 +102,13 @@ static irqreturn_t omap_kp_interrupt(int
 				     struct pt_regs *regs)
 {
 	struct omap_kp *omap_kp = dev_id;
+	unsigned long flags;
+	spin_lock_irqsave(&omap_kp->suspend_lock, flags);
+
+	if (omap_kp->suspended)
+		return IRQ_HANDLED;
+
+	spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);
 
 	/* disable keyboard interrupt and schedule for handling */
 	if (cpu_is_omap24xx()) {
@@ -271,15 +281,29 @@ static DEVICE_ATTR(enable, S_IRUGO | S_I
 #ifdef CONFIG_PM
 static int omap_kp_suspend(struct platform_device *dev, pm_message_t state)
 {
-	/* Nothing yet */
+	struct omap_kp *omap_kp = platform_get_drvdata(dev);
+	unsigned long flags;
+	spin_lock_irqsave(&omap_kp->suspend_lock, flags);
+
+	/*
+	 * Re-enable the interrupt in case it has been masked by the
+	 * handler and a key is still pressed.  We need the interrupt
+	 * to wake us up from suspended.
+	 */
+	if (cpu_class_is_omap1())
+		omap_writew(0, OMAP_MPUIO_BASE + OMAP_MPUIO_KBD_MASKIT);
 
+	omap_kp->suspended = 1;
+
+	spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);
 	return 0;
 }
 
 static int omap_kp_resume(struct platform_device *dev)
 {
-	/* Nothing yet */
+	struct omap_kp *omap_kp = platform_get_drvdata(dev);
 
+	omap_kp->suspended = 0;
 	return 0;
 }
 #else
@@ -351,6 +375,9 @@ static int __init omap_kp_probe(struct p
 		}
 	}
 
+	spin_lock_init(&omap_kp->suspend_lock);
+	omap_kp->suspended = 0;
+
 	init_timer(&omap_kp->timer);
 	omap_kp->timer.function = omap_kp_timer;
 	omap_kp->timer.data = (unsigned long) omap_kp;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: Suspending with keypad bug (RFC)

[Tony Lindgren]

* andrzej zaborowski <balrog@zabor.org> [060810 07:56]:
> On 06/08/06, Juha Yrjölä <juha.yrjola@solidboot.com> wrote:
> >andrzej zaborowski wrote:
> >
> >> I came up with a different fix, in the keypad driver but it's a bit
> >> more intrusive. With this the device sleeps peacefully until another
> >> key-press is detected. What should be the correct behavior?
> >> I'm attaching both changes.
> >
> >This approach is better; the generic PM code should not have to care
> >that much about different wakeup sources.
> >
> >I'm not exactly sure in which state the system is when the suspend
> >function is called.  Can there still be timer activity, for example?
> 
> I think yes, some suspend functions may rely on timers because they
> are not obliged to return immdiately.
> 
> >Nevertheless, you're safer doing a spin_lock_irqsave() in the suspend
> >function first, and releasing the lock only after setting the suspended
> >variable and re-enabling the interrupt.
> >
> >You should also check whether you're actually running on OMAP1 before
> >you write to the register.
> >
> >In the IRQ handler, you should also do a spin_lock_irqsave() before
> >accessing the suspended variable.
> 
> Thanks for help! I'm attaching the corrected version, it works nicely.
> 
> In the irq handler I release the lock just after accessing the
> "suspended" variable because as I understand an irq handler will never
> be interrupted by something that calls the suspend function.

Can you please repost with proper comments and Signed-off-by?

Thanks,

Tony

Re: Suspending with keypad bug (RFC)

[Juha Yrjölä]

Hi,

On Thu, Aug 10, 2006 at 06:55:26AM +0200, andrzej zaborowski wrote:

> Thanks for help! I'm attaching the corrected version, it works nicely.

+       unsigned long flags;
+       spin_lock_irqsave(&omap_kp->suspend_lock, flags);
+
+       if (omap_kp->suspended)
+               return IRQ_HANDLED;
+
+       spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);

Here you are returning with the lock held and IRQs disabled.  It should be
like this:

       spin_lock_irqsave(&omap_kp->suspend_lock, flags);
       if (omap_kp->suspended) {
               spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);
               return IRQ_HANDLED;
       }
       spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);

Cheers,
Juha

Re: Suspending with keypad bug (RFC)

[andrzej zaborowski]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

On 13/08/06, Juha Yrjölä <juha.yrjola@solidboot.com> wrote:
> Hi,
>
> On Thu, Aug 10, 2006 at 06:55:26AM +0200, andrzej zaborowski wrote:
>
> > Thanks for help! I'm attaching the corrected version, it works nicely.
>
> +       unsigned long flags;
> +       spin_lock_irqsave(&omap_kp->suspend_lock, flags);
> +
> +       if (omap_kp->suspended)
> +               return IRQ_HANDLED;
> +
> +       spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);
>
> Here you are returning with the lock held and IRQs disabled.  It should be
> like this:
>
>        spin_lock_irqsave(&omap_kp->suspend_lock, flags);
>        if (omap_kp->suspended) {
>                spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);
>                return IRQ_HANDLED;
>        }
>        spin_unlock_irqrestore(&omap_kp->suspend_lock, flags);

Oops, I must be blind! I'm going to send a corrected version. Thanks again.

>
> Cheers,
> Juha
>

Regards,
Andrzej
-- 
balrog 2oo6

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]