| Message ID | 1402357697-20976-1-git-send-email-peter@hurleysoftware.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
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
> 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
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
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
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
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.
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
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
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);