From: Magnus Damm <[email protected]>
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 <[email protected]>
---
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);
/**
* Magnus Damm <[email protected]> wrote:
> From: Magnus Damm <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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
Magnus,
On Tue, 10 Mar 2009, Magnus Damm wrote:
> From: Magnus Damm <[email protected]>
> 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 <[email protected]>
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 <[email protected]>
---
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;
* Thomas Gleixner <[email protected]> wrote:
> Magnus,
>
> On Tue, 10 Mar 2009, Magnus Damm wrote:
> > From: Magnus Damm <[email protected]>
> > 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
On Tue, 10 Mar 2009, Ingo Molnar wrote:
> * Thomas Gleixner <[email protected]> wrote:
>
> > Magnus,
> >
> > On Tue, 10 Mar 2009, Magnus Damm wrote:
> > > From: Magnus Damm <[email protected]>
> > > 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
* Thomas Gleixner <[email protected]> wrote:
> On Tue, 10 Mar 2009, Ingo Molnar wrote:
> > * Thomas Gleixner <[email protected]> wrote:
> >
> > > Magnus,
> > >
> > > On Tue, 10 Mar 2009, Magnus Damm wrote:
> > > > From: Magnus Damm <[email protected]>
> > > > 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
On Wed, Mar 11, 2009 at 12:33 AM, Ingo Molnar <[email protected]> wrote:
>> On Tue, 10 Mar 2009, Ingo Molnar wrote:
>> > * Thomas Gleixner <[email protected]> wrote:
>> > > On Tue, 10 Mar 2009, Magnus Damm wrote:
>> > > > From: Magnus Damm <[email protected]>
>> > > > 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