[GIT pull] genirq fixes for 2.6.31

[GIT pull] genirq fixes for 2.6.31

[Thomas Gleixner]

Linus,

Please pull the latest irq-fixes-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git irq-fixes-for-linus

The affinity problem went unnoticed in reviews and testing and just
exploded nicely in the preempt-rt tree where we force thread all
interrupts.

Thanks,

	tglx

------------------>
Thomas Gleixner (1):
      genirq: Delegate irq affinity setting to the irq thread


 include/linux/interrupt.h |    2 +
 kernel/irq/internals.h    |    3 +-
 kernel/irq/manage.c       |   50 +++++++++++++++++++++++++++++++++++++++-----
 kernel/irq/migration.c    |    2 +-
 4 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 2721f07..88b056a 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -64,11 +64,13 @@
  * IRQTF_RUNTHREAD - signals that the interrupt handler thread should run
  * IRQTF_DIED      - handler thread died
  * IRQTF_WARNED    - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed
+ * IRQTF_AFFINITY  - irq thread is requested to adjust affinity
  */
 enum {
 	IRQTF_RUNTHREAD,
 	IRQTF_DIED,
 	IRQTF_WARNED,
+	IRQTF_AFFINITY,
 };
 
 typedef irqreturn_t (*irq_handler_t)(int, void *);
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 7346825..e70ed55 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -42,8 +42,7 @@ static inline void unregister_handler_proc(unsigned int irq,
 
 extern int irq_select_affinity_usr(unsigned int irq);
 
-extern void
-irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask);
+extern void irq_set_thread_affinity(struct irq_desc *desc);
 
 /*
  * Debugging printout:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 50da676..f0de36f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -80,14 +80,22 @@ int irq_can_set_affinity(unsigned int irq)
 	return 1;
 }
 
-void
-irq_set_thread_affinity(struct irq_desc *desc, const struct cpumask *cpumask)
+/**
+ *	irq_set_thread_affinity - Notify irq threads to adjust affinity
+ *	@desc:		irq descriptor which has affitnity changed
+ *
+ *	We just set IRQTF_AFFINITY and delegate the affinity setting
+ *	to the interrupt thread itself. We can not call
+ *	set_cpus_allowed_ptr() here as we hold desc->lock and this
+ *	code can be called from hard interrupt context.
+ */
+void irq_set_thread_affinity(struct irq_desc *desc)
 {
 	struct irqaction *action = desc->action;
 
 	while (action) {
 		if (action->thread)
-			set_cpus_allowed_ptr(action->thread, cpumask);
+			set_bit(IRQTF_AFFINITY, &action->thread_flags);
 		action = action->next;
 	}
 }
@@ -112,7 +120,7 @@ int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
 	if (desc->status & IRQ_MOVE_PCNTXT) {
 		if (!desc->chip->set_affinity(irq, cpumask)) {
 			cpumask_copy(desc->affinity, cpumask);
-			irq_set_thread_affinity(desc, cpumask);
+			irq_set_thread_affinity(desc);
 		}
 	}
 	else {
@@ -122,7 +130,7 @@ int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
 #else
 	if (!desc->chip->set_affinity(irq, cpumask)) {
 		cpumask_copy(desc->affinity, cpumask);
-		irq_set_thread_affinity(desc, cpumask);
+		irq_set_thread_affinity(desc);
 	}
 #endif
 	desc->status |= IRQ_AFFINITY_SET;
@@ -176,7 +184,7 @@ int irq_select_affinity_usr(unsigned int irq)
 	spin_lock_irqsave(&desc->lock, flags);
 	ret = setup_affinity(irq, desc);
 	if (!ret)
-		irq_set_thread_affinity(desc, desc->affinity);
+		irq_set_thread_affinity(desc);
 	spin_unlock_irqrestore(&desc->lock, flags);
 
 	return ret;
@@ -444,6 +452,34 @@ static int irq_wait_for_interrupt(struct irqaction *action)
 }
 
 /*
+ * Check whether we need to change the affinity of the interrupt thread.
+ */
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+	cpumask_var_t mask;
+
+	if (!test_and_clear_bit(IRQTF_AFFINITY, &action->thread_flags))
+		return;
+
+	/*
+	 * In case we are out of memory we set IRQTF_AFFINITY again and
+	 * try again next time
+	 */
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
+		set_bit(IRQTF_AFFINITY, &action->thread_flags);
+		return;
+	}
+
+	spin_lock_irq(&desc->lock);
+	cpumask_copy(mask, desc->affinity);
+	spin_unlock_irq(&desc->lock);
+
+	set_cpus_allowed_ptr(current, mask);
+	free_cpumask_var(mask);
+}
+
+/*
  * Interrupt handler thread
  */
 static int irq_thread(void *data)
