Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 17:08:51 +0100
Message-ID: <87sfpv98j0.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Thomas Pfaff <tpfaff@pcs.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        <linux-kernel@vger.kernel.org>, <linux-rt-users@vger.kernel.org>,
        Hillf Danton <hdanton@sina.com>
Subject: Re: [PATCH v2] irq/core: synchronize irq_thread startup
In-Reply-To: <1e3f96b7-9294-1534-e83b-efe3602f876f@pcs.com>
References: <1e3f96b7-9294-1534-e83b-efe3602f876f@pcs.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

Hi Thomas,

Thanks for this, a few comments below.

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