[PATCH v4 0/2] serial/amba-pl011: Activate TX IRQ passively (rework)

[PATCH v4 0/2] serial/amba-pl011: Activate TX IRQ passively (rework)

[Dave Martin]

This is a repost against tty-next only.

There is no code change compared with v3.

The only difference from v3 is rewording of an accidentally cloned
commit message, so that the message actually describes the patch.

For history, see
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333498.html
and earlier posts.

(This series sent through itself, via PPP on pl011.  Eeesh, I'd
forgotten what life was like in the dial-up era...)


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 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 2/2] serial/amba-pl011: Refactor and simplify TX FIFO handling

[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 Kici?ski for his input and similar patch.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 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

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

[Jakub Kiciński]

On Fri, 27 Mar 2015 14:59:31 +0000, Dave Martin wrote:
> 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 <Dave.Martin@arm.com>
> ---
>  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
<snip>
> @@ -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;

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.

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

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

[Dave P Martin]

[Resend -- apologies again for any duplicates received]

On Fri, Mar 27, 2015 at 05:40:55PM +0100, Jakub Kicinski wrote:
> On Fri, 27 Mar 2015 14:59:31 +0000, Dave Martin wrote:
> > 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 <Dave.Martin@arm.com>
> > ---
> >  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

[...]

> > -
> > -	/*
> > -	 * 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;
> 
> 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?

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

My main reason for refactoring this loop was to split up the complex
termination condition.  Possibly replacing from_irq with from_irq &&
prev_from_irq in the loop would do the trick, but only of this change
is really needed...


Can you see any other problems?

There's nothing wrong with additional patches going on top of this,
but I've been giving Greg the runaround and don't want to keep
respinning the whole thing unless really necessary...

Cheers
---Dave

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

[Jakub Kiciński]

On Fri, 27 Mar 2015 17:42:24 +0000, Dave P Martin wrote:
> [Resend -- apologies again for any duplicates received]
> 
> On Fri, Mar 27, 2015 at 05:40:55PM +0100, Jakub Kicinski wrote:
> > On Fri, 27 Mar 2015 14:59:31 +0000, Dave Martin wrote:
> > > 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 <Dave.Martin@arm.com>
> > > ---
> > >  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
> 
> [...]
> 
> > > -
> > > -	/*
> > > -	 * 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;
> > 
> > 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?

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.

 - 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 and proceeds even though FIFO is full.

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

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

[Dave Martin]

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

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

> On Fri, 27 Mar 2015 17:42:24 +0000, Dave P Martin wrote:
> > [Resend -- apologies again for any duplicates received]
> > 
> > On Fri, Mar 27, 2015 at 05:40:55PM +0100, Jakub Kicinski wrote:

[...]

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

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.

TXIS should never be spuriously _asserted_ (i.e., at the point when you
read it, if it's asserted then you know for sure the FIFO is half-empty).


Referring to your sequence:

>  - start_tx() runs holding the lock,

FIFO may be > 1/2 full

>  - IRQ fires and waits for the lock,

>  - start_tx() exists and releases the lock,

>  - IRQ handler grabs the lock [...]

FIFO may still be > 1/2 full.

pl011_int() reads the interrups status from MIS.

TXIS is always masked when DMA is active; apart from this, the FIFO is
only written with port->lock held.

So whatever status MIS reported, the FIFO can only have got emptier.

So, if MIS reported TXIS, the FIFO is definitely <= 1/2 full.
If MIS reported TXIS, the fifo may be (but does not have to be) > 1/2
full.

Provided TXIS is polled before calling pl011_tx_chars(... from_irq=true),
and port->lock is *not* released in between the poll and the call,
it should be safe to assume that the FIFO is at least half-empty.
On all other call paths, pl011_tx_chars is called with from_irq=false.


Did I go wrong somewhere?

Cheers
---Dave

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

[Jakub Kiciński]

On Mon, 30 Mar 2015 13:28:40 +0100, Dave Martin wrote:
> On Fri, Mar 27, 2015 at 07:10:40PM +0100, Jakub Kicinski wrote:
> 
> [Gaah -- resend again!  Looks like most recipients didn't get this :( ]
> 
> > On Fri, 27 Mar 2015 17:42:24 +0000, Dave P Martin wrote:
> > > [Resend -- apologies again for any duplicates received]
> > > 
> > > On Fri, Mar 27, 2015 at 05:40:55PM +0100, Jakub Kicinski wrote:
> 
> [...]
> 
> > > > 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?
> > 
> > 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...).
> 
> 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.

I missed that IRQ is cleared by writing data.

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

[Dave Martin]

On Mon, Mar 30, 2015 at 04:26:52PM +0200, Jakub Kicinski wrote:
> On Mon, 30 Mar 2015 13:28:40 +0100, Dave Martin wrote:

[...]

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

No worries, it took me a fair while to be sure of that myself.  The TRM
is not very clear on it.

Thanks for the careful review.

Cheers
---Dave

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

[Jakub Kiciński]

On Fri, 27 Mar 2015 14:59:31 +0000, Dave Martin wrote:
> 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 <Dave.Martin@arm.com>

Signed-off-by: Jakub Kicinski <kubakici@wp.pl>

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

[Dave Martin]

On Mon, Mar 30, 2015 at 04:28:11PM +0200, Jakub Kicinski wrote:
> On Fri, 27 Mar 2015 14:59:31 +0000, Dave Martin wrote:
> > 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 <Dave.Martin@arm.com>
> 
> Signed-off-by: Jakub Kicinski <kubakici@wp.pl>

Agreed, shoulda had that (sorry!)

Greg, can you add Jakub's S-o-B when applying, or do you want me to
repost?

Cheers
---Dave