@@ -458,6 +494,8 @@ static int irq_thread(void *data)
 
 	while (!irq_wait_for_interrupt(action)) {
 
+		irq_thread_check_affinity(desc, action);
+
 		atomic_inc(&desc->threads_active);
 
 		spin_lock_irq(&desc->lock);
diff --git a/kernel/irq/migration.c b/kernel/irq/migration.c
index cfe767c..fcb6c96 100644
--- a/kernel/irq/migration.c
+++ b/kernel/irq/migration.c
@@ -45,7 +45,7 @@ void move_masked_irq(int irq)
 		   < nr_cpu_ids))
 		if (!desc->chip->set_affinity(irq, desc->pending_mask)) {
 			cpumask_copy(desc->affinity, desc->pending_mask);
-			irq_set_thread_affinity(desc, desc->pending_mask);
+			irq_set_thread_affinity(desc);
 		}
 
 	cpumask_clear(desc->pending_mask);

Re: [GIT pull] genirq fixes for 2.6.31

[Kevin Winchester]

Thomas Gleixner wrote:
> +/*
>   * Interrupt handler thread
>   */
>  static int irq_thread(void *data)
> @@ -458,6 +494,8 @@ static int irq_thread(void *data)
>  
>  	while (!irq_wait_for_interrupt(action)) {
>  
> +		irq_thread_check_affinity(desc, action);
> +
>  		atomic_inc(&desc->threads_active);
>  
>  		spin_lock_irq(&desc->lock);

Any chance we could do this in a way that doesn't break the build for CONFIG_SMP=n? :)

Should this call simply be wrapped in an #ifdef, or should the function be defined for !SMP?

-- 
Kevin Winchester

Re: [GIT pull] genirq fixes for 2.6.31

[Kevin Winchester]

Kevin Winchester wrote:
> Thomas Gleixner wrote:
>> +/*
>>   * Interrupt handler thread
>>   */
>>  static int irq_thread(void *data)
>> @@ -458,6 +494,8 @@ static int irq_thread(void *data)
>>  
>>  	while (!irq_wait_for_interrupt(action)) {
>>  
>> +		irq_thread_check_affinity(desc, action);
>> +
>>  		atomic_inc(&desc->threads_active);
>>  
>>  		spin_lock_irq(&desc->lock);
> 
> Any chance we could do this in a way that doesn't break the build for CONFIG_SMP=n? :)
> 
> Should this call simply be wrapped in an #ifdef, or should the function be defined for !SMP?
> 

Actually, I guess the function is defined always, but it references desc->affinity unconditionally.

Perhaps something like the following (likely whitespace damaged) patch:

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f0de36f..353e335 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -451,6 +451,8 @@ static int irq_wait_for_interrupt(struct irqaction *action)
        return -1;
 }
 
+#ifdef CONFIG_SMP
+
 /*
  * Check whether we need to change the affinity of the interrupt thread.
  */
@@ -479,6 +481,15 @@ irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
        free_cpumask_var(mask);
 }
 
+#else
+
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+}
+
+#endif
+
 /*
  * Interrupt handler thread
  */

[PATCH] irq: Add compiling definition of irq_thread_check_affinity for !CONFIG_SMP

[Kevin Winchester]

The addition and use of irq_thread_check_affinity neglected the fact
that struct irq_desc does not have an affinity member when CONFIG_SMP=n.

Fix this by adding an empty definition of the method for !SMP.

