[PATCH v3 0/3] serial: xuartps: hardware race condition and cleanup

[PATCH v3 0/3] serial: xuartps: hardware race condition and cleanup

[Helmut Grohne]

The character transmission in xuartps is racy. If the transmitter is disabled
early, the device is confused and produces a desync. Garbage on the remote end
can be a visible symptom. The second patch in this series tries to reduce that
race condition in accordance with the hardware documentation, but it cannot
remove it entirely. The first and third patches are code cleanup.

Changes since v2:
 * Do not attempt to disable the transmitter after a transmission (original
   behaviour around 3.14). These patches no longer touch with the transmitter
   state at all as requested by Sören Brinkmann.
 * Add Acked-by/Reviewed-by tags from Sören Brinkmann after addressing his
   remarks.

Earlier patches/discussion at:
    https://www.spinics.net/lists/linux-serial/msg23145.html
    https://www.spinics.net/lists/linux-serial/msg23156.html
    https://www.spinics.net/lists/linux-serial/msg23157.html

Helmut Grohne (3):
  serial: xuartps: fix typo in cdns_uart_startup
  serial: xuartps: reduce hardware TX race condition
  serial: xuartps: remove unnecessary register write

 drivers/tty/serial/xilinx_uartps.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH v3 1/3] serial: xuartps: fix typo in cdns_uart_startup

[Helmut Grohne]

The bit mask changes in commit 6e14f7c1f2c2 ("tty: xuartps: Improve
startup function") doesn't do what the commit message advertises. The
original behaviour was clearing the RX_DIS bit, but due to missing ~,
that bit is now the only bit kept.

Currently, the regression is harmless, because the previous write to the
control register sets it to TXRST | RXRST. Thus the RX_DIS bit is
previously cleared. The *RST bits are cleared by the hardware, so this
commit does not currently change behaviour, but makes future changes
less risky.

Link: https://www.spinics.net/lists/linux-serial/msg23157.html
Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
Fixes: 6e14f7c1f2c2 ("tty: xuartps: Improve startup function")
Reviewed-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index bd72dd843338..e4b2d8102a04 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -829,7 +829,7 @@ static int cdns_uart_startup(struct uart_port *port)
 	 * the receiver.
 	 */
 	status = readl(port->membase + CDNS_UART_CR);
-	status &= CDNS_UART_CR_RX_DIS;
+	status &= ~CDNS_UART_CR_RX_DIS;
 	status |= CDNS_UART_CR_RX_EN;
 	writel(status, port->membase + CDNS_UART_CR);
 
-- 
2.11.0

[PATCH v3 2/3] serial: xuartps: reduce hardware TX race condition

[Helmut Grohne]

After sending data to the uart, the driver was waiting until the TX
FIFO was empty (for every single char written). After that, TX was
disabled by writing the original TX state to the status register. At
that time however, the state machine could still be shifting
characters. Not waiting can result in strange hardware states,
especially when coupled with calls to cdns_uart_set_termios, whose
symptom generally is garbage characters being received from uart or a
hang.

According to UG585, the TACTIVE bit of the channel status register
indicates the shifter operation and we should be waiting for that bit
to clear.

Sending characters does not require the TX FIFO to be empty, but merely
to not be full. So cdns_uart_console_putchar is updated accordingly.

During tests with an instrumented kernel and an oscilloscope, we could
determine that the chance of a race is reduced by this patch. It is not
removed entirely. On the oscilloscope, one can see that disabling the
transmitter early can result in the transmission hanging in the middle
of a character for a tiny duration. This hiccup is enough to
desynchronize with a remote device for a sequence of characters until a
data bit doesn't match the start or stop bits anymore.

Link: https://www.spinics.net/lists/linux-serial/msg23156.html
Link: https://www.spinics.net/lists/linux-serial/msg26139.html
Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index e4b2d8102a04..a34b2c757593 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -167,6 +167,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
 #define CDNS_UART_SR_TXEMPTY	0x00000008 /* TX FIFO empty */
 #define CDNS_UART_SR_TXFULL	0x00000010 /* TX FIFO full */
 #define CDNS_UART_SR_RXTRIG	0x00000001 /* Rx Trigger */
+#define CDNS_UART_SR_TACTIVE	0x00000800 /* TX state machine active */
 
 /* baud dividers min/max values */
 #define CDNS_UART_BDIV_MIN	4
@@ -1138,23 +1139,14 @@ static struct uart_port *cdns_uart_get_port(int id)
 
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 /**
- * cdns_uart_console_wait_tx - Wait for the TX to be full
- * @port: Handle to the uart port structure
- */
-static void cdns_uart_console_wait_tx(struct uart_port *port)
-{
-	while (!(readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXEMPTY))
-		barrier();
-}
-
-/**
  * cdns_uart_console_putchar - write the character to the FIFO buffer
  * @port: Handle to the uart port structure
  * @ch: Character to be written
  */
 static void cdns_uart_console_putchar(struct uart_port *port, int ch)
 {
-	cdns_uart_console_wait_tx(port);
+	while (readl(port->membase + CDNS_UART_SR) & CDNS_UART_SR_TXFULL)
+		cpu_relax();
 	writel(ch, port->membase + CDNS_UART_FIFO);
 }
 
@@ -1241,7 +1233,10 @@ static void cdns_uart_console_write(struct console *co, const char *s,
 	writel(ctrl, port->membase + CDNS_UART_CR);
 
 	uart_console_write(port, s, count, cdns_uart_console_putchar);
-	cdns_uart_console_wait_tx(port);
+	while ((readl(port->membase + CDNS_UART_SR) &
+			(CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE)) !=
+			CDNS_UART_SR_TXEMPTY)
+		cpu_relax();
 
 	writel(ctrl, port->membase + CDNS_UART_CR);
 
-- 
2.11.0

[PATCH v3 3/3] serial: xuartps: remove unnecessary register write

[Helmut Grohne]

This writel writes the exact same value as the previous writel and is
thus unnecessary. It accidentally became unnecessary in e3538c37ee38
("tty: xuartps: Beautify read-modify writes"), but the new behaviour is
now expected.

Link: https://www.spinics.net/lists/linux-serial/msg23168.html
Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
---
 drivers/tty/serial/xilinx_uartps.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index a34b2c757593..7a2b1a7350ac 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1238,8 +1238,6 @@ static void cdns_uart_console_write(struct console *co, const char *s,
 			CDNS_UART_SR_TXEMPTY)
 		cpu_relax();
 
-	writel(ctrl, port->membase + CDNS_UART_CR);
-
 	/* restore interrupt state */
 	writel(imr, port->membase + CDNS_UART_IER);
 
-- 
2.11.0