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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Thomas Pfaff <tpfaff@pcs.com>, linux-kernel@vger.kernel.org
Cc:     linux-rt-users@vger.kernel.org, Marc Zyngier <maz@kernel.org>
Subject: Re: [PATCH] irq: synchronize irq_thread startup
In-Reply-To: <f18798-9e66-66e7-8c9d-cb8eeda364c4@pcs.com>
References: <f18798-9e66-66e7-8c9d-cb8eeda364c4@pcs.com>
Date:   Thu, 28 Apr 2022 17:42:55 +0200
Message-ID: <87r15huscg.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Thomas,

On Thu, Apr 28 2022 at 12:49, Thomas Pfaff wrote:

please always Cc maintainers when sending patches and please also have a
look at:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

> 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().

Right. Nice detective work.

> @@ -127,6 +129,8 @@ struct irqaction {
>  	unsigned int		flags;
>  	unsigned long		thread_flags;
>  	unsigned long		thread_mask;
> +	atomic_t		thread_is_up;
> +	wait_queue_head_t	wait_for_thread;

I don't think we need any of this extra ballast per irqaction.

irqaction::thread_flags is already there and can be used as indicator.
There are enough bits left.

irq_desc already has a wait queue, which is used for synchronize_irq()
to wait for threads to complete. That wait queue is unused at the point
where an interrupt is set up, so there is no reason to introduce a new
one which is only used once.

This is not a problem in the rare case that the interrupt fires and
wakes the thread which then in consequence might wake the waiter via
wake_threads_waitq().

> -	if (new->thread)
> +	if (new->thread) {
>  		wake_up_process(new->thread);
> -	if (new->secondary)
> +		wait_event(new->wait_for_thread,
> +			   atomic_read(&new->thread_is_up));

Please stick this into a helper function.

Thanks,

        tglx