Skip to contentSkip to conversation

genirq: {en,dis}able_irq_wake() need refcounting too

PATCH In review David Brownell 4 participants 8 messages Jul 9, 2006 – Jul 15, 2006 IRQ

lore
Share & tools
lore

David Brownell

author
#0 lore raw

PATCH 1/1 genirq: {en,dis}able_irq_wake() need refcounting too

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

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 <[email protected]>
include/linux/irq.h +3 -0
Index: at91/include/linux/irq.h
===================================================================
@@ -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;
kernel/irq/manage.c +26 -2
Index: at91/kernel/irq/manage.c
===================================================================
@@ -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;
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · David Brownell

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 <[email protected]>

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

#2 lore raw
Quoted 11 lines · Ingo Molnar
Quoted 7 lines · David Brownell

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...)

Quoted 5 lines
Acked-by Ingo Molnar <[email protected]>

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.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core
#3 lore raw
Quoted 6 lines · Russell King
Quoted 3 lines

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

#4 lore raw
In reply to Russell King
Quoted 12 lines · Russell King
Quoted 11 lines · Ingo Molnar
Quoted 7 lines · David Brownell

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...)

Quoted 8 lines
Quoted 5 lines
Acked-by Ingo Molnar <[email protected]>

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.)

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core
#5 lore raw
Quoted 3 lines · Russell King

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

Acked-by
#6 lore raw
In reply to Ingo Molnar
Quoted 11 lines · Ingo Molnar
Quoted 7 lines · David Brownell

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 <[email protected]>
Acked-by Thomas Gleixner <[email protected]>
Quoted 3 lines

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

author
#7 lore raw
In reply to Ingo Molnar
Quoted 11 lines · Ingo Molnar
Quoted 7 lines · David Brownell

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...)

Quoted 5 lines
Acked-by Ingo Molnar <[email protected]>

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