Linux 3.15: SPARC serial console regression

Linux 3.15: SPARC serial console regression

[Aaro Koskinen]

Hi,

While testing the final 3.15, I noticed the serial console (sunsab)
does not work anymore on Sun Ultra.

It will tx fine the console output during the boot. But then I try
to type something e.g. at the login: prompt, it behaves oddly:
it re-transmits some old output, and then stalls for a few secs, usually
for each character. Typed characters are however received correctly.
But the console is impossible to use...

I bisected this to:

717f3bbab3c7628736ef738fdbf3d9a28578c26c is the first bad commit
commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c
Author: Seth Bollinger <sethb@digi.com>
Date:   Tue Mar 25 12:55:37 2014 -0500

    serial_core: Fix conditional start_tx on ring buffer not empty

A.

On 06/09/2014 04:59 PM, Aaro Koskinen wrote:> Hi,

[Peter Hurley]

Thanks for the report Aaro.

Looks like Seth's fix exposes some broken assumptions in the Sun
serial drivers.

Can you test the patch below?

Regards,
Peter Hurley

PS - Do you have a way to test also your setup using hardware flow
control, ie. CRTSCTS?  I ask because I would expect that to be broken
even before Seth's patch.

--- %< ---
From: Peter Hurley <peter@hurleysoftware.com>
Date: Mon, 9 Jun 2014 19:21:43 -0400
Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart

Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
'serial_core: Fix conditional start_tx on ring buffer not empty'
exposes an incorrect assumption in sunsab's start_tx method; the
tx ring buffer can, in fact, be empty when restarting tx when
performing flow control.

Test for empty tx ring buffer when in sunsab's start_tx method.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/sunsab.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
index 5faa8e9..b99a4ea 100644
--- a/drivers/tty/serial/sunsab.c
+++ b/drivers/tty/serial/sunsab.c
@@ -427,6 +427,9 @@ static void sunsab_start_tx(struct uart_port *port)
 	struct circ_buf *xmit = &up->port.state->xmit;
 	int i;
 
+	if (uart_circ_empty(xmit))
+		return;
+
 	up->interrupt_mask1 &= ~(SAB82532_IMR1_ALLS|SAB82532_IMR1_XPR);
 	writeb(up->interrupt_mask1, &up->regs->w.imr1);
 	
-- 
2.0.0

FW: Linux 3.15: SPARC serial console regression

[Peter Hurley]

On 06/09/2014 04:59 PM, Aaro Koskinen wrote:> Hi,
> 
> While testing the final 3.15, I noticed the serial console (sunsab)
> does not work anymore on Sun Ultra.
> 
> It will tx fine the console output during the boot. But then I try
> to type something e.g. at the login: prompt, it behaves oddly:
> it re-transmits some old output, and then stalls for a few secs, usually
> for each character. Typed characters are however received correctly.
> But the console is impossible to use...
> 
> I bisected this to:
> 
> 717f3bbab3c7628736ef738fdbf3d9a28578c26c is the first bad commit
> commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c
> Author: Seth Bollinger <sethb@digi.com>
> Date:   Tue Mar 25 12:55:37 2014 -0500
> 
>      serial_core: Fix conditional start_tx on ring buffer not empty

