Re: [PATCH] synchronize_irq needs a barrier
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2007-10-18 23:41:57
Also in:
lkml
Subsystem:
irq subsystem, the rest · Maintainers:
Thomas Gleixner, Linus Torvalds
On Fri, 19 Oct 2007, Benjamin Herrenschmidt wrote:
I agree and you can see that in fact, we don't have enough barrier on the other side since spin_unlock doesn't prevent subsequent loads from crossing a previous store... I wonder if that's worth trying to address, adding a barrier in handle_IRQ_event for example, or we can continue ignoring the barrier and let some drivers do their own fixes in fancy ways.
So how about something like this (untested! not necessarily very well thought through either!) Basic notion: the only thing that serializes the IRQ_INPROGRESS flag is the descriptor lock. And we don't have to (or even want to!) hold it while waiting for the thing, but we want to *have*held*it* in between whatever we're synchronizing with. The internal irq handler functions already held the lock when they did whatever they need to serialize - and they are possibly performance critical too - so they use the "internal" function that doesn't get the lock unnecessarily again. Hmm? Linus --- kernel/irq/manage.c | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 80eab7a..f3e9575 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c@@ -14,6 +14,18 @@ #include "internals.h" +/* + * Internally wait for IRQ_INPROGRESS to go away on other CPU's, + * after having serialized with the descriptor lock. + */ +static inline void do_synchronize_irq(struct irq_desc *desc) +{ +#ifdef CONFIG_SMP + while (desc->status & IRQ_INPROGRESS) + cpu_relax(); +#endif +} + #ifdef CONFIG_SMP /**
@@ -28,13 +40,15 @@ */ void synchronize_irq(unsigned int irq) { + unsigned long flags; struct irq_desc *desc = irq_desc + irq; if (irq >= NR_IRQS) return; - while (desc->status & IRQ_INPROGRESS) - cpu_relax(); + spin_lock_irqsave(&desc->lock, flags); + spin_unlock_irqrestore(&desc->lock, flags); + do_synchronize_irq(desc); } EXPORT_SYMBOL(synchronize_irq);
@@ -129,7 +143,7 @@ void disable_irq(unsigned int irq) disable_irq_nosync(irq); if (desc->action) - synchronize_irq(irq); + do_synchronize_irq(desc); } EXPORT_SYMBOL(disable_irq);
@@ -443,7 +457,7 @@ void free_irq(unsigned int irq, void *dev_id) unregister_handler_proc(irq, action); /* Make sure it's not being used on another CPU */ - synchronize_irq(irq); + do_synchronize_irq(desc); #ifdef CONFIG_DEBUG_SHIRQ /* * It's a shared IRQ -- the driver ought to be