serial/amba-pl011: Activate TX IRQ passively (rework)

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

After some discussion [1], this turns out to be unnecessary.

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).

[1] Thanks to Jakub Kici?ski for his input and similar patch.

Signed-off-by Dave Martin <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |  119 +++++++++------------------------------
 1 file changed, 26 insertions(+), 93 deletions(-)

drivers/tty/serial/amba-pl011.c +0 -0

index 6f5a072..f5bd842 100644

drivers/tty/serial/amba-pl011.c +8 -30

@@ -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.
-		 */

drivers/tty/serial/amba-pl011.c +4 -6

 	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))

Quoted 22 lines · Dave Martin

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

After some discussion [1], this turns out to be unnecessary.

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).

[1] Thanks to Jakub Kicinski for his input and similar patch.

Signed-off-by Dave Martin <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |  119 +++++++++------------------------------
 1 file changed, 26 insertions(+), 93 deletions(-)

drivers/tty/serial/amba-pl011.c +0 -0

Quoted 7 lines

drivers/tty/serial/amba-pl011.c +0 -7

-
-	/*
-	 * 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))

drivers/tty/serial/amba-pl011.c __acquires(&uap->port.lock) · line 1262

Quoted 2 lines

drivers/tty/serial/amba-pl011.c +1 -1

-		return false;
+	int count = uap->fifosize >> 1;

Dave, I'd prefer if you kept my .prev_from_irq thing. If .start_tx() races with the IRQ we may have a situation where the IRQ arrives but .start_tx() already filled the FIFO. The guarantee of half of the FIFO being empty will not hold in this case. That's why I use the guarantee only if the previous load was also from FIFO.

Quoted 19 lines

drivers/tty/serial/amba-pl011.c +4 -3

 	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;

drivers/tty/serial/amba-pl011.c __acquires(&uap->port.lock) · line 1287

Quoted 13 lines

drivers/tty/serial/amba-pl011.c +8 -4

-	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 you add the .prev_* this loop will become even more ugly. Feel free to copy my code wherever you see fit.

[Resend -- apologies again for any duplicates received]

Quoted 23 lines · Jakub Kicinski

Quoted 22 lines · Dave Martin

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

After some discussion [1], this turns out to be unnecessary.

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).

[1] Thanks to Jakub Kicinski for his input and similar patch.

Signed-off-by Dave Martin <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |  119 +++++++++------------------------------
 1 file changed, 26 insertions(+), 93 deletions(-)

drivers/tty/serial/amba-pl011.c +0 -0

[...]

Quoted 7 lines

Quoted 7 lines

drivers/tty/serial/amba-pl011.c +0 -7

-
-	/*
-	 * 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))

Quoted 8 lines

drivers/tty/serial/amba-pl011.c __acquires(&uap->port.lock) · line 1262

Quoted 2 lines

drivers/tty/serial/amba-pl011.c +1 -1

-		return false;
+	int count = uap->fifosize >> 1;

Dave, I'd prefer if you kept my .prev_from_irq thing. If .start_tx() races with the IRQ we may have a situation where the IRQ arrives but .start_tx() already filled the FIFO. The guarantee of half of the FIFO being empty will not hold in this case. That's why I use the guarantee only if the previous load was also from FIFO.

I thought about this, but I think port->lock prevents this from happening. I was overly defensive about this in the earlier versions of the patches, and that made the code a fair bit more complicated... I was hoping it could just go away ;)

In pl011_int(), we have

-8<-

spin_lock_irqsave(&uap->port.lock, flags);

[...]

while (status = readw(uap->port.membase + UART012_MIS), status != 0) {

[...]

	if (status & UART011_TXIS)
		pl011_tx_chars(uap, true);

} while (status != 0);

[...]

spin_unlock_irqrestore(&uap->port.lock, flags);

->8-

serial_core always holds port->lock around _start_tx(), so it should be impossible for any part of _start_tx() to run in parallel with this. If TXIS is asserted and nothing can write the FIFO in the meantime, then that should mean that the FIFO is definitely half-empty on entry to pl011_tx_chars(..., from_irq=true) -- this is also why it's safe for pl011_int() to loop and potentially make repeated calls to pl011_tx_chars().

Can you see a way this could break, or does this reasoning sound good to you?

Quoted 3 lines

[Resend -- apologies again for any duplicates received]

On Fri, Mar 27, 2015 at 05:40:55PM +0100, Jakub Kicinski wrote:

Quoted 44 lines

Quoted 5 lines

Dave, I'd prefer if you kept my .prev_from_irq thing. If .start_tx() races with the IRQ we may have a situation where the IRQ arrives but .start_tx() already filled the FIFO. The guarantee of half of the FIFO being empty will not hold in this case. That's why I use the guarantee only if the previous load was also from FIFO.

I thought about this, but I think port->lock prevents this from happening. I was overly defensive about this in the earlier versions of the patches, and that made the code a fair bit more complicated... I was hoping it could just go away ;)

In pl011_int(), we have

-8<-

spin_lock_irqsave(&uap->port.lock, flags);

[...]

while (status = readw(uap->port.membase + UART012_MIS), status != 0) {

[...]

	if (status & UART011_TXIS)
		pl011_tx_chars(uap, true);

} while (status != 0);

[...]

spin_unlock_irqrestore(&uap->port.lock, flags);

->8-

serial_core always holds port->lock around _start_tx(), so it should be impossible for any part of _start_tx() to run in parallel with this. If TXIS is asserted and nothing can write the FIFO in the meantime, then that should mean that the FIFO is definitely half-empty on entry to pl011_tx_chars(..., from_irq=true) -- this is also why it's safe for pl011_int() to loop and potentially make repeated calls to pl011_tx_chars().

Quoted 2 lines

Can you see a way this could break, or does this reasoning sound good to you?

It doesn't have to run in parallel, perhaps using the word "race" was not entirely justified on my side. Sorry if my English is not super-clear ;) Even when the accesses are serialized the problem remains.

I think this would require some adverse condition to trigger (high load
+ high baudrate) or IRQ pending during first unmask (which I think shouldn't happen, but hey, let's not trust HW too much...).

- start_tx() runs holding the lock,

- IRQ fires and waits for the lock,

- start_tx() exists and releases the lock,

- IRQ handler grabs the lock [...]

On Fri, Mar 27, 2015 at 07:10:40PM +0100, Jakub Kicinski wrote:

[Gaah -- resend again! Looks like most recipients didn't get this :( ]

Quoted 4 lines · Dave P Martin

Quoted 3 lines

[Resend -- apologies again for any duplicates received]

On Fri, Mar 27, 2015 at 05:40:55PM +0100, Jakub Kicinski wrote:

[...]

Quoted 51 lines

Quoted 44 lines

Quoted 5 lines

Dave, I'd prefer if you kept my .prev_from_irq thing. If .start_tx() races with the IRQ we may have a situation where the IRQ arrives but .start_tx() already filled the FIFO. The guarantee of half of the FIFO being empty will not hold in this case. That's why I use the guarantee only if the previous load was also from FIFO.

I thought about this, but I think port->lock prevents this from happening. I was overly defensive about this in the earlier versions of the patches, and that made the code a fair bit more complicated... I was hoping it could just go away ;)

In pl011_int(), we have

-8<-

spin_lock_irqsave(&uap->port.lock, flags);

[...]

while (status = readw(uap->port.membase + UART012_MIS), status != 0) {

[...]

	if (status & UART011_TXIS)
		pl011_tx_chars(uap, true);

} while (status != 0);

[...]

spin_unlock_irqrestore(&uap->port.lock, flags);

->8-

serial_core always holds port->lock around _start_tx(), so it should be impossible for any part of _start_tx() to run in parallel with this. If TXIS is asserted and nothing can write the FIFO in the meantime, then that should mean that the FIFO is definitely half-empty on entry to pl011_tx_chars(..., from_irq=true) -- this is also why it's safe for pl011_int() to loop and potentially make repeated calls to pl011_tx_chars().

Quoted 2 lines

Can you see a way this could break, or does this reasoning sound good to you?

It doesn't have to run in parallel, perhaps using the word "race" was not entirely justified on my side. Sorry if my English is not super-clear ;) Even when the accesses are serialized the problem remains.

[...]

Quoted 3 lines

I think this would require some adverse condition to trigger (high load
+ high baudrate) or IRQ pending during first unmask (which I think shouldn't happen, but hey, let's not trust HW too much...).

Ah, I see where you're coming from.

TXIS reflects the live status of the FIFO, except that it is "spuriously" deasserted betweem reset/clear of the interrupt and the first TX IRQ, even though the FIFO may be empty.

On Mon, 30 Mar 2015 13:28:40 +0100, Dave Martin wrote:

Quoted 3 lines

TXIS reflects the live status of the FIFO, except that it is "spuriously" deasserted betweem reset/clear of the interrupt and the first TX IRQ, even though the FIFO may be empty.

I missed that IRQ is cleared by writing data.

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

After some discussion [1], this turns out to be unnecessary.

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).

[1] Thanks to Jakub Kici?ski for his input and similar patch.

Signed-off-by Dave Martin <[email protected]>

Quoted 1 line · Dave Martin

To this end, the TX softirq mechanism is removed and instead

Quoted 7 lines

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).

[1] Thanks to Jakub Kicinski for his input and similar patch.

Signed-off-by Dave Martin <[email protected]>

Signed-off-by Jakub Kicinski <[email protected]>