[Let's try that again. This time without the mess]

Thanks for the report Aaro.

Looks like Seth's fix exposes some broken assumptions in the Sun
serial drivers.

Can you test the patch below?

Regards,
Peter Hurley

PS - Do you have a way to test also your setup using hardware flow
control, ie. CRTSCTS?  I ask because I would expect that to be broken
even before Seth's patch.

--- %< ---
From: Peter Hurley <peter@hurleysoftware.com>
Date: Mon, 9 Jun 2014 19:21:43 -0400
Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart

Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
'serial_core: Fix conditional start_tx on ring buffer not empty'
exposes an incorrect assumption in sunsab's start_tx method; the
tx ring buffer can, in fact, be empty when restarting tx when
performing flow control.

Test for empty tx ring buffer when in sunsab's start_tx method.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/sunsab.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
index 5faa8e9..b99a4ea 100644
--- a/drivers/tty/serial/sunsab.c
+++ b/drivers/tty/serial/sunsab.c
@@ -427,6 +427,9 @@ static void sunsab_start_tx(struct uart_port *port)
 	struct circ_buf *xmit = &up->port.state->xmit;
 	int i;
 
+	if (uart_circ_empty(xmit))
+		return;
+
 	up->interrupt_mask1 &= ~(SAB82532_IMR1_ALLS|SAB82532_IMR1_XPR);
 	writeb(up->interrupt_mask1, &up->regs->w.imr1);
 	
-- 
2.0.0

Re: Linux 3.15: SPARC serial console regression

[Aaro Koskinen]

Hi,

On Mon, Jun 09, 2014 at 07:48:17PM -0400, Peter Hurley wrote:
> Looks like Seth's fix exposes some broken assumptions in the Sun
> serial drivers.

Thanks, that seems to fix the issue.

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

> PS - Do you have a way to test also your setup using hardware flow
> control, ie. CRTSCTS?  I ask because I would expect that to be broken
> even before Seth's patch.

Sorry, I'm unable do that at the moment.

A.

> --- %< ---
> From: Peter Hurley <peter@hurleysoftware.com>
> Date: Mon, 9 Jun 2014 19:21:43 -0400
> Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart
> 
> Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
> 'serial_core: Fix conditional start_tx on ring buffer not empty'
> exposes an incorrect assumption in sunsab's start_tx method; the
> tx ring buffer can, in fact, be empty when restarting tx when
> performing flow control.
> 
> Test for empty tx ring buffer when in sunsab's start_tx method.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/sunsab.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
> index 5faa8e9..b99a4ea 100644
> --- a/drivers/tty/serial/sunsab.c
> +++ b/drivers/tty/serial/sunsab.c
> @@ -427,6 +427,9 @@ static void sunsab_start_tx(struct uart_port *port)
>  	struct circ_buf *xmit = &up->port.state->xmit;
>  	int i;
>  
> +	if (uart_circ_empty(xmit))
> +		return;
> +
>  	up->interrupt_mask1 &= ~(SAB82532_IMR1_ALLS|SAB82532_IMR1_XPR);
>  	writeb(up->interrupt_mask1, &up->regs->w.imr1);
>  	
> -- 
> 2.0.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux 3.15: SPARC serial console regression

[Sam Ravnborg]

> From: Peter Hurley <peter@hurleysoftware.com>
> Date: Mon, 9 Jun 2014 19:21:43 -0400
> Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart
> 
> Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
> 'serial_core: Fix conditional start_tx on ring buffer not empty'
> exposes an incorrect assumption in sunsab's start_tx method; the
> tx ring buffer can, in fact, be empty when restarting tx when
> performing flow control.
> 
> Test for empty tx ring buffer when in sunsab's start_tx method.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

Hi Peter.

Can you please take a look at sunzilog.c.
It looks like the same test is missing in sunzilog_start_tx():
        } else {
                struct circ_buf *xmit = &port->state->xmit;

                writeb(xmit->buf[xmit->tail], &channel->data);
                ZSDELAY();
                ZS_WSYNC(channel);

                xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
                port->icount.tx++;

                if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
                        uart_write_wakeup(&up->port);
        }


I did not review all drives - only a few sun drivers
and this was the only one that occured to me also
required this check.

I also noticed the typical pattern is:

    if (uart_circ_empty(xmit) || uart_tx_stopped(port))

Should you use this pattern also in sunsab.c?

	Sam

Re: Linux 3.15: SPARC serial console regression

[Peter Hurley]

Hi Sam,

On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
>> From: Peter Hurley <peter@hurleysoftware.com>
>> Date: Mon, 9 Jun 2014 19:21:43 -0400
>> Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart
>>
>> Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
>> 'serial_core: Fix conditional start_tx on ring buffer not empty'
>> exposes an incorrect assumption in sunsab's start_tx method; the
>> tx ring buffer can, in fact, be empty when restarting tx when
>> performing flow control.
>>
>> Test for empty tx ring buffer when in sunsab's start_tx method.
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>
> Hi Peter.
>
> Can you please take a look at sunzilog.c.
> It looks like the same test is missing in sunzilog_start_tx():

