* FW: Linux 3.15: SPARC serial console regression
@ 2014-06-09 23:48 ` Peter Hurley
0 siblings, 0 replies; 28+ messages in thread
From: Peter Hurley @ 2014-06-09 23:48 UTC (permalink / raw)
To: Aaro Koskinen
Cc: David Miller, linux-kernel, sparclinux, Greg Kroah-Hartman,
Seth Bollinger, 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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
2014-06-09 23:48 ` Peter Hurley
@ 2014-06-10 18:53 ` Aaro Koskinen
-1 siblings, 0 replies; 28+ messages in thread
From: Aaro Koskinen @ 2014-06-10 18:53 UTC (permalink / raw)
To: Peter Hurley
Cc: David Miller, linux-kernel, sparclinux, Greg Kroah-Hartman,
Seth Bollinger
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
@ 2014-06-10 18:53 ` Aaro Koskinen
0 siblings, 0 replies; 28+ messages in thread
From: Aaro Koskinen @ 2014-06-10 18:53 UTC (permalink / raw)
To: Peter Hurley
Cc: David Miller, linux-kernel, sparclinux, Greg Kroah-Hartman,
Seth Bollinger
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
2014-06-09 23:48 ` Peter Hurley
@ 2014-06-10 19:24 ` Sam Ravnborg
-1 siblings, 0 replies; 28+ messages in thread
From: Sam Ravnborg @ 2014-06-10 19:24 UTC (permalink / raw)
To: Peter Hurley
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger
> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
@ 2014-06-10 19:24 ` Sam Ravnborg
0 siblings, 0 replies; 28+ messages in thread
From: Sam Ravnborg @ 2014-06-10 19:24 UTC (permalink / raw)
To: Peter Hurley
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger
> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
2014-06-10 19:24 ` Sam Ravnborg
@ 2014-06-10 21:20 ` Peter Hurley
-1 siblings, 0 replies; 28+ messages in thread
From: Peter Hurley @ 2014-06-10 21:20 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
@ 2014-06-10 21:20 ` Peter Hurley
0 siblings, 0 replies; 28+ messages in thread
From: Peter Hurley @ 2014-06-10 21:20 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
2014-06-10 21:20 ` Peter Hurley
@ 2014-06-15 18:56 ` Sam Ravnborg
-1 siblings, 0 replies; 28+ messages in thread
From: Sam Ravnborg @ 2014-06-15 18:56 UTC (permalink / raw)
To: Peter Hurley
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger, LinusTorvaldstorvalds
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
@ 2014-06-15 18:56 ` Sam Ravnborg
0 siblings, 0 replies; 28+ messages in thread
From: Sam Ravnborg @ 2014-06-15 18:56 UTC (permalink / raw)
To: Peter Hurley
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger, LinusTorvaldstorvalds
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
2014-06-15 18:56 ` Sam Ravnborg
@ 2014-06-16 14:24 ` Thomas Bogendoerfer
-1 siblings, 0 replies; 28+ messages in thread
From: Thomas Bogendoerfer @ 2014-06-16 14:24 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Peter Hurley, Aaro Koskinen, David Miller, linux-kernel,
sparclinux, Greg Kroah-Hartman, Seth Bollinger,
LinusTorvaldstorvalds
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 ]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
@ 2014-06-16 14:24 ` Thomas Bogendoerfer
0 siblings, 0 replies; 28+ messages in thread
From: Thomas Bogendoerfer @ 2014-06-16 14:24 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Peter Hurley, Aaro Koskinen, David Miller, linux-kernel,
sparclinux, Greg Kroah-Hartman, Seth Bollinger,
LinusTorvaldstorvalds
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 ]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
2014-06-15 18:56 ` Sam Ravnborg
@ 2014-06-16 15:37 ` Peter Hurley
-1 siblings, 0 replies; 28+ messages in thread
From: Peter Hurley @ 2014-06-16 15:37 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger, LinusTorvaldstorvalds
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
@ 2014-06-16 15:37 ` Peter Hurley
0 siblings, 0 replies; 28+ messages in thread
From: Peter Hurley @ 2014-06-16 15:37 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Aaro Koskinen, David Miller, linux-kernel, sparclinux,
Greg Kroah-Hartman, Seth Bollinger, LinusTorvaldstorvalds
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
2014-06-15 18:56 ` Sam Ravnborg
@ 2014-06-18 6:19 ` David Miller
-1 siblings, 0 replies; 28+ messages in thread
From: David Miller @ 2014-06-18 6:19 UTC (permalink / raw)
To: sam
Cc: peter, aaro.koskinen, linux-kernel, sparclinux, gregkh, sethb,
LinusTorvaldstorvalds
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: SPARC serial console regression
@ 2014-06-18 6:19 ` David Miller
0 siblings, 0 replies; 28+ messages in thread
From: David Miller @ 2014-06-18 6:19 UTC (permalink / raw)
To: sam
Cc: peter, aaro.koskinen, linux-kernel, sparclinux, gregkh, sethb,
LinusTorvaldstorvalds
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: serial console regression
2014-06-09 23:48 ` Peter Hurley
@ 2014-06-13 20:18 ` Aaro Koskinen
-1 siblings, 0 replies; 28+ messages in thread
From: Aaro Koskinen @ 2014-06-13 20:18 UTC (permalink / raw)
To: Peter Hurley, Benjamin Herrenschmidt
Cc: David Miller, linux-kernel, sparclinux, Greg Kroah-Hartman,
Seth Bollinger
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Linux 3.15: serial console regression
@ 2014-06-13 20:18 ` Aaro Koskinen
0 siblings, 0 replies; 28+ messages in thread
From: Aaro Koskinen @ 2014-06-13 20:18 UTC (permalink / raw)
To: Peter Hurley, Benjamin Herrenschmidt
Cc: David Miller, linux-kernel, sparclinux, Greg Kroah-Hartman,
Seth Bollinger
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
^ permalink raw reply [flat|nested] 28+ messages in thread