[PATCH v4 REPOST 0/2] serial/amba-pl011: Refactor and simplify TX FIFO handling

[PATCH v4 REPOST 0/2] serial/amba-pl011: Refactor and simplify TX FIFO handling

[Dave Martin]

Repost against v4.1-rc1.

This fixes a couple of known bugs.

 * The "Leave the TX IRQ alone when the UART is open" optimisation
   turned out to be a source of bugs because serial_core does shut down
   the UART with the FIFO still non-empty in certain situations -- this
   series simply reverts that patch.

 * The sortirq based mechanism was complex and still suffered from some
   race conditions which could lead to misbehaviour.  This series
   simplifies the mechanism to resolve these races.


No material changes from v2-v4.
Changes since v2 (= what is in -rc1):

 * Remove the optimisation that assumes TXIS stays asserted across port
   shutdown..startup (thanks to Jakub Kici?ski for spotting this on
   raspberrypi-2).  This turns out to be untrue in certain situations,
   namely when the only process with the port open is killed by a
   signal.  This may indicate a bug in serial_core, but the "correct"
   behaviour for this case is not completely obvious.

   For this repost, I just revert the optimisation so that the pl011
   driver doesn't hang in this situation.  It didn't bring much real
   benefit in any case.

 * Remove the tx softirq and rework the code for more intelligible
   code flow.

 * pl011_start_tx() now simply stuffs the FIFO until it's full (in which
   case the TX IRQ will definitely fire), or until the serial_core
   circular buffer is empty (in which case we don't care about the
   interrupt because there is nothing more to send).

Dave Martin (2):
  Revert "serial/amba-pl011: Leave the TX IRQ alone when the UART is
    not open"
  serial/amba-pl011: Refactor and simplify TX FIFO handling

 drivers/tty/serial/amba-pl011.c |  118 +++++++++------------------------------
 1 file changed, 27 insertions(+), 91 deletions(-)

-- 
1.7.10.4

[PATCH v4 REPOST 1/2] Revert "serial/amba-pl011: Leave the TX IRQ alone when the UART is not open"

[Dave Martin]

This reverts commit f2ee6dfa0e8597eea8b98d240b0033994e20d215.

Jakub Kici?ski observed that this patch can cause the pl011
driver to hang if if the only process with a pl011 port open is
killed by a signal, pl011_shutdown() can get called with an
arbitrary amount of data still in the FIFO.

Calling _shutdown() with the TX FIFO non-empty is questionable
behaviour and my itself be a bug.

Since the affected patch was speculative anyway, and brings limited
benefit, the simplest course is to remove the assumption that TXIS
will always be left asserted after the port is shut down.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 drivers/tty/serial/amba-pl011.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 5a4e9d5..6f5a072 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1639,6 +1639,9 @@ static int pl011_startup(struct uart_port *port)
 
 	writew(uap->vendor->ifls, uap->port.membase + UART011_IFLS);
 
+	/* Assume that TX IRQ doesn't work until we see one: */
+	uap->tx_irq_seen = 0;
+
 	spin_lock_irq(&uap->port.lock);
 
 	/* restore RTS and DTR */
@@ -1702,7 +1705,7 @@ static void pl011_shutdown(struct uart_port *port)
 	spin_lock_irq(&uap->port.lock);
 	uap->im = 0;
 	writew(uap->im, uap->port.membase + UART011_IMSC);
-	writew(0xffff & ~UART011_TXIS, uap->port.membase + UART011_ICR);
+	writew(0xffff, uap->port.membase + UART011_ICR);
 	spin_unlock_irq(&uap->port.lock);
 
 	pl011_dma_shutdown(uap);
-- 
1.7.10.4

[PATCH v4 REPOST 2/2] serial/amba-pl011: Refactor and simplify TX FIFO handling

[Dave Martin]

Commit 734745c serial/amba-pl011: Activate TX IRQ passively
adds some complexity and overhead in the form of a softirq
mechanism for transmitting in the absence of interrupts.

