[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It currently is required that all users of NO_SUSPEND interrupt
lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the
WARN_ON_ONCE() in irq_pm_install_action() will trigger.  That is
done to warn about situations in which unprepared interrupt handlers
may be run unnecessarily for suspended devices and may attempt to
access those devices by mistake.  However, it may cause drivers
that have no technical reasons for using IRQF_NO_SUSPEND to set
that flag just because they happen to share the interrupt line
with something like a timer.

Moreover, the generic handling of wakeup interrupts introduced by
commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works
for IRQs without any NO_SUSPEND users, so the drivers of wakeup
devices needing to use shared NO_SUSPEND interrupt lines for
signaling system wakeup generally have to detect wakeup in their
interrupt handlers.  Thus if they happen to share an interrupt line
with a NO_SUSPEND user, they also need to request that their
interrupt handlers be run after suspend_device_irqs().

In both cases the reason for using IRQF_NO_SUSPEND is not because
the driver in question has a genuine need to run its interrupt
handler after suspend_device_irqs(), but because it happens to
share the line with some other NO_SUSPEND user.  Otherwise, the
driver would do without IRQF_NO_SUSPEND just fine.

To make it possible to specify that condition explicitly, introduce
a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND,
that, when set, will indicate to the IRQ core that the interrupt
user is generally fine with suspending the IRQ, but it also can
tolerate handler invocations after suspend_device_irqs() and, in
particular, it is capable of detecting system wakeup and triggering
it as appropriate from its interrupt handler.

That will allow us to work around a problem with a shared timer
interrupt line on at91 platforms.

Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
Link: http://marc.info/?t=142252775300011&r=1&w=2
Linx: https://lkml.org/lkml/2014/12/15/552
Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/interrupt.h |    5 +++++
 include/linux/irqdesc.h   |    1 +
 kernel/irq/manage.c       |    7 ++++++-
 kernel/irq/pm.c           |    7 ++++++-
 4 files changed, 18 insertions(+), 2 deletions(-)

Index: linux-pm/include/linux/interrupt.h
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -57,6 +57,10 @@
  * IRQF_NO_THREAD - Interrupt cannot be threaded
  * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
  *                resume time.
+ * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this
+ *                interrupt handler after suspending interrupts. For system
+ *                wakeup devices users need to implement wakeup detection in
+ *                their interrupt handlers.
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SHARED		0x00000080
@@ -70,6 +74,7 @@
 #define IRQF_FORCE_RESUME	0x00008000
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
+#define IRQF_COND_SUSPEND	0x00040000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
Index: linux-pm/include/linux/irqdesc.h
===================================================================
--- linux-pm.orig/include/linux/irqdesc.h
+++ linux-pm/include/linux/irqdesc.h
@@ -78,6 +78,7 @@ struct irq_desc {
 #ifdef CONFIG_PM_SLEEP
 	unsigned int		nr_actions;
 	unsigned int		no_suspend_depth;
+	unsigned int		cond_suspend_depth;
 	unsigned int		force_resume_depth;
 #endif
 #ifdef CONFIG_PROC_FS
Index: linux-pm/kernel/irq/pm.c
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -43,9 +43,12 @@ void irq_pm_install_action(struct irq_de
 
 	if (action->flags & IRQF_NO_SUSPEND)
 		desc->no_suspend_depth++;
+	else if (action->flags & IRQF_COND_SUSPEND)
+		desc->cond_suspend_depth++;
 
 	WARN_ON_ONCE(desc->no_suspend_depth &&
-		     desc->no_suspend_depth != desc->nr_actions);
+		     (desc->no_suspend_depth +
+			desc->cond_suspend_depth) != desc->nr_actions);
 }
 
 /*
@@ -61,6 +64,8 @@ void irq_pm_remove_action(struct irq_des
 
 	if (action->flags & IRQF_NO_SUSPEND)
 		desc->no_suspend_depth--;
+	else if (action->flags & IRQF_COND_SUSPEND)
+		desc->cond_suspend_depth--;
 }
 
 static bool suspend_device_irq(struct irq_desc *desc, int irq)
Index: linux-pm/kernel/irq/manage.c
===================================================================
--- linux-pm.orig/kernel/irq/manage.c
+++ linux-pm/kernel/irq/manage.c
@@ -1474,8 +1474,13 @@ int request_threaded_irq(unsigned int ir
 	 * otherwise we'll have trouble later trying to figure out
 	 * which interrupt is which (messes up the interrupt freeing
 	 * logic etc).
+	 *
+	 * Also IRQF_COND_SUSPEND only makes sense for shared interrupts and
+	 * it cannot be set along with IRQF_NO_SUSPEND.
 	 */
-	if ((irqflags & IRQF_SHARED) && !dev_id)
+	if (((irqflags & IRQF_SHARED) && !dev_id) ||
+	    (!(irqflags & IRQF_SHARED) && (irqflags & IRQF_COND_SUSPEND)) ||
+	    ((irqflags & IRQF_NO_SUSPEND) && (irqflags & IRQF_COND_SUSPEND)))
 		return -EINVAL;
 
 	desc = irq_to_desc(irq);

Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Peter Zijlstra]

