Johan,
On Tue, Mar 16 2021 at 11:56, Johan Hovold wrote:
> We've gotten reports of lockdep splats correctly identifying a potential
> deadlock in serial drivers when running with forced interrupt threading.
>
> Typically, a serial driver takes the port spin lock in its interrupt
> handler, but unless also disabling interrupts the handler can be
> preempted by another interrupt which can end up calling printk. The
> console code takes then tries to take the port lock and we deadlock.
>
> It seems to me that forced interrupt threading cannot generally work
> without updating drivers that expose locks that can be taken by other
> interrupt handlers, for example, by using spin_lock_irqsave() in their
> interrupt handlers or marking their interrupts as IRQF_NO_THREAD.
The latter is the worst option because that will break PREEMPT_RT.
> What are your thoughts on this given that forced threading isn't that
> widely used and was said to be "mostly a debug option". Do we need to
> vet all current and future drivers and adapt them for "threadirqs"?
>
> Note that we now have people sending cleanup patches for interrupt
> handlers by search-and-replacing spin_lock_irqsave() with spin_lock()
> which can end up exposing this more.
It's true that for !RT it's primarily a debug option, but occasionaly a
very valuable one because it does not take the whole machine down when
something explodes in an interrupt handler. Used it just a couple of
weeks ago successfully :)
So we have several ways out of that:
1) Do the lock() -> lock_irqsave() dance
2) Delay printing from hard interrupt context (which is what RT does)
3) Actually disable interrupts before calling the force threaded
handler.
I'd say #3 is the right fix here. It's preserving the !RT semantics
and the usefulness of threadirqs for debugging and spare us dealing with
the script kiddies.
Something like the below.
Thanks,
tglx
---
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -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);
irq_finalize_oneshot(desc, action);
+ if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+ local_enable();
local_bh_enable();
return ret;
}