Signed-off-by: Kevin Winchester <kjwinchester@gmail.com>
---

I'll try not to be lazy and send a real patch.  Please let me know if
I've done it wrong.

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f0de36f..82f01c8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -190,11 +190,46 @@ int irq_select_affinity_usr(unsigned int irq)
 	return ret;
 }
 
+/*
+ * Check whether we need to change the affinity of the interrupt thread.
+ */
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+	cpumask_var_t mask;
+
+	if (!test_and_clear_bit(IRQTF_AFFINITY, &action->thread_flags))
+		return;
+
+	/*
+	 * In case we are out of memory we set IRQTF_AFFINITY again and
+	 * try again next time
+	 */
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
+		set_bit(IRQTF_AFFINITY, &action->thread_flags);
+		return;
+	}
+
+	spin_lock_irq(&desc->lock);
+	cpumask_copy(mask, desc->affinity);
+	spin_unlock_irq(&desc->lock);
+
+	set_cpus_allowed_ptr(current, mask);
+	free_cpumask_var(mask);
+}
+
 #else
+
 static inline int setup_affinity(unsigned int irq, struct irq_desc *desc)
 {
 	return 0;
 }
+
+static void
+irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
+{
+}
+
 #endif
 
 void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
@@ -452,34 +487,6 @@ static int irq_wait_for_interrupt(struct irqaction *action)
 }
 
 /*
- * Check whether we need to change the affinity of the interrupt thread.
- */
-static void
-irq_thread_check_affinity(struct irq_desc *desc, struct irqaction *action)
-{
-	cpumask_var_t mask;
-
-	if (!test_and_clear_bit(IRQTF_AFFINITY, &action->thread_flags))
-		return;
-
-	/*
-	 * In case we are out of memory we set IRQTF_AFFINITY again and
-	 * try again next time
-	 */
-	if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {
-		set_bit(IRQTF_AFFINITY, &action->thread_flags);
-		return;
-	}
-
-	spin_lock_irq(&desc->lock);
-	cpumask_copy(mask, desc->affinity);
-	spin_unlock_irq(&desc->lock);
-
-	set_cpus_allowed_ptr(current, mask);
-	free_cpumask_var(mask);
-}
-
-/*
  * Interrupt handler thread
  */
 static int irq_thread(void *data)

[GIT pull] genirq fixes for 2.6.31

[Thomas Gleixner]

Linus,

Please pull the latest irq-fixes-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git irq-fixes-for-linus

Thanks,

	tglx

------------------>
Thomas Gleixner (1):
      genirq: Prevent race between free_irq() and handle_IRQ_event()


 kernel/irq/handle.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 065205b..4e7f17a 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -403,8 +403,16 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
 			 */
 			if (likely(!test_bit(IRQTF_DIED,
 					     &action->thread_flags))) {
+				struct task_struct *tsk = action->thread;
+
 				set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
-				wake_up_process(action->thread);
+				/*
+				 * Check tsk as we might race against
+				 * free_irq which sets action->thread
+				 * to NULL
+				 */
+				if (tsk)
+					wake_up_process(tsk);
 			}
 
 			/* Fall through to add to randomness */

Re: [GIT pull] genirq fixes for 2.6.31

[Linus Torvalds]

On Thu, 13 Aug 2009, Thomas Gleixner wrote:
>
> Please pull the latest irq-fixes-for-linus git tree from:
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git irq-fixes-for-linus

Not without lots more explanations - or at least _fixed_ explanations.

