diff mbox series

tty: serial: samsung_tty: remove spinlock flags in interrupt handlers

Message ID 20210315181212.113217-1-krzysztof.kozlowski@canonical.com
State New
Headers show
Series tty: serial: samsung_tty: remove spinlock flags in interrupt handlers | expand

Commit Message

Krzysztof Kozlowski March 15, 2021, 6:12 p.m. UTC
Since interrupt handler is called with disabled local interrupts, there
is no need to use the spinlock primitives disabling interrupts as well.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 drivers/tty/serial/samsung_tty.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Johan Hovold March 16, 2021, 9:02 a.m. UTC | #1
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.

Johan
Andy Shevchenko March 16, 2021, 9:40 a.m. UTC | #2
On Tue, Mar 16, 2021 at 11:02 AM Johan Hovold <johan@kernel.org> 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.

Finn, Barry, something to check I think?

--
With Best Regards,
Andy Shevchenko
Krzysztof Kozlowski March 16, 2021, 9:47 a.m. UTC | #3
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?

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
                              - s3c24xx_serial_tx_irq
                                - spin_lock()


Best regards,
Krzysztof
Johan Hovold March 16, 2021, 9:56 a.m. UTC | #4
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
Krzysztof Kozlowski March 16, 2021, 10:11 a.m. UTC | #5
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.

Best regards,
Krzysztof
Johan Hovold March 16, 2021, 11:25 a.m. UTC | #6
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
Johan Hovold March 19, 2021, 8:10 a.m. UTC | #7
On Fri, Mar 19, 2021 at 06:36:39AM +0000, Song Bao Hua (Barry Song) wrote:
> 

> 

> > -----Original Message-----

> > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> > Sent: Tuesday, March 16, 2021 10:41 PM

> > To: Johan Hovold <johan@kernel.org>; Finn Thain <fthain@telegraphics.com.au>;

> > Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>

> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>; Greg

> > Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>;

> > linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>; Linux Samsung

> > SOC <linux-samsung-soc@vger.kernel.org>; open list:SERIAL DRIVERS

> > <linux-serial@vger.kernel.org>; Linux Kernel Mailing List

> > <linux-kernel@vger.kernel.org>; Hector Martin <marcan@marcan.st>; Arnd

> > Bergmann <arnd@kernel.org>

> > Subject: Re: [PATCH] tty: serial: samsung_tty: remove spinlock flags in

> > interrupt handlers

> > 

> > On Tue, Mar 16, 2021 at 11:02 AM Johan Hovold <johan@kernel.org> 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.

> > 

> > Finn, Barry, something to check I think?

> 

> My understanding is that spin_lock_irqsave can't protect the context

> the console_write() is called in hardirq for threaded_irq case mainly

> for preempt-rt scenarios as spin_lock_irqsave doesn't disable irq in

> that case at all.


Forced threaded interrupts have so far run with interrupts enabled and
spin_lock_irqsave() would suffice on non-RT. This is about to change
though so that drivers don't need to worry about "threadirqs":

	https://lore.kernel.org/r/20210317143859.513307808@linutronix.de

> See:

> https://www.kernel.org/doc/html/latest/locking/locktypes.html

> spinlock_t and PREEMPT_RT

> On a PREEMPT_RT kernel spinlock_t is mapped to a separate implementation

> based on rt_mutex which changes the semantics:

> Preemption is not disabled.

> The hard interrupt related suffixes for spin_lock / spin_unlock operations

> (_irq, _irqsave / _irqrestore) do not affect the CPU’s interrupt disabled

> state.

> 

> So if console_write() can interrupt our code in hardirq, we should

> move to raw_spin_lock_irqsave for this driver.


No, no. RT handles this by deferring console writes apparently.

> I think it is almost always wrong to call spin_lock_irqsave in hardirq.


Again, no. It's even been a requirement due to "threadirqs" in some
cases (e.g. hrtimers) up until now (or rather until the above patch is
in mainline).

Johan
Johan Hovold March 22, 2021, 11:23 a.m. UTC | #8
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.

> 

> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Interrupts are now disabled also with forced interrupt threading even if
this never was an issue for this driver which currently doesn't take the
port lock in the console paths.

Reviewed-by: Johan Hovold <johan@kernel.org>


Johan
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 80df842bf4c7..d9e4b67a12a0 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -715,13 +715,12 @@  static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 	struct s3c24xx_uart_dma *dma = ourport->dma;
 	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
 	struct tty_port *t = &port->state->port;
-	unsigned long flags;
 	struct dma_tx_state state;
 
 	utrstat = rd_regl(port, S3C2410_UTRSTAT);
 	rd_regl(port, S3C2410_UFSTAT);
 
-	spin_lock_irqsave(&port->lock, flags);
+	spin_lock(&port->lock);
 
 	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
 		s3c64xx_start_rx_dma(ourport);
@@ -750,7 +749,7 @@  static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 	wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
 
 finish:
-	spin_unlock_irqrestore(&port->lock, flags);
+	spin_unlock(&port->lock);
 
 	return IRQ_HANDLED;
 }
@@ -846,11 +845,10 @@  static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 {
 	struct s3c24xx_uart_port *ourport = dev_id;
 	struct uart_port *port = &ourport->port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
+	spin_lock(&port->lock);
 	s3c24xx_serial_rx_drain_fifo(ourport);
-	spin_unlock_irqrestore(&port->lock, flags);
+	spin_unlock(&port->lock);
 
 	return IRQ_HANDLED;
 }
@@ -934,13 +932,12 @@  static irqreturn_t s3c24xx_serial_tx_irq(int irq, void *id)
 {
 	struct s3c24xx_uart_port *ourport = id;
 	struct uart_port *port = &ourport->port;
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
+	spin_lock(&port->lock);
 
 	s3c24xx_serial_tx_chars(ourport);
 
-	spin_unlock_irqrestore(&port->lock, flags);
+	spin_unlock(&port->lock);
 	return IRQ_HANDLED;
 }