[PATCH] genirq: update irq_chip struct comment for irq_chip->disable hook

[PATCH] genirq: update irq_chip struct comment for irq_chip->disable hook

[Kevin Hilman]

The irq_chip disable hook no longer defaults to a irq_chip->unmask.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 include/linux/irq.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index b7cbeed..3674046 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -88,7 +88,7 @@ struct msi_desc;
  * @startup:		start up the interrupt (defaults to ->enable if NULL)
  * @shutdown:		shut down the interrupt (defaults to ->disable if NULL)
  * @enable:		enable the interrupt (defaults to chip->unmask if NULL)
- * @disable:		disable the interrupt (defaults to chip->mask if NULL)
+ * @disable:		disable the interrupt
  * @ack:		start of a new interrupt
  * @mask:		mask an interrupt source
  * @mask_ack:		ack and mask an interrupt source
-- 
1.6.2.2

[PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
behavior of disable_irq() was changed to delay the disable until it is
next handled.

However, this leaves open the possibility that the system can go into
suspend with an interrupt enabled.  For example, if a driver calls
disable_irq() in its suspend_hook (for example, to prevent that device
IRQ from causing a system wakeup) there's now a possibility that the
system is suspended before the lazy disable happens.

The result is an unwanted wakeup from suspend if the IRQ is capable of
waking the system (common on embedded SoCs.)

This patch ensures that the lazy disable is done, and masked by
the irq_chip before the system goes into suspend.

Note that even though __disable_irq() also calls irq_chip->disable, it
is quite common for the irq_chip not to provide a disable hook in
which case the IRQ is never masked/disabled in hardware before going
into suspend.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 kernel/irq/manage.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2734eca..5d2cc1c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -190,6 +190,11 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
 		if (!desc->action || (desc->action->flags & IRQF_TIMER))
 			return;
 		desc->status |= IRQ_SUSPENDED;
+
+		/* Lazy disable: handles case where lazy disable in
+		 * handler doesn't happen before suspend. */
+		if (desc->status & IRQ_DISABLED)
+			desc->chip->mask(irq);
 	}
 
 	if (!desc->depth++) {
-- 
1.6.2.2

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Ingo Molnar]

* Kevin Hilman <khilman@deeprootsystems.com> wrote:

> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
> behavior of disable_irq() was changed to delay the disable until it is
> next handled.
> 
> However, this leaves open the possibility that the system can go into
> suspend with an interrupt enabled.  For example, if a driver calls
> disable_irq() in its suspend_hook (for example, to prevent that device
> IRQ from causing a system wakeup) there's now a possibility that the
> system is suspended before the lazy disable happens.
> 
> The result is an unwanted wakeup from suspend if the IRQ is capable of
> waking the system (common on embedded SoCs.)
> 
> This patch ensures that the lazy disable is done, and masked by
> the irq_chip before the system goes into suspend.
> 
> Note that even though __disable_irq() also calls irq_chip->disable, it
> is quite common for the irq_chip not to provide a disable hook in
> which case the IRQ is never masked/disabled in hardware before going
> into suspend.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  kernel/irq/manage.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2734eca..5d2cc1c 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -190,6 +190,11 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
>  		if (!desc->action || (desc->action->flags & IRQF_TIMER))
>  			return;
>  		desc->status |= IRQ_SUSPENDED;
> +
> +		/* Lazy disable: handles case where lazy disable in
> +		 * handler doesn't happen before suspend. */
> +		if (desc->status & IRQ_DISABLED)
> +			desc->chip->mask(irq);

Please look at the rest of the file and follow the multi-line 
comment style that is used in the 29 multi-line comment instances 
there. (which is also what Documentation/CodingStyle specifies)

	Ingo

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Ingo Molnar]

* Kevin Hilman <khilman@deeprootsystems.com> wrote:

> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
> behavior of disable_irq() was changed to delay the disable until it is
> next handled.
> 
> However, this leaves open the possibility that the system can go into
> suspend with an interrupt enabled.  For example, if a driver calls
> disable_irq() in its suspend_hook (for example, to prevent that device
> IRQ from causing a system wakeup) there's now a possibility that the
> system is suspended before the lazy disable happens.
> 
> The result is an unwanted wakeup from suspend if the IRQ is capable of
> waking the system (common on embedded SoCs.)
> 
> This patch ensures that the lazy disable is done, and masked by
> the irq_chip before the system goes into suspend.
> 
> Note that even though __disable_irq() also calls irq_chip->disable, it
> is quite common for the irq_chip not to provide a disable hook in
> which case the IRQ is never masked/disabled in hardware before going
> into suspend.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  kernel/irq/manage.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2734eca..5d2cc1c 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -190,6 +190,11 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
>  		if (!desc->action || (desc->action->flags & IRQF_TIMER))
>  			return;
>  		desc->status |= IRQ_SUSPENDED;
> +
> +		/* Lazy disable: handles case where lazy disable in
> +		 * handler doesn't happen before suspend. */
> +		if (desc->status & IRQ_DISABLED)
> +			desc->chip->mask(irq);

