Threaded interrupt handlers broken?

Threaded interrupt handlers broken?

[Michael Buesch]

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);
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 }

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Michael Buesch]

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 }
> 



-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Sunday 16 August 2009 12:14:36 Michael Buesch wrote:
> > 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

I need the following patch for threaded IRQs to work.
The first hunk obviously is incorrect. But without it the thread_fn is
never called.

Index: wireless-testing/kernel/irq/manage.c
===================================================================
--- wireless-testing.orig/kernel/irq/manage.c	2009-08-15 22:22:07.000000000 +0200
+++ wireless-testing/kernel/irq/manage.c	2009-08-16 14:05:23.000000000 +0200
@@ -504,7 +504,7 @@ static int irq_thread(void *data)
 		atomic_inc(&desc->threads_active);
 
 		spin_lock_irq(&desc->lock);
-		if (unlikely(desc->status & IRQ_DISABLED)) {
+		if (0&&unlikely(desc->status & IRQ_DISABLED)) {
 			/*
 			 * CHECKME: We might need a dedicated
 			 * IRQ_THREAD_PENDING flag here, which
@@ -520,7 +520,7 @@ static int irq_thread(void *data)
 			action->thread_fn(action->irq, action->dev_id);
 		}
 
-		wake = atomic_dec_and_test(&desc->threads_active);
+		wake = !atomic_dec_and_test(&desc->threads_active);
 
 		if (wake && waitqueue_active(&desc->wait_for_threads))
 			wake_up(&desc->wait_for_threads);



-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Sun, 16 Aug 2009, Michael Buesch wrote:
> On Sunday 16 August 2009 12:14:36 Michael Buesch wrote:
> I need the following patch for threaded IRQs to work.
> The first hunk obviously is incorrect. But without it the thread_fn is
> never called.
> 
> Index: wireless-testing/kernel/irq/manage.c
> ===================================================================
> --- wireless-testing.orig/kernel/irq/manage.c	2009-08-15 22:22:07.000000000 +0200
> +++ wireless-testing/kernel/irq/manage.c	2009-08-16 14:05:23.000000000 +0200
> @@ -504,7 +504,7 @@ static int irq_thread(void *data)
>  		atomic_inc(&desc->threads_active);
>  
>  		spin_lock_irq(&desc->lock);
> -		if (unlikely(desc->status & IRQ_DISABLED)) {
> +		if (0&&unlikely(desc->status & IRQ_DISABLED)) {

So the interrupt is marked disabled. How do you setup the handler ?
And what does the primary handler do ? Can you post your driver code please?
>  			/*
>  			 * CHECKME: We might need a dedicated
>  			 * IRQ_THREAD_PENDING flag here, which
> @@ -520,7 +520,7 @@ static int irq_thread(void *data)
>  			action->thread_fn(action->irq, action->dev_id);
>  		}
>  
> -		wake = atomic_dec_and_test(&desc->threads_active);
> +		wake = !atomic_dec_and_test(&desc->threads_active);

So you wake when the thread counter is != 0 after the decrement.

 #define atomic_dec_and_test(v) (atomic_sub_return(1, (v)) == 0)

Thanks,

	tglx

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Sunday 16 August 2009 15:22:29 Thomas Gleixner wrote:
> On Sun, 16 Aug 2009, Michael Buesch wrote:
> > On Sunday 16 August 2009 12:14:36 Michael Buesch wrote:
> > I need the following patch for threaded IRQs to work.
> > The first hunk obviously is incorrect. But without it the thread_fn is
> > never called.
> > 
> > Index: wireless-testing/kernel/irq/manage.c
> > ===================================================================
> > --- wireless-testing.orig/kernel/irq/manage.c	2009-08-15 22:22:07.000000000 +0200
> > +++ wireless-testing/kernel/irq/manage.c	2009-08-16 14:05:23.000000000 +0200
> > @@ -504,7 +504,7 @@ static int irq_thread(void *data)
> >  		atomic_inc(&desc->threads_active);
> >  
> >  		spin_lock_irq(&desc->lock);
> > -		if (unlikely(desc->status & IRQ_DISABLED)) {
> > +		if (0&&unlikely(desc->status & IRQ_DISABLED)) {
> 
> So the interrupt is marked disabled. How do you setup the handler ?
> And what does the primary handler do ? Can you post your driver code please?

This patch converts the b43 driver to threaded interrupts:
http://bu3sch.de/patches/wireless-testing/20090816-1535/patches/002-b43-threaded-irq-handler.patch

It kind of works with this hack applied to kernel/irq/manage.c

> >  			/*
> >  			 * CHECKME: We might need a dedicated
> >  			 * IRQ_THREAD_PENDING flag here, which
> > @@ -520,7 +520,7 @@ static int irq_thread(void *data)
> >  			action->thread_fn(action->irq, action->dev_id);
> >  		}
> >  
> > -		wake = atomic_dec_and_test(&desc->threads_active);
> > +		wake = !atomic_dec_and_test(&desc->threads_active);
> 
> So you wake when the thread counter is != 0 after the decrement.
> 
>  #define atomic_dec_and_test(v) (atomic_sub_return(1, (v)) == 0)

Yeah, isn't that what we want to do? I read the test as "wake other threads,
if there are other threads" or something like that.

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Sun, 16 Aug 2009, Michael Buesch wrote:
> On Sunday 16 August 2009 15:22:29 Thomas Gleixner wrote:
>
> > > +		if (0&&unlikely(desc->status & IRQ_DISABLED)) {
> > 
> > So the interrupt is marked disabled. How do you setup the handler
> > ?  And what does the primary handler do ? Can you post your driver
> > code please?
> 
> This patch converts the b43 driver to threaded interrupts:
> http://bu3sch.de/patches/wireless-testing/20090816-1535/patches/002-b43-threaded-irq-handler.patch

On the first glance this looks not too bad. the unlocked access to the
irq status registers looks a bit scary, but that is not relevant for
the problem at hand.
 
> It kind of works with this hack applied to kernel/irq/manage.c

Hmm. Is the interrupt of the device shared ? If yes, what's the other
device on that interrupt line ? what puzzles me is the fact that the
IRQ_DISABLED flag is set. Is there anything unusual in dmesg ?

> > So you wake when the thread counter is != 0 after the decrement.
> > 
> >  #define atomic_dec_and_test(v) (atomic_sub_return(1, (v)) == 0)
> 
> Yeah, isn't that what we want to do? I read the test as "wake other threads,
> if there are other threads" or something like that.

No, it's for synchronize_irq(). When there are threaded handlers in
progress, then sychronize_irq() waits on the waitqueue until they are
finished. So we wake the queue when the last threaded handler of this
irq line returns from thread_fn.

Thanks,

	tglx

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Sunday 16 August 2009 16:25:13 Thomas Gleixner wrote:
> On Sun, 16 Aug 2009, Michael Buesch wrote:
> > On Sunday 16 August 2009 15:22:29 Thomas Gleixner wrote:
> >
> > > > +		if (0&&unlikely(desc->status & IRQ_DISABLED)) {
> > > 
> > > So the interrupt is marked disabled. How do you setup the handler
> > > ?  And what does the primary handler do ? Can you post your driver
> > > code please?
> > 
> > This patch converts the b43 driver to threaded interrupts:
> > http://bu3sch.de/patches/wireless-testing/20090816-1535/patches/002-b43-threaded-irq-handler.patch
> 
> On the first glance this looks not too bad. the unlocked access to the
> irq status registers looks a bit scary, but that is not relevant for
> the problem at hand.

Yeah it does ;)

> > It kind of works with this hack applied to kernel/irq/manage.c
> 
> Hmm. Is the interrupt of the device shared ?

It's registered as shared, but on my machine it is not shared with anything else.

> If yes, what's the other 
> device on that interrupt line ? what puzzles me is the fact that the
> IRQ_DISABLED flag is set. Is there anything unusual in dmesg ?

Here's my current kernel log with the two patches applied:
http://bu3sch.de/misc/dmesg

> > > So you wake when the thread counter is != 0 after the decrement.
> > > 
> > >  #define atomic_dec_and_test(v) (atomic_sub_return(1, (v)) == 0)
> > 
> > Yeah, isn't that what we want to do? I read the test as "wake other threads,
> > if there are other threads" or something like that.
> 
> No, it's for synchronize_irq(). When there are threaded handlers in
> progress, then sychronize_irq() waits on the waitqueue until they are
> finished. So we wake the queue when the last threaded handler of this
> irq line returns from thread_fn.

Ok, I understand.

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Sun, 16 Aug 2009, Michael Buesch wrote:
> On Sunday 16 August 2009 16:25:13 Thomas Gleixner wrote:
> > On Sun, 16 Aug 2009, Michael Buesch wrote:
> > > On Sunday 16 August 2009 15:22:29 Thomas Gleixner wrote:
> > >
> > > > > +		if (0&&unlikely(desc->status & IRQ_DISABLED)) {
> > > > 
> > > > So the interrupt is marked disabled. How do you setup the handler
> > > > ?  And what does the primary handler do ? Can you post your driver
> > > > code please?
> > > 
> > > This patch converts the b43 driver to threaded interrupts:
> > > http://bu3sch.de/patches/wireless-testing/20090816-1535/patches/002-b43-threaded-irq-handler.patch
> > 
> > On the first glance this looks not too bad. the unlocked access to the
> > irq status registers looks a bit scary, but that is not relevant for
> > the problem at hand.
> 
> Yeah it does ;)
> 
> > > It kind of works with this hack applied to kernel/irq/manage.c
> > 
> > Hmm. Is the interrupt of the device shared ?
> 
> It's registered as shared, but on my machine it is not shared with anything else.
> 
> > If yes, what's the other 
> > device on that interrupt line ? what puzzles me is the fact that the
> > IRQ_DISABLED flag is set. Is there anything unusual in dmesg ?
> 
> Here's my current kernel log with the two patches applied:
> http://bu3sch.de/misc/dmesg

Hmm. Nothing interesting AFAICT, but it would be really interesting to
find out why the IRQ_DISABLED flag is set.

Can you add some debug into disable_irq() e.g. WARN_ON(irq ==
BC43_IRQ_NR); so we can see what disables that interrupt.

Thanks,

	tglx

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Sunday 16 August 2009 22:05:59 Thomas Gleixner wrote:
> On Sun, 16 Aug 2009, Michael Buesch wrote:
> > On Sunday 16 August 2009 16:25:13 Thomas Gleixner wrote:
> > > On Sun, 16 Aug 2009, Michael Buesch wrote:
> > > > On Sunday 16 August 2009 15:22:29 Thomas Gleixner wrote:
> > > >
> > > > > > +		if (0&&unlikely(desc->status & IRQ_DISABLED)) {
> > > > > 
> > > > > So the interrupt is marked disabled. How do you setup the handler
> > > > > ?  And what does the primary handler do ? Can you post your driver
> > > > > code please?
> > > > 
> > > > This patch converts the b43 driver to threaded interrupts:
> > > > http://bu3sch.de/patches/wireless-testing/20090816-1535/patches/002-b43-threaded-irq-handler.patch
> > > 
> > > On the first glance this looks not too bad. the unlocked access to the
> > > irq status registers looks a bit scary, but that is not relevant for
> > > the problem at hand.
> > 
> > Yeah it does ;)
> > 
> > > > It kind of works with this hack applied to kernel/irq/manage.c
> > > 
> > > Hmm. Is the interrupt of the device shared ?
> > 
> > It's registered as shared, but on my machine it is not shared with anything else.
> > 
> > > If yes, what's the other 
> > > device on that interrupt line ? what puzzles me is the fact that the
> > > IRQ_DISABLED flag is set. Is there anything unusual in dmesg ?
> > 
> > Here's my current kernel log with the two patches applied:
> > http://bu3sch.de/misc/dmesg
> 
> Hmm. Nothing interesting AFAICT, but it would be really interesting to
> find out why the IRQ_DISABLED flag is set.
> 
> Can you add some debug into disable_irq() e.g. WARN_ON(irq ==
> BC43_IRQ_NR); so we can see what disables that interrupt.

I do not see a warning, if I put this into __disable_irq():
WARN_ON(irq == 52);
/proc/interrupts shows 52 as IRQ number for b43.
And dmesg shows "... irq 52 on host ... mapped to virtual irq 52".
So I guess the test is OK and the flag is added by some other means.
Maybe by some weird powerpc architecture code?

It seems a little bit weird, however, that the WARN_ON does not even trigger
on module unload, which as far as I can tell should disable the IRQ line
in the free_irq() call (no other shared devices on this IRQ).

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Sun, 16 Aug 2009, Michael Buesch wrote:
> On Sunday 16 August 2009 22:05:59 Thomas Gleixner wrote:
> > Hmm. Nothing interesting AFAICT, but it would be really interesting to
> > find out why the IRQ_DISABLED flag is set.
> > 
> > Can you add some debug into disable_irq() e.g. WARN_ON(irq ==
> > BC43_IRQ_NR); so we can see what disables that interrupt.
> 
> I do not see a warning, if I put this into __disable_irq():
> WARN_ON(irq == 52);
> /proc/interrupts shows 52 as IRQ number for b43.
> And dmesg shows "... irq 52 on host ... mapped to virtual irq 52".
> So I guess the test is OK and the flag is added by some other means.
> Maybe by some weird powerpc architecture code?
> 
> It seems a little bit weird, however, that the WARN_ON does not even trigger
> on module unload, which as far as I can tell should disable the IRQ line
> in the free_irq() call (no other shared devices on this IRQ).

free_irq does not call disable_irq().

There are only a few places which set that flag.

dynamic_irq_init() which should not be called in your system

__set_irq_handler() only if a handler gets uninstalled and replaced by
handle_bad_irq

__disable_irq() which you already excluded

__freq_irq() which should not be called

note_interrupt() which should be visible in dmesg, but we do not see
such a thing.

IRQ_DISABLED is cleared at even less places:

request_irq() and __enable_irq() 

I'm really confused. Can you please add debug into those places and
provide the output. Also please print desc->status of irq 52 before
and after calling request_threaded_irq().

Thanks,

	tglx

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Sunday 16 August 2009 23:28:37 Thomas Gleixner wrote:
> On Sun, 16 Aug 2009, Michael Buesch wrote:
> > On Sunday 16 August 2009 22:05:59 Thomas Gleixner wrote:
> > > Hmm. Nothing interesting AFAICT, but it would be really interesting to
> > > find out why the IRQ_DISABLED flag is set.
> > > 
> > > Can you add some debug into disable_irq() e.g. WARN_ON(irq ==
> > > BC43_IRQ_NR); so we can see what disables that interrupt.
> > 
> > I do not see a warning, if I put this into __disable_irq():
> > WARN_ON(irq == 52);
> > /proc/interrupts shows 52 as IRQ number for b43.
> > And dmesg shows "... irq 52 on host ... mapped to virtual irq 52".
> > So I guess the test is OK and the flag is added by some other means.
> > Maybe by some weird powerpc architecture code?
> > 
> > It seems a little bit weird, however, that the WARN_ON does not even trigger
> > on module unload, which as far as I can tell should disable the IRQ line
> > in the free_irq() call (no other shared devices on this IRQ).
> 
> free_irq does not call disable_irq().
> 
> There are only a few places which set that flag.
> 
> dynamic_irq_init() which should not be called in your system
> 
> __set_irq_handler() only if a handler gets uninstalled and replaced by
> handle_bad_irq
> 
> __disable_irq() which you already excluded
> 
> __freq_irq() which should not be called
> 
> note_interrupt() which should be visible in dmesg, but we do not see
> such a thing.
> 
> IRQ_DISABLED is cleared at even less places:
> 
> request_irq() and __enable_irq() 
> 
> I'm really confused. Can you please add debug into those places and
> provide the output. Also please print desc->status of irq 52 before
> and after calling request_threaded_irq().
> 
> Thanks,
> 
> 	tglx

Ok, I added some more debugging code:
http://bu3sch.de/patches/wireless-testing/20090817-1219/patches/001-hack-threaded-irqs.patch

Here's the result:
http://bu3sch.de/misc/dmesg2

Is it possible that the irq_to_desc() in irq_thread() fails and the resulting desc
pointer points to something random? That could probably explain why the bit is set
and why the spinlock is uninitialized. But it would not explain why desc->lock would
still work... Maybe irq_to_desc() returns a descriptor to another irq (!= 52)?

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Mon, 17 Aug 2009, Michael Buesch wrote:
> Ok, I added some more debugging code:
> http://bu3sch.de/patches/wireless-testing/20090817-1219/patches/001-hack-threaded-irqs.patch
> 
> Here's the result:
> http://bu3sch.de/misc/dmesg2
>
> Is it possible that the irq_to_desc() in irq_thread() fails and the
> resulting desc pointer points to something random? That could
> probably explain why the bit is set and why the spinlock is
> uninitialized. But it would not explain why desc->lock would still
> work... Maybe irq_to_desc() returns a descriptor to another irq (!=
> 52)?

That would cause the whole irq code to fail. Can you send/upload your
.config please ?

Thanks,

	tglx

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Mon, 17 Aug 2009, Thomas Gleixner wrote:

> On Mon, 17 Aug 2009, Michael Buesch wrote:
> > Ok, I added some more debugging code:
> > http://bu3sch.de/patches/wireless-testing/20090817-1219/patches/001-hack-threaded-irqs.patch
> > 
> > Here's the result:
> > http://bu3sch.de/misc/dmesg2
> >
> > Is it possible that the irq_to_desc() in irq_thread() fails and the
> > resulting desc pointer points to something random? That could
> > probably explain why the bit is set and why the spinlock is
> > uninitialized. But it would not explain why desc->lock would still
> > work... Maybe irq_to_desc() returns a descriptor to another irq (!=
> > 52)?
> 
> That would cause the whole irq code to fail. Can you send/upload your
> .config please ?

Also just add printk("desc: %p \n", desc); to the various places to
make sure that your pointers are correct.

Thanks,

	tglx

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Mon, 17 Aug 2009, Thomas Gleixner wrote:
> On Mon, 17 Aug 2009, Thomas Gleixner wrote:
> 
> > On Mon, 17 Aug 2009, Michael Buesch wrote:
> > > Ok, I added some more debugging code:
> > > http://bu3sch.de/patches/wireless-testing/20090817-1219/patches/001-hack-threaded-irqs.patch
> > > 
> > > Here's the result:
> > > http://bu3sch.de/misc/dmesg2
> > >
> > > Is it possible that the irq_to_desc() in irq_thread() fails and the
> > > resulting desc pointer points to something random? That could
> > > probably explain why the bit is set and why the spinlock is
> > > uninitialized. But it would not explain why desc->lock would still
> > > work... Maybe irq_to_desc() returns a descriptor to another irq (!=
> > > 52)?
> > 
> > That would cause the whole irq code to fail. Can you send/upload your
> > .config please ?
> 
> Also just add printk("desc: %p \n", desc); to the various places to
> make sure that your pointers are correct.

Gah. I think I found it.

I wonder why nobody else ever tripped over this.

Thanks,

	tglx
---
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index d222515..223b062 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -607,7 +607,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 */
 		get_task_struct(t);
 		new->thread = t;
-		wake_up_process(t);
 	}
 
 	/*
@@ -712,6 +711,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	new->dir = NULL;
 	register_handler_proc(irq, new);
 
+	if (new->thread)
+		wake_up_process(new->thread);
+
 	return 0;
 
 mismatch:

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Monday 17 August 2009 13:12:09 Thomas Gleixner wrote:
> On Mon, 17 Aug 2009, Thomas Gleixner wrote:
> > On Mon, 17 Aug 2009, Thomas Gleixner wrote:
> > 
> > > On Mon, 17 Aug 2009, Michael Buesch wrote:
> > > > Ok, I added some more debugging code:
> > > > http://bu3sch.de/patches/wireless-testing/20090817-1219/patches/001-hack-threaded-irqs.patch
> > > > 
> > > > Here's the result:
> > > > http://bu3sch.de/misc/dmesg2
> > > >
> > > > Is it possible that the irq_to_desc() in irq_thread() fails and the
> > > > resulting desc pointer points to something random? That could
> > > > probably explain why the bit is set and why the spinlock is
> > > > uninitialized. But it would not explain why desc->lock would still
> > > > work... Maybe irq_to_desc() returns a descriptor to another irq (!=
> > > > 52)?
> > > 
> > > That would cause the whole irq code to fail. Can you send/upload your
> > > .config please ?
> > 
> > Also just add printk("desc: %p \n", desc); to the various places to
> > make sure that your pointers are correct.
> 
> Gah. I think I found it.
> 
> I wonder why nobody else ever tripped over this.
> 
> Thanks,
> 
> 	tglx
> ---
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index d222515..223b062 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -607,7 +607,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  		 */
>  		get_task_struct(t);
>  		new->thread = t;
> -		wake_up_process(t);
>  	}
>  
>  	/*
> @@ -712,6 +711,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  	new->dir = NULL;
>  	register_handler_proc(irq, new);
>  
> +	if (new->thread)
> +		wake_up_process(new->thread);
> +
>  	return 0;
>  
>  mismatch:
> 
> 

This fixes it :)

Tested-by: Michael Buesch <mb@bu3sch.de>

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

Michael,

On Mon, 17 Aug 2009, Michael Buesch wrote:
> This fixes it :)

can you please test the final version of the fix ?

Thanks,

	tglx
---
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -607,7 +607,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 */
 		get_task_struct(t);
 		new->thread = t;
