[RFC PATCH 0/2] serial: pl011: Fix TX dropping race

[RFC PATCH 0/2] serial: pl011: Fix TX dropping race

[Dave Martin]

When serial_core pushes some new TX chars via a call to
pl011_start_tx(), it can race with irqs triggered by ongoing
transmission, overfilling the FIFO and causing characters to be silently
dropped.

This was originally reported by Phil Elwell [1], who proposed an initial
fix.

This series aims for a simpler and more robust solution to the problem.

Any testing much appreciated!  If all looks good, I can repost this on
v5.3-rc1 when that arrives.


As noted in the patches, I'm not sure that the second patch is necessary
(or even desirable).  Please test both with and without the second
patch, and please comment if you have any thoughts on it :)


[1] [PATCH] tty: amba-pl011: Make TX optimisation conditional
http://lists.infradead.org/pipermail/linux-rpi-kernel/2019-July/008832.html


Dave Martin (2):
  serial: pl011: Fix dropping of TX chars due to irq race
  serial: pl011: Don't bother pushing more TX data while TX irq is
    active

 drivers/tty/serial/amba-pl011.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

-- 
2.1.4


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

[RFC PATCH 1/2] serial: pl011: Fix dropping of TX chars due to irq race

[Dave Martin]

When serial_core pushes some new TX chars via a call to
pl011_start_tx(), it can race with irqs triggered by ongoing
transmission.

Normally the port lock protects against this kind of thing, but
temporary releasing of the lock during calls from pl011_int() to
pl011_{,dma_}rx_chars() allows pl011_start_tx() to race.

For performance reasons, pl011_tx_chars(, true) always assumes that
the TX FIFO interrupt trigger condition holds, i.e., the FIFO is
empty to the trigger threshold.  This means that we can write chars
to fill the FIFO back up without the expense of polling the FIFO
fill status.  However, this assumes that no data is written to the
FIFO in the meantime by other code: this is where the race with
pl011_start_tx_pio() breaks things.

Reorder pl011_int() so that no code releases the port lock in
between reading the interrupt status bits and calling
pl011_tx_chars().  This ensures that TXIS in the fetched status
accurately reflects the state of the TX FIFO, and ensures that
there is no race to fill the FIFO.

Fixes: 1e84d22322ce ("serial/amba-pl011: Refactor and simplify TX FIFO handling")
Reported-by: Phil Elwell <phil@raspberrypi.org>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 drivers/tty/serial/amba-pl011.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 89ade21..e24bbc0 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1492,6 +1492,13 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 					       UART011_RXIS),
 				    uap, REG_ICR);
 
+			/*
+			 * Don't unlock uap->port.lock before here:
+			 * Stale TXIS status can lead to a FIFO overfill.
+			 */
+			if (status & UART011_TXIS)
+				pl011_tx_chars(uap, true);
+
 			if (status & (UART011_RTIS|UART011_RXIS)) {
 				if (pl011_dma_rx_running(uap))
 					pl011_dma_rx_irq(uap);
-- 
2.1.4


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

[RFC PATCH 2/2] serial: pl011: Don't bother pushing more TX data while TX irq is active

[Dave Martin]

When the TX irq is active, writing chars to the TX FIFO from
anywhere except pl011_int() is pointless: the UART is already busy,
and new chars will be picked up by pl011_int() as soon as there is
FIFO space.

To reduce the scope for surprises, bail out of pl011_start_tx_pio()
without attempting to write to the FIFO or start TX DMA if the TX FIFO
interrupt is already in use.

This should also avoid pointless overhead in some situations.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Please test both with and without this patch.

I believe with the previous patch in place, this patch is not strictly
necessary.  However, if the UART is actively transmitting in the
background already, it does make sense not to waste time trying polling
the FIFO fill status or setting up DMA etc.
---
 drivers/tty/serial/amba-pl011.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e24bbc0..f28935a 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1318,6 +1318,10 @@ static void pl011_start_tx(struct uart_port *port)
 	struct uart_amba_port *uap =
 	    container_of(port, struct uart_amba_port, port);
 
+	/* It's pointless to kick the UART if it's already transmitting... */
+	if (uap->im & UART011_TXIM)
+		return;
+
 	if (!pl011_dma_tx_start(uap))
 		pl011_start_tx_pio(uap);
 }
-- 
2.1.4


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