| From 76e24a2218069fadb28c0c2f5d813302ad5f85a3 Mon Sep 17 00:00:00 2001 |
| From: Phil Elwell <phil@raspberrypi.org> |
| Date: Thu, 11 Jul 2019 13:13:39 +0100 |
| Subject: [PATCH] tty: amba-pl011: Make TX optimisation conditional |
| |
| pl011_tx_chars takes a "from_irq" parameter to reduce the number of |
| register accesses. When from_irq is true the function assumes that the |
| FIFO is half empty and writes up to half a FIFO's worth of bytes |
| without polling the FIFO status register, the reasoning being that |
| the function is being called as a result of the TX interrupt being |
| raised. This logic would work were it not for the fact that |
| pl011_rx_chars, called from pl011_int before pl011_tx_chars, releases |
| the spinlock before calling tty_flip_buffer_push. |
| |
| A user thread writing to the UART claims the spinlock and ultimately |
| calls pl011_tx_chars with from_irq set to false. This reverts to the |
| older logic that polls the FIFO status register before sending every |
| byte. If this happen on an SMP system during the section of the IRQ |
| handler where the spinlock has been released, then by the time the TX |
| interrupt handler is called, the FIFO may already be full, and any |
| further writes are likely to be lost. |
| |
| The fix involves adding a per-port flag that is true iff running from |
| within the interrupt handler and the spinlock has not yet been released. |
| This flag is then used as the value for the from_irq parameter of |
| pl011_tx_chars, causing polling to be used in the unsafe case. |
| |
| Fixes: 1e84d22322ce ("serial/amba-pl011: Refactor and simplify TX FIFO handling") |
| |
| Signed-off-by: Phil Elwell <phil@raspberrypi.org> |
| --- |
| drivers/tty/serial/amba-pl011.c | 7 ++++++- |
| 1 file changed, 6 insertions(+), 1 deletion(-) |
| |
| --- a/drivers/tty/serial/amba-pl011.c |
| +++ b/drivers/tty/serial/amba-pl011.c |
| @@ -270,6 +270,7 @@ struct uart_amba_port { |
| unsigned int old_cr; /* state during shutdown */ |
| unsigned int fixed_baud; /* vendor-set fixed baud rate */ |
| char type[12]; |
| + bool irq_locked; /* in irq, unreleased lock */ |
| #ifdef CONFIG_DMA_ENGINE |
| /* DMA stuff */ |
| bool using_tx_dma; |
| @@ -816,6 +817,7 @@ __acquires(&uap->port.lock) |
| if (!uap->using_tx_dma) |
| return; |
| |
| + uap->irq_locked = 0; |
| dmaengine_terminate_async(uap->dmatx.chan); |
| |
| if (uap->dmatx.queued) { |
| @@ -942,6 +944,7 @@ static void pl011_dma_rx_chars(struct ua |
| fifotaken = pl011_fifo_to_tty(uap); |
| } |
| |
| + uap->irq_locked = 0; |
| spin_unlock(&uap->port.lock); |
| dev_vdbg(uap->port.dev, |
| "Took %d chars from DMA buffer and %d chars from the FIFO\n", |
| @@ -1362,6 +1365,7 @@ __acquires(&uap->port.lock) |
| { |
| pl011_fifo_to_tty(uap); |
| |
| + uap->irq_locked = 0; |
| spin_unlock(&uap->port.lock); |
| tty_flip_buffer_push(&uap->port.state->port); |
| /* |
| @@ -1497,6 +1501,7 @@ static irqreturn_t pl011_int(int irq, vo |
| int handled = 0; |
| |
| spin_lock_irqsave(&uap->port.lock, flags); |
| + uap->irq_locked = 1; |
| status = pl011_read(uap, REG_RIS) & uap->im; |
| if (status) { |
| do { |
| @@ -1516,7 +1521,7 @@ static irqreturn_t pl011_int(int irq, vo |
| UART011_CTSMIS|UART011_RIMIS)) |
| pl011_modem_status(uap); |
| if (status & UART011_TXIS) |
| - pl011_tx_chars(uap, true); |
| + pl011_tx_chars(uap, uap->irq_locked); |
| |
| if (pass_counter-- == 0) |
| break; |