> Thomas Gleixner (1):
>       genirq: Prevent race between free_irq() and handle_IRQ_event()
> 
> 
>  kernel/irq/handle.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 065205b..4e7f17a 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -403,8 +403,16 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
>  			 */
>  			if (likely(!test_bit(IRQTF_DIED,
>  					     &action->thread_flags))) {
> +				struct task_struct *tsk = action->thread;
> +
>  				set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> -				wake_up_process(action->thread);
> +				/*
> +				 * Check tsk as we might race against
> +				 * free_irq which sets action->thread
> +				 * to NULL
> +				 */
> +				if (tsk)
> +					wake_up_process(tsk);

Seriously, this looks entirely bogus.

The thing is, if "action" has been free'd, then dammit, 'tsk' is gone too. 
In fact, just look at _free_irq(), and realise how it does the

        if (irqthread) {
                if (!test_bit(IRQTF_DIED, &action->thread_flags))
                        kthread_stop(irqthread);
                put_task_struct(irqthread);
        }

_before_ it free's 'action'. So what does that fix?

There is no race that I can see - we're holding "action->lock" while all 
this happens.

Now, I can see a bug, which is that "action->tsk" may have been set to 
NULL. But I can't see a race, and I can't see a reason for all the code 
movement. So quite frankly, I think the comments (both in the code and in 
the commit message) are just wrong. And the odd "load it first, then do 
other things" code looks confused.

So why is this not just a

	if (action->thread)
		wake_up_process(action->thread);

with appropriate comments?

Or, alternatively, just move all the "clear action->thread" in free_irq() 
to after having done the "synchronize_irq()" thing, and then - afaik - 
you'll not need that test at all, because you're guaranteed that as long 
as you're in an interrupt handler, the thing shouldn't be cleared.

No?

		Linus

Re: [GIT pull] genirq fixes for 2.6.31

[Thomas Gleixner]

On Thu, 13 Aug 2009, Linus Torvalds wrote:
> Now, I can see a bug, which is that "action->tsk" may have been set to 
> NULL. But I can't see a race, and I can't see a reason for all the code 
> movement. So quite frankly, I think the comments (both in the code and in 
> the commit message) are just wrong. And the odd "load it first, then do 
> other things" code looks confused.
> 
> So why is this not just a
> 
> 	if (action->thread)
> 		wake_up_process(action->thread);
> 
> with appropriate comments?

What guarantees that the compiler does not dereference action->thread
twice and the action->thread = NULL; operation happens between the
check and the wake_up_process() call? I might be paranoid, but ...

> Or, alternatively, just move all the "clear action->thread" in free_irq() 
> to after having done the "synchronize_irq()" thing, and then - afaik - 
> you'll not need that test at all, because you're guaranteed that as long 
> as you're in an interrupt handler, the thing shouldn't be cleared.

Right, I looked at that as well, but we need to do it different than
just calling synchronize_irq(), as we need to keep desc->lock after we
established that no interrupt is in progress. Otherwise we can run
into the same problem which we have right now. Patch below.

Thanks,

	tglx
---
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 76e109a..06fa022 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -895,7 +907,28 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	if (!desc)
 		return NULL;
 
-	atomic_spin_lock_irqsave(&desc->lock, flags);
+	while (1) {
+		/*
+		 * Wait until we're out of the critical section.  This might
+		 * give the wrong answer due to the lack of memory barriers.
+		 */
+		while (desc->status & IRQ_INPROGRESS)
+			cpu_relax();
+
+		/*
+		 * Check under the lock again. If irq is not in
+		 * progress we keep the lock held until we removed
+		 * action. We do not care about an already running irq
+		 * thread here. We care about it when we stop the thread.
+		 */
+		atomic_spin_lock_irqsave(&desc->lock, flags);
+
+		if (!(desc->status & IRQ_INPROGRESS))
+			break;
+
+		/* Try again */
+		atomic_spin_unlock_irqrestore(&desc->lock, flags);
+	}
 
 	/*
 	 * There can be multiple actions per IRQ descriptor, find the right

Re: [GIT pull] genirq fixes for 2.6.31

[Thomas Gleixner]

Sorry, sent out the wrong patch :(

On Thu, 13 Aug 2009, Thomas Gleixner wrote:

> On Thu, 13 Aug 2009, Linus Torvalds wrote:
> > Now, I can see a bug, which is that "action->tsk" may have been set to 
> > NULL. But I can't see a race, and I can't see a reason for all the code 
> > movement. So quite frankly, I think the comments (both in the code and in 
> > the commit message) are just wrong. And the odd "load it first, then do 
> > other things" code looks confused.
> > 
> > So why is this not just a
> > 
> > 	if (action->thread)
> > 		wake_up_process(action->thread);
> > 
> > with appropriate comments?
> 
> What guarantees that the compiler does not dereference action->thread
> twice and the action->thread = NULL; operation happens between the
> check and the wake_up_process() call? I might be paranoid, but ...
> 
> > Or, alternatively, just move all the "clear action->thread" in free_irq() 
> > to after having done the "synchronize_irq()" thing, and then - afaik - 
> > you'll not need that test at all, because you're guaranteed that as long 
> > as you're in an interrupt handler, the thing shouldn't be cleared.
> 
> Right, I looked at that as well, but we need to do it different than
> just calling synchronize_irq(), as we need to keep desc->lock after we
> established that no interrupt is in progress. Otherwise we can run
> into the same problem which we have right now. Patch below.
> 
> Thanks,
> 
> 	tglx

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -895,7 +907,28 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	if (!desc)
 		return NULL;
 
-	spin_lock_irqsave(&desc->lock, flags);
+	while (1) {
+		/*
+		 * Wait until we're out of the critical section.  This might
+		 * give the wrong answer due to the lack of memory barriers.
+		 */
+		while (desc->status & IRQ_INPROGRESS)
+			cpu_relax();
+
+		/*
+		 * Check under the lock again. If irq is not in
+		 * progress we keep the lock held until we removed
+		 * action. We do not care about an already running irq
+		 * thread here. We care about it when we stop the thread.
+		 */
+		spin_lock_irqsave(&desc->lock, flags);
+
+		if (!(desc->status & IRQ_INPROGRESS))
+			break;
+
+		/* Try again */
+		spin_unlock_irqrestore(&desc->lock, flags);
+	}
 
 	/*
 	 * There can be multiple actions per IRQ descriptor, find the right

Re: [GIT pull] genirq fixes for 2.6.31

[Linus Torvalds]

On Thu, 13 Aug 2009, Thomas Gleixner wrote:

> On Thu, 13 Aug 2009, Linus Torvalds wrote:
> > Now, I can see a bug, which is that "action->tsk" may have been set to 
> > NULL. But I can't see a race, and I can't see a reason for all the code 
> > movement. So quite frankly, I think the comments (both in the code and in 
> > the commit message) are just wrong. And the odd "load it first, then do 
> > other things" code looks confused.
> > 
> > So why is this not just a
> > 
> > 	if (action->thread)
> > 		wake_up_process(action->thread);
> > 
> > with appropriate comments?
> 
> What guarantees that the compiler does not dereference action->thread
> twice and the action->thread = NULL; operation happens between the
> check and the wake_up_process() call? I might be paranoid, but ...

Aren't we holding the lock here?

And if we are _not_ holding the lock, then it's racy anyway, and the right 
fix is the other one I suggested:

> > Or, alternatively, just move all the "clear action->thread" in free_irq() 
> > to after having done the "synchronize_irq()" thing, and then - afaik - 
> > you'll not need that test at all, because you're guaranteed that as long 
> > as you're in an interrupt handler, the thing shouldn't be cleared.
> 
> Right, I looked at that as well, but we need to do it different than
> just calling synchronize_irq(), as we need to keep desc->lock after we
> established that no interrupt is in progress. Otherwise we can run
> into the same problem which we have right now. Patch below.

But we already _do_ call synchronize_irq().

And no, we'd better not be running into the same problem, becaue dang it, 
if we do, then 'action' itself is unreliable (since we'll be doing a 
'kfree()' in it in free_irq())

IOW, why not just make the patch do something like the appended? 

NOTE! This is UNTESTED. And I also - on purpose - removed the "set 
action->thread to NULL", because we're going to free 'action', so if 
anything depends on it, it's already buggy.

What am I missing?

		Linus

---
 kernel/irq/manage.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 61c679d..0747f22 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -809,9 +809,6 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 			desc->chip->disable(irq);
 	}
 
-	irqthread = action->thread;
-	action->thread = NULL;
-
 	spin_unlock_irqrestore(&desc->lock, flags);
 
 	unregister_handler_proc(irq, action);
@@ -819,12 +816,6 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	/* Make sure it's not being used on another CPU: */
 	synchronize_irq(irq);
 
