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

Eduardo Valentin eduardo.valentin at nokia.com
Thu Jan 6 01:24:04 EST 2011


On Wed, Jan 05, 2011 at 03:22:51PM -0800, ext Kevin Hilman wrote:
> Eduardo Valentin <eduardo.valentin at nokia.com> writes:
> 
> > 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:
> >> > 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?
> >
> 
> As Russell pointed out, it's not actually "supposed" to be.
> 
> With lazy disable, what disable_irq() does is prevent the *handler* from
> ever being called.  If another interrupt arrives, it will be caught by
> the genirq core, marked as IRQ_PENDING and then masked.  This "don't
> disable unless we really have to" is the desired behavior of the lazy
> disable feature.

Right. I'm convinced that the handler won't be called because of the lazy
disable mechanism.

> 
> >> 
> >> 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.
> 
> If the GPIO is configured as a wakeup source, then wouldn't you want
> activity on that GPIO to wake up the system?

Yes I would want it.. of course, if the interrupt is enabled though..

I'm still trying to find a valid situation where someone disables an irq
but still wants its activity to be a wakeup source. I couldn't find so far..


> 
> If you don't want wakeups on that GPIO, then the driver should probably
> be using disable_irq_wake().

Yes. Let's take this situation. Let's assume a driver, at its suspend callback,
explicitly reports to the system that its irq can be disabled and also should
not be seen as a wakeup source, by calling disable_irq(gpio_irq) and
disable_irq_wake(gpio_irq).

What should be the expected system behavior when the user says echo mem > /sys/power/state?



More information about the linux-arm-kernel mailing list