| From 8707898e22fd665bc1d7b18b809be4b56ce25bdd Mon Sep 17 00:00:00 2001 |
| From: Thomas Pfaff <tpfaff@pcs.com> |
| Date: Mon, 2 May 2022 13:28:29 +0200 |
| Subject: genirq: Synchronize interrupt thread startup |
| |
| From: Thomas Pfaff <tpfaff@pcs.com> |
| |
| commit 8707898e22fd665bc1d7b18b809be4b56ce25bdd upstream. |
| |
| A kernel hang can be observed when running setserial in a loop on a kernel |
| with force threaded interrupts. The sequence of events is: |
| |
| setserial |
| open("/dev/ttyXXX") |
| request_irq() |
| do_stuff() |
| -> serial interrupt |
| -> wake(irq_thread) |
| desc->threads_active++; |
| close() |
| free_irq() |
| kthread_stop(irq_thread) |
| synchronize_irq() <- hangs because desc->threads_active != 0 |
| |
| The thread is created in request_irq() and woken up, but does not get on a |
| CPU to reach the actual thread function, which would handle the pending |
| wake-up. kthread_stop() sets the should stop condition which makes the |
| thread immediately exit, which in turn leaves the stale threads_active |
| count around. |
| |
| This problem was introduced with commit 519cc8652b3a, which addressed a |
| interrupt sharing issue in the PCIe code. |
| |
| Before that commit free_irq() invoked synchronize_irq(), which waits for |
| the hard interrupt handler and also for associated threads to complete. |
| |
| To address the PCIe issue synchronize_irq() was replaced with |
| __synchronize_hardirq(), which only waits for the hard interrupt handler to |
| complete, but not for threaded handlers. |
| |
| This was done under the assumption, that the interrupt thread already |
| reached the thread function and waits for a wake-up, which is guaranteed to |
| be handled before acting on the stop condition. The problematic case, that |
| the thread would not reach the thread function, was obviously overlooked. |
| |
| Make sure that the interrupt thread is really started and reaches |
| thread_fn() before returning from __setup_irq(). |
| |
| This utilizes the existing wait queue in the interrupt descriptor. The |
| wait queue is unused for non-shared interrupts. For shared interrupts the |
| usage might cause a spurious wake-up of a waiter in synchronize_irq() or the |
| completion of a threaded handler might cause a spurious wake-up of the |
| waiter for the ready flag. Both are harmless and have no functional impact. |
| |
| [ tglx: Amended changelog ] |
| |
| Fixes: 519cc8652b3a ("genirq: Synchronize only with single thread on free_irq()") |
| Signed-off-by: Thomas Pfaff <tpfaff@pcs.com> |
| Signed-off-by: Thomas Gleixner <tglx@linutronix.de> |
| Reviewed-by: Marc Zyngier <maz@kernel.org> |
| Cc: stable@vger.kernel.org |
| Link: https://lore.kernel.org/r/552fe7b4-9224-b183-bb87-a8f36d335690@pcs.com |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| kernel/irq/internals.h | 2 ++ |
| kernel/irq/irqdesc.c | 2 ++ |
| kernel/irq/manage.c | 39 +++++++++++++++++++++++++++++---------- |
| 3 files changed, 33 insertions(+), 10 deletions(-) |
| |
| --- a/kernel/irq/internals.h |
| +++ b/kernel/irq/internals.h |
| @@ -29,12 +29,14 @@ extern struct irqaction chained_action; |
| * IRQTF_WARNED - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed |
| * IRQTF_AFFINITY - irq thread is requested to adjust affinity |
| * IRQTF_FORCED_THREAD - irq action is force threaded |
| + * IRQTF_READY - signals that irq thread is ready |
| */ |
| enum { |
| IRQTF_RUNTHREAD, |
| IRQTF_WARNED, |
| IRQTF_AFFINITY, |
| IRQTF_FORCED_THREAD, |
| + IRQTF_READY, |
| }; |
| |
| /* |
| --- a/kernel/irq/irqdesc.c |
| +++ b/kernel/irq/irqdesc.c |
| @@ -405,6 +405,7 @@ static struct irq_desc *alloc_desc(int i |
| lockdep_set_class(&desc->lock, &irq_desc_lock_class); |
| mutex_init(&desc->request_mutex); |
| init_rcu_head(&desc->rcu); |
| + init_waitqueue_head(&desc->wait_for_threads); |
| |
| desc_set_defaults(irq, desc, node, affinity, owner); |
| irqd_set(&desc->irq_data, flags); |
| @@ -573,6 +574,7 @@ int __init early_irq_init(void) |
| raw_spin_lock_init(&desc[i].lock); |
| lockdep_set_class(&desc[i].lock, &irq_desc_lock_class); |
| mutex_init(&desc[i].request_mutex); |
| + init_waitqueue_head(&desc[i].wait_for_threads); |
| desc_set_defaults(i, &desc[i], node, NULL, NULL); |
| } |
| return arch_early_irq_init(); |
| --- a/kernel/irq/manage.c |
| +++ b/kernel/irq/manage.c |
| @@ -1149,6 +1149,31 @@ static void irq_wake_secondary(struct ir |
| } |
| |
| /* |
| + * Internal function to notify that a interrupt thread is ready. |
| + */ |
| +static void irq_thread_set_ready(struct irq_desc *desc, |
| + struct irqaction *action) |
| +{ |
| + set_bit(IRQTF_READY, &action->thread_flags); |
| + wake_up(&desc->wait_for_threads); |
| +} |
| + |
| +/* |
| + * Internal function to wake up a interrupt thread and wait until it is |
| + * ready. |
| + */ |
| +static void wake_up_and_wait_for_irq_thread_ready(struct irq_desc *desc, |
| + struct irqaction *action) |
| +{ |
| + if (!action || !action->thread) |
| + return; |
| + |
| + wake_up_process(action->thread); |
| + wait_event(desc->wait_for_threads, |
| + test_bit(IRQTF_READY, &action->thread_flags)); |
| +} |
| + |
| +/* |
| * Interrupt handler thread |
| */ |
| static int irq_thread(void *data) |
| @@ -1159,6 +1184,8 @@ static int irq_thread(void *data) |
| irqreturn_t (*handler_fn)(struct irq_desc *desc, |
| struct irqaction *action); |
| |
| + irq_thread_set_ready(desc, action); |
| + |
| if (force_irqthreads && test_bit(IRQTF_FORCED_THREAD, |
| &action->thread_flags)) |
| handler_fn = irq_forced_thread_fn; |
| @@ -1583,8 +1610,6 @@ __setup_irq(unsigned int irq, struct irq |
| } |
| |
| if (!shared) { |
| - init_waitqueue_head(&desc->wait_for_threads); |
| - |
| /* Setup the type (level, edge polarity) if configured: */ |
| if (new->flags & IRQF_TRIGGER_MASK) { |
| ret = __irq_set_trigger(desc, |
| @@ -1674,14 +1699,8 @@ __setup_irq(unsigned int irq, struct irq |
| |
| irq_setup_timings(desc, new); |
| |
| - /* |
| - * Strictly no need to wake it up, but hung_task complains |
| - * when no hard interrupt wakes the thread up. |
| - */ |
| - if (new->thread) |
| - wake_up_process(new->thread); |
| - if (new->secondary) |
| - wake_up_process(new->secondary->thread); |
| + wake_up_and_wait_for_irq_thread_ready(desc, new); |
| + wake_up_and_wait_for_irq_thread_ready(desc, new->secondary); |
| |
| register_irq_proc(irq, desc); |
| new->dir = NULL; |