Please look at the rest of the file and follow the multi-line 
comment style that is used in the 29 multi-line comment instances 
there. (which is also what Documentation/CodingStyle specifies)

	Ingo

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Ingo Molnar <mingo@elte.hu> writes:

> * Kevin Hilman <khilman@deeprootsystems.com> wrote:
>

[...]

>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -190,6 +190,11 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
>>  		if (!desc->action || (desc->action->flags & IRQF_TIMER))
>>  			return;
>>  		desc->status |= IRQ_SUSPENDED;
>> +
>> +		/* Lazy disable: handles case where lazy disable in
>> +		 * handler doesn't happen before suspend. */
>> +		if (desc->status & IRQ_DISABLED)
>> +			desc->chip->mask(irq);
>
> Please look at the rest of the file and follow the multi-line 
> comment style that is used in the 29 multi-line comment instances 
> there. (which is also what Documentation/CodingStyle specifies)

Doh, sorry.  Updated patch below.

Kevin

>From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@deeprootsystems.com>
Date: Wed, 6 May 2009 16:00:07 -0700
Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
behavior of disable_irq() was changed to delay the disable until it is
next handled.

However, this leaves open the possibility that the system can go into
suspend with an interrupt enabled.  For example, if a driver calls
disable_irq() in its suspend_hook there's now a possibility that the
system is suspended before the lazy disable happens.

The result is an unwanted wakeup from suspend if the IRQ is capable of
waking the system (common on embedded SoCs.)

This patch ensures that the lazy disable is done, and masked by
the irq_chip before the system goes into suspend.

Note that even though __disable_irq() also calls irq_chip->disable, it
is quite common for the irq_chip not to provide a disable hook in
which case the IRQ is never masked/disabled in hardware before going
into suspend.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 kernel/irq/manage.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2734eca..c786820 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -190,6 +190,13 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
 		if (!desc->action || (desc->action->flags & IRQF_TIMER))
 			return;
 		desc->status |= IRQ_SUSPENDED;
+
+		/*
+		 * Lazy disable: handles case where lazy disable in
+		 * handler doesn't happen before suspend.
+		 */
+		if (desc->status & IRQ_DISABLED)
+			desc->chip->mask(irq);
 	}
 
 	if (!desc->depth++) {
-- 
1.6.2.2

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Ingo Molnar <mingo@elte.hu> writes:

> * Kevin Hilman <khilman@deeprootsystems.com> wrote:
>

[...]

>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -190,6 +190,11 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
>>  		if (!desc->action || (desc->action->flags & IRQF_TIMER))
>>  			return;
>>  		desc->status |= IRQ_SUSPENDED;
>> +
>> +		/* Lazy disable: handles case where lazy disable in
>> +		 * handler doesn't happen before suspend. */
>> +		if (desc->status & IRQ_DISABLED)
>> +			desc->chip->mask(irq);
>
> Please look at the rest of the file and follow the multi-line 
> comment style that is used in the 29 multi-line comment instances 
> there. (which is also what Documentation/CodingStyle specifies)

Doh, sorry.  Updated patch below.

Kevin

>From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@deeprootsystems.com>
Date: Wed, 6 May 2009 16:00:07 -0700
Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
behavior of disable_irq() was changed to delay the disable until it is
next handled.

However, this leaves open the possibility that the system can go into
suspend with an interrupt enabled.  For example, if a driver calls
disable_irq() in its suspend_hook there's now a possibility that the
system is suspended before the lazy disable happens.

The result is an unwanted wakeup from suspend if the IRQ is capable of
waking the system (common on embedded SoCs.)

This patch ensures that the lazy disable is done, and masked by
the irq_chip before the system goes into suspend.

Note that even though __disable_irq() also calls irq_chip->disable, it
is quite common for the irq_chip not to provide a disable hook in
which case the IRQ is never masked/disabled in hardware before going
into suspend.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 kernel/irq/manage.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2734eca..c786820 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -190,6 +190,13 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
 		if (!desc->action || (desc->action->flags & IRQF_TIMER))
 			return;
 		desc->status |= IRQ_SUSPENDED;
+
+		/*
+		 * Lazy disable: handles case where lazy disable in
+		 * handler doesn't happen before suspend.
+		 */
+		if (desc->status & IRQ_DISABLED)
+			desc->chip->mask(irq);
 	}
 
 	if (!desc->depth++) {
-- 
1.6.2.2

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Arve Hjønnevåg]