Yeah, when I saw that yesterday, I put
   * audit uart drivers' start_tx() methods
on my TODO list.

>          } else {
>                  struct circ_buf *xmit = &port->state->xmit;
>
>                  writeb(xmit->buf[xmit->tail], &channel->data);
>                  ZSDELAY();
>                  ZS_WSYNC(channel);
>
>                  xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>                  port->icount.tx++;
>
>                  if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>                          uart_write_wakeup(&up->port);
>          }
>
>
> I did not review all drives - only a few sun drivers
> and this was the only one that occured to me also
> required this check.
>
> I also noticed the typical pattern is:
>
>      if (uart_circ_empty(xmit) || uart_tx_stopped(port))
>
> Should you use this pattern also in sunsab.c?

What a mess.

uart_tx_stopped() _should_ be redundant for the start_tx() method.
However, I see that uart_resume_port() doesn't check either flow state
and uart_handle_cts_change() doesn't check the soft-flow state.

The semantics of the start_tx() method should be 'tx may commence; begin
xmitting if data is ready'. Except in the case where send_xchar() is not
supported by the uart driver. <sigh>

Regards,
Peter Hurley

Re: Linux 3.15: SPARC serial console regression

[Sam Ravnborg]

Hi Peter.

On Tue, Jun 10, 2014 at 05:20:08PM -0400, Peter Hurley wrote:
> Hi Sam,
> 
> On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
> >>From: Peter Hurley <peter@hurleysoftware.com>
> >>Date: Mon, 9 Jun 2014 19:21:43 -0400
> >>Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart
> >>
> >>Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
> >>'serial_core: Fix conditional start_tx on ring buffer not empty'
> >>exposes an incorrect assumption in sunsab's start_tx method; the
> >>tx ring buffer can, in fact, be empty when restarting tx when
> >>performing flow control.

We have a report that commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c
("serial_core: Fix conditional start_tx on ring buffer not empty")
broke at least one more driver:

    Aaro Koskinen <aaro.koskinen@iki.fi> reported:
    The same problem is present also on PowerPC G5 Xserve (pmac_zilog driver)

And based on code review a third driver is broken:

    Sam Ravnborg <sam@ravnborg.org> reported:
    Can you please take a look at sunzilog.c.
    It looks like the same test is missing in sunzilog_start_tx():

This is a regression so we should either revert the above commit
or review and fix the remaining drivers.

	Sam

Re: Linux 3.15: SPARC serial console regression

[Thomas Bogendoerfer]

On Sun, Jun 15, 2014 at 08:56:35PM +0200, Sam Ravnborg wrote:
> On Tue, Jun 10, 2014 at 05:20:08PM -0400, Peter Hurley wrote:
> > On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
> > >>From: Peter Hurley <peter@hurleysoftware.com>
> > >>Date: Mon, 9 Jun 2014 19:21:43 -0400
> > >>Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart
> > >>
> > >>Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
> > >>'serial_core: Fix conditional start_tx on ring buffer not empty'
> > >>exposes an incorrect assumption in sunsab's start_tx method; the
> > >>tx ring buffer can, in fact, be empty when restarting tx when
> > >>performing flow control.
> 
> We have a report that commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c
> ("serial_core: Fix conditional start_tx on ring buffer not empty")
> broke at least one more driver:
> 
>     Aaro Koskinen <aaro.koskinen@iki.fi> reported:
>     The same problem is present also on PowerPC G5 Xserve (pmac_zilog driver)
> 
> And based on code review a third driver is broken:
> 
>     Sam Ravnborg <sam@ravnborg.org> reported:
>     Can you please take a look at sunzilog.c.
>     It looks like the same test is missing in sunzilog_start_tx():
> 
> This is a regression so we should either revert the above commit
> or review and fix the remaining drivers.

ip22zilog.c is also broken (clone of sunzilog).

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: Linux 3.15: SPARC serial console regression

