[PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[Kevin Hilman]

Now that the generic IRQ and GPIO frameworks are used for enabling and
disabling GPIO IRQ wakeup sources, there is no longer a need to call
[enable|disable]_irq_wake() in the low-level code.  Doing so results
in recursive calls to [enable|disable]_irq_wake().

This was discovered in the suspend/resume path on OMAP3/Beagle using
the gpio-keys driver which disables/re-enables GPIO IRQ wakeups in the
suspend/resume path.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/plat-omap/gpio.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index f856a90..798a8cd 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -837,13 +837,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
 	case METHOD_MPUIO:
 	case METHOD_GPIO_1610:
 		spin_lock_irqsave(&bank->lock, flags);
-		if (enable) {
+		if (enable)
 			bank->suspend_wakeup |= (1 << gpio);
-			enable_irq_wake(bank->irq);
-		} else {
-			disable_irq_wake(bank->irq);
+		else
 			bank->suspend_wakeup &= ~(1 << gpio);
-		}
 		spin_unlock_irqrestore(&bank->lock, flags);
 		return 0;
 #endif
@@ -856,13 +853,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
 			return -EINVAL;
 		}
 		spin_lock_irqsave(&bank->lock, flags);
-		if (enable) {
+		if (enable)
 			bank->suspend_wakeup |= (1 << gpio);
-			enable_irq_wake(bank->irq);
-		} else {
-			disable_irq_wake(bank->irq);
+		else
 			bank->suspend_wakeup &= ~(1 << gpio);
-		}
 		spin_unlock_irqrestore(&bank->lock, flags);
 		return 0;
 #endif
-- 
1.6.1

Re: [PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> Now that the generic IRQ and GPIO frameworks are used for enabling and
> disabling GPIO IRQ wakeup sources, there is no longer a need to call
> [enable|disable]_irq_wake() in the low-level code.  Doing so results
> in recursive calls to [enable|disable]_irq_wake().
>
> This was discovered in the suspend/resume path on OMAP3/Beagle using
> the gpio-keys driver which disables/re-enables GPIO IRQ wakeups in the
> suspend/resume path.
>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

Tony,

Not sure if it's too late, but this could go into omap-fixes too.

Kevin


> ---
>  arch/arm/plat-omap/gpio.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index f856a90..798a8cd 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -837,13 +837,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>  	case METHOD_MPUIO:
>  	case METHOD_GPIO_1610:
>  		spin_lock_irqsave(&bank->lock, flags);
> -		if (enable) {
> +		if (enable)
>  			bank->suspend_wakeup |= (1 << gpio);
> -			enable_irq_wake(bank->irq);
> -		} else {
> -			disable_irq_wake(bank->irq);
> +		else
>  			bank->suspend_wakeup &= ~(1 << gpio);
> -		}
>  		spin_unlock_irqrestore(&bank->lock, flags);
>  		return 0;
>  #endif
> @@ -856,13 +853,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>  			return -EINVAL;
>  		}
>  		spin_lock_irqsave(&bank->lock, flags);
> -		if (enable) {
> +		if (enable)
>  			bank->suspend_wakeup |= (1 << gpio);
> -			enable_irq_wake(bank->irq);
> -		} else {
> -			disable_irq_wake(bank->irq);
> +		else
>  			bank->suspend_wakeup &= ~(1 << gpio);
> -		}
>  		spin_unlock_irqrestore(&bank->lock, flags);
>  		return 0;
>  #endif
> -- 
> 1.6.1

Re: [PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[Tony Lindgren]

* Kevin Hilman <khilman@deeprootsystems.com> [090202 09:53]:
> Kevin Hilman <khilman@deeprootsystems.com> writes:
> 
> > Now that the generic IRQ and GPIO frameworks are used for enabling and
> > disabling GPIO IRQ wakeup sources, there is no longer a need to call
> > [enable|disable]_irq_wake() in the low-level code.  Doing so results
> > in recursive calls to [enable|disable]_irq_wake().
> >
> > This was discovered in the suspend/resume path on OMAP3/Beagle using
> > the gpio-keys driver which disables/re-enables GPIO IRQ wakeups in the
> > suspend/resume path.
> >
> > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> 
> Tony,
> 
> Not sure if it's too late, but this could go into omap-fixes too.

I'll add it after the previous omap-fixes are merged to the mainline.

Tony



> 
> Kevin
> 
> 
> > ---
> >  arch/arm/plat-omap/gpio.c |   14 ++++----------
> >  1 files changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> > index f856a90..798a8cd 100644
> > --- a/arch/arm/plat-omap/gpio.c
> > +++ b/arch/arm/plat-omap/gpio.c
> > @@ -837,13 +837,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
> >  	case METHOD_MPUIO:
> >  	case METHOD_GPIO_1610:
> >  		spin_lock_irqsave(&bank->lock, flags);
> > -		if (enable) {
> > +		if (enable)
> >  			bank->suspend_wakeup |= (1 << gpio);
> > -			enable_irq_wake(bank->irq);
> > -		} else {
> > -			disable_irq_wake(bank->irq);
> > +		else
> >  			bank->suspend_wakeup &= ~(1 << gpio);
> > -		}
> >  		spin_unlock_irqrestore(&bank->lock, flags);
> >  		return 0;
> >  #endif
> > @@ -856,13 +853,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
> >  			return -EINVAL;
> >  		}
> >  		spin_lock_irqsave(&bank->lock, flags);
> > -		if (enable) {
> > +		if (enable)
> >  			bank->suspend_wakeup |= (1 << gpio);
> > -			enable_irq_wake(bank->irq);
> > -		} else {
> > -			disable_irq_wake(bank->irq);
> > +		else
> >  			bank->suspend_wakeup &= ~(1 << gpio);
> > -		}
> >  		spin_unlock_irqrestore(&bank->lock, flags);
> >  		return 0;
> >  #endif
> > -- 
> > 1.6.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[David Brownell]