-		wake_up_process(t);
 	}
 
 	/*
@@ -690,6 +689,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 				(int)(new->flags & IRQF_TRIGGER_MASK));
 	}
 
+	new->irq = irq;
 	*old_ptr = new;
 
 	/* Reset broken irq detection when installing new handler */
@@ -707,7 +707,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 
 	spin_unlock_irqrestore(&desc->lock, flags);
 
-	new->irq = irq;
 	register_irq_proc(irq, desc);
 	new->dir = NULL;
 	register_handler_proc(irq, new);

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Monday 17 August 2009 14:14:01 Thomas Gleixner wrote:
> Michael,
> 
> On Mon, 17 Aug 2009, Michael Buesch wrote:
> > This fixes it :)
> 
> can you please test the final version of the fix ?
> 
> Thanks,
> 
> 	tglx
> ---
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -607,7 +607,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  		 */
>  		get_task_struct(t);
>  		new->thread = t;
> -		wake_up_process(t);
>  	}
>  
>  	/*
> @@ -690,6 +689,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  				(int)(new->flags & IRQF_TRIGGER_MASK));
>  	}
>  
> +	new->irq = irq;
>  	*old_ptr = new;
>  
>  	/* Reset broken irq detection when installing new handler */
> @@ -707,7 +707,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  
>  	spin_unlock_irqrestore(&desc->lock, flags);
>  
> -	new->irq = irq;
>  	register_irq_proc(irq, desc);
>  	new->dir = NULL;
>  	register_handler_proc(irq, new);
> 
> 

