Re: [PATCH v3] irq/core: synchronize irq_thread startup

From: Thomas Pfaff <tpfaff@pcs.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: <linux-kernel@vger.kernel.org>, <linux-rt-users@vger.kernel.org>
Subject: Re: [PATCH v3] irq/core: synchronize irq_thread startup
Date: Tue, 10 May 2022 10:43:15 +0200	[thread overview]
Message-ID: <e64aa9cb-a1ae-7534-8bf1-446d9d8c512@pcs.com> (raw)
In-Reply-To: <87mtg0m2jb.ffs@tglx>

On Mon, 2 May 2022, Thomas Gleixner wrote:

> On Mon, May 02 2022 at 13:28, Thomas Pfaff wrote:
> > 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.
> 
> This comment made me look deeper because I expected that free_irq()
> would hang.
> 
> But free_irq() stopped issuing synchronize_irq() with commit
> 519cc8652b3a ("genirq: Synchronize only with single thread on
> free_irq()"). And that turns out to be the root cause of the problem.
> I should have caught that back then, but in hindsight ....
> 

Sorry for coming back to this again late, but this makes me believe that 
the real problem for the freeze in setserial is that uart_port_shutdown() 
is calling synchronize_irq() after free_irq(), which is illegal in my 
opinion.

It can be done only before the interrupt thread is stopped, and free_irq() 
itself is already taking care about synchronizing, no matter if its done by 
__synchronize_hardirq() or by synchronize_irq(), like it was before commit 
519cc8652b3a.
If it is called after free_irq(), the context is already lost.

I am not sure about all the other drivers, but at least serial_core should 
be fixed if you agree.

Thanks,
    Thomas