On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@deeprootsystems.com>
> Date: Wed, 6 May 2009 16:00:07 -0700
> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>
> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
> behavior of disable_irq() was changed to delay the disable until it is
> next handled.
>
> However, this leaves open the possibility that the system can go into
> suspend with an interrupt enabled.  For example, if a driver calls
> disable_irq() in its suspend_hook there's now a possibility that the
> system is suspended before the lazy disable happens.
>
> The result is an unwanted wakeup from suspend if the IRQ is capable of
> waking the system (common on embedded SoCs.)

If the interrupt contoller uses the same enable register for wakeup
and interrupts, I think it is the responsibility of the platform code,
not individual drivers, to disable the interrupts that are not marked
for wakeup before entering suspend.

> This patch ensures that the lazy disable is done, and masked by
> the irq_chip before the system goes into suspend.

This will create a window where wakeup interrupts can be lost if the
driver has masked the interrupt (by calling disable_irq). If the
hardware does not allow edge detection on disabled interrupts (the msm
platform has this limitation) then this change will turn off the edge
detection. If suspend_ops->enter does not turn the interrupt (and edge
detection) back on (without this change it may never need to turn on
any interrupt) it will not wakeup at all.

> Note that even though __disable_irq() also calls irq_chip->disable, it
> is quite common for the irq_chip not to provide a disable hook in
> which case the IRQ is never masked/disabled in hardware before going
> into suspend.
>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  kernel/irq/manage.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2734eca..c786820 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -190,6 +190,13 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
>                if (!desc->action || (desc->action->flags & IRQF_TIMER))
>                        return;
>                desc->status |= IRQ_SUSPENDED;
> +
> +               /*
> +                * Lazy disable: handles case where lazy disable in
> +                * handler doesn't happen before suspend.
> +                */
> +               if (desc->status & IRQ_DISABLED)
> +                       desc->chip->mask(irq);
>        }
>
>        if (!desc->depth++) {
> --
> 1.6.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Arve Hjønnevåg

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>> From: Kevin Hilman <khilman@deeprootsystems.com>
>> Date: Wed, 6 May 2009 16:00:07 -0700
>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>
>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>> behavior of disable_irq() was changed to delay the disable until it is
>> next handled.
>>
>> However, this leaves open the possibility that the system can go into
>> suspend with an interrupt enabled.  For example, if a driver calls
>> disable_irq() in its suspend_hook there's now a possibility that the
>> system is suspended before the lazy disable happens.
>>
>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>> waking the system (common on embedded SoCs.)
>
> If the interrupt contoller uses the same enable register for wakeup
> and interrupts, I think it is the responsibility of the platform code,
> not individual drivers, to disable the interrupts that are not marked
> for wakeup before entering suspend.

I agree, for wakeup interrupts, drivers should use
[set|disable]_irq_wake() and the platform code should handle this.

I used wakeup interrupts in this description as an example which
turned out to be a bad example.  The 2nd version of this patch I
posted, I removed the reference to wakeup interrupts in favor of just
talking about the delayed disable piece.

But ignoring wakup interrupts, would you agree that the delayed
disable of an interrupt should not wait until after resume?

>> This patch ensures that the lazy disable is done, and masked by
>> the irq_chip before the system goes into suspend.
>
> This will create a window where wakeup interrupts can be lost if the
> driver has masked the interrupt (by calling disable_irq). If the
> hardware does not allow edge detection on disabled interrupts (the msm
> platform has this limitation) then this change will turn off the edge
> detection. If suspend_ops->enter does not turn the interrupt (and edge
> detection) back on (without this change it may never need to turn on
> any interrupt) it will not wakeup at all.

Not sure I follow you here...

It seems like you're relying on the delayed disable to wait until
after resume so that disabled interrupts can wake the system.  How
did this work before the delayed disable patch?

If the interrupt is being used as a wakeup, why would anyone be
calling disable_irq()?

Kevin

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>> From: Kevin Hilman <khilman@deeprootsystems.com>
>> Date: Wed, 6 May 2009 16:00:07 -0700
>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>
>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>> behavior of disable_irq() was changed to delay the disable until it is
>> next handled.
>>
>> However, this leaves open the possibility that the system can go into
>> suspend with an interrupt enabled.  For example, if a driver calls
>> disable_irq() in its suspend_hook there's now a possibility that the
>> system is suspended before the lazy disable happens.
>>
>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>> waking the system (common on embedded SoCs.)
>
> If the interrupt contoller uses the same enable register for wakeup
> and interrupts, I think it is the responsibility of the platform code,
> not individual drivers, to disable the interrupts that are not marked
> for wakeup before entering suspend.

I agree, for wakeup interrupts, drivers should use
[set|disable]_irq_wake() and the platform code should handle this.

I used wakeup interrupts in this description as an example which
turned out to be a bad example.  The 2nd version of this patch I
posted, I removed the reference to wakeup interrupts in favor of just
talking about the delayed disable piece.

But ignoring wakup interrupts, would you agree that the delayed
disable of an interrupt should not wait until after resume?

>> This patch ensures that the lazy disable is done, and masked by
>> the irq_chip before the system goes into suspend.
>
> This will create a window where wakeup interrupts can be lost if the
> driver has masked the interrupt (by calling disable_irq). If the
> hardware does not allow edge detection on disabled interrupts (the msm
> platform has this limitation) then this change will turn off the edge
> detection. If suspend_ops->enter does not turn the interrupt (and edge
> detection) back on (without this change it may never need to turn on
> any interrupt) it will not wakeup at all.

Not sure I follow you here...

It seems like you're relying on the delayed disable to wait until
after resume so that disabled interrupts can wake the system.  How
did this work before the delayed disable patch?

If the interrupt is being used as a wakeup, why would anyone be
calling disable_irq()?

Kevin

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Arve Hjønnevåg]