On Wednesday 28 January 2009, Kevin Hilman wrote:
> Now that the generic IRQ and GPIO frameworks are used for enabling and
> disabling GPIO IRQ wakeup sources, there is no longer a need to call
> [enable|disable]_irq_wake() in the low-level code.  Doing so results
> in recursive calls to [enable|disable]_irq_wake().

Could you clarify what actually made that requirement go away?

The recursion was introduced -- using the generic IRQ framework! -- as
a simple way to ensure the parent IRQ was properly wake-enabled.  Is
the issue that something changed, so that something else wake-enables
the parent?

- Dave


> This was discovered in the suspend/resume path on OMAP3/Beagle using
> the gpio-keys driver which disables/re-enables GPIO IRQ wakeups in the
> suspend/resume path.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/plat-omap/gpio.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index f856a90..798a8cd 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -837,13 +837,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>  	case METHOD_MPUIO:
>  	case METHOD_GPIO_1610:
>  		spin_lock_irqsave(&bank->lock, flags);
> -		if (enable) {
> +		if (enable)
>  			bank->suspend_wakeup |= (1 << gpio);
> -			enable_irq_wake(bank->irq);
> -		} else {
> -			disable_irq_wake(bank->irq);
> +		else
>  			bank->suspend_wakeup &= ~(1 << gpio);
> -		}
>  		spin_unlock_irqrestore(&bank->lock, flags);
>  		return 0;
>  #endif
> @@ -856,13 +853,10 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
>  			return -EINVAL;
>  		}
>  		spin_lock_irqsave(&bank->lock, flags);
> -		if (enable) {
> +		if (enable)
>  			bank->suspend_wakeup |= (1 << gpio);
> -			enable_irq_wake(bank->irq);
> -		} else {
> -			disable_irq_wake(bank->irq);
> +		else
>  			bank->suspend_wakeup &= ~(1 << gpio);
> -		}
>  		spin_unlock_irqrestore(&bank->lock, flags);
>  		return 0;
>  #endif
> -- 
> 1.6.1
> 
> 

Re: [PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[Kevin Hilman]

David Brownell <david-b@pacbell.net> writes:

> On Wednesday 28 January 2009, Kevin Hilman wrote:
>> Now that the generic IRQ and GPIO frameworks are used for enabling and
>> disabling GPIO IRQ wakeup sources, there is no longer a need to call
>> [enable|disable]_irq_wake() in the low-level code.  Doing so results
>> in recursive calls to [enable|disable]_irq_wake().
>
> Could you clarify what actually made that requirement go away?
>
> The recursion was introduced -- using the generic IRQ framework! -- as
> a simple way to ensure the parent IRQ was properly wake-enabled.  Is
> the issue that something changed, so that something else wake-enables
> the parent?
>

My description was not very descriptive... sorry...

>From what I can understand here, I don't see the point in
wake-enabling the parent IRQ since there is no wakeup glue for the
bank IRQs, thus these calls are actually failing and causing
'unbalanced IRQ disable' noise in the generic IRQ layer.

Here is what is happening.  Consider the gpio-keys driver.  Upon
suspend, it enables the IRQ wake for its GPIO.  The OMAP GPIO code
then calls enable_wake_irq() for the parent irq (the GPIO bank IRQ).
This call is actually failing because the bank IRQ has no 'set_wake'
method.  Because of this failure, the generic IRQ code doesn't
actually do anything, and sets the 'disable_depth' to zero for the
bank IRQ.

Then, upon resume, the resume path disables IRQ wakeups for the GPIO.
The OMAP GPIO code then tries to call disable_irq_wake() for the bank
IRQ and you get noisy 'unbalanced IRQ disable' warnings from the
generic IRQ layere because of the attempt to disable the IRQ wake of
an IRQ that was never enabled.

So the options that I see are:

1) fix the OMAP GPIO code to check the return code of the parent enable, or
2) drop the parent enable/disable

I prefer (1) since those calls will always fail AFAICT.

Does that make any sense?

If you're OK with (1), I will re-issue the patch with a better discription.

Kevin

