lkml.org 
[lkml]   [2022]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.19 66/88] genirq: Synchronize interrupt thread startup
    Date
    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
    @@ -404,6 +404,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);
    @@ -568,6 +569,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
    @@ -1064,6 +1064,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)
    @@ -1074,6 +1099,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;
    @@ -1462,8 +1489,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,
    @@ -1553,14 +1578,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;

    \
     
     \ /
      Last update: 2022-05-10 15:41    [W:4.211 / U:0.588 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site