2009/5/8 Kevin Hilman <khilman@deeprootsystems.com>:
> Arve Hjønnevåg <arve@android.com> writes:
>
>> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>>> From: Kevin Hilman <khilman@deeprootsystems.com>
>>> Date: Wed, 6 May 2009 16:00:07 -0700
>>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>>
>>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>>> behavior of disable_irq() was changed to delay the disable until it is
>>> next handled.
>>>
>>> However, this leaves open the possibility that the system can go into
>>> suspend with an interrupt enabled.  For example, if a driver calls
>>> disable_irq() in its suspend_hook there's now a possibility that the
>>> system is suspended before the lazy disable happens.
>>>
>>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>>> waking the system (common on embedded SoCs.)
>>
>> If the interrupt contoller uses the same enable register for wakeup
>> and interrupts, I think it is the responsibility of the platform code,
>> not individual drivers, to disable the interrupts that are not marked
>> for wakeup before entering suspend.
>
> I agree, for wakeup interrupts, drivers should use
> [set|disable]_irq_wake() and the platform code should handle this.
>
> I used wakeup interrupts in this description as an example which
> turned out to be a bad example.  The 2nd version of this patch I
> posted, I removed the reference to wakeup interrupts in favor of just
> talking about the delayed disable piece.
>
> But ignoring wakup interrupts, would you agree that the delayed
> disable of an interrupt should not wait until after resume?
>

No. The platform code needs to turn off interrupts that are not wakeup
interrupts anyway, so there is not much point in disabling some
interrupts early. Also, if the interrupt in question is not a wakeup
interrupt you leave it in a state where it does not detect an edge. A
driver that enables its hardware in resume, then unmasks the interrupt
would loose an interrupt that triggered between enabling the hardware
and unmasking the interrupt.

>>> This patch ensures that the lazy disable is done, and masked by
>>> the irq_chip before the system goes into suspend.
>>
>> This will create a window where wakeup interrupts can be lost if the
>> driver has masked the interrupt (by calling disable_irq). If the
>> hardware does not allow edge detection on disabled interrupts (the msm
>> platform has this limitation) then this change will turn off the edge
>> detection. If suspend_ops->enter does not turn the interrupt (and edge
>> detection) back on (without this change it may never need to turn on
>> any interrupt) it will not wakeup at all.
>
> Not sure I follow you here...
>
> It seems like you're relying on the delayed disable to wait until
> after resume so that disabled interrupts can wake the system.  How
> did this work before the delayed disable patch?
>

It did not.

> If the interrupt is being used as a wakeup, why would anyone be
> calling disable_irq()?
>

Drivers call disable_irq to make sure their interrupt handler does not
get called. A driver may not be able take interrupts after its suspend
hook has been called. If it calls disable_irq on a wakeup interrupt
then this interrupt should abort suspend or wake up from suspend. The
driver will then see the interrupt when it call enable_irq in its
resume handler.

-- 
Arve Hjønnevåg

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Arve Hjønnevåg]

2009/5/8 Kevin Hilman <khilman@deeprootsystems.com>:
> Arve Hjønnevåg <arve@android.com> writes:
>
>> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>>> From: Kevin Hilman <khilman@deeprootsystems.com>
>>> Date: Wed, 6 May 2009 16:00:07 -0700
>>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>>
>>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>>> behavior of disable_irq() was changed to delay the disable until it is
>>> next handled.
>>>
>>> However, this leaves open the possibility that the system can go into
>>> suspend with an interrupt enabled.  For example, if a driver calls
>>> disable_irq() in its suspend_hook there's now a possibility that the
>>> system is suspended before the lazy disable happens.
>>>
>>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>>> waking the system (common on embedded SoCs.)
>>
>> If the interrupt contoller uses the same enable register for wakeup
>> and interrupts, I think it is the responsibility of the platform code,
>> not individual drivers, to disable the interrupts that are not marked
>> for wakeup before entering suspend.
>
> I agree, for wakeup interrupts, drivers should use
> [set|disable]_irq_wake() and the platform code should handle this.
>
> I used wakeup interrupts in this description as an example which
> turned out to be a bad example.  The 2nd version of this patch I
> posted, I removed the reference to wakeup interrupts in favor of just
> talking about the delayed disable piece.
>
> But ignoring wakup interrupts, would you agree that the delayed
> disable of an interrupt should not wait until after resume?
>