[Peter Hurley]

On 06/15/2014 02:56 PM, Sam Ravnborg wrote:
> Hi Peter.
>
> On Tue, Jun 10, 2014 at 05:20:08PM -0400, Peter Hurley wrote:
>> Hi Sam,
>>
>> On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
>>>> From: Peter Hurley <peter@hurleysoftware.com>
>>>> Date: Mon, 9 Jun 2014 19:21:43 -0400
>>>> Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart
>>>>
>>>> Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
>>>> 'serial_core: Fix conditional start_tx on ring buffer not empty'
>>>> exposes an incorrect assumption in sunsab's start_tx method; the
>>>> tx ring buffer can, in fact, be empty when restarting tx when
>>>> performing flow control.
>
> We have a report that commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c
> ("serial_core: Fix conditional start_tx on ring buffer not empty")
> broke at least one more driver:
>
>      Aaro Koskinen <aaro.koskinen@iki.fi> reported:
>      The same problem is present also on PowerPC G5 Xserve (pmac_zilog driver)
>
> And based on code review a third driver is broken:
>
>      Sam Ravnborg <sam@ravnborg.org> reported:
>      Can you please take a look at sunzilog.c.
>      It looks like the same test is missing in sunzilog_start_tx():
>
> This is a regression so we should either revert the above commit
> or review and fix the remaining drivers.

I'm working on the audit right now, but its not entirely accurate to
call this is a regression.

All of these drivers were already broken on resume-from-suspend
and hard flow control restart.

Regards,
Peter Hurley

Re: Linux 3.15: SPARC serial console regression

[David Miller]

From: Sam Ravnborg <sam@ravnborg.org>
Date: Sun, 15 Jun 2014 20:56:35 +0200

> This is a regression so we should either revert the above commit
> or review and fix the remaining drivers.

I plan to review and integrate the sparc serial driver fixes
soon.

Re: Linux 3.15: serial console regression

[Aaro Koskinen]

Hi,

On Mon, Jun 09, 2014 at 07:48:17PM -0400, Peter Hurley wrote:
> On 06/09/2014 04:59 PM, Aaro Koskinen wrote:> Hi,
> > While testing the final 3.15, I noticed the serial console (sunsab)
> > does not work anymore on Sun Ultra.
> > 
> > It will tx fine the console output during the boot. But then I try
> > to type something e.g. at the login: prompt, it behaves oddly:
> > it re-transmits some old output, and then stalls for a few secs, usually
> > for each character. Typed characters are however received correctly.
> > But the console is impossible to use...
> > 
> > I bisected this to:
> > 
> > 717f3bbab3c7628736ef738fdbf3d9a28578c26c is the first bad commit
> > commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c
> > Author: Seth Bollinger <sethb@digi.com>
> > Date:   Tue Mar 25 12:55:37 2014 -0500
> > 
> >      serial_core: Fix conditional start_tx on ring buffer not empty
> 
> [Let's try that again. This time without the mess]
> 
> Thanks for the report Aaro.
> 
> Looks like Seth's fix exposes some broken assumptions in the Sun
> serial drivers.

The same problem is present also on PowerPC G5 Xserve (pmac_zilog driver),
and the below type of fix works there too.

A.

> --- %< ---
> From: Peter Hurley <peter@hurleysoftware.com>
> Date: Mon, 9 Jun 2014 19:21:43 -0400
> Subject: [PATCH] serial: sunsab: Test for no tx data on tx restart
> 
> Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
> 'serial_core: Fix conditional start_tx on ring buffer not empty'
> exposes an incorrect assumption in sunsab's start_tx method; the
> tx ring buffer can, in fact, be empty when restarting tx when
> performing flow control.
> 
> Test for empty tx ring buffer when in sunsab's start_tx method.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/sunsab.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
> index 5faa8e9..b99a4ea 100644
> --- a/drivers/tty/serial/sunsab.c
> +++ b/drivers/tty/serial/sunsab.c
> @@ -427,6 +427,9 @@ static void sunsab_start_tx(struct uart_port *port)
>  	struct circ_buf *xmit = &up->port.state->xmit;
>  	int i;
>  
> +	if (uart_circ_empty(xmit))
> +		return;
> +
>  	up->interrupt_mask1 &= ~(SAB82532_IMR1_ALLS|SAB82532_IMR1_XPR);
>  	writeb(up->interrupt_mask1, &up->regs->w.imr1);
>  	
> -- 
> 2.0.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] fixes for empty tx buffer breakage