-	if (irqthread) {
-		if (!test_bit(IRQTF_DIED, &action->thread_flags))
-			kthread_stop(irqthread);
-		put_task_struct(irqthread);
-	}
-
 #ifdef CONFIG_DEBUG_SHIRQ
 	/*
 	 * It's a shared IRQ -- the driver ought to be prepared for an IRQ
@@ -840,6 +831,14 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		local_irq_restore(flags);
 	}
 #endif
+
+	irqthread = action->thread;
+	if (irqthread) {
+		if (!test_bit(IRQTF_DIED, &action->thread_flags))
+			kthread_stop(irqthread);
+		put_task_struct(irqthread);
+	}
+
 	return action;
 }
 

Re: [GIT pull] genirq fixes for 2.6.31

[Thomas Gleixner]

On Thu, 13 Aug 2009, Linus Torvalds wrote:
> On Thu, 13 Aug 2009, Thomas Gleixner wrote:
> > What guarantees that the compiler does not dereference action->thread
> > twice and the action->thread = NULL; operation happens between the
> > check and the wake_up_process() call? I might be paranoid, but ...
> 
> Aren't we holding the lock here?

No, we don't. The lock is dropped before handle_IRQ_event() is called.
 
> And if we are _not_ holding the lock, then it's racy anyway, and the right 
> fix is the other one I suggested:
> 
> > > Or, alternatively, just move all the "clear action->thread" in free_irq() 
> > > to after having done the "synchronize_irq()" thing, and then - afaik - 
> > > you'll not need that test at all, because you're guaranteed that as long 
> > > as you're in an interrupt handler, the thing shouldn't be cleared.
> > 
> > Right, I looked at that as well, but we need to do it different than
> > just calling synchronize_irq(), as we need to keep desc->lock after we
> > established that no interrupt is in progress. Otherwise we can run
> > into the same problem which we have right now. Patch below.
> 
> But we already _do_ call synchronize_irq().
> 
> And no, we'd better not be running into the same problem, becaue dang it, 
> if we do, then 'action' itself is unreliable (since we'll be doing a 
> 'kfree()' in it in free_irq())

action->thread is the thing which became unreliable due to setting it
to NULL. Yes, I did not think about the fact that we can remove the
action while the interrupt is in progress on another CPU. So setting
action->thread to NULL _before_ calling synchronize_irq() is the cause
for the oops which has been reported.
 
> IOW, why not just make the patch do something like the appended? 
> 
> NOTE! This is UNTESTED. And I also - on purpose - removed the "set 
> action->thread to NULL", because we're going to free 'action', so if 
> anything depends on it, it's already buggy.

Works fine with my test case.

> What am I missing?

Nothing, as far as I can tell.

Acked-by-me.

	tglx

> 
> ---
>  kernel/irq/manage.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 61c679d..0747f22 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -809,9 +809,6 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>  			desc->chip->disable(irq);
>  	}
>  
> -	irqthread = action->thread;
> -	action->thread = NULL;
> -
>  	spin_unlock_irqrestore(&desc->lock, flags);
>  
>  	unregister_handler_proc(irq, action);
> @@ -819,12 +816,6 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>  	/* Make sure it's not being used on another CPU: */
>  	synchronize_irq(irq);
>  
> -	if (irqthread) {
> -		if (!test_bit(IRQTF_DIED, &action->thread_flags))
> -			kthread_stop(irqthread);
> -		put_task_struct(irqthread);
> -	}
> -
>  #ifdef CONFIG_DEBUG_SHIRQ
>  	/*
>  	 * It's a shared IRQ -- the driver ought to be prepared for an IRQ
> @@ -840,6 +831,14 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>  		local_irq_restore(flags);
>  	}
>  #endif
> +
> +	irqthread = action->thread;
> +	if (irqthread) {
> +		if (!test_bit(IRQTF_DIED, &action->thread_flags))
> +			kthread_stop(irqthread);
> +		put_task_struct(irqthread);
> +	}
> +
>  	return action;
>  }
>  
>