Hi Thomas,
Thanks for this, a few comments below.
On Fri, 29 Apr 2022 12:52:48 +0100,
Thomas Pfaff <[email protected]> wrote:
>
> From: Thomas Pfaff <[email protected]>
>
> While running
> "while /bin/true; do setserial /dev/ttyS0 uart none;
> setserial /dev/ttyS0 uart 16550A; done"
> on a kernel with threaded irqs, setserial is hung after some calls.
>
> setserial opens the device, this will install an irq handler if the uart is
> not none, followed by TIOCGSERIAL and TIOCSSERIAL ioctls.
> Then the device is closed. On close, synchronize_irq() is called by
> serial_core.
>
> If the close comes too fast, the irq_thread does not really start,
> it is terminated immediately without going into irq_thread().
> But an interrupt might already been handled by
> irq_default_primary_handler(), going to __irq_wake_thread() and
> incrementing threads_active.
> If this happens, synchronize_irq() will hang forever, because the
> irq_thread is already dead, and threads_active will never be decremented.
>
> The fix is to make sure that the irq_thread is really started
> during __setup_irq().
>
> Signed-off-by: Thomas Pfaff <[email protected]>
> ---
> v1-v2:
> - use already existing resources
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index 99cbdf55a8bd..dca57bed0d96 100644
> --- 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_UP - signals that irq thread is ready
nit: Why not call the flag IRQTF_READY then? I find it slightly more
readable than 'UP'.
> */
> enum {
> IRQTF_RUNTHREAD,
> IRQTF_WARNED,
> IRQTF_AFFINITY,
> IRQTF_FORCED_THREAD,
> + IRQTF_UP,
> };
>
> /*
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index f1d5a94c6c9f..7efa24629694 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1263,6 +1263,30 @@ static void irq_wake_secondary(struct irq_desc *desc, struct irqaction *action)
> raw_spin_unlock_irq(&desc->lock);
> }
>
> +/*
> + * Internal function to notify that irq_thread is ready
> + */
> +static void irq_thread_is_up(struct irq_desc *desc,
> + struct irqaction *action)
nit again: the name of this function makes it look like a predicate.
The rest of the IRQ core uses the 'set' word to... set a bit.
Something like irq_thread_set_ready() would have my preference.
> +{
> + set_bit(IRQTF_UP, &action->thread_flags);
> + wake_up(&desc->wait_for_threads);
> +}
> +
> +/*
> + * Internal function to wake up irq_thread
> + * and wait until it is really up
> + */
> +static void wait_for_irq_thread_startup(struct irq_desc *desc,
> + struct irqaction *action)
and this would be wait_for_irq_thread_ready().
> +{
> + if (action && action->thread) {
> + wake_up_process(action->thread);
> + wait_event(desc->wait_for_threads,
> + test_bit(IRQTF_UP, &action->thread_flags));
> + }
> +}
> +
> /*
> * Interrupt handler thread
> */
> @@ -1287,6 +1311,8 @@ static int irq_thread(void *data)
>
> irq_thread_check_affinity(desc, action);
>
> + irq_thread_is_up (desc, action);
nit: extra space after the function.
> +
> while (!irq_wait_for_interrupt(action)) {
> irqreturn_t action_ret;
>
> @@ -1522,6 +1548,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> }
> }
>
> + init_waitqueue_head(&desc->wait_for_threads);
> +
I'm trying to convince myself that this one is safe.
It was so far only done when registering the first handler of a
threaded interrupt, while it is now done on every call to
__setup_irq(). However, this is now done outside of the protection of
any of the locks, meaning that a concurrent __setup_irq() for a shared
interrupt can now barge in and corrupt the wait queue.
So I don't think this is right. You may be able to hoist the
request_lock up, but I haven't checked what could break, if anything.
> /*
> * Create a handler thread when a thread function is supplied
> * and the interrupt does not nest into another interrupt
> @@ -1698,8 +1726,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> }
>
> 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,
> @@ -1795,14 +1821,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>
> 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);
> + wait_for_irq_thread_startup(desc, new);
> + wait_for_irq_thread_startup(desc, new->secondary);
>
> register_irq_proc(irq, desc);
> new->dir = NULL;
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Fri, Apr 29 2022 at 17:08, Marc Zyngier wrote:
> On Fri, 29 Apr 2022 12:52:48 +0100,
> Thomas Pfaff <[email protected]> wrote:
> +static void wait_for_irq_thread_startup(struct irq_desc *desc,
> + struct irqaction *action)
and this would be wait_for_irq_thread_ready().
which is sill a misnomer as this actually wakes and waits.
>> @@ -1522,6 +1548,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>> }
>> }
>>
>> + init_waitqueue_head(&desc->wait_for_threads);
>> +
>
> I'm trying to convince myself that this one is safe.
>
> It was so far only done when registering the first handler of a
> threaded interrupt, while it is now done on every call to
> __setup_irq(). However, this is now done outside of the protection of
> any of the locks, meaning that a concurrent __setup_irq() for a shared
> interrupt can now barge in and corrupt the wait queue.
>
> So I don't think this is right. You may be able to hoist the
> request_lock up, but I haven't checked what could break, if anything.
It can't be moved here, but I can see why Thomas wants to move it. With
a spurious wakeup of the irq thread (should not happen), the thread
would try to invoke wake_up() on a non initialize wait queue head.
Something like this should do the trick.
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 939d21cd55c3..0099b87dd853 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -407,6 +407,7 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
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);
@@ -575,6 +576,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();
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c03f71d5ec10..6a0942f4d068 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1683,8 +1683,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
}
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,
Thanks,
tglx