This patch simplifies the code flow to reduce the reliance on
subtle behaviour and avoid fragility under future maintenance.

To this end, the TX softirq mechanism is removed and instead
pl011_start_tx() will now simply stuff the FIFO until full
(guaranteeing future TX IRQs), or until there are no more chars
to write (in which case we don't care whether an IRQ happens).

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
 drivers/tty/serial/amba-pl011.c |  119 +++++++++------------------------------
 1 file changed, 26 insertions(+), 93 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6f5a072..f5bd842 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -58,7 +58,6 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/sizes.h>
 #include <linux/io.h>
-#include <linux/workqueue.h>
 
 #define UART_NR			14
 
@@ -157,9 +156,7 @@ struct uart_amba_port {
 	unsigned int		lcrh_tx;	/* vendor-specific */
 	unsigned int		lcrh_rx;	/* vendor-specific */
 	unsigned int		old_cr;		/* state during shutdown */
-	struct delayed_work	tx_softirq_work;
 	bool			autorts;
-	unsigned int		tx_irq_seen;	/* 0=none, 1=1, 2=2 or more */
 	char			type[12];
 #ifdef CONFIG_DMA_ENGINE
 	/* DMA stuff */
@@ -1172,15 +1169,14 @@ static void pl011_stop_tx(struct uart_port *port)
 	pl011_dma_tx_stop(uap);
 }
 
-static bool pl011_tx_chars(struct uart_amba_port *uap);
+static void pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
 
 /* Start TX with programmed I/O only (no DMA) */
 static void pl011_start_tx_pio(struct uart_amba_port *uap)
 {
 	uap->im |= UART011_TXIM;
 	writew(uap->im, uap->port.membase + UART011_IMSC);
-	if (!uap->tx_irq_seen)
-		pl011_tx_chars(uap);
+	pl011_tx_chars(uap, false);
 }
 
 static void pl011_start_tx(struct uart_port *port)
@@ -1247,87 +1243,54 @@ __acquires(&uap->port.lock)
 	spin_lock(&uap->port.lock);
 }
 
-/*
- * Transmit a character
- * There must be at least one free entry in the TX FIFO to accept the char.
- *
- * Returns true if the FIFO might have space in it afterwards;
- * returns false if the FIFO definitely became full.
- */
-static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c)
+static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
+			  bool from_irq)
 {
+	if (unlikely(!from_irq) &&
+	    readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF)
+		return false; /* unable to transmit character */
+
 	writew(c, uap->port.membase + UART01x_DR);
 	uap->port.icount.tx++;
 
-	if (likely(uap->tx_irq_seen > 1))
-		return true;
-
-	return !(readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF);
+	return true;
 }
 
-static bool pl011_tx_chars(struct uart_amba_port *uap)
+static void pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
 {
 	struct circ_buf *xmit = &uap->port.state->xmit;
-	int count;
-
-	if (unlikely(uap->tx_irq_seen < 2))
-		/*
-		 * Initial FIFO fill level unknown: we must check TXFF
-		 * after each write, so just try to fill up the FIFO.
-		 */
-		count = uap->fifosize;
-	else /* tx_irq_seen >= 2 */
-		/*
-		 * FIFO initially at least half-empty, so we can simply
-		 * write half the FIFO without polling TXFF.
-
-		 * Note: the *first* TX IRQ can still race with
-		 * pl011_start_tx_pio(), which can result in the FIFO
-		 * being fuller than expected in that case.
-		 */
-		count = uap->fifosize >> 1;
-
-	/*
-	 * If the FIFO is full we're guaranteed a TX IRQ at some later point,
-	 * and can't transmit immediately in any case:
-	 */
-	if (unlikely(uap->tx_irq_seen < 2 &&
-		     readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF))
-		return false;
+	int count = uap->fifosize >> 1;
 
 	if (uap->port.x_char) {
-		pl011_tx_char(uap, uap->port.x_char);
+		if (!pl011_tx_char(uap, uap->port.x_char, from_irq))
+			return;
 		uap->port.x_char = 0;
 		--count;
 	}
 	if (uart_circ_empty(xmit) || uart_tx_stopped(&uap->port)) {
 		pl011_stop_tx(&uap->port);
-		goto done;
+		return;
 	}
 
 	/* If we are using DMA mode, try to send some characters. */
 	if (pl011_dma_tx_irq(uap))
-		goto done;
+		return;
 
-	while (count-- > 0 && pl011_tx_char(uap, xmit->buf[xmit->tail])) {
-		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
-		if (uart_circ_empty(xmit))
+	do {
+		if (likely(from_irq) && count-- == 0)
 			break;
-	}
+
+		if (!pl011_tx_char(uap, xmit->buf[xmit->tail], from_irq))
+			break;
+
+		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+	} while (!uart_circ_empty(xmit));
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&uap->port);
 