On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It currently is required that all users of NO_SUSPEND interrupt
> lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the
> WARN_ON_ONCE() in irq_pm_install_action() will trigger.  That is
> done to warn about situations in which unprepared interrupt handlers
> may be run unnecessarily for suspended devices and may attempt to
> access those devices by mistake.  However, it may cause drivers
> that have no technical reasons for using IRQF_NO_SUSPEND to set
> that flag just because they happen to share the interrupt line
> with something like a timer.
> 
> Moreover, the generic handling of wakeup interrupts introduced by
> commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works
> for IRQs without any NO_SUSPEND users, so the drivers of wakeup
> devices needing to use shared NO_SUSPEND interrupt lines for
> signaling system wakeup generally have to detect wakeup in their
> interrupt handlers.  Thus if they happen to share an interrupt line
> with a NO_SUSPEND user, they also need to request that their
> interrupt handlers be run after suspend_device_irqs().
> 
> In both cases the reason for using IRQF_NO_SUSPEND is not because
> the driver in question has a genuine need to run its interrupt
> handler after suspend_device_irqs(), but because it happens to
> share the line with some other NO_SUSPEND user.  Otherwise, the
> driver would do without IRQF_NO_SUSPEND just fine.
> 
> To make it possible to specify that condition explicitly, introduce
> a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND,
> that, when set, will indicate to the IRQ core that the interrupt
> user is generally fine with suspending the IRQ, but it also can
> tolerate handler invocations after suspend_device_irqs() and, in
> particular, it is capable of detecting system wakeup and triggering
> it as appropriate from its interrupt handler.
> 
> That will allow us to work around a problem with a shared timer
> interrupt line on at91 platforms.
> 
> Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> Link: http://marc.info/?t=142252775300011&r=1&w=2
> Linx: https://lkml.org/lkml/2014/12/15/552
> Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Seems good to me. Should I take this through tip/irq ?

Also, should we warn if people use enable_irq_wake() where there is only
a single descriptor with NO_SUSPEND?

Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Rafael J. Wysocki]

On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote:
> On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > It currently is required that all users of NO_SUSPEND interrupt
> > lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the
> > WARN_ON_ONCE() in irq_pm_install_action() will trigger.  That is
> > done to warn about situations in which unprepared interrupt handlers
> > may be run unnecessarily for suspended devices and may attempt to
> > access those devices by mistake.  However, it may cause drivers
> > that have no technical reasons for using IRQF_NO_SUSPEND to set
> > that flag just because they happen to share the interrupt line
> > with something like a timer.
> > 
> > Moreover, the generic handling of wakeup interrupts introduced by
> > commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works
> > for IRQs without any NO_SUSPEND users, so the drivers of wakeup
> > devices needing to use shared NO_SUSPEND interrupt lines for
> > signaling system wakeup generally have to detect wakeup in their
> > interrupt handlers.  Thus if they happen to share an interrupt line
> > with a NO_SUSPEND user, they also need to request that their
> > interrupt handlers be run after suspend_device_irqs().
> > 
> > In both cases the reason for using IRQF_NO_SUSPEND is not because
> > the driver in question has a genuine need to run its interrupt
> > handler after suspend_device_irqs(), but because it happens to
> > share the line with some other NO_SUSPEND user.  Otherwise, the
> > driver would do without IRQF_NO_SUSPEND just fine.
> > 
> > To make it possible to specify that condition explicitly, introduce
> > a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND,
> > that, when set, will indicate to the IRQ core that the interrupt
> > user is generally fine with suspending the IRQ, but it also can
> > tolerate handler invocations after suspend_device_irqs() and, in
> > particular, it is capable of detecting system wakeup and triggering
> > it as appropriate from its interrupt handler.
> > 
> > That will allow us to work around a problem with a shared timer
> > interrupt line on at91 platforms.
> > 
> > Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> > Link: http://marc.info/?t=142252775300011&r=1&w=2
> > Linx: https://lkml.org/lkml/2014/12/15/552
> > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Seems good to me. Should I take this through tip/irq ?