No. The platform code needs to turn off interrupts that are not wakeup
interrupts anyway, so there is not much point in disabling some
interrupts early. Also, if the interrupt in question is not a wakeup
interrupt you leave it in a state where it does not detect an edge. A
driver that enables its hardware in resume, then unmasks the interrupt
would loose an interrupt that triggered between enabling the hardware
and unmasking the interrupt.

>>> This patch ensures that the lazy disable is done, and masked by
>>> the irq_chip before the system goes into suspend.
>>
>> This will create a window where wakeup interrupts can be lost if the
>> driver has masked the interrupt (by calling disable_irq). If the
>> hardware does not allow edge detection on disabled interrupts (the msm
>> platform has this limitation) then this change will turn off the edge
>> detection. If suspend_ops->enter does not turn the interrupt (and edge
>> detection) back on (without this change it may never need to turn on
>> any interrupt) it will not wakeup at all.
>
> Not sure I follow you here...
>
> It seems like you're relying on the delayed disable to wait until
> after resume so that disabled interrupts can wake the system.  How
> did this work before the delayed disable patch?
>

It did not.

> If the interrupt is being used as a wakeup, why would anyone be
> calling disable_irq()?
>

Drivers call disable_irq to make sure their interrupt handler does not
get called. A driver may not be able take interrupts after its suspend
hook has been called. If it calls disable_irq on a wakeup interrupt
then this interrupt should abort suspend or wake up from suspend. The
driver will then see the interrupt when it call enable_irq in its
resume handler.

-- 
Arve Hjønnevåg

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> 2009/5/8 Kevin Hilman <khilman@deeprootsystems.com>:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>>> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
>>> <khilman@deeprootsystems.com> wrote:
>>>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>>>> From: Kevin Hilman <khilman@deeprootsystems.com>
>>>> Date: Wed, 6 May 2009 16:00:07 -0700
>>>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>>>
>>>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>>>> behavior of disable_irq() was changed to delay the disable until it is
>>>> next handled.
>>>>
>>>> However, this leaves open the possibility that the system can go into
>>>> suspend with an interrupt enabled.  For example, if a driver calls
>>>> disable_irq() in its suspend_hook there's now a possibility that the
>>>> system is suspended before the lazy disable happens.
>>>>
>>>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>>>> waking the system (common on embedded SoCs.)
>>>
>>> If the interrupt contoller uses the same enable register for wakeup
>>> and interrupts, I think it is the responsibility of the platform code,
>>> not individual drivers, to disable the interrupts that are not marked
>>> for wakeup before entering suspend.
>>
>> I agree, for wakeup interrupts, drivers should use
>> [set|disable]_irq_wake() and the platform code should handle this.
>>
>> I used wakeup interrupts in this description as an example which
>> turned out to be a bad example.  The 2nd version of this patch I
>> posted, I removed the reference to wakeup interrupts in favor of just
>> talking about the delayed disable piece.
>>
>> But ignoring wakup interrupts, would you agree that the delayed
>> disable of an interrupt should not wait until after resume?
>>
>
> No. The platform code needs to turn off interrupts that are not wakeup
> interrupts anyway, so there is not much point in disabling some
> interrupts early. Also, if the interrupt in question is not a wakeup
> interrupt you leave it in a state where it does not detect an edge. A
> driver that enables its hardware in resume, then unmasks the interrupt
> would loose an interrupt that triggered between enabling the hardware
> and unmasking the interrupt.

Got it.  Thanks for your (repeated) explanations.  I understand now
your reasons for allowing the interrupt to remain across suspend.  I
will modify my platform to rely on [enable|disable]_irq_wake() in
addition to ensuring platform code is disabling non wakup IRQs.

>>>> This patch ensures that the lazy disable is done, and masked by
>>>> the irq_chip before the system goes into suspend.
>>>
>>> This will create a window where wakeup interrupts can be lost if the
>>> driver has masked the interrupt (by calling disable_irq). If the
>>> hardware does not allow edge detection on disabled interrupts (the msm
>>> platform has this limitation) then this change will turn off the edge
>>> detection. If suspend_ops->enter does not turn the interrupt (and edge
>>> detection) back on (without this change it may never need to turn on
>>> any interrupt) it will not wakeup at all.
>>
>> Not sure I follow you here...
>>
>> It seems like you're relying on the delayed disable to wait until
>> after resume so that disabled interrupts can wake the system.  How
>> did this work before the delayed disable patch?
>>
>
> It did not.
>
>> If the interrupt is being used as a wakeup, why would anyone be
>> calling disable_irq()?
>>
>
> Drivers call disable_irq to make sure their interrupt handler does not
> get called. A driver may not be able take interrupts after its suspend
> hook has been called. If it calls disable_irq on a wakeup interrupt
> then this interrupt should abort suspend or wake up from suspend. The
> driver will then see the interrupt when it call enable_irq in its
> resume handler.

