[PATCH 1/1] arm: omap: gpio: define .disable callback for gpio irq chip

Currently, if one calls disable_irq(gpio_irq), the irq
won't get disabled.

This is happening because the omap gpio code defines only
a .mask callback. And the default_disable function is just
a stub. The result is that, when someone calls disable_irq
for an irq in a gpio line, it will be kept enabled.

This patch solves this issue by setting the .disable
callback to point to the same .mask callback.

Signed-off-by: Eduardo Valentin <redacted>
---
 arch/arm/plat-omap/gpio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index c05c653..033197f 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c

@@ -1433,6 +1433,7 @@ static struct irq_chip gpio_irq_chip = {
 	.shutdown	= gpio_irq_shutdown,
 	.ack		= gpio_ack_irq,
 	.mask		= gpio_mask_irq,
+	.disable	= gpio_mask_irq,
 	.unmask		= gpio_unmask_irq,
 	.set_type	= gpio_irq_type,
 	.set_wake	= gpio_wake_enable,

@@ -1469,6 +1470,7 @@ static struct irq_chip mpuio_irq_chip = {
 	.name		= "MPUIO",
 	.ack		= mpuio_ack_irq,
 	.mask		= mpuio_mask_irq,
+	.disable	= mpuio_mask_irq,
 	.unmask		= mpuio_unmask_irq,
 	.set_type	= gpio_irq_type,
 #ifdef CONFIG_ARCH_OMAP16XX

-- 
1.6.3.GIT

On Wed, Jan 05, 2011 at 07:58:03PM +0200, Eduardo Valentin wrote:

Currently, if one calls disable_irq(gpio_irq), the irq
won't get disabled.

This is happening because the omap gpio code defines only
a .mask callback. And the default_disable function is just
a stub. The result is that, when someone calls disable_irq
for an irq in a gpio line, it will be kept enabled.

This patch solves this issue by setting the .disable
callback to point to the same .mask callback.

Amd this is a problem because?

The way this works is that although it isn't disabled at that point,
if it never triggers, then everything remains happy.  However, if it
does trigger, the genirq code will then mask the interrupt and won't
call the handler.

Hello Russell,

On Wed, Jan 05, 2011 at 06:19:18PM +0000, Russell King wrote:

On Wed, Jan 05, 2011 at 07:58:03PM +0200, Eduardo Valentin wrote:

quoted

Currently, if one calls disable_irq(gpio_irq), the irq
won't get disabled.

This is happening because the omap gpio code defines only
a .mask callback. And the default_disable function is just
a stub. The result is that, when someone calls disable_irq
for an irq in a gpio line, it will be kept enabled.

This patch solves this issue by setting the .disable
callback to point to the same .mask callback.

Amd this is a problem because?

errr.. because the interrupt is enabled when it was supposed to be disabled?

The way this works is that although it isn't disabled at that point,
if it never triggers, then everything remains happy.  However, if it
does trigger, the genirq code will then mask the interrupt and won't
call the handler.

Right.. I didn't see from this point. I will check how that gets unmasked.
But even so, if I understood correctly what you described, it would still
open a time window which the system would see at least 1 interrupt during
the time it was not suppose to. And that can wakeup a system which  is in
deep sleep mode, either via dynamic idle or static suspend.

It is unlikely, I know. But it can still happen. And can be avoided.

Regards,

-- 
Eduardo Valentin

On Wed, Jan 05, 2011 at 09:24:25PM +0200, Eduardo Valentin wrote:

quoted

The way this works is that although it isn't disabled at that point,
if it never triggers, then everything remains happy.  However, if it
does trigger, the genirq code will then mask the interrupt and won't
call the handler.

Right.. I didn't see from this point. I will check how that gets unmasked.
But even so, if I understood correctly what you described, it would still
open a time window which the system would see at least 1 interrupt during
the time it was not suppose to. And that can wakeup a system which  is in
deep sleep mode, either via dynamic idle or static suspend.

It is unlikely, I know. But it can still happen. And can be avoided.

Maybe a system going into deep sleep mode should update the masked state
of the interrupts to reflect the lazy-disable state?

o lazy-disable state

This is an odd state, and confusion regularly
comes up ... I've never been a fan of having the
imperatively named disable_irq() act like a

disable_irq_at a_random_later_time_ _but_nyet().  If
one must have the latter function, clearer IMO
to name it better and have disable_irq()
do exactly that by the time it returns ... that
is after all what folk expect given its name and conventional interpretation of "disable".
 (ergo confusion when that's not what happens)

lazy_disable_irq() would be accurate, butI'm not
sure many folk would choose to use it.

- Dave