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 <[email protected]>
Date: Tue Mar 25 12:55:37 2014 -0500
serial_core: Fix conditional start_tx on ring buffer not empty
A.
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Peter Hurley <[email protected]>
> 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 <[email protected]>
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
Hi Sam,
On 06/10/2014 03:24 PM, Sam Ravnborg wrote:
>> From: Peter Hurley <[email protected]>
>> 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 <[email protected]>
>
> 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
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 <[email protected]>
> > 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> >>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 <[email protected]> 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 <[email protected]> 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
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 <[email protected]>
> > >>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 <[email protected]> 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 <[email protected]> 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 ]
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 <[email protected]>
>>>> 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 <[email protected]> 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 <[email protected]> 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
From: Sam Ravnborg <[email protected]>
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.
Replace open-coded test for empty tx ring buffer with equivalent
helper function, uart_circ_empty(). No functional change.
Signed-off-by: Peter Hurley <[email protected]>
---
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
Cc: Seth Bollinger <[email protected]>
Cc: Aaro Koskinen <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
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
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 <[email protected]>
Cc: Seth Bollinger <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
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
On Sun, Jul 06, 2014 at 11:29:51AM -0400, Peter Hurley wrote:
> Cc: Seth Bollinger <[email protected]>
> Cc: Aaro Koskinen <[email protected]>
> Cc: Sam Ravnborg <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
>
> 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
From: Greg Kroah-Hartman <[email protected]>
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 <[email protected]>
>> Cc: Aaro Koskinen <[email protected]>
>> Cc: Sam Ravnborg <[email protected]>
>> Cc: "David S. Miller" <[email protected]>
>> Cc: Thomas Bogendoerfer <[email protected]>
>>
>> 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.
On Thu, Jul 10, 2014 at 04:12:05PM -0700, David Miller wrote:
> From: Greg Kroah-Hartman <[email protected]>
> 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 <[email protected]>
> >> Cc: Aaro Koskinen <[email protected]>
> >> Cc: Sam Ravnborg <[email protected]>
> >> Cc: "David S. Miller" <[email protected]>
> >> Cc: Thomas Bogendoerfer <[email protected]>
> >>
> >> 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