Patchwork Linux 3.15: SPARC serial console regression

login
register
mail settings
Submitter Peter Hurley
Date June 9, 2014, 11:48 p.m.
Message ID <1402357697-20976-1-git-send-email-peter@hurleysoftware.com>
Download mbox | patch
Permalink /patch/357645/
State RFC
Delegated to: David Miller
Headers show

Comments

Peter Hurley - June 9, 2014, 11:48 p.m.
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(+)
Aaro Koskinen - June 10, 2014, 6:53 p.m.
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
--
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
Sam Ravnborg - June 10, 2014, 7:24 p.m.
> 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
--
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
Peter Hurley - June 10, 2014, 9:20 p.m.
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


--
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
Aaro Koskinen - June 13, 2014, 8:18 p.m.
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
--
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
Sam Ravnborg - June 15, 2014, 6:56 p.m.
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
--
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
Thomas Bogendoerfer - June 16, 2014, 2:24 p.m.
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.
Peter Hurley - June 16, 2014, 3:37 p.m.
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
--
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
David Miller - June 18, 2014, 6:19 a.m.
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.
--
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

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