[patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

It's not just "normal" mode operation that needs refcounting for the
{en,dis}able_irq() calls ... "wakeup" mode calls need it too, for the
very same reasons.

This patch adds that refcounting.  I expect that some ARM drivers will
be triggering the new warning, but this call isn't yet widely used.
(Which is probably why the bug has lingered this long...)

- Dave


[-- Attachment #2: irqwake.patch --]
[-- Type: text/x-diff, Size: 3805 bytes --]

IRQs need refcounting and a state flag to track whether the the IRQ should
be enabled or disabled as a "normal IRQ" source after a series of calls to
disable_irq() and enable_irq().  For shared IRQs, the IRQ must be enabled
so long as at least one driver needs it active.

Likewise, IRQs need the same support to track whether the IRQ should be
enabled or disabled as a "wakeup event" source after a series of calls to
enable_irq_wake() and disable_irq_wake().  For shared IRQs, the IRQ must
be wakeup-enabled so long as at least one driver needs it.

But right now they don't have that refcounting ... which means sharing
wakeup-capable IRQs can't work correctly in some configurations.  This
patch adds the refcount and flag mechanisms to set_irq_wake(), and a
minimal description of what an irq wake mechanism is.

Drivers relying on the older (broken) "toggle" semantics will trigger a
warning; that'll be a handful of drivers on ARM systems.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>


Index: at91/include/linux/irq.h
===================================================================
--- at91.orig/include/linux/irq.h	2006-07-09 13:46:34.000000000 -0700
+++ at91/include/linux/irq.h	2006-07-09 13:57:35.000000000 -0700
@@ -58,6 +58,7 @@
 #define IRQ_NOREQUEST		0x04000000	/* IRQ cannot be requested */
 #define IRQ_NOAUTOEN		0x08000000	/* IRQ will not be enabled on request irq */
 #define IRQ_DELAYED_DISABLE	0x10000000	/* IRQ disable (masking) happens delayed. */
+#define IRQ_WAKEUP		0x20000000	/* IRQ triggers system wakeup */
 
 struct proc_dir_entry;
 
@@ -124,6 +125,7 @@ struct irq_chip {
  * @action:		the irq action chain
  * @status:		status information
  * @depth:		disable-depth, for nested irq_disable() calls
+ * @wake_depth:		enable depth, for multiple set_irq_wake() callers
  * @irq_count:		stats field to detect stalled irqs
  * @irqs_unhandled:	stats field for spurious unhandled interrupts
  * @lock:		locking for SMP
@@ -147,6 +149,7 @@ struct irq_desc {
 	unsigned int		status;		/* IRQ status */
 
 	unsigned int		depth;		/* nested irq disables */
+	unsigned int		wake_depth;	/* nested wake enables */
 	unsigned int		irq_count;	/* For detecting broken IRQs */
 	unsigned int		irqs_unhandled;
 	spinlock_t		lock;
Index: at91/kernel/irq/manage.c
===================================================================
--- at91.orig/kernel/irq/manage.c	2006-07-09 13:46:34.000000000 -0700
+++ at91/kernel/irq/manage.c	2006-07-09 13:57:35.000000000 -0700
@@ -137,16 +137,40 @@ EXPORT_SYMBOL(enable_irq);
  *	@irq:	interrupt to control
  *	@on:	enable/disable power management wakeup
  *
- *	Enable/disable power management wakeup mode
+ *	Enable/disable power management wakeup mode, which is
+ *	disabled by default.  Enables and disables must match,
+ *	just as they match for non-wakeup mode support.
+ *
+ *	Wakeup mode lets this IRQ wake the system from sleep
+ *	states like "suspend to RAM".
  */
 int set_irq_wake(unsigned int irq, unsigned int on)
 {
 	struct irq_desc *desc = irq_desc + irq;
 	unsigned long flags;
 	int ret = -ENXIO;
+	int (*set_wake)(unsigned, unsigned) = desc->chip->set_wake;
 
+	/* wakeup-capable irqs can be shared between drivers that
+	 * don't need to have the same sleep mode behaviors.
+	 */
 	spin_lock_irqsave(&desc->lock, flags);
-	if (desc->chip->set_wake)
+	if (on) {
+		if (desc->wake_depth++ == 0)
+			desc->status |= IRQ_WAKEUP;
+		else
+			set_wake = NULL;
+	} else {
+		if (desc->wake_depth == 0) {
+			printk(KERN_WARNING "Unbalanced IRQ %d "
+					"wake disable\n", irq);
+			WARN_ON(1);
+		} else if (--desc->wake_depth == 0)
+			desc->status &= ~IRQ_WAKEUP;
+		else
+			set_wake = NULL;
+	}
+	if (set_wake)
 		ret = desc->chip->set_wake(irq, on);
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return ret;

Re: [patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[Ingo Molnar]

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

> It's not just "normal" mode operation that needs refcounting for the 
> {en,dis}able_irq() calls ... "wakeup" mode calls need it too, for the 
> very same reasons.
> 
> This patch adds that refcounting.  I expect that some ARM drivers will 
> be triggering the new warning, but this call isn't yet widely used. 
> (Which is probably why the bug has lingered this long...)

Acked-by: Ingo Molnar <mingo@elte.hu>

we should also add disable_irq_wake() / enable_irq_wake() APIs and start 
migrating most ARM users over to the new APIs, agreed? That makes the 
APIs more symmetric and the code more readable too.

	Ingo

Re: [patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[Ingo Molnar]

* Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> > we should also add disable_irq_wake() / enable_irq_wake() APIs and 
> > start migrating most ARM users over to the new APIs, agreed? That 
> > makes the APIs more symmetric and the code more readable too.
> 
> That _is_ the API anyway.  set_irq_wake() was never intended to be 
> called directly from drivers.

doh - right. So the patch is the right thing to do :-)

	Ingo

Re: [patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[Russell King]

On Mon, Jul 10, 2006 at 10:58:49AM +0200, Ingo Molnar wrote:
> 
> * David Brownell <david-b@pacbell.net> wrote:
> 
> > It's not just "normal" mode operation that needs refcounting for the 
> > {en,dis}able_irq() calls ... "wakeup" mode calls need it too, for the 
> > very same reasons.
> > 
> > This patch adds that refcounting.  I expect that some ARM drivers will 
> > be triggering the new warning, but this call isn't yet widely used. 
> > (Which is probably why the bug has lingered this long...)
> 
> Acked-by: Ingo Molnar <mingo@elte.hu>
> 
> we should also add disable_irq_wake() / enable_irq_wake() APIs and start 
> migrating most ARM users over to the new APIs, agreed? That makes the 
> APIs more symmetric and the code more readable too.

That _is_ the API anyway.  set_irq_wake() was never intended to be called
directly from drivers.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[Ingo Molnar]

* Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> Let's first get genirq to a point where it's a direct replacement for 
> my IRQ subsystem before thinking about changing existing APIs (and 
> thereby possibly introducing other breakage.)

note that the API change i suggested was a NOP operation :-) The APIs 
were already shaped that way. No need to worry.

	Ingo

Re: [patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[Russell King]

On Mon, Jul 10, 2006 at 10:13:40AM +0100, Russell King wrote:
> On Mon, Jul 10, 2006 at 10:58:49AM +0200, Ingo Molnar wrote:
> > 
> > * David Brownell <david-b@pacbell.net> wrote:
> > 
> > > It's not just "normal" mode operation that needs refcounting for the 
> > > {en,dis}able_irq() calls ... "wakeup" mode calls need it too, for the 
> > > very same reasons.
> > > 
> > > This patch adds that refcounting.  I expect that some ARM drivers will 
> > > be triggering the new warning, but this call isn't yet widely used. 
> > > (Which is probably why the bug has lingered this long...)
> > 
> > Acked-by: Ingo Molnar <mingo@elte.hu>
> > 
> > we should also add disable_irq_wake() / enable_irq_wake() APIs and start 
> > migrating most ARM users over to the new APIs, agreed? That makes the 
> > APIs more symmetric and the code more readable too.
> 
> That _is_ the API anyway.  set_irq_wake() was never intended to be called
> directly from drivers.

Also note that genirq has a bigger problem to fry at the moment - it's
currently broken for ARM SMP + hotplug CPU due to missing functionality
(as in, the kernel doesn't even link.)  Thomas has a patch which is
going through the test mills at the moment.

Let's first get genirq to a point where it's a direct replacement for
my IRQ subsystem before thinking about changing existing APIs (and
thereby possibly introducing other breakage.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[Thomas Gleixner]

On Mon, 2006-07-10 at 10:58 +0200, Ingo Molnar wrote:
> * David Brownell <david-b@pacbell.net> wrote:
> 
> > It's not just "normal" mode operation that needs refcounting for the 
> > {en,dis}able_irq() calls ... "wakeup" mode calls need it too, for the 
> > very same reasons.
> > 
> > This patch adds that refcounting.  I expect that some ARM drivers will 
> > be triggering the new warning, but this call isn't yet widely used. 
> > (Which is probably why the bug has lingered this long...)
> 
> Acked-by: Ingo Molnar <mingo@elte.hu>

  Acked-by: Thomas Gleixner <tglx@linutronix.de>

> we should also add disable_irq_wake() / enable_irq_wake() APIs and start 
> migrating most ARM users over to the new APIs, agreed? That makes the 
> APIs more symmetric and the code more readable too.

see linux/interrupt.h:

/* IRQ wakeup (PM) control: */
extern int set_irq_wake(unsigned int  irq, unsigned int on);

static inline int enable_irq_wake(unsigned int irq)
{
        return set_irq_wake(irq, 1);
}

static inline int disable_irq_wake(unsigned int irq)
{
        return set_irq_wake(irq, 0);
}

It's already there and ARM uses those functions :)

	tglx

Re: [patch 2.6.18-rc1] genirq: {en,dis}able_irq_wake() need refcounting too

[David Brownell]

On Monday 10 July 2006 1:58 am, Ingo Molnar wrote:
> 
> * David Brownell <david-b@pacbell.net> wrote:
> 
> > It's not just "normal" mode operation that needs refcounting for the 
> > {en,dis}able_irq() calls ... "wakeup" mode calls need it too, for the 
> > very same reasons.
> > 
> > This patch adds that refcounting.  I expect that some ARM drivers will 
> > be triggering the new warning, but this call isn't yet widely used. 
> > (Which is probably why the bug has lingered this long...)
> 
> Acked-by: Ingo Molnar <mingo@elte.hu>
> 
> we should also add disable_irq_wake() / enable_irq_wake() APIs and start 
> migrating most ARM users over to the new APIs, agreed? That makes the 
> APIs more symmetric and the code more readable too.

To recap, the driver code _is_ that symmetric, it's just the implementation
that's asymmetric.  That is, {en,dis}able_irq() are two separate routines,
while {en,dis}able_irq_wake() are just wrap set_irq_wake().

I'll forward this patch to the the ARM kernel list, to help avoid surprises.
There aren't many in-tree drivers using these calls.

- Dave