Re: Threaded interrupt handlers broken?

Michael Buesch
2009-08-16T10:14:38Z

On Sunday 16 August 2009 11:53:13 Michael Buesch wrote:
> Hi,
>
> I was trying to use threaded interrupt handlers, but the code always crashes
> within irq_thread() with a "BUG: spinlock bad magic 00000000".
> The spinlock that's not properly initialized is from the wait_for_threads waitqueue.
> It crashes on line 526 (see below).
> The initialization of the waitqueue struct seems to depend on whether the IRQ is
> shared or not. I don't know if that's correct, but I patched it to unconditionally
> initialize the struct. That did not help.
>
> Any ideas?
>
>
> 490 static int irq_thread(void *data)
> 491 {
> 492 struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2, };
> 493 struct irqaction *action = data;
> 494 struct irq_desc *desc = irq_to_desc(action->irq);
> 495 int wake;
> 496
> 497 sched_setscheduler(current, SCHED_FIFO, &param);
> 498 current->irqaction = action;
> 499
> 500 while (!irq_wait_for_interrupt(action)) {
> 501
> 502 irq_thread_check_affinity(desc, action);
> 503
> 504 atomic_inc(&desc->threads_active);
> 505
> 506 spin_lock_irq(&desc->lock);
> 507 if (unlikely(desc->status & IRQ_DISABLED)) {
> 508 /*
> 509 * CHECKME: We might need a dedicated
> 510 * IRQ_THREAD_PENDING flag here, which
> 511 * retriggers the thread in check_irq_resend()
> 512 * but AFAICT IRQ_PENDING should be fine as it
> 513 * retriggers the interrupt itself --- tglx
> 514 */
> 515 desc->status |= IRQ_PENDING;
> 516 spin_unlock_irq(&desc->lock);
> 517 } else {
> 518 spin_unlock_irq(&desc->lock);
> 519
> 520 action->thread_fn(action->irq, action->dev_id);
> 521 }
> 522
> 523 wake = atomic_dec_and_test(&desc->threads_active);

Is this test logic inverted? atomic_dec_and_test() means
(threads_active - 1) == 0
Shouldn't it be like this?
(threads_active - 1) != 0

> 524
> 525 if (wake && waitqueue_active(&desc->wait_for_threads))
> 526 wake_up(&desc->wait_for_threads); <<<<<<<<<<<<<<<<<<<<<<<<
> 527 }
> 528
> 529 /*
> 530 * Clear irqaction. Otherwise exit_irq_thread() would make
> 531 * fuzz about an active irq thread going into nirvana.
> 532 */
> 533 current->irqaction = NULL;
> 534 return 0;
> 535 }