OK.

Kevin

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> 2009/5/8 Kevin Hilman <khilman@deeprootsystems.com>:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>>> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
>>> <khilman@deeprootsystems.com> wrote:
>>>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>>>> From: Kevin Hilman <khilman@deeprootsystems.com>
>>>> Date: Wed, 6 May 2009 16:00:07 -0700
>>>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>>>
>>>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>>>> behavior of disable_irq() was changed to delay the disable until it is
>>>> next handled.
>>>>
>>>> However, this leaves open the possibility that the system can go into
>>>> suspend with an interrupt enabled.  For example, if a driver calls
>>>> disable_irq() in its suspend_hook there's now a possibility that the
>>>> system is suspended before the lazy disable happens.
>>>>
>>>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>>>> waking the system (common on embedded SoCs.)
>>>
>>> If the interrupt contoller uses the same enable register for wakeup
>>> and interrupts, I think it is the responsibility of the platform code,
>>> not individual drivers, to disable the interrupts that are not marked
>>> for wakeup before entering suspend.
>>
>> I agree, for wakeup interrupts, drivers should use
>> [set|disable]_irq_wake() and the platform code should handle this.
>>
>> I used wakeup interrupts in this description as an example which
>> turned out to be a bad example.  The 2nd version of this patch I
>> posted, I removed the reference to wakeup interrupts in favor of just
>> talking about the delayed disable piece.
>>
>> But ignoring wakup interrupts, would you agree that the delayed
>> disable of an interrupt should not wait until after resume?
>>
>
> No. The platform code needs to turn off interrupts that are not wakeup
> interrupts anyway, so there is not much point in disabling some
> interrupts early. Also, if the interrupt in question is not a wakeup
> interrupt you leave it in a state where it does not detect an edge. A
> driver that enables its hardware in resume, then unmasks the interrupt
> would loose an interrupt that triggered between enabling the hardware
> and unmasking the interrupt.

Got it.  Thanks for your (repeated) explanations.  I understand now
your reasons for allowing the interrupt to remain across suspend.  I
will modify my platform to rely on [enable|disable]_irq_wake() in
addition to ensuring platform code is disabling non wakup IRQs.

>>>> This patch ensures that the lazy disable is done, and masked by
>>>> the irq_chip before the system goes into suspend.
>>>
>>> This will create a window where wakeup interrupts can be lost if the
>>> driver has masked the interrupt (by calling disable_irq). If the
>>> hardware does not allow edge detection on disabled interrupts (the msm
>>> platform has this limitation) then this change will turn off the edge
>>> detection. If suspend_ops->enter does not turn the interrupt (and edge
>>> detection) back on (without this change it may never need to turn on
>>> any interrupt) it will not wakeup at all.
>>
>> Not sure I follow you here...
>>
>> It seems like you're relying on the delayed disable to wait until
>> after resume so that disabled interrupts can wake the system.  How
>> did this work before the delayed disable patch?
>>
>
> It did not.
>
>> If the interrupt is being used as a wakeup, why would anyone be
>> calling disable_irq()?
>>
>
> Drivers call disable_irq to make sure their interrupt handler does not
> get called. A driver may not be able take interrupts after its suspend
> hook has been called. If it calls disable_irq on a wakeup interrupt
> then this interrupt should abort suspend or wake up from suspend. The
> driver will then see the interrupt when it call enable_irq in its
> resume handler.

OK.

