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 12:25:09 +0100	[thread overview]
Message-ID: <YFCVldHS7CTf4j2f@hovoldconsulting.com> (raw)
In-Reply-To: <816834ba-8dc2-05cd-9c81-a11d65980cfd@canonical.com>

On Tue, Mar 16, 2021 at 11:11:43AM +0100, Krzysztof Kozlowski wrote:
> On 16/03/2021 10:56, Johan Hovold wrote:
> > 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.
> 
> Thanks, I understand now. I see three patterns shared by serial drivers:
> 
> 1. Do not take the lock in console_write() handler,
> 2. Take the lock like:
> if (port->sysrq)
>     locked = 0;
> else if (oops_in_progress)
>     locked = spin_trylock_irqsave(&port->lock, flags);
> else
>     spin_lock_irqsave(&port->lock, flags)
> 
> 3. Take the lock like above but preceded with local_irq_save().
> 
> It seems the choice of pattern depends which driver was used as a base.

Right, this is messy and we've been playing whack-a-mole with this for
years (as usual) it seems.

Some version of 2 above is probably what we want but the sysrq bits
aren't handled uniformly either (e.g. since 596f63da42b9 ("serial: 8250:
Process sysrq at port unlock time")).

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel