Message-ID: <87sfpv98j0.wl-maz@kernel.org>
Date:   Fri, 29 Apr 2022 17:08:51 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Thomas Pfaff <tpfaff@....com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        <linux-kernel@...r.kernel.org>, <linux-rt-users@...r.kernel.org>,
        Hillf Danton <hdanton@...a.com>
Subject: Re: [PATCH v2] irq/core: synchronize irq_thread startup

Hi Thomas,

Thanks for this, a few comments below.

On Fri, 29 Apr 2022 12:52:48 +0100,
Thomas Pfaff <tpfaff@....com> wrote:
> 
> From: Thomas Pfaff <tpfaff@....com>
> 
> 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 <tpfaff@....com>
> ---
> 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.

Powered by blists - more mailing lists