genirq: Sanitize spurious interrupt detection of threaded irqs

Till reported that he spurious interrupt detection of threaded interrupts is broken in two ways:

- note_interrupt() is called for each action thread of a shared interrupt line. That's wrong as we are only interested whether none of the device drivers felt responsible for the interrupt, but by calling multiple times for a single interrupt line we account IRQ_NONE even if one of the drivers felt responsible.

- note_interrupt() when called from the thread handler is not serialized. That leaves the members of irq_desc which are used for the spurious detection unprotected.

To solve this we need to defer the spurious detection of a threaded interrupt to the next hardware interrupt context where we have implicit serialization.

If note_interrupt is called with action_ret == IRQ_WAKE_THREAD, we check whether the previous interrupt requested a deferred check. If not, we request a deferred check for the next hardware interrupt and return.

If set, we check whether one of the interrupt threads signaled success. Depending on this information we feed the result into the spurious detector.

If one primary handler of a shared interrupt returns IRQ_HANDLED we disable the deferred check of irq threads on the same line, as we have found at least one device driver who cared.

Reported-by Till Straumann <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
  include/linux/irqdesc.h |    4 +++
  kernel/irq/internals.h  |    4 +++
  kernel/irq/manage.c     |    4 +--
  kernel/irq/spurious.c   |   57 ++++++++++++++++++++++++++++++++++++++++++++----
  4 files changed, 63 insertions(+), 6 deletions(-)

Index: linux-2.6/include/linux/irqdesc.h
===================================================================

include/linux/irqdesc.h +4 -0

@@ -27,6 +27,8 @@ struct irq_desc;
   * @irq_count:		stats field to detect stalled irqs
   * @last_unhandled:	aging timer for unhandled count
   * @irqs_unhandled:	stats field for spurious unhandled interrupts
+ * @threads_handled:	stats field for deferred spurious detection of threaded handlers
+ * @threads_handled_last: comparator field for deferred spurious detection of theraded handlers
   * @lock:		locking for SMP
   * @affinity_hint:	hint to user space for preferred irq affinity
   * @affinity_notify:	context for notification of affinity changes

@@ -52,6 +54,8 @@ struct irq_desc {
  	unsigned int		irq_count;	/* For detecting broken IRQs */
  	unsigned long		last_unhandled;	/* Aging timer for unhandled count */
  	unsigned int		irqs_unhandled;
+	atomic_t		threads_handled;
+	int			threads_handled_last;
  	raw_spinlock_t		lock;
  	struct cpumask		*percpu_enabled;
  #ifdef CONFIG_SMP

Index: linux-2.6/kernel/irq/internals.h
===================================================================

kernel/irq/internals.h +4 -0

@@ -55,6 +55,10 @@ enum {
  	IRQS_SUSPENDED		= 0x00000800,
  };
 
+/* Bits for spurious detection of threaded handlers */
+#define SPURIOUS_MASK		0x7fffffff
+#define SPURIOUS_DEFERRED	0x80000000
+
  #include "debug.h"
  #include "settings.h"
 

Index: linux-2.6/kernel/irq/manage.c
===================================================================

kernel/irq/manage.c +2 -2

@@ -867,8 +867,8 @@ static int irq_thread(void *data)
  		irq_thread_check_affinity(desc, action);
 
  		action_ret = handler_fn(desc, action);
-		if (!noirqdebug)
-			note_interrupt(action->irq, desc, action_ret);
+		if (!noirqdebug && action_ret == IRQ_HANDLED)
+			atomic_inc(&desc->threads_handled);
 
  		wake_threads_waitq(desc);
  	}

Index: linux-2.6/kernel/irq/spurious.c
===================================================================

kernel/irq/spurious.c +53 -4

