[PATCH] Free setup_irq() interrupt V2

[PATCH] Free setup_irq() interrupt V2

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch adds a __free_irq() function for releasing 
interrupts requested with setup_irq().

Without this patch we have no way of releasing such
interrupts since free_irq() today tries to kfree()
the irqaction passed with setup_irq().

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Changes since V1:
 - break out a __free_irq() function, thanks Ingo! 

 Why again are we not using struct list_head for the irqaction list?

 include/linux/irq.h |    1 +
 kernel/irq/manage.c |   40 ++++++++++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 14 deletions(-)

--- 0001/include/linux/irq.h
+++ work/include/linux/irq.h	2009-03-10 19:00:34.000000000 +0900
@@ -236,6 +236,7 @@ typedef struct irq_desc		irq_desc_t;
 #include <asm/hw_irq.h>
 
 extern int setup_irq(unsigned int irq, struct irqaction *new);
+extern struct irqaction *__free_irq(unsigned int irq, void *dev_id);
 
 #ifdef CONFIG_GENERIC_HARDIRQS
 
--- 0001/kernel/irq/manage.c
+++ work/kernel/irq/manage.c	2009-03-10 19:03:56.000000000 +0900
@@ -551,20 +551,14 @@ int setup_irq(unsigned int irq, struct i
 }
 
 /**
- *	free_irq - free an interrupt
+ *	__free_irq - free an interrupt
  *	@irq: Interrupt line to free
  *	@dev_id: Device identity to free
  *
- *	Remove an interrupt handler. The handler is removed and if the
- *	interrupt line is no longer in use by any driver it is disabled.
- *	On a shared IRQ the caller must ensure the interrupt is disabled
- *	on the card it drives before calling this function. The function
- *	does not return until any executing interrupts for this IRQ
- *	have completed.
- *
- *	This function must not be called from interrupt context.
+ * Used to remove interrupts statically setup by the early boot process.
  */
-void free_irq(unsigned int irq, void *dev_id)
+
+struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	struct irqaction **p;
@@ -573,7 +567,7 @@ void free_irq(unsigned int irq, void *de
 	WARN_ON(in_interrupt());
 
 	if (!desc)
-		return;
+		return NULL;
 
 	spin_lock_irqsave(&desc->lock, flags);
 	p = &desc->action;
@@ -623,17 +617,35 @@ void free_irq(unsigned int irq, void *de
 				local_irq_restore(flags);
 			}
 #endif
-			kfree(action);
-			return;
+			return action;
 		}
 		printk(KERN_ERR "Trying to free already-free IRQ %d\n", irq);
 #ifdef CONFIG_DEBUG_SHIRQ
 		dump_stack();
 #endif
 		spin_unlock_irqrestore(&desc->lock, flags);
-		return;
+		return NULL;
 	}
 }