-	if (uart_circ_empty(xmit)) {
+	if (uart_circ_empty(xmit))
 		pl011_stop_tx(&uap->port);
-		goto done;
-	}
-
-	if (unlikely(!uap->tx_irq_seen))
-		schedule_delayed_work(&uap->tx_softirq_work, uap->port.timeout);
-
-done:
-	return false;
 }
 
 static void pl011_modem_status(struct uart_amba_port *uap)
@@ -1354,28 +1317,6 @@ static void pl011_modem_status(struct uart_amba_port *uap)
 	wake_up_interruptible(&uap->port.state->port.delta_msr_wait);
 }
 
-static void pl011_tx_softirq(struct work_struct *work)
-{
-	struct delayed_work *dwork = to_delayed_work(work);
-	struct uart_amba_port *uap =
-		container_of(dwork, struct uart_amba_port, tx_softirq_work);
-
-	spin_lock(&uap->port.lock);
-	while (pl011_tx_chars(uap)) ;
-	spin_unlock(&uap->port.lock);
-}
-
-static void pl011_tx_irq_seen(struct uart_amba_port *uap)
-{
-	if (likely(uap->tx_irq_seen > 1))
-		return;
-
-	uap->tx_irq_seen++;
-	if (uap->tx_irq_seen < 2)
-		/* first TX IRQ */
-		cancel_delayed_work(&uap->tx_softirq_work);
-}
-
 static irqreturn_t pl011_int(int irq, void *dev_id)
 {
 	struct uart_amba_port *uap = dev_id;
@@ -1414,10 +1355,8 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 			if (status & (UART011_DSRMIS|UART011_DCDMIS|
 				      UART011_CTSMIS|UART011_RIMIS))
 				pl011_modem_status(uap);
-			if (status & UART011_TXIS) {
-				pl011_tx_irq_seen(uap);
-				pl011_tx_chars(uap);
-			}
+			if (status & UART011_TXIS)
+				pl011_tx_chars(uap, true);
 
 			if (pass_counter-- == 0)
 				break;
@@ -1639,9 +1578,6 @@ static int pl011_startup(struct uart_port *port)
 
 	writew(uap->vendor->ifls, uap->port.membase + UART011_IFLS);
 
-	/* Assume that TX IRQ doesn't work until we see one: */
-	uap->tx_irq_seen = 0;
-
 	spin_lock_irq(&uap->port.lock);
 
 	/* restore RTS and DTR */
@@ -1697,8 +1633,6 @@ static void pl011_shutdown(struct uart_port *port)
 	    container_of(port, struct uart_amba_port, port);
 	unsigned int cr;
 
-	cancel_delayed_work_sync(&uap->tx_softirq_work);
-
 	/*
 	 * disable all interrupts
 	 */
@@ -2245,7 +2179,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->port.ops = &amba_pl011_pops;
 	uap->port.flags = UPF_BOOT_AUTOCONF;
 	uap->port.line = i;
-	INIT_DELAYED_WORK(&uap->tx_softirq_work, pl011_tx_softirq);
 
 	/* Ensure interrupts from this UART are masked and cleared */
 	writew(0, uap->port.membase + UART011_IMSC);
-- 
1.7.10.4