Kevin

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> 2009/5/8 Kevin Hilman <khilman@deeprootsystems.com>:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>>> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
>>> <khilman@deeprootsystems.com> wrote:
>>>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>>>> From: Kevin Hilman <khilman@deeprootsystems.com>
>>>> Date: Wed, 6 May 2009 16:00:07 -0700
>>>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>>>
>>>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>>>> behavior of disable_irq() was changed to delay the disable until it is
>>>> next handled.
>>>>
>>>> However, this leaves open the possibility that the system can go into
>>>> suspend with an interrupt enabled.  For example, if a driver calls
>>>> disable_irq() in its suspend_hook there's now a possibility that the
>>>> system is suspended before the lazy disable happens.
>>>>
>>>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>>>> waking the system (common on embedded SoCs.)
>>>
>>> If the interrupt contoller uses the same enable register for wakeup
>>> and interrupts, I think it is the responsibility of the platform code,
>>> not individual drivers, to disable the interrupts that are not marked
>>> for wakeup before entering suspend.
>>
>> I agree, for wakeup interrupts, drivers should use
>> [set|disable]_irq_wake() and the platform code should handle this.
>>
>> I used wakeup interrupts in this description as an example which
>> turned out to be a bad example.  The 2nd version of this patch I
>> posted, I removed the reference to wakeup interrupts in favor of just
>> talking about the delayed disable piece.
>>
>> But ignoring wakup interrupts, would you agree that the delayed
>> disable of an interrupt should not wait until after resume?
>>
>
> No. The platform code needs to turn off interrupts that are not wakeup
> interrupts anyway, so there is not much point in disabling some
> interrupts early. Also, if the interrupt in question is not a wakeup
> interrupt you leave it in a state where it does not detect an edge. A
> driver that enables its hardware in resume, then unmasks the interrupt
> would loose an interrupt that triggered between enabling the hardware
> and unmasking the interrupt.

Got it.  Thanks for your (repeated) explanations.  I understand now
your reasons for allowing the interrupt to remain across suspend.  I
will modify my platform to rely on [enable|disable]_irq_wake() in
addition to ensuring platform code is disabling non wakup IRQs.

>>>> This patch ensures that the lazy disable is done, and masked by
>>>> the irq_chip before the system goes into suspend.
>>>
>>> This will create a window where wakeup interrupts can be lost if the
>>> driver has masked the interrupt (by calling disable_irq). If the
>>> hardware does not allow edge detection on disabled interrupts (the msm
>>> platform has this limitation) then this change will turn off the edge
>>> detection. If suspend_ops->enter does not turn the interrupt (and edge
>>> detection) back on (without this change it may never need to turn on
>>> any interrupt) it will not wakeup at all.
>>
>> Not sure I follow you here...
>>
>> It seems like you're relying on the delayed disable to wait until
>> after resume so that disabled interrupts can wake the system.  How
>> did this work before the delayed disable patch?
>>
>
> It did not.
>
>> If the interrupt is being used as a wakeup, why would anyone be
>> calling disable_irq()?
>>
>
> Drivers call disable_irq to make sure their interrupt handler does not
> get called. A driver may not be able take interrupts after its suspend
> hook has been called. If it calls disable_irq on a wakeup interrupt
> then this interrupt should abort suspend or wake up from suspend. The
> driver will then see the interrupt when it call enable_irq in its
> resume handler.

OK.

Kevin

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> 2009/5/8 Kevin Hilman <khilman@deeprootsystems.com>:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>>> On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
>>> <khilman@deeprootsystems.com> wrote:
>>>> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
>>>> From: Kevin Hilman <khilman@deeprootsystems.com>
>>>> Date: Wed, 6 May 2009 16:00:07 -0700
>>>> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>>>>
>>>> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
>>>> behavior of disable_irq() was changed to delay the disable until it is
>>>> next handled.
>>>>
>>>> However, this leaves open the possibility that the system can go into
>>>> suspend with an interrupt enabled.  For example, if a driver calls
>>>> disable_irq() in its suspend_hook there's now a possibility that the
>>>> system is suspended before the lazy disable happens.
>>>>
>>>> The result is an unwanted wakeup from suspend if the IRQ is capable of
>>>> waking the system (common on embedded SoCs.)
>>>
>>> If the interrupt contoller uses the same enable register for wakeup
>>> and interrupts, I think it is the responsibility of the platform code,
>>> not individual drivers, to disable the interrupts that are not marked
>>> for wakeup before entering suspend.
>>
>> I agree, for wakeup interrupts, drivers should use
>> [set|disable]_irq_wake() and the platform code should handle this.
>>
>> I used wakeup interrupts in this description as an example which
>> turned out to be a bad example.  The 2nd version of this patch I
>> posted, I removed the reference to wakeup interrupts in favor of just
>> talking about the delayed disable piece.
>>
>> But ignoring wakup interrupts, would you agree that the delayed
>> disable of an interrupt should not wait until after resume?
>>
>
> No. The platform code needs to turn off interrupts that are not wakeup
> interrupts anyway, so there is not much point in disabling some
> interrupts early. Also, if the interrupt in question is not a wakeup
> interrupt you leave it in a state where it does not detect an edge. A
> driver that enables its hardware in resume, then unmasks the interrupt
> would loose an interrupt that triggered between enabling the hardware
> and unmasking the interrupt.

Got it.  Thanks for your (repeated) explanations.  I understand now
your reasons for allowing the interrupt to remain across suspend.  I
will modify my platform to rely on [enable|disable]_irq_wake() in
addition to ensuring platform code is disabling non wakup IRQs.

