Skip to content

Commit 1e77d0a

Browse files
committed
genirq: Sanitize spurious interrupt detection of threaded irqs
Till reported that the 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]> Tested-by: Austin Schuh <[email protected]> Cc: Oliver Hartkopp <[email protected]> Cc: Wolfgang Grandegger <[email protected]> Cc: Pavel Pisa <[email protected]> Cc: Marc Kleine-Budde <[email protected]> Cc: [email protected] Cc: [email protected] Link: http://lkml.kernel.org/r/alpine.LFD.2.02.1303071450130.22263@ionos
1 parent 0384dca commit 1e77d0a

File tree

3 files changed

+108
-6
lines changed

3 files changed

+108
-6
lines changed

include/linux/irqdesc.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ struct irq_desc;
2727
* @irq_count: stats field to detect stalled irqs
2828
* @last_unhandled: aging timer for unhandled count
2929
* @irqs_unhandled: stats field for spurious unhandled interrupts
30+
* @threads_handled: stats field for deferred spurious detection of threaded handlers
31+
* @threads_handled_last: comparator field for deferred spurious detection of theraded handlers
3032
* @lock: locking for SMP
3133
* @affinity_hint: hint to user space for preferred irq affinity
3234
* @affinity_notify: context for notification of affinity changes
@@ -52,6 +54,8 @@ struct irq_desc {
5254
unsigned int irq_count; /* For detecting broken IRQs */
5355
unsigned long last_unhandled; /* Aging timer for unhandled count */
5456
unsigned int irqs_unhandled;
57+
atomic_t threads_handled;
58+
int threads_handled_last;
5559
raw_spinlock_t lock;
5660
struct cpumask *percpu_enabled;
5761
#ifdef CONFIG_SMP

kernel/irq/manage.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -886,8 +886,8 @@ static int irq_thread(void *data)
886886
irq_thread_check_affinity(desc, action);
887887

888888
action_ret = handler_fn(desc, action);
889-
if (!noirqdebug)
890-
note_interrupt(action->irq, desc, action_ret);
889+
if (action_ret == IRQ_HANDLED)
890+
atomic_inc(&desc->threads_handled);
891891

892892
wake_threads_waitq(desc);
893893
}

kernel/irq/spurious.c

Lines changed: 102 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -270,22 +270,120 @@ try_misrouted_irq(unsigned int irq, struct irq_desc *desc,
270270
return action && (action->flags & IRQF_IRQPOLL);
271271
}
272272

273+
#define SPURIOUS_DEFERRED 0x80000000
274+
273275
void note_interrupt(unsigned int irq, struct irq_desc *desc,
274276
irqreturn_t action_ret)
275277
{
276278
if (desc->istate & IRQS_POLL_INPROGRESS ||
277279
irq_settings_is_polled(desc))
278280
return;
279281

280-
/* we get here again via the threaded handler */
281-
if (action_ret == IRQ_WAKE_THREAD)
282-
return;
283-
284282
if (bad_action_ret(action_ret)) {
285283
report_bad_irq(irq, desc, action_ret);
286284
return;
287285
}
288286

287+
/*
288+
* We cannot call note_interrupt from the threaded handler
289+
* because we need to look at the compound of all handlers
290+
* (primary and threaded). Aside of that in the threaded
291+
* shared case we have no serialization against an incoming
292+
* hardware interrupt while we are dealing with a threaded
293+
* result.
294+
*
295+
* So in case a thread is woken, we just note the fact and
296+
* defer the analysis to the next hardware interrupt.
297+
*
298+
* The threaded handlers store whether they sucessfully
299+
* handled an interrupt and we check whether that number
300+
* changed versus the last invocation.
301+
*
302+
* We could handle all interrupts with the delayed by one
303+
* mechanism, but for the non forced threaded case we'd just
304+
* add pointless overhead to the straight hardirq interrupts
305+
* for the sake of a few lines less code.
306+
*/
307+
if (action_ret & IRQ_WAKE_THREAD) {
308+
/*
309+
* There is a thread woken. Check whether one of the
310+
* shared primary handlers returned IRQ_HANDLED. If
311+
* not we defer the spurious detection to the next
312+
* interrupt.
313+
*/
314+
if (action_ret == IRQ_WAKE_THREAD) {
315+
int handled;
316+
/*
317+
* We use bit 31 of thread_handled_last to
318+
* denote the deferred spurious detection
319+
* active. No locking necessary as
320+
* thread_handled_last is only accessed here
321+
* and we have the guarantee that hard
322+
* interrupts are not reentrant.
323+
*/
324+
if (!(desc->threads_handled_last & SPURIOUS_DEFERRED)) {
325+
desc->threads_handled_last |= SPURIOUS_DEFERRED;
326+
return;
327+
}
328+
/*
329+
* Check whether one of the threaded handlers
330+
* returned IRQ_HANDLED since the last
331+
* interrupt happened.
332+
*
333+
* For simplicity we just set bit 31, as it is
334+
* set in threads_handled_last as well. So we
335+
* avoid extra masking. And we really do not
336+
* care about the high bits of the handled
337+
* count. We just care about the count being
338+
* different than the one we saw before.
339+
*/
340+
handled = atomic_read(&desc->threads_handled);
341+
handled |= SPURIOUS_DEFERRED;
342+
if (handled != desc->threads_handled_last) {
343+
action_ret = IRQ_HANDLED;
344+
/*
345+
* Note: We keep the SPURIOUS_DEFERRED
346+
* bit set. We are handling the
347+
* previous invocation right now.
348+
* Keep it for the current one, so the
349+
* next hardware interrupt will
350+
* account for it.
351+
*/
352+
desc->threads_handled_last = handled;
353+
} else {
354+
/*
355+
* None of the threaded handlers felt
356+
* responsible for the last interrupt
357+
*
358+
* We keep the SPURIOUS_DEFERRED bit
359+
* set in threads_handled_last as we
360+
* need to account for the current
361+
* interrupt as well.
362+
*/
363+
action_ret = IRQ_NONE;
364+
}
365+
} else {
366+
/*
367+
* One of the primary handlers returned
368+
* IRQ_HANDLED. So we don't care about the
369+
* threaded handlers on the same line. Clear
370+
* the deferred detection bit.
371+
*
372+
* In theory we could/should check whether the
373+
* deferred bit is set and take the result of
374+
* the previous run into account here as
375+
* well. But it's really not worth the
376+
* trouble. If every other interrupt is
377+
* handled we never trigger the spurious
378+
* detector. And if this is just the one out
379+
* of 100k unhandled ones which is handled
380+
* then we merily delay the spurious detection
381+
* by one hard interrupt. Not a real problem.
382+
*/
383+
desc->threads_handled_last &= ~SPURIOUS_DEFERRED;
384+
}
385+
}
386+
289387
if (unlikely(action_ret == IRQ_NONE)) {
290388
/*
291389
* If we are seeing only the odd spurious IRQ caused by

0 commit comments

Comments
 (0)