@@ -271,15 +271,64 @@ void note_interrupt(unsigned int irq, st
  	if (desc->istate & IRQS_POLL_INPROGRESS)
  		return;
 
-	/* we get here again via the threaded handler */
-	if (action_ret == IRQ_WAKE_THREAD)
-		return;
-
  	if (bad_action_ret(action_ret)) {
  		report_bad_irq(irq, desc, action_ret);
  		return;
  	}
 
+	/*
+	 * We cannot call note_interrupt from the threaded handler. So
+	 * the threaded handlers store whether they sucessfully
+	 * handled an interrupt. Here is the right place to take care
+	 * of this.
+	 */
+	if (action_ret & IRQ_WAKE_THREAD) {
+		/*
+		 * There is a thread woken. Check whether one of the
+		 * shared primary handlers returned IRQ_HANDLED. If
+		 * not we defer the spurious detection to the next
+		 * interrupt.
+		 */
+		if (action_ret == IRQ_WAKE_THREAD) {
+			int handled;
+
+			/*
+			 * We use bit 31 of thread_handled_last to
+			 * note the deferred spurious detection
+			 * active. No locking necessary as
+			 * thread_handled_last is only accessed here
+			 * and we have the guarantee that hard
+			 * interrupts are not reentrant.
+			 */
+			if (!(desc->threads_handled_last & SPURIOUS_DEFERRED)) {
+				desc->threads_handled_last |= SPURIOUS_DEFERRED;
+				return;
+			}
+			/*
+			 * Check whether one of the threaded handlers
+			 * returned IRQ_HANDLED since the last
+			 * interrupt happened. Or the spurious
+			 * deferred flag for ease of comparison and
+			 * preservation in the writeback.
+			 */
+			handled = atomic_read(&desc->threads_handled);
+			handled |= SPURIOUS_DEFERRED;
+			if (handled != desc->threads_handled_last) {
+				action_ret = IRQ_HANDLED;
+				desc->threads_handled_last = handled;
+			} else {
+				action_ret = IRQ_NONE;
+			}
+		} else {
+			/*
+			 * One of the primary handlers returned
+			 * IRQ_HANDLED. So we don't care about the
+			 * threaded handlers on the same line.
+			 */
+			desc->threads_handled_last &= ~SPURIOUS_DEFERRED;
+		}
+	}
+
  	if (unlikely(action_ret == IRQ_NONE)) {
  		/*
  		 * If we are seeing only the odd spurious IRQ caused by

1) I'm not sure adding the SPURIOUS_DEFERRED flag into threads_handled_last is OK - what happens if the atomic_t counter can hold more than 31 bits? In this case, when thread handlers increment the counter there is interference with the flag. If this is not harmful then it is at least ugly.

I'm not as familiar with the code as you are but wouldn't it be
simpler to always defer spurious detection thus avoiding to have to keep track of the state (deferral active/inactive)? I.e., if any primary handler returns IRQ_HANDLED then we simply increment the counter. note_interrupt() could then always compare the previous count to the current count and if they are equal conclude that the interrupt was not handled:

    handle_irq_event_percpu()
    {
    ...
      if (!noirqdebug)
        note_interrupt(irq, desc, retval);

      if ( (retval & IRQ_HANDLED) )
        atomic_inc(&desc->threads_handled);
    }

    and in 'note_interrupt()'

      handled = atomic_read(&desc->threads_handled);
      if ( desc->threads_handled_last == handled ) {
        action_ret = IRQ_NONE;
      } else {
        action_ret = IRQ_HANDLED;
        desc->threads_handled_last = handled;
      }

Either way - I'm not sure what deferral does to the part of the algorithm in note_interrupt() which deals with misrouted interrupts since the 'action_ret' that goes into try_misrouted_irq() is delayed by one interrupt cycle.

2) note_interrupt is also called from irq/chip.c:handle_nested_irq() and I believe
this routine would also need to increment the 'threads_handled' counter
rather
than calling note_interrupt.

Quoted 5 lines · Till Straumann

1) I'm not sure adding the SPURIOUS_DEFERRED flag into threads_handled_last is OK - what happens if the atomic_t counter can hold more than 31 bits? In this case, when thread handlers increment the counter there is interference with the flag. If this is not harmful then it is at least ugly.

atomic_t is going to stay 32 bit otherwise we'll have more horrible problems than that one.

Quoted 8 lines

I'm not as familiar with the code as you are but wouldn't it be
simpler to always defer spurious detection thus avoiding to have to keep track of the state (deferral active/inactive)? I.e., if any primary handler returns IRQ_HANDLED then we simply increment the counter. note_interrupt() could then always compare the previous count to the current count and if they are equal conclude that the interrupt was not handled:

Yeah, we could do it that way. Would probably be simpler.

Quoted 16 lines

     handle_irq_event_percpu()
     {
     ...
       if (!noirqdebug)
         note_interrupt(irq, desc, retval);

       if ( (retval & IRQ_HANDLED) )
         atomic_inc(&desc->threads_handled);
     }

     and in 'note_interrupt()'

       handled = atomic_read(&desc->threads_handled);
       if ( desc->threads_handled_last == handled ) {
         action_ret = IRQ_NONE;
       } else {

Quoted 8 lines

         action_ret = IRQ_HANDLED;
         desc->threads_handled_last = handled;
       }

    Either way - I'm not sure what deferral does to the part of the algorithm
    in note_interrupt() which deals with misrouted interrupts since the
    'action_ret' that goes into try_misrouted_irq() is delayed by one interrupt
    cycle.

That should not matter much methinks, but I'll try what explodes on one of my affected machines.

Quoted 5 lines

2) note_interrupt is also called from irq/chip.c:handle_nested_irq() and I believe
this routine would also need to increment the 'threads_handled' counter
rather
than calling note_interrupt.

That's a different issue. The nested_irq handler is for interrupts which are demultiplexed by a primary threaded handler. That interrupt is never handled in hard interrupt context. It's always called from the context of the demultiplxing thread.

Thanks,

tglx

Quoted 7 lines · Thomas Gleixner

Quoted 5 lines · Till Straumann

1) I'm not sure adding the SPURIOUS_DEFERRED flag into threads_handled_last is OK - what happens if the atomic_t counter can hold more than 31 bits? In this case, when thread handlers increment the counter there is interference with the flag. If this is not harmful then it is at least ugly.

Quoted 2 lines

atomic_t is going to stay 32 bit otherwise we'll have more horrible problems than that one.

I know. But this means that when the counter overflows 31 bits (2^31 - 1) then it spills into the SPURIOUS_DEFERRED flag, right?

Quoted 10 lines

Quoted 6 lines

2) note_interrupt is also called from irq/chip.c:handle_nested_irq() and I believe
this routine would also need to increment the 'threads_handled'
counter
rather
than calling note_interrupt.

That's a different issue. The nested_irq handler is for interrupts which are demultiplexed by a primary threaded handler. That interrupt is never handled in hard interrupt context. It's always called from the context of the demultiplxing thread.

So you are saying that there 'handle_nested_irq()' can never be executed from more than one thread for a single interrupt?

I find, however, that e.g., the gpio-sx150x.c driver calls

request_threaded_irq() with IRQF_SHARED set and it's thread_fn does call handle_nested_irq(). It would thus be possible that multiple drivers could share an interrupt and each driver would call handle_nested_irq() which in-turn executes note_interrupt(). This would again raise the issues we already discussed (note_interrupt() not serialized and thinking that an interrupt was not handled because it was handled by a different thread).

Probably I'm missing something regarding the use of nested interrupts
- I would really appreciate if you could help me understand why it should be OK for handle_nested_irq() to call note_interrupt().

Quoted 8 lines · Till Straumann

Quoted 7 lines · Thomas Gleixner

Quoted 5 lines · Till Straumann

1) I'm not sure adding the SPURIOUS_DEFERRED flag into threads_handled_last is OK - what happens if the atomic_t counter can hold more than 31 bits? In this case, when thread handlers increment the counter there is interference with the flag. If this is not harmful then it is at least ugly.

Quoted 4 lines

Quoted 2 lines

atomic_t is going to stay 32 bit otherwise we'll have more horrible problems than that one.

I know. But this means that when the counter overflows 31 bits (2^31 - 1) then it spills into the SPURIOUS_DEFERRED flag, right?

Gah, yes. /me should stop doing overoptimizations :)

Quoted 23 lines

Quoted 10 lines

Quoted 6 lines

2) note_interrupt is also called from irq/chip.c:handle_nested_irq() and I believe
this routine would also need to increment the 'threads_handled'
counter
rather
than calling note_interrupt.

That's a different issue. The nested_irq handler is for interrupts which are demultiplexed by a primary threaded handler. That interrupt is never handled in hard interrupt context. It's always called from the context of the demultiplxing thread.

So you are saying that there 'handle_nested_irq()' can never be executed from more than one thread for a single interrupt?

I find, however, that e.g., the gpio-sx150x.c driver calls

request_threaded_irq() with IRQF_SHARED set and it's thread_fn does call handle_nested_irq(). It would thus be possible that multiple drivers could share an interrupt and each driver would call handle_nested_irq() which in-turn executes note_interrupt(). This would again raise the issues we already discussed (note_interrupt() not serialized and thinking that an interrupt was not handled because it was handled by a different thread).

Quoted 3 lines

Probably I'm missing something regarding the use of nested interrupts
- I would really appreciate if you could help me understand why it should be OK for handle_nested_irq() to call note_interrupt().

The thing about nested irqs is:

main irq is threaded (requested by the driver for stuff like i2c)

The handler of this irq reads a pending irq register in the chip and then invokes handle_nested_irq() for each of the pending bits.

Those interrupts cannot be shared even if the driver request them as shared:

         irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
         raw_spin_unlock_irq(&desc->lock);

         action_ret = action->thread_fn(action->irq, action->dev_id);
         if (!noirqdebug)
                 note_interrupt(irq, desc, action_ret);

         raw_spin_lock_irq(&desc->lock);
         irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);

So there is no loop over action->next. And even if that code would loop over action next, then it still would be serialized

main irq is raised

      -> wake thread

thread runs

        read pending reg()

        for each pending bit {

        	   handle_nested_irq();
		action = desc->action;

		while (action) {
		      action->thread_fn()
		      action = action->next)
		}
		note_interrupt();
        }

thread done

Hope that helps. Thanks,

tglx