Also works. Thanks.

Tested-by: Michael Buesch <mb@bu3sch.de>

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Monday 17 August 2009 14:30:31 Michael Buesch wrote:
> Also works. Thanks.
> 
> Tested-by: Michael Buesch <mb@bu3sch.de>

Hm, I've got a strange problem here related to threaded IRQs and rmmod.

If I do this sequence, it crashes the machine:
request_threaded_irq();
free_irq();
rmmod module
modprobe module
request_threaded_irq();
*boom*

Here are some oops messages. These are almost always different, so I
guess it crashes in IRQ context at random places when the IRQ triggers.
http://bu3sch.de/misc/irq_crash1.JPG
http://bu3sch.de/misc/irq_crash2.JPG

It seems to be a NULL pointer dereference somewhere, but I can't locate it.
Note that it does not happen, if I omit the rmmod.

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Friday 04 September 2009 20:55:52 Michael Buesch wrote:
> On Monday 17 August 2009 14:30:31 Michael Buesch wrote:
> > Also works. Thanks.
> > 
> > Tested-by: Michael Buesch <mb@bu3sch.de>
> 
> Hm, I've got a strange problem here related to threaded IRQs and rmmod.
> 
> If I do this sequence, it crashes the machine:
> request_threaded_irq();
> free_irq();
> rmmod module
> modprobe module
> request_threaded_irq();
> *boom*
> 
> Here are some oops messages. These are almost always different, so I
> guess it crashes in IRQ context at random places when the IRQ triggers.
> http://bu3sch.de/misc/irq_crash1.JPG
> http://bu3sch.de/misc/irq_crash2.JPG
> 
> It seems to be a NULL pointer dereference somewhere, but I can't locate it.
> Note that it does not happen, if I omit the rmmod.
> 