[Peter Hurley]

Cc: Seth Bollinger <sethb@digi.com>
Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

Greg,

I completed the audit of serial drivers after reports that
several Sun serial drivers were broken by
commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
'serial_core: Fix conditional start_tx on ring buffer not empty'.

I apologize for not submitting this sooner. The delay was due to
an ongoing analysis of serial flow control prompted by Sam Ravnborg's
question:

On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
> I also noticed the typical pattern is:
> 
>      if (uart_circ_empty(xmit) || uart_tx_stopped(port))
> 
> Should you use this pattern also in sunsab.c?

Unfortunately, that analysis revealed that tx flow control is
largely SMP-unsafe, and it's fairly easy to corrupt the hardware
state wrt. the tty flow control state.

I'm still working on the solutions to that; they're too
extensive to submit for 3.16 anyway.

Regards,

Peter Hurley (2):
  serial: Test for no tx data on tx restart
  serial: arc_uart: Use uart_circ_empty() for open-coded comparison

 drivers/tty/serial/arc_uart.c   | 2 +-
 drivers/tty/serial/imx.c        | 3 +++
 drivers/tty/serial/ip22zilog.c  | 2 ++
 drivers/tty/serial/m32r_sio.c   | 8 +++++---
 drivers/tty/serial/pmac_zilog.c | 3 +++
 drivers/tty/serial/sunsab.c     | 3 +++
 drivers/tty/serial/sunzilog.c   | 2 ++
 7 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.0.0

[PATCH 1/2] serial: Test for no tx data on tx restart

[Peter Hurley]

Commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
'serial_core: Fix conditional start_tx on ring buffer not empty'
exposes an incorrect assumption in several drivers' start_tx methods;
the tx ring buffer can, in fact, be empty when restarting tx while
performing flow control.

Affected drivers:
sunsab.c
ip22zilog.c
pmac_zilog.c
sunzilog.c
m32r_sio.c
imx.c

Other in-tree serial drivers either are not affected or already
test for empty tx ring buffer before transmitting.

Test for empty tx ring buffer in start_tx() method, after transmitting
x_char (if applicable).

Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: Seth Bollinger <sethb@digi.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/imx.c        | 3 +++
 drivers/tty/serial/ip22zilog.c  | 2 ++
 drivers/tty/serial/m32r_sio.c   | 8 +++++---
 drivers/tty/serial/pmac_zilog.c | 3 +++
 drivers/tty/serial/sunsab.c     | 3 +++
 drivers/tty/serial/sunzilog.c   | 2 ++
 6 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3b6c1a2..23c1dae 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -563,6 +563,9 @@ static void imx_start_tx(struct uart_port *port)
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long temp;
 
