* [PATCH 0/2] fixes for empty tx buffer breakage
[not found] <20140609205917.GA913@drone.musicnaut.iki.fi>
@ 2014-07-06 15:29 ` Peter Hurley
2014-07-06 15:29 ` [PATCH 1/2] serial: Test for no tx data on tx restart Peter Hurley
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Peter Hurley @ 2014-07-06 15:29 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, linux-serial, Peter Hurley, Seth Bollinger,
Aaro Koskinen, Sam Ravnborg, David S. Miller, Thomas Bogendoerfer
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] serial: Test for no tx data on tx restart
2014-07-06 15:29 ` [PATCH 0/2] fixes for empty tx buffer breakage Peter Hurley
@ 2014-07-06 15:29 ` Peter Hurley
2014-07-06 15:29 ` [PATCH 2/2] serial: arc_uart: Use uart_circ_empty() for open-coded comparison Peter Hurley
2014-07-10 23:14 ` [PATCH 0/2] fixes for empty tx buffer breakage Greg Kroah-Hartman
2 siblings, 0 replies; 6+ messages in thread
From: Peter Hurley @ 2014-07-06 15:29 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, linux-serial, Peter Hurley, Seth Bollinger,
David S. Miller, Sam Ravnborg, Thomas Bogendoerfer
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] serial: arc_uart: Use uart_circ_empty() for open-coded comparison
2014-07-06 15:29 ` [PATCH 0/2] fixes for empty tx buffer breakage Peter Hurley
2014-07-06 15:29 ` [PATCH 1/2] serial: Test for no tx data on tx restart Peter Hurley
@ 2014-07-06 15:29 ` Peter Hurley
2014-07-10 23:14 ` [PATCH 0/2] fixes for empty tx buffer breakage Greg Kroah-Hartman
2 siblings, 0 replies; 6+ messages in thread
From: Peter Hurley @ 2014-07-06 15:29 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, linux-serial, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] fixes for empty tx buffer breakage
2014-07-10 23:14 ` [PATCH 0/2] fixes for empty tx buffer breakage Greg Kroah-Hartman
@ 2014-07-10 23:12 ` David Miller
2014-07-10 23:29 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2014-07-10 23:12 UTC (permalink / raw)
To: gregkh
Cc: peter, linux-kernel, linux-serial, sethb, aaro.koskinen, sam,
tsbogend
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] fixes for empty tx buffer breakage
2014-07-06 15:29 ` [PATCH 0/2] fixes for empty tx buffer breakage Peter Hurley
2014-07-06 15:29 ` [PATCH 1/2] serial: Test for no tx data on tx restart Peter Hurley
2014-07-06 15:29 ` [PATCH 2/2] serial: arc_uart: Use uart_circ_empty() for open-coded comparison Peter Hurley
@ 2014-07-10 23:14 ` Greg Kroah-Hartman
2014-07-10 23:12 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2014-07-10 23:14 UTC (permalink / raw)
To: Peter Hurley
Cc: linux-kernel, linux-serial, Seth Bollinger, Aaro Koskinen,
Sam Ravnborg, David S. Miller, Thomas Bogendoerfer
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] fixes for empty tx buffer breakage
2014-07-10 23:12 ` David Miller
@ 2014-07-10 23:29 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2014-07-10 23:29 UTC (permalink / raw)
To: David Miller
Cc: peter, linux-kernel, linux-serial, sethb, aaro.koskinen, sam,
tsbogend
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-10 23:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140609205917.GA913@drone.musicnaut.iki.fi>
2014-07-06 15:29 ` [PATCH 0/2] fixes for empty tx buffer breakage Peter Hurley
2014-07-06 15:29 ` [PATCH 1/2] serial: Test for no tx data on tx restart Peter Hurley
2014-07-06 15:29 ` [PATCH 2/2] serial: arc_uart: Use uart_circ_empty() for open-coded comparison Peter Hurley
2014-07-10 23:14 ` [PATCH 0/2] fixes for empty tx buffer breakage Greg Kroah-Hartman
2014-07-10 23:12 ` David Miller
2014-07-10 23:29 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).