+
+/**
+ *	free_irq - free an interrupt allocated with request_irq
+ *	@irq: Interrupt line to free
+ *	@dev_id: Device identity to free
+ *
+ *	Remove an interrupt handler. The handler is removed and if the
+ *	interrupt line is no longer in use by any driver it is disabled.
+ *	On a shared IRQ the caller must ensure the interrupt is disabled
+ *	on the card it drives before calling this function. The function
+ *	does not return until any executing interrupts for this IRQ
+ *	have completed.
+ *
+ *	This function must not be called from interrupt context.
+ */
+void free_irq(unsigned int irq, void *dev_id)
+{
+	kfree(__free_irq(irq, dev_id));
+}
 EXPORT_SYMBOL(free_irq);
 
 /**

Re: [PATCH] Free setup_irq() interrupt V2

[Ingo Molnar]

* Magnus Damm <magnus.damm@gmail.com> wrote:

> From: Magnus Damm <damm@igel.co.jp>
> 
> This patch adds a __free_irq() function for releasing 
> interrupts requested with setup_irq().
> 
> Without this patch we have no way of releasing such
> interrupts since free_irq() today tries to kfree()
> the irqaction passed with setup_irq().
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Changes since V1:
>  - break out a __free_irq() function, thanks Ingo! 

Looks good. Could you please send it against the latest genirq 
tree:

  http://people.redhat.com/mingo/tip.git/README

as your patch does not apply cleanly anymore:

  1 out of 3 hunks FAILED -- rejects in file kernel/irq/manage.c

>  Why again are we not using struct list_head for the irqaction list?

irq_desc size mostly.

	Ingo

Re: [PATCH] Free setup_irq() interrupt V2

[Thomas Gleixner]

Magnus,

On Tue, 10 Mar 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> This patch adds a __free_irq() function for releasing 
> interrupts requested with setup_irq().

I think there is a simpler solution than adding yet another function
for the confusion of driver writers. See below.

>  Why again are we not using struct list_head for the irqaction list?

Why should we ? The add/remove of interrupt handlers is not a high
frequency hot path operation.

Thanks,

	tglx
--------

Subject: genirq: make free_irq() aware of static irqactions
From: Thomas Gleixner <tglx@linutronix.de>
Date: Tue, 10 Mar 2009 15:00:59 +0100

Impact: enhancement

Interrupts which are set up via setup_irq() with a static irqaction
can not be released via free_irq(). The irqs set up via request_irq()
use a kmalloc'ed irqaction. Mark them as allocated in irqaction->flags
and check the flag in free_irq().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/interrupt.h |    2 ++
 kernel/irq/manage.c       |    5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6/include/linux/interrupt.h
===================================================================
--- linux-2.6.orig/include/linux/interrupt.h
+++ linux-2.6/include/linux/interrupt.h
@@ -49,6 +49,7 @@
  * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is
  *                registered first in an shared interrupt is considered for
  *                performance reasons)
+ * IRQF_ALLOCATED - Mark the irqaction as allocated by request_irq
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SAMPLE_RANDOM	0x00000040
@@ -58,6 +59,7 @@
 #define IRQF_PERCPU		0x00000400
 #define IRQF_NOBALANCING	0x00000800
 #define IRQF_IRQPOLL		0x00001000
+#define IRQF_ALLOCATED		0x00002000
 
 typedef irqreturn_t (*irq_handler_t)(int, void *);
 
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/kernel/irq/manage.c
@@ -623,7 +623,8 @@ void free_irq(unsigned int irq, void *de
 				local_irq_restore(flags);
 			}
 #endif
-			kfree(action);
+			if (action->flags & IRQF_ALLOCATED)
+				kfree(action);
 			return;
 		}
 		printk(KERN_ERR "Trying to free already-free IRQ %d\n", irq);
@@ -714,7 +715,7 @@ int request_irq(unsigned int irq, irq_ha
 		return -ENOMEM;
 
 	action->handler = handler;
-	action->flags = irqflags;
+	action->flags = irqflags | IRQF_ALLOCATED;
 	cpus_clear(action->mask);
 	action->name = devname;
 	action->next = NULL;

Re: [PATCH] Free setup_irq() interrupt V2

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Magnus,
> 
> On Tue, 10 Mar 2009, Magnus Damm wrote:
> > From: Magnus Damm <damm@igel.co.jp>
> > This patch adds a __free_irq() function for releasing 
> > interrupts requested with setup_irq().
> 
> I think there is a simpler solution than adding yet another 
> function for the confusion of driver writers. See below.

Uhm, i asked for that solution ;-) To reduce the confusion in 
generic code with yet another IRQF flag.

	Ingo

Re: [PATCH] Free setup_irq() interrupt V2

[Thomas Gleixner]

On Tue, 10 Mar 2009, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Magnus,
> > 
> > On Tue, 10 Mar 2009, Magnus Damm wrote:
> > > From: Magnus Damm <damm@igel.co.jp>
> > > This patch adds a __free_irq() function for releasing 
> > > interrupts requested with setup_irq().
> > 
> > I think there is a simpler solution than adding yet another 
> > function for the confusion of driver writers. See below.
> 
> Uhm, i asked for that solution ;-) 

Uhm, I missed that :) 

> To reduce the confusion in generic code with yet another IRQF flag.

Hmm. I still prefer a solution which confuses the few people hacking
on kernel/irq/* instead of having another function which confuses the
already confused driver writers.

I can live with the two functions as well, but then please let us use
a function name which is more intuitive than __free_irq().

setup_irq()   -> remove_irq()
request_irq() -> free_irq()

Thanks,

	tglx

Re: [PATCH] Free setup_irq() interrupt V2

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 10 Mar 2009, Ingo Molnar wrote:
> > * Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > Magnus,
> > > 
> > > On Tue, 10 Mar 2009, Magnus Damm wrote:
> > > > From: Magnus Damm <damm@igel.co.jp>
> > > > This patch adds a __free_irq() function for releasing 
> > > > interrupts requested with setup_irq().
> > > 
> > > I think there is a simpler solution than adding yet another 
> > > function for the confusion of driver writers. See below.
> > 
> > Uhm, i asked for that solution ;-) 
> 
> Uhm, I missed that :) 
> 
> > To reduce the confusion in generic code with yet another IRQF flag.
> 
> Hmm. I still prefer a solution which confuses the few people hacking
> on kernel/irq/* instead of having another function which confuses the
> already confused driver writers.
> 
> I can live with the two functions as well, but then please let us use
> a function name which is more intuitive than __free_irq().
> 
> setup_irq()   -> remove_irq()
> request_irq() -> free_irq()

Sure, that's fine with me.

I almost suggested shutdown_irq() to Magnus originally, then 
went for __free_irq().

	Ingo

Re: [PATCH] Free setup_irq() interrupt V2

[Magnus Damm]

On Wed, Mar 11, 2009 at 12:33 AM, Ingo Molnar <mingo@elte.hu> wrote:
>> On Tue, 10 Mar 2009, Ingo Molnar wrote:
>> > * Thomas Gleixner <tglx@linutronix.de> wrote:
>> > > On Tue, 10 Mar 2009, Magnus Damm wrote:
>> > > > From: Magnus Damm <damm@igel.co.jp>
>> > > > This patch adds a __free_irq() function for releasing
>> > > > interrupts requested with setup_irq().
>> > >
>> > > I think there is a simpler solution than adding yet another
>> > > function for the confusion of driver writers. See below.
>> >
>> > Uhm, i asked for that solution ;-)
>>
>> Uhm, I missed that :)

Thomas, your suggestion is quite similar to V1. =)

>> I can live with the two functions as well, but then please let us use
>> a function name which is more intuitive than __free_irq().
>>
>> setup_irq()   -> remove_irq()
>> request_irq() -> free_irq()
>
> Sure, that's fine with me.
>
> I almost suggested shutdown_irq() to Magnus originally, then
> went for __free_irq().

How about this change for V2:

Keep __free_irq() but make it static. Let free_irq() and a new
remove_irq() function call __free_irq(). This makes the free functions
similar to setup_irq(), __setup_irq() and request_irq(). The new
remove_irq() function takes interrupt number and a struct irqaction
pointer as arguments - this to match setup_irq().

Sounds ok? If so I'll fix up and repost a V3 against the tip tree.

While at it, what about exporting the setup_irq() and remove_irq() symbols?

You probably dislike it since it may confuse people. I'd like to
export these functions since I have timer code that needs to use
setup_irq() early on (too early for request_irq()), and the same code
can also be compiled as a module. The solution so far has been using
setup_irq()/remove_irq(), but you may have some better suggestions?

Thanks for your help!

Cheers,

/ magnus