+	if (uart_circ_empty(&port.state->xmit))
+		return;
+
 	if (USE_IRDA(sport)) {
 		/* half duplex in IrDA mode; have to disable receive mode */
 		temp = readl(sport->port.membase + UCR4);
diff --git a/drivers/tty/serial/ip22zilog.c b/drivers/tty/serial/ip22zilog.c
index 1efd4c3..99b7b86 100644
--- a/drivers/tty/serial/ip22zilog.c
+++ b/drivers/tty/serial/ip22zilog.c
@@ -603,6 +603,8 @@ static void ip22zilog_start_tx(struct uart_port *port)
 	} else {
 		struct circ_buf *xmit = &port->state->xmit;
 
+		if (uart_circ_empty(xmit))
+			return;
 		writeb(xmit->buf[xmit->tail], &channel->data);
 		ZSDELAY();
 		ZS_WSYNC(channel);
diff --git a/drivers/tty/serial/m32r_sio.c b/drivers/tty/serial/m32r_sio.c
index 68f2c53..5702828 100644
--- a/drivers/tty/serial/m32r_sio.c
+++ b/drivers/tty/serial/m32r_sio.c
@@ -266,9 +266,11 @@ static void m32r_sio_start_tx(struct uart_port *port)
 	if (!(up->ier & UART_IER_THRI)) {
 		up->ier |= UART_IER_THRI;
 		serial_out(up, UART_IER, up->ier);
-		serial_out(up, UART_TX, xmit->buf[xmit->tail]);
-		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
-		up->port.icount.tx++;
+		if (!uart_circ_empty(xmit)) {
+			serial_out(up, UART_TX, xmit->buf[xmit->tail]);
+			xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+			up->port.icount.tx++;
+		}
 	}
 	while((serial_in(up, UART_LSR) & UART_EMPTY) != UART_EMPTY);
 #else
diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index 8193635..f7ad5b9 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -653,6 +653,8 @@ static void pmz_start_tx(struct uart_port *port)
 	} else {
 		struct circ_buf *xmit = &port->state->xmit;
 
+		if (uart_circ_empty(xmit))
+			goto out;
 		write_zsdata(uap, xmit->buf[xmit->tail]);
 		zssync(uap);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
@@ -661,6 +663,7 @@ static void pmz_start_tx(struct uart_port *port)
 		if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 			uart_write_wakeup(&uap->port);
 	}
+ out:
 	pmz_debug("pmz: start_tx() done.\n");
 }
 
diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
index 80a58ec..2f57df9 100644
--- a/drivers/tty/serial/sunsab.c
+++ b/drivers/tty/serial/sunsab.c
@@ -427,6 +427,9 @@ static void sunsab_start_tx(struct uart_port *port)
 	struct circ_buf *xmit = &up->port.state->xmit;
 	int i;
 
+	if (uart_circ_empty(xmit))
+		return;
+
 	up->interrupt_mask1 &= ~(SAB82532_IMR1_ALLS|SAB82532_IMR1_XPR);
 	writeb(up->interrupt_mask1, &up->regs->w.imr1);
 	
