[PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU

[PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU

[Liu, Chuansheng]

From: liu chuansheng <chuansheng.liu@intel.com>
Subject: [PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU

When preparing to unmask the irq in fixup_irqs(), using irqd_irq_masked()
as the condition to determine if do real unmasking action or not.

Because in some chips, the .irq_disable is NULL, so calling disable_irq()
does not mean the irq is masked immediately, and before enable_irq() if
the irq is coming, it can be pending state. Using irqd_irq_disabled() will
lose this chance.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 arch/x86/kernel/irq.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 6c0802e..b9dd37f 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -277,7 +277,7 @@ void fixup_irqs(void)
                        set_affinity = 0;
 
                if (!irqd_can_move_in_process_context(data) &&
-                   !irqd_irq_disabled(data) && chip->irq_unmask)
+                   !irqd_irq_masked(data) && chip->irq_unmask)
                        chip->irq_unmask(data);
 
                raw_spin_unlock(&desc->lock);
-- 
1.7.0.4

Re: [PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU

[Yanmin Zhang]

On Mon, 2012-03-26 at 07:11 +0000, Liu, Chuansheng wrote:
> From: liu chuansheng <chuansheng.liu@intel.com>
> Subject: [PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU
> 
> When preparing to unmask the irq in fixup_irqs(), using irqd_irq_masked()
> as the condition to determine if do real unmasking action or not.
> 
> Because in some chips, the .irq_disable is NULL, so calling disable_irq()
> does not mean the irq is masked immediately, and before enable_irq() if
> the irq is coming, it can be pending state. Using irqd_irq_disabled() will
> lose this chance.
> 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  arch/x86/kernel/irq.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index 6c0802e..b9dd37f 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -277,7 +277,7 @@ void fixup_irqs(void)
>                         set_affinity = 0;
>  
>                 if (!irqd_can_move_in_process_context(data) &&
> -                   !irqd_irq_disabled(data) && chip->irq_unmask)
> +                   !irqd_irq_masked(data) && chip->irq_unmask)
>                         chip->irq_unmask(data);
>  
>                 raw_spin_unlock(&desc->lock);
Thomas,

Originally, we found an issue when enabling Android (Gingerbread) with kernel
2.6.35 on Medfield.

When system goes to suspend-2-ram, some device drivers call disable_irq to
disable some device irq at device driver suspend callbacks. In addition,
some specific irq_chips just provide mask/unmask callbacks and have no
enable/disable callbacks. When drivers call
disable_irq=>...=>irq_disable=>desc->irq_data.chip->irq_disable, they don't
really mask the irq at the irq_chip. With such specific irq_chip, we could
keep the irq as wake up sources. When the irq arrives after driver disables them,
IRQ handling framework would mark it PENDING and calls the irq handler when
drivers call enable_irq.

After device driver's suspend/suspend_noirq callbacks are called, suspend-2-ram
would disable nonboot cpus, which calls fixup_irqs. fixup_irqs really masks the irq
at irq_chip as the drivers calls disable_irq before. So one of the side effects of
calling fixup_irq is to really mask the irq and we lose the wakeup capability of
the corresponding irq.

You might ask why drivers call disable_irq at suspend callback. That's because
another specific reason. suspend callback calls disable_irq and resume callback calls
enable_irq. Between suspend and resume callbacks, we don't want kernel to deliver
the irq to the driver irq handler as the device is not FULLY resumed yet.

Chuangsheng's patch tries to fix above issue, to keep the difference between
enable/disable and mask/unmask callbacks of irq_chip.

Hope my explanation could be a little clearer.

Sorry for taking you too much time.

Yanmin

Re: [PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU

[Srivatsa S. Bhat]

[ Adding a few more CCs ]

On 03/28/2012 11:08 AM, Yanmin Zhang wrote:

> On Mon, 2012-03-26 at 07:11 +0000, Liu, Chuansheng wrote:
>> From: liu chuansheng <chuansheng.liu@intel.com>
>> Subject: [PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU
>>
>> When preparing to unmask the irq in fixup_irqs(), using irqd_irq_masked()
>> as the condition to determine if do real unmasking action or not.
>>
>> Because in some chips, the .irq_disable is NULL, so calling disable_irq()
>> does not mean the irq is masked immediately, and before enable_irq() if
>> the irq is coming, it can be pending state. Using irqd_irq_disabled() will
>> lose this chance.
>>
>> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
>> ---
>>  arch/x86/kernel/irq.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> index 6c0802e..b9dd37f 100644
>> --- a/arch/x86/kernel/irq.c
>> +++ b/arch/x86/kernel/irq.c
>> @@ -277,7 +277,7 @@ void fixup_irqs(void)
>>                         set_affinity = 0;
>>  
>>                 if (!irqd_can_move_in_process_context(data) &&
>> -                   !irqd_irq_disabled(data) && chip->irq_unmask)
>> +                   !irqd_irq_masked(data) && chip->irq_unmask)
>>                         chip->irq_unmask(data);
>>  
>>                 raw_spin_unlock(&desc->lock);
> Thomas,
> 
> Originally, we found an issue when enabling Android (Gingerbread) with kernel
> 2.6.35 on Medfield.
> 
> When system goes to suspend-2-ram, some device drivers call disable_irq to
> disable some device irq at device driver suspend callbacks. In addition,
> some specific irq_chips just provide mask/unmask callbacks and have no
> enable/disable callbacks. When drivers call
> disable_irq=>...=>irq_disable=>desc->irq_data.chip->irq_disable, they don't
> really mask the irq at the irq_chip. With such specific irq_chip, we could
> keep the irq as wake up sources. When the irq arrives after driver disables them,
> IRQ handling framework would mark it PENDING and calls the irq handler when
> drivers call enable_irq.
> 
> After device driver's suspend/suspend_noirq callbacks are called, suspend-2-ram
> would disable nonboot cpus, which calls fixup_irqs. fixup_irqs really masks the irq
> at irq_chip as the drivers calls disable_irq before. So one of the side effects of
> calling fixup_irq is to really mask the irq and we lose the wakeup capability of
> the corresponding irq.
> 
> You might ask why drivers call disable_irq at suspend callback. That's because
> another specific reason. suspend callback calls disable_irq and resume callback calls
> enable_irq. Between suspend and resume callbacks, we don't want kernel to deliver
> the irq to the driver irq handler as the device is not FULLY resumed yet.
> 
> Chuangsheng's patch tries to fix above issue, to keep the difference between
> enable/disable and mask/unmask callbacks of irq_chip.
> 
> Hope my explanation could be a little clearer.
> 
> Sorry for taking you too much time.
> 
> Yanmin
> 
> 

Re: [PATCH] x86_fixup_irqs: Fix possible missing interrupt handle when disabling CPU

[Thomas Gleixner]

On Wed, 28 Mar 2012, Yanmin Zhang wrote:
> On Mon, 2012-03-26 at 07:11 +0000, Liu, Chuansheng wrote:
> Hope my explanation could be a little clearer.

Yeah. I fixed up the changelog already.

Thanks,

	tglx

[tip:x86/urgent] x86: Preserve lazy irq disable semantics in fixup_irqs()

[tip-bot for Liu, Chuansheng]

Commit-ID:  93a4ccee37c6d11d3b2fa43fa7ea70e7c4d15349
Gitweb:     http://git.kernel.org/tip/93a4ccee37c6d11d3b2fa43fa7ea70e7c4d15349
Author:     Liu, Chuansheng <chuansheng.liu@intel.com>
AuthorDate: Mon, 26 Mar 2012 07:11:50 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 29 Mar 2012 12:17:02 +0200

x86: Preserve lazy irq disable semantics in fixup_irqs()

The default irq_disable() sematics are to mark the interrupt disabled,
but keep it unmasked. If the interrupt is delivered while marked
disabled, the low level interrupt handler masks it and marks it
pending. This is important for detecting wakeup interrupts during
suspend and for edge type interrupts to avoid losing interrupts.

fixup_irqs() moves the interrupts away from an offlined cpu. For
certain interrupt types it needs to mask the interrupt line before
changing the affinity. After affinity has changed the interrupt line
is unmasked again, but only if it is not marked disabled.

This breaks the lazy irq disable semantics and causes problems in
suspend as the interrupt can be lost or wakeup functionality is
broken.

Check irqd_irq_masked() instead of irqd_irq_disabled() because
irqd_irq_masked() is only set, when the core code actually masked the
interrupt line. If it's not set, we unmask the interrupt and let the
lazy irq disable logic deal with an eventually incoming interrupt.

[ tglx: Massaged changelog and added a comment ]

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
Cc: Yanmin Zhang <yanmin_zhang@linux.intel.com>
Link: http://lkml.kernel.org/r/27240C0AC20F114CBF8149A2696CBE4A05DFB3@SHSMSX101.ccr.corp.intel.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/irq.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 7943e0c..2759a8c 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -282,8 +282,13 @@ void fixup_irqs(void)
 		else if (!(warned++))
 			set_affinity = 0;
 
-		if (!irqd_can_move_in_process_context(data) &&
-		    !irqd_irq_disabled(data) && chip->irq_unmask)
+		/*
+		 * We unmask if the irq was not marked masked by the
+		 * core code. That respects the lazy irq disable
+		 * behaviour.
+		 */
+		if (!irqd_canx_move_in_process_context(data) &&
+		    !irqd_irq_masked(data) && chip->irq_unmask)
 			chip->irq_unmask(data);
 
 		raw_spin_unlock(&desc->lock);

[tip:x86/urgent] x86: Preserve lazy irq disable semantics in fixup_irqs()

[tip-bot for Liu, Chuansheng]

Commit-ID:  99dd5497e5be4fe4194cad181d45fd6569a930db
Gitweb:     http://git.kernel.org/tip/99dd5497e5be4fe4194cad181d45fd6569a930db
Author:     Liu, Chuansheng <chuansheng.liu@intel.com>
AuthorDate: Mon, 26 Mar 2012 07:11:50 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 29 Mar 2012 15:28:47 +0200

x86: Preserve lazy irq disable semantics in fixup_irqs()

The default irq_disable() sematics are to mark the interrupt disabled,
but keep it unmasked. If the interrupt is delivered while marked
disabled, the low level interrupt handler masks it and marks it
pending. This is important for detecting wakeup interrupts during
suspend and for edge type interrupts to avoid losing interrupts.

fixup_irqs() moves the interrupts away from an offlined cpu. For
certain interrupt types it needs to mask the interrupt line before
changing the affinity. After affinity has changed the interrupt line
is unmasked again, but only if it is not marked disabled.

This breaks the lazy irq disable semantics and causes problems in
suspend as the interrupt can be lost or wakeup functionality is
broken.

Check irqd_irq_masked() instead of irqd_irq_disabled() because
irqd_irq_masked() is only set, when the core code actually masked the
interrupt line. If it's not set, we unmask the interrupt and let the
lazy irq disable logic deal with an eventually incoming interrupt.

[ tglx: Massaged changelog and added a comment ]

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
Cc: Yanmin Zhang <yanmin_zhang@linux.intel.com>
Link: http://lkml.kernel.org/r/27240C0AC20F114CBF8149A2696CBE4A05DFB3@SHSMSX101.ccr.corp.intel.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/irq.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 7943e0c..3dafc60 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -282,8 +282,13 @@ void fixup_irqs(void)
 		else if (!(warned++))
 			set_affinity = 0;
 
+		/*
+		 * We unmask if the irq was not marked masked by the
+		 * core code. That respects the lazy irq disable
+		 * behaviour.
+		 */
 		if (!irqd_can_move_in_process_context(data) &&
-		    !irqd_irq_disabled(data) && chip->irq_unmask)
+		    !irqd_irq_masked(data) && chip->irq_unmask)
 			chip->irq_unmask(data);
 
 		raw_spin_unlock(&desc->lock);