I can apply it along with the previous IRQF_NO_SUSPEND documentation patch
from Mark Rutland if you ACK it for me. :-)

> Also, should we warn if people use enable_irq_wake() where there is only
> a single descriptor with NO_SUSPEND?

We probably should do that, but that would be a separate patch IMO?

Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Peter Zijlstra]

On Fri, Feb 27, 2015 at 11:13:57PM +0100, Rafael J. Wysocki wrote:
> On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote:

> > Seems good to me. Should I take this through tip/irq ?
> 
> I can apply it along with the previous IRQF_NO_SUSPEND documentation patch
> from Mark Rutland if you ACK it for me. :-)

Works for me,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > Also, should we warn if people use enable_irq_wake() where there is only
> > a single descriptor with NO_SUSPEND?
> 
> We probably should do that, but that would be a separate patch IMO?

Agreed. Just wanted to raise the point.

Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Mark Rutland]

Hi Rafael,

I'm a little late to the party here, but I have just a couple of minor
comments...

[...]

> Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> Link: http://marc.info/?t=142252775300011&r=1&w=2
> Linx: https://lkml.org/lkml/2014/12/15/552

s/x/k/ ?

> Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  include/linux/interrupt.h |    5 +++++
>  include/linux/irqdesc.h   |    1 +
>  kernel/irq/manage.c       |    7 ++++++-
>  kernel/irq/pm.c           |    7 ++++++-
>  4 files changed, 18 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/include/linux/interrupt.h
> ===================================================================
> --- linux-pm.orig/include/linux/interrupt.h
> +++ linux-pm/include/linux/interrupt.h
> @@ -57,6 +57,10 @@
>   * IRQF_NO_THREAD - Interrupt cannot be threaded
>   * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
>   *                resume time.
> + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this
> + *                interrupt handler after suspending interrupts. For system
> + *                wakeup devices users need to implement wakeup detection in
> + *                their interrupt handlers.

It's probably worth documenting this in suspend-and-interrupts.txt, as
this invalidates some of the "IRQF_NO_SUSPEND and enable_irq_wake()"
section. I'll send a patch momentarily to that effect.

Otherwise, this patch looks good, thanks for handling this!

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Mark Rutland]

With certain restrictions it is possible for a wakeup device to share
and IRQ with an IRQF_NO_SUSPEND user, and the warnings introduced by
commit cab303be91dc47942bc25de33dc1140123540800 are spurious. The new
IRQF_COND_SUSPEND flag allows drivers to tell the core when these
restrictions are met, allowing spurious warnings to be silenced.

This patch documents how IRQF_COND_SUSPEND is expected to be used,
updating some of the text now made invalid by its addition.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/power/suspend-and-interrupts.txt | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

As promised previously, take IRQF_COND_SUSPEND into account in the
documentation.

Rafael, does this look OK to you?

Thanks,
Mark.

diff --git a/Documentation/power/suspend-and-interrupts.txt b/Documentation/power/suspend-and-interrupts.txt
index 50493c9..8afb29a 100644
--- a/Documentation/power/suspend-and-interrupts.txt
+++ b/Documentation/power/suspend-and-interrupts.txt
@@ -112,8 +112,9 @@ any special interrupt handling logic for it to work.
 IRQF_NO_SUSPEND and enable_irq_wake()
 -------------------------------------
 
-There are no valid reasons to use both enable_irq_wake() and the IRQF_NO_SUSPEND
-flag on the same IRQ.
+There are very few valid reasons to use both enable_irq_wake() and the
+IRQF_NO_SUSPEND flag on the same IRQ, and it is never valid to use both for the
+same device.
 
 First of all, if the IRQ is not shared, the rules for handling IRQF_NO_SUSPEND
 interrupts (interrupt handlers are invoked after suspend_device_irqs()) are
@@ -122,4 +123,13 @@ handlers are not invoked after suspend_device_irqs()).
 
 Second, both enable_irq_wake() and IRQF_NO_SUSPEND apply to entire IRQs and not
 to individual interrupt handlers, so sharing an IRQ between a system wakeup
-interrupt source and an IRQF_NO_SUSPEND interrupt source does not make sense.
+interrupt source and an IRQF_NO_SUSPEND interrupt source does not generally
+make sense.
+
+In rare cases an IRQ can be shared between a wakeup device driver and an
+IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver
+must be able to discern spurious IRQs from genuine wakeup events (signalling
+the latter to the core with pm_system_wakeup()), must use enable_irq_wake() to
+ensure that the IRQ will function as a wakeup source, and must request the IRQ
+with IRQF_COND_SUSPEND to tell the core that it meets these requirements. If
+these requirements are not met, it is not valid to use IRQF_COND_SUSPEND.
-- 
1.9.1