Ok, what I see now is that the IRQ thread that belongs to the driver is not
destroyed on free_irq(). So it is dangling and after rmmod/modprobe it will crash
on interrupt, because the module is relocated.
Bringing the device up and down several times (without reloading the module), which
does several request_threaded_irq(); free_irq() sequences in a row, creates a new
IRQ thread each time but does not destroy the old one.

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Michael Buesch]

On Friday 04 September 2009 21:05:51 Michael Buesch wrote:
> On Friday 04 September 2009 20:55:52 Michael Buesch wrote:
> > On Monday 17 August 2009 14:30:31 Michael Buesch wrote:
> > > Also works. Thanks.
> > > 
> > > Tested-by: Michael Buesch <mb@bu3sch.de>
> > 
> > Hm, I've got a strange problem here related to threaded IRQs and rmmod.
> > 
> > If I do this sequence, it crashes the machine:
> > request_threaded_irq();
> > free_irq();
> > rmmod module
> > modprobe module
> > request_threaded_irq();
> > *boom*
> > 
> > Here are some oops messages. These are almost always different, so I
> > guess it crashes in IRQ context at random places when the IRQ triggers.
> > http://bu3sch.de/misc/irq_crash1.JPG
> > http://bu3sch.de/misc/irq_crash2.JPG
> > 
> > It seems to be a NULL pointer dereference somewhere, but I can't locate it.
> > Note that it does not happen, if I omit the rmmod.
> > 
> 
> Ok, what I see now is that the IRQ thread that belongs to the driver is not
> destroyed on free_irq(). So it is dangling and after rmmod/modprobe it will crash
> on interrupt, because the module is relocated.
> Bringing the device up and down several times (without reloading the module), which
> does several request_threaded_irq(); free_irq() sequences in a row, creates a new
> IRQ thread each time but does not destroy the old one.