>>>> This patch ensures that the lazy disable is done, and masked by
>>>> the irq_chip before the system goes into suspend.
>>>
>>> This will create a window where wakeup interrupts can be lost if the
>>> driver has masked the interrupt (by calling disable_irq). If the
>>> hardware does not allow edge detection on disabled interrupts (the msm
>>> platform has this limitation) then this change will turn off the edge
>>> detection. If suspend_ops->enter does not turn the interrupt (and edge
>>> detection) back on (without this change it may never need to turn on
>>> any interrupt) it will not wakeup at all.
>>
>> Not sure I follow you here...
>>
>> It seems like you're relying on the delayed disable to wait until
>> after resume so that disabled interrupts can wake the system.  How
>> did this work before the delayed disable patch?
>>
>
> It did not.
>
>> If the interrupt is being used as a wakeup, why would anyone be
>> calling disable_irq()?
>>
>
> Drivers call disable_irq to make sure their interrupt handler does not
> get called. A driver may not be able take interrupts after its suspend
> hook has been called. If it calls disable_irq on a wakeup interrupt
> then this interrupt should abort suspend or wake up from suspend. The
> driver will then see the interrupt when it call enable_irq in its
> resume handler.

OK.

Kevin

Re: [PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Arve Hjønnevåg]

On Thu, May 7, 2009 at 9:19 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> From a3f359c66bd0ae1bb2603e7cf120d9d4d68591b7 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@deeprootsystems.com>
> Date: Wed, 6 May 2009 16:00:07 -0700
> Subject: [PATCH] genirq: ensure IRQs are lazy disabled before suspend
>
> In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
> behavior of disable_irq() was changed to delay the disable until it is
> next handled.
>
> However, this leaves open the possibility that the system can go into
> suspend with an interrupt enabled.  For example, if a driver calls
> disable_irq() in its suspend_hook there's now a possibility that the
> system is suspended before the lazy disable happens.
>
> The result is an unwanted wakeup from suspend if the IRQ is capable of
> waking the system (common on embedded SoCs.)

If the interrupt contoller uses the same enable register for wakeup
and interrupts, I think it is the responsibility of the platform code,
not individual drivers, to disable the interrupts that are not marked
for wakeup before entering suspend.

> This patch ensures that the lazy disable is done, and masked by
> the irq_chip before the system goes into suspend.

This will create a window where wakeup interrupts can be lost if the
driver has masked the interrupt (by calling disable_irq). If the
hardware does not allow edge detection on disabled interrupts (the msm
platform has this limitation) then this change will turn off the edge
detection. If suspend_ops->enter does not turn the interrupt (and edge
detection) back on (without this change it may never need to turn on
any interrupt) it will not wakeup at all.

> Note that even though __disable_irq() also calls irq_chip->disable, it
> is quite common for the irq_chip not to provide a disable hook in
> which case the IRQ is never masked/disabled in hardware before going
> into suspend.
>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  kernel/irq/manage.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2734eca..c786820 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -190,6 +190,13 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
>                if (!desc->action || (desc->action->flags & IRQF_TIMER))
>                        return;
>                desc->status |= IRQ_SUSPENDED;
> +
> +               /*
> +                * Lazy disable: handles case where lazy disable in
> +                * handler doesn't happen before suspend.
> +                */
> +               if (desc->status & IRQ_DISABLED)
> +                       desc->chip->mask(irq);
>        }
>
>        if (!desc->depth++) {
> --
> 1.6.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Arve Hjønnevåg

[PATCH] genirq: ensure IRQs are lazy disabled before suspend

[Kevin Hilman]

In commit 76d2160147f43f982dfe881404cfde9fd0a9da21, the default
behavior of disable_irq() was changed to delay the disable until it is
next handled.

However, this leaves open the possibility that the system can go into
suspend with an interrupt enabled.  For example, if a driver calls
disable_irq() in its suspend_hook (for example, to prevent that device
IRQ from causing a system wakeup) there's now a possibility that the
system is suspended before the lazy disable happens.

The result is an unwanted wakeup from suspend if the IRQ is capable of
waking the system (common on embedded SoCs.)

This patch ensures that the lazy disable is done, and masked by
the irq_chip before the system goes into suspend.

Note that even though __disable_irq() also calls irq_chip->disable, it
is quite common for the irq_chip not to provide a disable hook in
which case the IRQ is never masked/disabled in hardware before going
into suspend.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 kernel/irq/manage.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2734eca..5d2cc1c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -190,6 +190,11 @@ void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
 		if (!desc->action || (desc->action->flags & IRQF_TIMER))
 			return;
 		desc->status |= IRQ_SUSPENDED;
+
+		/* Lazy disable: handles case where lazy disable in
+		 * handler doesn't happen before suspend. */
+		if (desc->status & IRQ_DISABLED)
+			desc->chip->mask(irq);
 	}
 
 	if (!desc->depth++) {
-- 
1.6.2.2