Re: [PATCH] genirq: describe IRQF_COND_SUSPEND

[Rafael J. Wysocki]

On Wednesday, March 04, 2015 08:00:40 PM Mark Rutland wrote:
> With certain restrictions it is possible for a wakeup device to share
> and IRQ with an IRQF_NO_SUSPEND user, and the warnings introduced by
> commit cab303be91dc47942bc25de33dc1140123540800 are spurious. The new
> IRQF_COND_SUSPEND flag allows drivers to tell the core when these
> restrictions are met, allowing spurious warnings to be silenced.
> 
> This patch documents how IRQF_COND_SUSPEND is expected to be used,
> updating some of the text now made invalid by its addition.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  Documentation/power/suspend-and-interrupts.txt | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> As promised previously, take IRQF_COND_SUSPEND into account in the
> documentation.
> 
> Rafael, does this look OK to you?

Yes, it does, thanks!

I'll queue it up along with the rest of the IRQF_COND_SUSPEND patches.


> diff --git a/Documentation/power/suspend-and-interrupts.txt b/Documentation/power/suspend-and-interrupts.txt
> index 50493c9..8afb29a 100644
> --- a/Documentation/power/suspend-and-interrupts.txt
> +++ b/Documentation/power/suspend-and-interrupts.txt
> @@ -112,8 +112,9 @@ any special interrupt handling logic for it to work.
>  IRQF_NO_SUSPEND and enable_irq_wake()
>  -------------------------------------
>  
> -There are no valid reasons to use both enable_irq_wake() and the IRQF_NO_SUSPEND
> -flag on the same IRQ.
> +There are very few valid reasons to use both enable_irq_wake() and the
> +IRQF_NO_SUSPEND flag on the same IRQ, and it is never valid to use both for the
> +same device.
>  
>  First of all, if the IRQ is not shared, the rules for handling IRQF_NO_SUSPEND
>  interrupts (interrupt handlers are invoked after suspend_device_irqs()) are
> @@ -122,4 +123,13 @@ handlers are not invoked after suspend_device_irqs()).
>  
>  Second, both enable_irq_wake() and IRQF_NO_SUSPEND apply to entire IRQs and not
>  to individual interrupt handlers, so sharing an IRQ between a system wakeup
> -interrupt source and an IRQF_NO_SUSPEND interrupt source does not make sense.
> +interrupt source and an IRQF_NO_SUSPEND interrupt source does not generally
> +make sense.
> +
> +In rare cases an IRQ can be shared between a wakeup device driver and an
> +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver
> +must be able to discern spurious IRQs from genuine wakeup events (signalling
> +the latter to the core with pm_system_wakeup()), must use enable_irq_wake() to
> +ensure that the IRQ will function as a wakeup source, and must request the IRQ
> +with IRQF_COND_SUSPEND to tell the core that it meets these requirements. If
> +these requirements are not met, it is not valid to use IRQF_COND_SUSPEND.
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] genirq: describe IRQF_COND_SUSPEND

[Alexandre Belloni]

tiny tiny nitpick:

On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> With certain restrictions it is possible for a wakeup device to share
> and IRQ with an IRQF_NO_SUSPEND user, and the warnings introduced by
  ^ an