Whoopsy, I think I messed up my driver and did not call free_irq(). Sorry for the noise. :(

-- 
Greetings, Michael.

Re: Threaded interrupt handlers broken?

[Sebastian Andrzej Siewior]

* Michael Buesch | 2009-09-04 21:05:51 [+0200]:

>> Hm, I've got a strange problem here related to threaded IRQs and rmmod.
>> 
>> If I do this sequence, it crashes the machine:
>> request_threaded_irq();
>> free_irq();
>> rmmod module
>> modprobe module
>> request_threaded_irq();
>> *boom*
>> 
>> Here are some oops messages. These are almost always different, so I
>> guess it crashes in IRQ context at random places when the IRQ triggers.
>> http://bu3sch.de/misc/irq_crash1.JPG
>> http://bu3sch.de/misc/irq_crash2.JPG
>> 
>> It seems to be a NULL pointer dereference somewhere, but I can't locate it.
>> Note that it does not happen, if I omit the rmmod.
>> 
>
>Ok, what I see now is that the IRQ thread that belongs to the driver is not
>destroyed on free_irq(). So it is dangling and after rmmod/modprobe it will crash
>on interrupt, because the module is relocated.
>Bringing the device up and down several times (without reloading the module), which
>does several request_threaded_irq(); free_irq() sequences in a row, creates a new
>IRQ thread each time but does not destroy the old one.

The threaded interrupt infrastructure is okay as far as I can see it. My
currently OOT driver uses request_threaded_irq() and I can
modprobe/rmmod it with no problems. I just rebased it on top of -rc8 and
see no problems.

>Greetings, Michael.

Sebastian

Re: Threaded interrupt handlers broken?

[Thomas Gleixner]

On Sun, 16 Aug 2009, 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.

Hmm. The waitqueue is initialized when the first handler is set up. In
that case shared == 0. When the second handler is installed we do not
initialize it again as it is already initialized and even might have
waiters queued. I'll have a look.

Thanks,

	tglx