diff --git a/drivers/tty/serial/sunzilog.c b/drivers/tty/serial/sunzilog.c
index a85db8b..02df394 100644
--- a/drivers/tty/serial/sunzilog.c
+++ b/drivers/tty/serial/sunzilog.c
@@ -703,6 +703,8 @@ static void sunzilog_start_tx(struct uart_port *port)
 	} else {
 		struct circ_buf *xmit = &port->state->xmit;
 
+		if (uart_circ_empty(xmit))
+			return;
 		writeb(xmit->buf[xmit->tail], &channel->data);
 		ZSDELAY();
 		ZS_WSYNC(channel);
-- 
2.0.0

[PATCH 2/2] serial: arc_uart: Use uart_circ_empty() for open-coded comparison

[Peter Hurley]

Replace open-coded test for empty tx ring buffer with equivalent
helper function, uart_circ_empty(). No functional change.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/arc_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/arc_uart.c b/drivers/tty/serial/arc_uart.c
index c9f5c9d..008c223 100644
--- a/drivers/tty/serial/arc_uart.c
+++ b/drivers/tty/serial/arc_uart.c
@@ -177,7 +177,7 @@ static void arc_serial_tx_chars(struct arc_uart_port *uart)
 		uart->port.icount.tx++;
 		uart->port.x_char = 0;
 		sent = 1;
-	} else if (xmit->tail != xmit->head) {	/* TODO: uart_circ_empty */
+	} else if (!uart_circ_empty(xmit)) {
 		ch = xmit->buf[xmit->tail];
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		uart->port.icount.tx++;
-- 
2.0.0

Re: [PATCH 0/2] fixes for empty tx buffer breakage

[Greg Kroah-Hartman]

On Sun, Jul 06, 2014 at 11:29:51AM -0400, Peter Hurley wrote:
> Cc: Seth Bollinger <sethb@digi.com>
> Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> 
> Greg,
> 
> I completed the audit of serial drivers after reports that
> several Sun serial drivers were broken by
> commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
> 'serial_core: Fix conditional start_tx on ring buffer not empty'.
> 
> I apologize for not submitting this sooner. The delay was due to
> an ongoing analysis of serial flow control prompted by Sam Ravnborg's
> question:
> 
> On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
> > I also noticed the typical pattern is:
> > 
> >      if (uart_circ_empty(xmit) || uart_tx_stopped(port))
> > 
> > Should you use this pattern also in sunsab.c?
> 
> Unfortunately, that analysis revealed that tx flow control is
> largely SMP-unsafe, and it's fairly easy to corrupt the hardware
> state wrt. the tty flow control state.
> 
> I'm still working on the solutions to that; they're too
> extensive to submit for 3.16 anyway.

So these should go into 3.16-final?  Or 3.17?

confused,

greg k-h

Re: [PATCH 0/2] fixes for empty tx buffer breakage

[David Miller]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Thu, 10 Jul 2014 16:14:32 -0700

> On Sun, Jul 06, 2014 at 11:29:51AM -0400, Peter Hurley wrote:
>> Cc: Seth Bollinger <sethb@digi.com>
>> Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>> 
>> Greg,
>> 
>> I completed the audit of serial drivers after reports that
>> several Sun serial drivers were broken by
>> commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
>> 'serial_core: Fix conditional start_tx on ring buffer not empty'.
>> 
>> I apologize for not submitting this sooner. The delay was due to
>> an ongoing analysis of serial flow control prompted by Sam Ravnborg's
>> question:
>> 
>> On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
>> > I also noticed the typical pattern is:
>> > 
>> >      if (uart_circ_empty(xmit) || uart_tx_stopped(port))
>> > 
>> > Should you use this pattern also in sunsab.c?
>> 
>> Unfortunately, that analysis revealed that tx flow control is
>> largely SMP-unsafe, and it's fairly easy to corrupt the hardware
>> state wrt. the tty flow control state.
>> 
>> I'm still working on the solutions to that; they're too
>> extensive to submit for 3.16 anyway.
> 
> So these should go into 3.16-final?  Or 3.17?

Definitely 3.16, this regression is several releases old.

Re: [PATCH 0/2] fixes for empty tx buffer breakage

[Greg KH]

On Thu, Jul 10, 2014 at 04:12:05PM -0700, David Miller wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date: Thu, 10 Jul 2014 16:14:32 -0700
> 
> > On Sun, Jul 06, 2014 at 11:29:51AM -0400, Peter Hurley wrote:
> >> Cc: Seth Bollinger <sethb@digi.com>
> >> Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
> >> Cc: Sam Ravnborg <sam@ravnborg.org>
> >> Cc: "David S. Miller" <davem@davemloft.net>
> >> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> >> 
> >> Greg,
> >> 
> >> I completed the audit of serial drivers after reports that
> >> several Sun serial drivers were broken by
> >> commit 717f3bbab3c7628736ef738fdbf3d9a28578c26c,
> >> 'serial_core: Fix conditional start_tx on ring buffer not empty'.
> >> 
> >> I apologize for not submitting this sooner. The delay was due to
> >> an ongoing analysis of serial flow control prompted by Sam Ravnborg's
> >> question:
> >> 
> >> On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
> >> > I also noticed the typical pattern is:
> >> > 
> >> >      if (uart_circ_empty(xmit) || uart_tx_stopped(port))
> >> > 
> >> > Should you use this pattern also in sunsab.c?
> >> 
> >> Unfortunately, that analysis revealed that tx flow control is
> >> largely SMP-unsafe, and it's fairly easy to corrupt the hardware
> >> state wrt. the tty flow control state.
> >> 
> >> I'm still working on the solutions to that; they're too
> >> extensive to submit for 3.16 anyway.
> > 
> > So these should go into 3.16-final?  Or 3.17?
> 
> Definitely 3.16, this regression is several releases old.

Ok, will queue it up, thanks.

gre gk-h