Re: [PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[Tony Lindgren]

* Kevin Hilman <khilman@deeprootsystems.com> [090210 16:42]:
> David Brownell <david-b@pacbell.net> writes:
> 
> > On Wednesday 28 January 2009, Kevin Hilman wrote:
> >> Now that the generic IRQ and GPIO frameworks are used for enabling and
> >> disabling GPIO IRQ wakeup sources, there is no longer a need to call
> >> [enable|disable]_irq_wake() in the low-level code.  Doing so results
> >> in recursive calls to [enable|disable]_irq_wake().
> >
> > Could you clarify what actually made that requirement go away?
> >
> > The recursion was introduced -- using the generic IRQ framework! -- as
> > a simple way to ensure the parent IRQ was properly wake-enabled.  Is
> > the issue that something changed, so that something else wake-enables
> > the parent?
> >
> 
> My description was not very descriptive... sorry...
> 
> From what I can understand here, I don't see the point in
> wake-enabling the parent IRQ since there is no wakeup glue for the
> bank IRQs, thus these calls are actually failing and causing
> 'unbalanced IRQ disable' noise in the generic IRQ layer.
> 
> Here is what is happening.  Consider the gpio-keys driver.  Upon
> suspend, it enables the IRQ wake for its GPIO.  The OMAP GPIO code
> then calls enable_wake_irq() for the parent irq (the GPIO bank IRQ).
> This call is actually failing because the bank IRQ has no 'set_wake'
> method.  Because of this failure, the generic IRQ code doesn't
> actually do anything, and sets the 'disable_depth' to zero for the
> bank IRQ.
> 
> Then, upon resume, the resume path disables IRQ wakeups for the GPIO.
> The OMAP GPIO code then tries to call disable_irq_wake() for the bank
> IRQ and you get noisy 'unbalanced IRQ disable' warnings from the
> generic IRQ layere because of the attempt to disable the IRQ wake of
> an IRQ that was never enabled.
> 
> So the options that I see are:
> 
> 1) fix the OMAP GPIO code to check the return code of the parent enable, or
> 2) drop the parent enable/disable
> 
> I prefer (1) since those calls will always fail AFAICT.
> 
> Does that make any sense?
> 
> If you're OK with (1), I will re-issue the patch with a better discription.

Ignoring this for now, please let me know if you want me to queue this
for omap-upstream with the updates mentioned above.

Regards,

Tony

Re: [PATCH omap-fixes] OMAP2/3: GPIO: remove recursion in IRQ wakeup path

[Kevin Hilman]

Tony Lindgren wrote:
> * Kevin Hilman <khilman@deeprootsystems.com> [090210 16:42]:
>> David Brownell <david-b@pacbell.net> writes:
>>
>>> On Wednesday 28 January 2009, Kevin Hilman wrote:
>>>> Now that the generic IRQ and GPIO frameworks are used for enabling and
>>>> disabling GPIO IRQ wakeup sources, there is no longer a need to call
>>>> [enable|disable]_irq_wake() in the low-level code.  Doing so results
>>>> in recursive calls to [enable|disable]_irq_wake().
>>> Could you clarify what actually made that requirement go away?
>>>
>>> The recursion was introduced -- using the generic IRQ framework! -- as
>>> a simple way to ensure the parent IRQ was properly wake-enabled.  Is
>>> the issue that something changed, so that something else wake-enables
>>> the parent?
>>>
>> My description was not very descriptive... sorry...
>>
>> From what I can understand here, I don't see the point in
>> wake-enabling the parent IRQ since there is no wakeup glue for the
>> bank IRQs, thus these calls are actually failing and causing
>> 'unbalanced IRQ disable' noise in the generic IRQ layer.
>>
>> Here is what is happening.  Consider the gpio-keys driver.  Upon
>> suspend, it enables the IRQ wake for its GPIO.  The OMAP GPIO code
>> then calls enable_wake_irq() for the parent irq (the GPIO bank IRQ).
>> This call is actually failing because the bank IRQ has no 'set_wake'
>> method.  Because of this failure, the generic IRQ code doesn't
>> actually do anything, and sets the 'disable_depth' to zero for the
>> bank IRQ.
>>
>> Then, upon resume, the resume path disables IRQ wakeups for the GPIO.
>> The OMAP GPIO code then tries to call disable_irq_wake() for the bank
>> IRQ and you get noisy 'unbalanced IRQ disable' warnings from the
>> generic IRQ layere because of the attempt to disable the IRQ wake of
>> an IRQ that was never enabled.
>>
>> So the options that I see are:
>>
>> 1) fix the OMAP GPIO code to check the return code of the parent enable, or
>> 2) drop the parent enable/disable
>>
>> I prefer (1) since those calls will always fail AFAICT.
>>
>> Does that make any sense?
>>
>> If you're OK with (1), I will re-issue the patch with a better discription.
> 
> Ignoring this for now, please let me know if you want me to queue this
> for omap-upstream with the updates mentioned above.

OK.  I'll re-send with updated description if above is OK with Dave.

Kevin