| 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 |
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
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
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
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
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
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
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
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 --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; }
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(-)