Re: [PATCH] tty: serial: samsung_tty: remove spinlock flags in interrupt handlers

From: Johan Hovold <johan@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-kernel@vger.kernel.org, marcan@marcan.st, arnd@kernel.org,
	Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [PATCH] tty: serial: samsung_tty: remove spinlock flags in interrupt handlers
Date: Tue, 16 Mar 2021 10:56:54 +0100	[thread overview]
Message-ID: <YFCA5jFLV0Cu9YNe@hovoldconsulting.com> (raw)
In-Reply-To: <7f348e4c-3051-13cf-d461-eeda0ef53fdd@canonical.com>

On Tue, Mar 16, 2021 at 10:47:53AM +0100, Krzysztof Kozlowski wrote:
> On 16/03/2021 10:02, Johan Hovold wrote:
> > On Mon, Mar 15, 2021 at 07:12:12PM +0100, Krzysztof Kozlowski wrote:
> >> Since interrupt handler is called with disabled local interrupts, there
> >> is no need to use the spinlock primitives disabling interrupts as well.
> > 
> > This isn't generally true due to "threadirqs" and that can lead to
> > deadlocks if the console code is called from hard irq context.
> > 
> > Now, this is *not* the case for this particular driver since it doesn't
> > even bother to take the port lock in console_write(). That should
> > probably be fixed instead.
> > 
> > See https://lore.kernel.org/r/X7kviiRwuxvPxC8O@localhost.
> 
> Thanks for the link, quite interesting! For one type of device we have
> two interrupts (RX and TX) so I guess it's a valid point/risk. However
> let me try to understand it more.
> 
> Assuming we had only one interrupt line, how this interrupt handler with
> threadirqs could be called from hardirq context?

No, it's console_write() which can end up being called in hard irq
context and if that path takes the port lock after the now threaded
interrupt handler has been preempted you have a deadlock.

> You wrote there:
> > For console drivers this can even happen for the same interrupt as the
> > generic interrupt code can call printk(), and so can any other handler
> > that isn't threaded (e.g. hrtimers or explicit IRQF_NO_THREAD).
> 
> However I replaced here only interrupt handler's spin lock to non-irq.
> This code path will be executed only when interrupt is masked therefore
> for one interrupt line there is *no possibility of*:
> 
> -> s3c64xx_serial_handle_irq
>    - interrupts are masked
>    - s3c24xx_serial_tx_irq
>      - spin_lock()
>                        -> hrtimers or other IRQF_NO_THREAD
>                           - console_write() or something
>                             - s3c64xx_serial_handle_irq

You don't end up in s3c64xx_serial_handle_irq() here. It's just that
console_write() (typically) takes the port lock which is already held by
the preempted s3c24xx_serial_tx_irq().

>                               - s3c24xx_serial_tx_irq
>                                 - spin_lock()

Johan