genirq: Disable interrupts for force threaded handlers

With interrupt force threading all device interrupt handlers are invoked from kernel threads. Contrary to hard interrupt context the invocation only disables bottom halfs, but not interrupts. This was an oversight back then because any code like this will have an issue:

thread(irq_A)
irq_handler(A)

    spin_lock(&foo->lock);

interrupt(irq_B)
irq_handler(B)

    spin_lock(&foo->lock);

This has been triggered with networking (NAPI vs. hrtimers) and console drivers where printk() happens from an interrupt which interrupted the force threaded handler.

Now people noticed and started to change the spin_lock() in the handler to spin_lock_irqsave() which affects performance or add IRQF_NOTHREAD to the interrupt request which in turn breaks RT.

Fix the root cause and not the symptom and disable interrupts before invoking the force threaded handler which preserves the regular semantics and the usefulness of the interrupt force threading as a general debugging tool.

For not RT this is not changing much, except that during the execution of the threaded handler interrupts are delayed until the handler returns. Vs. scheduling and softirq processing there is no difference.

For RT kernels there is no issue.

Fixes 8d32a307e4fa ("genirq: Provide forced interrupt threading")

Reported-by Johan Hovold <[email protected]>

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

Cc Eric Dumazet <[email protected]>

Cc Sebastian Andrzej Siewior <[email protected]>

Cc netdev <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Andy Shevchenko <[email protected]>

CC Peter Zijlstra <[email protected]>

Cc [email protected]

Cc netdev <[email protected]>

---
 kernel/irq/manage.c |    4 ++++
 1 file changed, 4 insertions(+)

kernel/irq/manage.c +2 -0

@@ -1142,11 +1142,15 @@ irq_forced_thread_fn(struct irq_desc *de
 	irqreturn_t ret;
 
 	local_bh_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_disable();

Quoted 7 lines · Thomas Gleixner

thread(irq_A)
irq_handler(A)

    spin_lock(&foo->lock);

interrupt(irq_B)
irq_handler(B)

    spin_lock(&foo->lock);

It will not because both threads will wake_up(thread). It is an issue if
- if &foo->lock is shared between a hrtimer and threaded-IRQ
- if &foo->lock is shared between a non-threaded and thread-IRQ
- if &foo->lock is shared between a printk() in hardirq context and thread-IRQ as I learned today.

Quoted 9 lines · Thomas Gleixner

With interrupt force threading all device interrupt handlers are invoked from kernel threads. Contrary to hard interrupt context the invocation only disables bottom halfs, but not interrupts. This was an oversight back then because any code like this will have an issue:

thread(irq_A)
irq_handler(A)

    spin_lock(&foo->lock);

Quoted 3 lines

interrupt(irq_B)
irq_handler(B)

    spin_lock(&foo->lock);

It will not because both threads will wake_up(thread).

It is an issue if
- if &foo->lock is shared between a hrtimer and threaded-IRQ
- if &foo->lock is shared between a non-threaded and thread-IRQ

- if &foo->lock is shared between a printk() in hardirq context and thread-IRQ as I learned today.

Quoted 13 lines

This has been triggered with networking (NAPI vs. hrtimers) and console drivers where printk() happens from an interrupt which interrupted the force threaded handler.

Now people noticed and started to change the spin_lock() in the handler to spin_lock_irqsave() which affects performance or add IRQF_NOTHREAD to the interrupt request which in turn breaks RT.

Fix the root cause and not the symptom and disable interrupts before invoking the force threaded handler which preserves the regular semantics and the usefulness of the interrupt force threading as a general debugging tool.

Quoted 5 lines

For not RT this is not changing much, except that during the execution of the threaded handler interrupts are delayed until the handler returns. Vs. scheduling and softirq processing there is no difference.

For RT kernels there is no issue.

Acked-by Sebastian Andrzej Siewior <[email protected]>

Quoted 29 lines

Fixes 8d32a307e4fa ("genirq: Provide forced interrupt threading")

Reported-by Johan Hovold <[email protected]>

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

Cc Eric Dumazet <[email protected]>

Cc Sebastian Andrzej Siewior <[email protected]>

Cc netdev <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Andy Shevchenko <[email protected]>

CC Peter Zijlstra <[email protected]>

Cc [email protected]

Cc netdev <[email protected]>

---
 kernel/irq/manage.c |    4 ++++
 1 file changed, 4 insertions(+)

kernel/irq/manage.c +2 -0

@@ -1142,11 +1142,15 @@ irq_forced_thread_fn(struct irq_desc *de
 	irqreturn_t ret;
 
 	local_bh_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_disable();
 	ret = action->thread_fn(action->irq, action->dev_id);
 	if (ret == IRQ_HANDLED)
 		atomic_inc(&desc->threads_handled);
 

kernel/irq/manage.c irq_forced_thread_fn(struct irq_desc *de · line 1156

Quoted 6 lines

kernel/irq/manage.c +2 -0

 	irq_finalize_oneshot(desc, action);
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_irq_enable();
 	local_bh_enable();
 	return ret;
 }

Quoted 4 lines

Quoted 4 lines

thread(irq_A)
irq_handler(A)

    spin_lock(&foo->lock);

Quoted 5 lines

Quoted 3 lines

interrupt(irq_B)
irq_handler(B)

    spin_lock(&foo->lock);

It will not because both threads will wake_up(thread).

Note that the above says "interrupt(irq_B)" suggesting it's a non-threaded interrupt unlike irq_A.