> +In rare cases an IRQ can be shared between a wakeup device driver and an
> +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver
> +must be able to discern spurious IRQs from genuine wakeup events (signalling

And genuine question, should we use British English or American English
or we don't care ?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] genirq: describe IRQF_COND_SUSPEND

[Arnd Bergmann]

On Wednesday 04 March 2015 23:17:29 Alexandre Belloni wrote:
> On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> > With certain restrictions it is possible for a wakeup device to share
> > and IRQ with an IRQF_NO_SUSPEND user, and the warnings introduced by
>   ^ an
> 
> 
> > +In rare cases an IRQ can be shared between a wakeup device driver and an
> > +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver
> > +must be able to discern spurious IRQs from genuine wakeup events (signalling
> 
> And genuine question, should we use British English or American English
> or we don't care ?

I believe most developers use Incorrect English, the second most common
is American English, followed by Indian English and British English. ;-)

More seriously, just try to be consistent within one file or subsystem.

	Arnd

Re: [PATCH] genirq: describe IRQF_COND_SUSPEND

[Mark Rutland]

On Wed, Mar 04, 2015 at 10:17:29PM +0000, Alexandre Belloni wrote:
> tiny tiny nitpick:
> 
> On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> > With certain restrictions it is possible for a wakeup device to share
> > and IRQ with an IRQF_NO_SUSPEND user, and the warnings introduced by
>   ^ an

Whoops.

Rafael, are you happy to fix that up, or would you like me to resend?

> > +In rare cases an IRQ can be shared between a wakeup device driver and an
> > +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver
> > +must be able to discern spurious IRQs from genuine wakeup events (signalling
> 
> And genuine question, should we use British English or American English
> or we don't care ?

Have I written something that isn't valid American English there? I read
over this a few times and failed to spot anything obvious.

I'm happy to change for consistency, I generally assume that's the most
important thing.

Mark.

Re: [PATCH] genirq: describe IRQF_COND_SUSPEND

[Alexandre Belloni]

On 05/03/2015 at 11:04:11 +0000, Mark Rutland wrote :
> > > +In rare cases an IRQ can be shared between a wakeup device driver and an
> > > +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver
> > > +must be able to discern spurious IRQs from genuine wakeup events (signalling
> > 
> > And genuine question, should we use British English or American English
> > or we don't care ?
> 
> Have I written something that isn't valid American English there? I read
> over this a few times and failed to spot anything obvious.
> 
> I'm happy to change for consistency, I generally assume that's the most
> important thing.

I'd say signalling vs signaling. I actually had to look up which one was
correct. I'm personally using Incorrect/Broken English so I'm definitely
not here to give lessons.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] genirq: describe IRQF_COND_SUSPEND

[Mark Rutland]

On Thu, Mar 05, 2015 at 11:33:06AM +0000, Alexandre Belloni wrote:
> On 05/03/2015 at 11:04:11 +0000, Mark Rutland wrote :
> > > > +In rare cases an IRQ can be shared between a wakeup device driver and an
> > > > +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver
> > > > +must be able to discern spurious IRQs from genuine wakeup events (signalling
> > > 
> > > And genuine question, should we use British English or American English
> > > or we don't care ?
> > 
> > Have I written something that isn't valid American English there? I read
> > over this a few times and failed to spot anything obvious.
> > 
> > I'm happy to change for consistency, I generally assume that's the most
> > important thing.
> 
> I'd say signalling vs signaling. I actually had to look up which one was
> correct. I'm personally using Incorrect/Broken English so I'm definitely
> not here to give lessons.

Easy option to keep everyone happy: s/signalling/indicating/

That should be valid for the English variants I'm aware of, and it has
the same number of characters so we don't need to reflow the text.

Mark.

Re: [PATCH] genirq: describe IRQF_COND_SUSPEND

[Rafael J. Wysocki]

On Thursday, March 05, 2015 11:04:11 AM Mark Rutland wrote:
> On Wed, Mar 04, 2015 at 10:17:29PM +0000, Alexandre Belloni wrote:
> > tiny tiny nitpick:
> > 
> > On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> > > With certain restrictions it is possible for a wakeup device to share
> > > and IRQ with an IRQF_NO_SUSPEND user, and the warnings introduced by
> >   ^ an
> 
> Whoops.
> 
> Rafael, are you happy to fix that up, or would you like me to resend?

Fixed, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Rafael J. Wysocki]

On Wednesday, March 04, 2015 07:42:46 PM Mark Rutland wrote:
> Hi Rafael,
> 
> I'm a little late to the party here, but I have just a couple of minor
> comments...
> 
> [...]
> 
> > Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> > Link: http://marc.info/?t=142252775300011&r=1&w=2
> > Linx: https://lkml.org/lkml/2014/12/15/552
> 
> s/x/k/ ?

Yes, thanks!

> > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  include/linux/interrupt.h |    5 +++++
> >  include/linux/irqdesc.h   |    1 +
> >  kernel/irq/manage.c       |    7 ++++++-
> >  kernel/irq/pm.c           |    7 ++++++-
> >  4 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > Index: linux-pm/include/linux/interrupt.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/interrupt.h
> > +++ linux-pm/include/linux/interrupt.h
> > @@ -57,6 +57,10 @@
> >   * IRQF_NO_THREAD - Interrupt cannot be threaded
> >   * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
> >   *                resume time.
> > + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this
> > + *                interrupt handler after suspending interrupts. For system
> > + *                wakeup devices users need to implement wakeup detection in
> > + *                their interrupt handlers.
> 
> It's probably worth documenting this in suspend-and-interrupts.txt, as
> this invalidates some of the "IRQF_NO_SUSPEND and enable_irq_wake()"
> section. I'll send a patch momentarily to that effect.
>
> Otherwise, this patch looks good, thanks for handling this!
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks!