[PATH RESEND v2 00/10] tty: xuartps: Fix lock ups

[PATH RESEND v2 00/10] tty: xuartps: Fix lock ups

[Soren Brinkmann]

Hi,

I guess this series got probably lost during the merge window. Hence, I
resend the patches.

This is v2, hopefully without any build issues. The changes are the same
as before, just the warning reported by kbuild test robot about unused
variables have been fixed. And, I added one more patch with some
cleanup/refactoring.

I recently found my system locking up under some conditions. I dug
through the code a bit and the result is this collections of various
changes. Some of it may not be required and has just been created out of
half-baked theories and re-reading the documentation. The actual failing
scenarios seem to be:
 - RX IRQ conditions are handled while the receiver is disabled, leaving
   the driver in an infinite loop
 - console_put_char seems to be interrupted by uart_shutdown disabling
   the transmitter while/just before printing

I.e. overall I tried to serialize all operations using the port lock to
avoid such interaction.

	Thanks,
	Sören

Sören Brinkmann (10):
  tty: xuartps: Beautify read-modify writes
  tty: xuartps: Use spinlock to serialize HW access
  tty: xuartps: Always enable transmitter in start_tx
  tty: xuartps: Clear interrupt status register in shutdown
  tty: xuartps: Improve startup function
  tty: xuartps: Keep lock for whole ISR
  tty: xuartps: Acquire port lock for shutdown
  tty: xuartps: Move RX path into helper function
  tty: xuartps: Only handle RX IRQs when RX is enabled
  tty: xuartps: Cleanup: Reformat if-else

 drivers/tty/serial/xilinx_uartps.c | 247 ++++++++++++++++++++-----------------
 1 file changed, 136 insertions(+), 111 deletions(-)

-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 01/10] tty: xuartps: Beautify read-modify writes

[Soren Brinkmann]

Non-functional, formatting changes to ease reading the code.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 009e0dbc12d2..50d4082d2354 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -515,12 +515,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
 	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
 		return;
 
-	status = readl(port->membase + CDNS_UART_CR_OFFSET);
-	/* Set the TX enable bit and clear the TX disable bit to enable the
+	/*
+	 * Set the TX enable bit and clear the TX disable bit to enable the
 	 * transmitter.
 	 */
-	writel((status & ~CDNS_UART_CR_TX_DIS) | CDNS_UART_CR_TX_EN,
-			port->membase + CDNS_UART_CR_OFFSET);
+	status = readl(port->membase + CDNS_UART_CR_OFFSET);
+	status &= ~CDNS_UART_CR_TX_DIS;
+	status |= CDNS_UART_CR_TX_EN;
+	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
 	while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
 				CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
@@ -1123,8 +1125,9 @@ static void cdns_uart_console_write(struct console *co, const char *s,
 	 * clear the TX disable bit to enable the transmitter.
 	 */
 	ctrl = readl(port->membase + CDNS_UART_CR_OFFSET);
-	writel((ctrl & ~CDNS_UART_CR_TX_DIS) | CDNS_UART_CR_TX_EN,
-			port->membase + CDNS_UART_CR_OFFSET);
+	ctrl &= ~CDNS_UART_CR_TX_DIS;
+	ctrl |= CDNS_UART_CR_TX_EN;
+	writel(ctrl, port->membase + CDNS_UART_CR_OFFSET);
 
 	uart_console_write(port, s, count, cdns_uart_console_putchar);
 	cdns_uart_console_wait_tx(port);
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 02/10] tty: xuartps: Use spinlock to serialize HW access

[Soren Brinkmann]

Instead of disabling the IRQ, use the spin lock to serialize accesses to
the HW. This protects the driver from interference of non-IRQ callbacks
with each other and makes the driver more consistent in its
serialization method.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v2:
 - remove unused variable
---
 drivers/tty/serial/xilinx_uartps.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 50d4082d2354..2c98c357d9a0 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -945,12 +945,10 @@ static void cdns_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 #ifdef CONFIG_CONSOLE_POLL
 static int cdns_uart_poll_get_char(struct uart_port *port)
 {
-	u32 imr;
 	int c;
+	unsigned long flags;
 
-	/* Disable all interrupts */
-	imr = readl(port->membase + CDNS_UART_IMR_OFFSET);
-	writel(imr, port->membase + CDNS_UART_IDR_OFFSET);
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Check if FIFO is empty */
 	if (readl(port->membase + CDNS_UART_SR_OFFSET) & CDNS_UART_SR_RXEMPTY)
@@ -959,19 +957,16 @@ static int cdns_uart_poll_get_char(struct uart_port *port)
 		c = (unsigned char) readl(
 					port->membase + CDNS_UART_FIFO_OFFSET);
 
-	/* Enable interrupts */
-	writel(imr, port->membase + CDNS_UART_IER_OFFSET);
+	spin_unlock_irqrestore(&port->lock, flags);
 
 	return c;
 }
 
 static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
 {
-	u32 imr;
+	unsigned long flags;
 
-	/* Disable all interrupts */
-	imr = readl(port->membase + CDNS_UART_IMR_OFFSET);
-	writel(imr, port->membase + CDNS_UART_IDR_OFFSET);
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Wait until FIFO is empty */
 	while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
@@ -986,8 +981,7 @@ static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
 				CDNS_UART_SR_TXEMPTY))
 		cpu_relax();
 
-	/* Enable interrupts */
-	writel(imr, port->membase + CDNS_UART_IER_OFFSET);
+	spin_unlock_irqrestore(&port->lock, flags);
 
 	return;
 }
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Soren Brinkmann]

start_tx must start transmitting characters. Regardless of the state of
the circular buffer, always enable the transmitter hardware.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2c98c357d9a0..df6778d17949 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -512,9 +512,6 @@ static void cdns_uart_start_tx(struct uart_port *port)
 {
 	unsigned int status, numbytes = port->fifosize;
 
-	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
-		return;
-
 	/*
 	 * Set the TX enable bit and clear the TX disable bit to enable the
 	 * transmitter.
@@ -524,6 +521,9 @@ static void cdns_uart_start_tx(struct uart_port *port)
 	status |= CDNS_UART_CR_TX_EN;
 	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
+	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
+		return;
+
 	while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
 				CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
 		/* Break if no more data available in the UART buffer */
-- 
2.6.3.3.g9bb996a

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Peter Hurley]

On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
> start_tx must start transmitting characters. Regardless of the state of
> the circular buffer, always enable the transmitter hardware.

Why?

Does cdns_uart_stop_tx() actually stop the transmitter so that
data remains in the transmitter?

> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 2c98c357d9a0..df6778d17949 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -512,9 +512,6 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  {
>  	unsigned int status, numbytes = port->fifosize;
>  
> -	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
> -		return;

The test for tx stopped needs to remain; otherwise, transmission will
restart even if the tty has been stopped (for example by userspace
or IXON software flow control).

Regards,
Peter Hurley

> -
>  	/*
>  	 * Set the TX enable bit and clear the TX disable bit to enable the
>  	 * transmitter.
> @@ -524,6 +521,9 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  	status |= CDNS_UART_CR_TX_EN;
>  	writel(status, port->membase + CDNS_UART_CR_OFFSET);
>  
> +	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
> +		return;
> +
>  	while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
>  				CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
>  		/* Break if no more data available in the UART buffer */
> 

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Sören Brinkmann]

Hi Peter,

On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
> 
> 
> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
> > start_tx must start transmitting characters. Regardless of the state of
> > the circular buffer, always enable the transmitter hardware.
> 
> Why?
> 
> Does cdns_uart_stop_tx() actually stop the transmitter so that
> data remains in the transmitter?

Well, I saw my system freezing and the cause seemed to be that the UART
receiver and/or transmitters were disabled while the system was trying
to print. Hence, I started questioning all locations touching the
transmitter/receiver enable. I read the docs in
https://www.kernel.org/doc/Documentation/serial/driver, which simply
says "Start transmitting characters." for start_tx(). Hence, I thought,
this function is probably supposed to just do that and start the
transmitter. I'll test whether this patch can be dropped.

	Thanks,
	Sören

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Peter Hurley]

Hi Sören,

On 11/20/2015 10:28 AM, Sören Brinkmann wrote:
> On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
>> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
>>> start_tx must start transmitting characters. Regardless of the state of
>>> the circular buffer, always enable the transmitter hardware.
>>
>> Why?
>>
>> Does cdns_uart_stop_tx() actually stop the transmitter so that
>> data remains in the transmitter?
> 
> Well, I saw my system freezing and the cause seemed to be that the UART
> receiver and/or transmitters were disabled while the system was trying
> to print. Hence, I started questioning all locations touching the
> transmitter/receiver enable. I read the docs in
> https://www.kernel.org/doc/Documentation/serial/driver, which simply
> says "Start transmitting characters." for start_tx(). Hence, I thought,
> this function is probably supposed to just do that and start the
> transmitter. I'll test whether this patch can be dropped.

I don't think that patch would fix any freeze problems, but restarting
the transmitter even if the circ buffer is empty may be necessary to
push out remaining data when the port is restarted after being stopped.

IOW, something like

	if (uart_tx_stopped(port))
		return;

	....


	if (uart_circ_empty(&port->state->xmit)
		return;


Below is a (work-in-progress) serial driver validation test for flow
control handling (it may need some tuning for slow line speeds).
Usual caveats apply. Takes ~40secs @ 115200.

--- >% ---
--- /dev/null	2015-11-20 07:19:13.265468435 -0500
+++ flow.c	2015-11-20 11:21:44.254139192 -0500
@@ -0,0 +1,343 @@
+/*
+ * flow control unit test for tty/serial core
+ *
+ * Copyright (c) 2014 Peter Hurley
+ * __fls() by  Alexander van Heukelum <heukelum@mailshack.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This test creates two additional threads, a writer which continuously
+ * transmits a pattern and a reader which reads and verifies the pattern
+ * received matches, while the leader thread cycles sends START_CHAR/STOP_CHAR
+ * (via ioctl(TCIOFF)/ioctl(TCION).
+ *
+ *                           I/O path
+ *     Target (running this test)                Reflector
+ *
+ *     writer  ----------| |------------------------>
+ *                        ^                          \
+ *                   tty->stopped                     |
+ *                        ^                          /
+ *     reader  <-- (    input   ) ------------------
+ *                 ( processing )
+ *
+ * Expected: PASS
+ *
+ * Build:  gcc -Wall -o flow flow.c -pthread
+ *
+ * Use:
+ *   1. Connect a null-modem cable between test tty and reflector tty.
+ *
+ *   2. Confirm the test tty and reflector tty are using the same line
+ *      settings; eg., 115200n81.
+ *
+ *   3. Make sure both test and reflector serial ports are not in use;
+ *      eg., `sudo lsof /dev/ttyS0`. getty will mess up the test :)
+ *
+ *   4. Start the reflector. For example,
+ *	   stty raw -echo -iexten < /dev/ttyS1
+ *	   cat < /dev/ttyS1 > /dev/ttyS1
+ *
+ *   5. Start the test. For example,
+ *         ./flow /dev/ttyS0
+ *
+ *   6. Test terminates with EXIT_SUCCESS if tests pass. Otherwise, test
+ *      terminates with EXIT_FAILURE and diagnostic message(s).
+ *
+ * Diagnostics:
+ *     No output after 'Test flow control on /dev/ttyXXX'
+ *					Check if tty is already in use
+ *
+ *     main: opening tty: Permission denied (code: 13)
+ *					Check tty permissions or run as su
+ *
+ *     reader: FAILED, i/o stalled
+ *					Make sure line settings are same;
+ *					otherwise core or driver bug
+ */
+
+#include <stdio.h>
+#include <stdarg.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdlib.h>
+#include <termios.h>
+#include <stdint.h>
+#include <pthread.h>
+#include <time.h>		/* nanosleep */
+#include <limits.h>		/* UINT_* */
+#include <signal.h>
+
+#include "common.h"
+
+#if UINT_MAX == UINT64_MAX
+#define BITS_PER_LONG 64
+#elif UINT_MAX == UINT32_MAX
+#define BITS_PER_LONG 32
+#else
+#error BITS_PER_LONG not 32 or 64
+#endif
+
+int tty;
+struct termios *saved_termios;
+struct termios termios, orig_termios;
+int total_writes, total_reads;
+int read_distribution[10];
+static char pattern[] = ASCII_PRINTABLE;
+static size_t pattern_length = sizeof(pattern) - 1;
+
+static void restore_termios(void)
+{
+	if (saved_termios) {
+		int saved_errno = errno;
+		if (tcsetattr(tty, TCSANOW, saved_termios) < 0)
+			error_msg("tcsetattr");
+		errno = saved_errno;
+		saved_termios = NULL;
+	}
+}
+
+static void sig_handler(int sig)
+{
+	restore_termios();
+	_exit(EXIT_FAILURE);
+}
+
+static unsigned long __fls(unsigned long word)
+{
+	int num = BITS_PER_LONG - 1;
+
+#if BITS_PER_LONG == 64
+	if (!(word & (~0ul << 32))) {
+		num -= 32;
+		word <<= 32;
+	}
+#endif
+	if (!(word & (~0ul << (BITS_PER_LONG-16)))) {
+		num -= 16;
+		word <<= 16;
+	}
+	if (!(word & (~0ul << (BITS_PER_LONG-8)))) {
+		num -= 8;
+		word <<= 8;
+	}
+	if (!(word & (~0ul << (BITS_PER_LONG-4)))) {
+		num -= 4;
+		word <<= 4;
+	}
+	if (!(word & (~0ul << (BITS_PER_LONG-2)))) {
+		num -= 2;
+		word <<= 2;
+	}
+	if (!(word & (~0ul << (BITS_PER_LONG-1))))
+		num -= 1;
+	return num;
+}
+
+static int ilog2(size_t n) {
+	return __fls(n);
+}
+
+#define NSEC_IN_SEC	1000000000L
+static int uwait(long usec) {
+	struct timespec wait = { .tv_sec  = (usec * 1000) / NSEC_IN_SEC,
+				 .tv_nsec = (usec * 1000) % NSEC_IN_SEC };
+
+	while (nanosleep(&wait, &wait) == -1) {
+		if (errno == EINTR)
+			continue;
+		return -1;
+	}
+	return 0;
+}
+
+static void *reader(void *arg)
+{
+	size_t n = 0;
+	char buf[8192];
+
+	while (1) {
+		ssize_t c;
+		int r;
+
+		c = read(tty, buf, sizeof(buf));
+		if (c < 0)
+			error_exit("read");
+		if (c == 0)
+			msg_exit("FAILED, i/o stalled\n");
+
+		r = ilog2((size_t)c);
+		r = min(r, ARRAYSIZE(read_distribution) - 1);
+		read_distribution[r]++;
+
+		check_pattern(buf, c, pattern, n, pattern_length);
+
+		n += c;
+		total_reads = n / pattern_length;
+	}
+
+	return NULL;
+}
+
+static void *writer(void *arg)
+{
+	while (1) {
+		ssize_t c;
+		size_t sz = pattern_length;
+
+		c = write(tty, pattern, sz);
+		if (c < 0)
+			error_exit("write");
+		if (c != sz)
+			msg_exit("bad write: wrote:%zd (expected:%zu)\n", c, sz);
+		total_writes++;
+	}
+
+	return NULL;
+}
+
+/*
+ * test1 - start reader and writer threads
+ *
+ * Start 1 reader and 1 writer thread and trigger output flow control
+ * by sending START/STOP to the reflector.
+ *
+ * Expected: Uncorrupted i/o
+ *           tty does not become wedged
+ */
+static void test1()
+{
+ 	pthread_t id_writer;
+	pthread_t id_reader;
+	void *retval;
+	int i;
+
+	if (pthread_create(&id_reader, NULL, &reader, NULL))
+		error_exit("creating reader thread");
+	if (pthread_create(&id_writer, NULL, &writer, NULL))
+		error_exit("creating writer thread");
+
+#if 0
+	sleep(10);
+#else
+	/* TODO: Condition the loop count and timers based on baud rate */
+	for (i = 0; i < 100000; i++) {
+
+		/* TODO: These waits are too short for slow (9600) links */
+		if (uwait(100))
+			error_exit("uwait 1");
+		if (tcflow(tty, TCIOFF))
+			error_exit("tcflow(TCIOFF)");
+
+		if (uwait(100))
+			error_exit("uwait 2");
+		if (tcflow(tty, TCION))
+			error_exit("tcflow(TCION)");
+	}
+#endif
+
+	if (pthread_cancel(id_writer))
+		error_exit("cancelling writer thread");
+	if (pthread_cancel(id_reader))
+		error_exit("cancelling reader thread");
+
+	if (pthread_join(id_writer, &retval))
+		error_exit("waiting for writer thread");
+	if (retval != PTHREAD_CANCELED)
+		msg_exit("writer thread returned %p\n", retval);
+
+	if (pthread_join(id_reader, &retval))
+		error_exit("waiting for reader thread");
+	if (retval != PTHREAD_CANCELED)
+		msg_exit("reader thread returned %p\n", retval);
+}
+
+static void test(char *fname)
+{
+	printf("Test flow control on %s\n", fname);
+
+	tty = open(fname, O_RDWR);
+	if (tty < 0)
+		error_exit("opening %s", fname);
+
+	if (tcgetattr(tty, &termios) < 0)
+		error_exit("tcgetattr");
+
+	orig_termios = termios;
+	saved_termios = &orig_termios;
+
+	termios.c_lflag &= ~(ICANON | ISIG | IEXTEN | ECHO);
+	termios.c_iflag |= IXON;
+	termios.c_oflag &= ~OPOST;
+	termios.c_cc[VMIN] = 0;
+	termios.c_cc[VTIME] = 5;	/* 1/2 sec. */
+
+	if (tcsetattr(tty, TCSAFLUSH, &termios))
+		error_exit("tcsetattr");
+
+	printf("begin test1\n");
+	test1();
+
+	printf("patterns sent: %d  recvd: %d\n", total_writes, total_reads);
+	printf("read distribution: 1   = %d\n"
+	       "                   2+  = %d\n"
+	       "                   4+  = %d\n"
+	       "                   8+  = %d\n"
+	       "                  16+  = %d\n"
+	       "                  32+  = %d\n"
+	       "                  64+  = %d\n"
+	       "                 128+  = %d\n"
+	       "                 256+  = %d\n"
+	       "                 512+  = %d\n"
+	       ,
+	       read_distribution[0],
+	       read_distribution[1],
+	       read_distribution[2],
+	       read_distribution[3],
+	       read_distribution[4],
+	       read_distribution[5],
+	       read_distribution[6],
+	       read_distribution[7],
+	       read_distribution[8],
+	       read_distribution[9]);
+}
+
+static void usage(char *argv[]) {
+	msg_exit("Usage: %s tty\n"
+		 "\ttty   device filename (eg., /dev/ttyS0)\n",
+		 argv[0]);
+}
+
+int main(int argc, char* argv[])
+{
+	struct sigaction sa = { .sa_handler = sig_handler, .sa_flags = 0, };
+
+	setbuf(stdout, NULL);
+
+	if (argc < 2)
+		usage(argv);
+
+	if (atexit(restore_termios) < 0)
+		error_exit("atexit");
+
+	if (sigemptyset(&sa.sa_mask) < 0)
+		error_exit("sigemptyset");
+	if (sigaction(SIGINT, &sa, NULL) < 0)
+		error_exit("sigaction(SIGINT)");
+	if (sigaction(SIGTERM, &sa, NULL) < 0)
+		error_exit("sigaction(SIGTERM)");
+
+	test(argv[1]);
+
+	printf("PASSED\n");
+	return EXIT_SUCCESS;
+}
--- /dev/null	2015-11-20 07:19:13.265468435 -0500
+++ common.h	2015-11-20 10:58:05.514073298 -0500
@@ -0,0 +1,67 @@
+#ifndef _COMMON_H_
+#define _COMMON_H_
+
+#include <stdio.h>
+#include <stdarg.h>
+#include <errno.h>
+#include <string.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <sys/wait.h>
+#include <termios.h>
+#include <unistd.h>
+#include <stdarg.h>
+
+#define ARRAYSIZE(a)	(sizeof(a) / sizeof((a)[0]))
+#define max(a,b) \
+   ({ __typeof__ (a) _a = (a); \
+       __typeof__ (b) _b = (b); \
+     _a > _b ? _a : _b; })
+#define min(a,b) \
+   ({ __typeof__ (a) _a = (a); \
+       __typeof__ (b) _b = (b); \
+     _a < _b ? _a : _b; })
+
+#define ASCII_PRINTABLE							       \
+	/*          1         2         3         4         5		       \
+	 * 12345678901234567890123456789012345678901234567890 */	       \
+	"+_)(*&^%$#@!~=-0987654321`|}{POIUYTREWQ][poiuytrew"	  /*  50 */    \
+	"q:LKJHGFDSA;lkjhgfdsa?><MNBVCXZ/.,mnbvcxz"		  /* +41 */    \
+	"\"\'\\"						  /* + 3 */    \
+	"+_)(*&^%$#@!~=-0987654321`|}{POIUYTREWQ][poiuytrew"	  /*  50 */    \
+	"q:LKJHGFDSA;lkjhgfdsa?><MNBV\n"			  /* +29 */    \
+								  /* 173 */
+
+#define error_msg(f, ...)						\
+	fprintf(stderr, "%s: " f ": %s (code: %d)\n", __func__, ##__VA_ARGS__, \
+		strerror(errno), errno)
+
+#define msg_exit(f, ...) ({					 \
+	fprintf(stderr, "%s: " f "\n", __func__, ##__VA_ARGS__); \
+	exit(EXIT_FAILURE);					 \
+})
+
+#define error_exit(f, ...) ({		\
+	error_msg(f, ##__VA_ARGS__);	\
+	exit(EXIT_FAILURE);		\
+})
+
+/* validate buffer[0..c-1] == pattern[n..n+c-1] */
+static inline void check_pattern(const void *buf, size_t c,
+				 const void *pattern, size_t n, size_t sz)
+{
+	size_t i, j, t, len;
+
+	for (i = 0, j = n, len = c; len; i += t, j+= t, len -= t) {
+		j %= sz;
+		t = min(sz - j, len);
+		if (memcmp(buf + i, pattern + j, t) != 0)
+			msg_exit("\n\tcorrupted data: '%.*s'"
+				 "\n\t      expected: '%.*s'\n",
+				 (int)t, (char *)buf + i, (int)t,
+				 (char *)pattern + j);
+	}
+}
+
+#endif /* _COMMON_H_ */

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Sören Brinkmann]

Hi Peter,

On Fri, 2015-11-20 at 11:30AM -0500, Peter Hurley wrote:
> Hi Sören,
> 
> On 11/20/2015 10:28 AM, Sören Brinkmann wrote:
> > On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
> >> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
> >>> start_tx must start transmitting characters. Regardless of the state of
> >>> the circular buffer, always enable the transmitter hardware.
> >>
> >> Why?
> >>
> >> Does cdns_uart_stop_tx() actually stop the transmitter so that
> >> data remains in the transmitter?
> > 
> > Well, I saw my system freezing and the cause seemed to be that the UART
> > receiver and/or transmitters were disabled while the system was trying
> > to print. Hence, I started questioning all locations touching the
> > transmitter/receiver enable. I read the docs in
> > https://www.kernel.org/doc/Documentation/serial/driver, which simply
> > says "Start transmitting characters." for start_tx(). Hence, I thought,
> > this function is probably supposed to just do that and start the
> > transmitter. I'll test whether this patch can be dropped.
> 
> I don't think that patch would fix any freeze problems, but restarting
> the transmitter even if the circ buffer is empty may be necessary to
> push out remaining data when the port is restarted after being stopped.
> 
> IOW, something like
> 
> 	if (uart_tx_stopped(port))
> 		return;
> 
> 	....
> 
> 
> 	if (uart_circ_empty(&port->state->xmit)
> 		return;

Thanks! I'll change the patch accordingly.

> 
> 
> Below is a (work-in-progress) serial driver validation test for flow
> control handling (it may need some tuning for slow line speeds).
> Usual caveats apply. Takes ~40secs @ 115200.

I'll try to get that running on my system.

	Thanks,
	Sören

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Peter Hurley]

On 11/20/2015 11:58 AM, Sören Brinkmann wrote:
> On Fri, 2015-11-20 at 11:30AM -0500, Peter Hurley wrote:
>> On 11/20/2015 10:28 AM, Sören Brinkmann wrote:
>>> On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
>>>> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
>>>>> start_tx must start transmitting characters. Regardless of the state of
>>>>> the circular buffer, always enable the transmitter hardware.
>>>>
>>>> Why?
>>>>
>>>> Does cdns_uart_stop_tx() actually stop the transmitter so that
>>>> data remains in the transmitter?
>>>
>>> Well, I saw my system freezing and the cause seemed to be that the UART
>>> receiver and/or transmitters were disabled while the system was trying
>>> to print. Hence, I started questioning all locations touching the
>>> transmitter/receiver enable. I read the docs in
>>> https://www.kernel.org/doc/Documentation/serial/driver, which simply
>>> says "Start transmitting characters." for start_tx(). Hence, I thought,
>>> this function is probably supposed to just do that and start the
>>> transmitter. I'll test whether this patch can be dropped.
>>
>> I don't think that patch would fix any freeze problems, but restarting
>> the transmitter even if the circ buffer is empty may be necessary to
>> push out remaining data when the port is restarted after being stopped.
>>
>> IOW, something like
>>
>> 	if (uart_tx_stopped(port))
>> 		return;
>>
>> 	....
>>
>>
>> 	if (uart_circ_empty(&port->state->xmit)
>> 		return;
> 
> Thanks! I'll change the patch accordingly.
> 
>>
>>
>> Below is a (work-in-progress) serial driver validation test for flow
>> control handling (it may need some tuning for slow line speeds).
>> Usual caveats apply. Takes ~40secs @ 115200.
> 
> I'll try to get that running on my system.

The test below should pass too, but I know it won't because this xilinx
driver isn't handling x_char at all.

Aside: does this h/w have rts driver/cts receiver?

--- >% ---
--- /dev/null	2015-11-20 07:19:13.265468435 -0500
+++ xchar.c	2015-11-20 11:55:26.210233102 -0500
@@ -0,0 +1,354 @@
+/*
+ * x_char unit test for tty drivers
+ *
+ * Copyright (c) 2014 Peter Hurley
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ * Build:  gcc -Wall [-DWAKE_TEST] -o xchar xchar.c
+ *   The optional WAKE_TEST define includes tests for spurious write wakeups
+ *   which should not be generated by sending x_char. As of kernel mainline
+ *   v3.15, the write wakeup tests will generate false positive results.
+ *   However, serial_core UART drivers should not generate spurious write
+ *   wakeups when sending x_char, and should be tested on v3.16+ kernels.
+ *
+ * Use:
+ *   1. Connect a null-modem cable between test tty and reflector tty.
+ *
+ *   2. Confirm the test tty and reflector tty are using the same line
+ *      settings; eg., 115200n81.
+ *
+ *   3. Make sure both test and reflector serial ports are not in use;
+ *      eg., `sudo lsof /dev/ttyS0`. getty will mess up the test :)
+ *
+ *   4. Start the reflector.
+ *	   stty raw -echo -iexten < /dev/ttyS1
+ *	   cat < /dev/ttyS1 > /dev/ttyS1
+ *
+ *   5. Start the test. For example,
+ *         ./xchar /dev/ttyS0
+ *
+ *   6. Test terminates with EXIT_SUCCESS if tests pass. Otherwise, test
+ *      terminates with EXIT_FAILURE and diagnostic message(s).
+ *
+ * Diagnostics:
+ *     No output after 'Test xchar on /dev/ttyXXX'
+ *					Check if tty is already in use
+ *
+ *     main: opening tty: Permission denied (code: 13)
+ *					Check tty permissions or run as su
+ *
+ * Test results:
+ *     PASSED				Test(s) passed
+ *
+ *     test1: broken x_char: not sent	No data was received from reflector,
+ *     test2: broken x_char: not sent	x_char never sent
+ *
+ *     test1: broken x_char: n = XX, ch = XX
+ *     test2: broken x_char: n = XX, ch = XX
+ *					If n > 1, too much data was received
+ *					from reflector; only START should
+ *					be received. ch is the first char
+ *					received from reflector.
+ *
+ *     test1: spurious write wakeup detected
+ *     test2: spurious write wakeup detected
+ *					Sending x_char caused a write wakeup
+ *					(false positive on kernels <= 3.16)
+ *					False positive if the tty driver does
+ *					not declare ops->send_xchar().
+ */
+
+#include <stdio.h>
+#include <stdarg.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdlib.h>
+#include <termios.h>
+#include <limits.h>
+#include <signal.h>
+
+#ifdef WAKE_TEST
+#include <sys/epoll.h>
+
+int ep;
+#endif
+
+#include "common.h"
+
+int tty;
+struct termios *saved_termios;
+struct termios orig_termios, termios;
+static char pattern[] = ASCII_PRINTABLE;
+static size_t pattern_length = sizeof(pattern) - 1;
+
+static void restore_termios(void)
+{
+	if (saved_termios) {
+		int saved_errno = errno;
+		if (tcsetattr(tty, TCSANOW, saved_termios) < 0)
+			error_msg("tcsetattr");
+		errno = saved_errno;
+		saved_termios = NULL;
+	}
+}
+
+static void sig_handler(int sig)
+{
+	restore_termios();
+	_exit(EXIT_FAILURE);
+}
+
+#ifdef WAKE_TEST
+static void setup_wake_test()
+{
+	struct epoll_event ev = { .data = { .fd = tty, },
+				  .events = EPOLLOUT | EPOLLET,
+	};
+	int n;
+
+	/* setup epoll to detect write wakeup when sending the x_char */
+	ep = epoll_create(1);
+	if (ep < 0)
+		error_exit("epoll_create");
+	if (epoll_ctl(ep, EPOLL_CTL_ADD, tty, &ev) < 0)
+		error_exit("epoll_ctl");
+
+	n = epoll_wait(ep, &ev, 1, 0);
+	if (n < 0)
+		error_exit("epoll_wait");
+	if (n != 1 || ev.data.fd != tty || (~ev.events & EPOLLOUT))
+		msg_exit("tty not ready for write??\n");
+}
+
+static void wake_test()
+{
+	struct epoll_event ev;
+	int n;
+
+	n = epoll_wait(ep, &ev, 1, 0);
+	if (n < 0)
+		error_exit("epoll_wait");
+	if (n > 0)
+		error_msg("spurious write wakeup detected\n");
+
+	close(ep);
+}
+#else
+static void setup_wake_test() {}
+static void wake_test() {}
+#endif
+
+static void read_verify(size_t expected)
+{
+	size_t n = 0;
+	char buf[8192];
+
+	do {
+		ssize_t c;
+
+		c = read(tty, buf, sizeof(buf));
+		if (c < 0)
+			error_exit("read");
+		if (c == 0)
+			msg_exit("FAILED, i/o stalled\n");
+
+		check_pattern(buf, c, pattern, n, pattern_length);
+
+		n += c;
+
+	} while (n < expected);
+
+	if (n != expected)
+		msg_exit("FAILED, read %zu (expected %zu)\n", n, expected);
+}
+
+/**
+ * test1 - send START, idle tty
+ *
+ * Send START x_char while tty is idle (ie., not currently outputting).
+ * Uses edge-triggered epoll check to detect if write wakeup is
+ * generated (sending x_char should not generate a local write wakeup).
+ *
+ * Expected: reflector returns 1 START char.
+ *           no write wakeup detected
+ */
+static void test1(void)
+{
+	size_t n;
+	char buf[MAX_INPUT];
+
+	setup_wake_test();
+
+	/* cause START x_char to be sent */
+	if (tcflow(tty, TCION) < 0)
+		error_exit("tcflow(TCION)");
+
+	/* read the reflected START char */
+	n = read(tty, buf, sizeof(buf));
+	if (n < 0)
+		error_exit("waiting for START");
+	if (n == 0)
+		msg_exit("FAILED, broken x_char: not sent\n");
+	if (n != 1 || buf[0] != termios.c_cc[VSTART])
+		msg_exit("FAILED, broken x_char: n = %zu, ch = %hhx (expected = %hhx)\n",
+			 n, buf[0], termios.c_cc[VSTART]);
+
+	/* check for spurious write wakeup -
+	 * sending x_char should not cause a local write wakeup.
+	 * Check driver start_tx() and tx int handler for bad logic
+	 * which may perform a write wakeup.
+	 */
+	wake_test();
+}
+
+/**
+ * test2 - send START from write-stalled tty
+ *
+ * Test that sending x_char does not cause local output to restart.
+ * Send data while tty output is disabled; this adds data to the tx ring
+ * buffer. Send START x_char while tty output is still disabled.
+ *
+ * Expected: reflector returns 1 START char
+ *           no write wakeup detected
+ *           no other output is reflected, neither before nor after
+ */
+static void test2(void)
+{
+	size_t n, expected;
+	char buf[8192];
+
+	/* turn off tty output */
+	if (tcflow(tty, TCOOFF) < 0)
+		error_exit("tcflow(TCOOFF)");
+
+	expected = pattern_length;
+	n = write(tty, pattern, expected);
+	if (n < 0)
+		error_exit("write error");
+	if (n != expected)
+		msg_exit("bad write: wrote %zu (expected %zu)\n", n, expected);
+
+	n = read(tty, buf, sizeof(buf));
+	if (n < 0)
+		error_exit("read error");
+	/* test if the tty wrote even though output is turned off */
+	if (n > 0)
+		msg_exit("FAILED, broken output flow control: received data after TCOOFF\n");
+
+	setup_wake_test();
+
+	/* cause START x_char to be sent */
+	if (tcflow(tty, TCION) < 0)
+		error_exit("tcflow(TCION)");
+
+	/* read the reflected START char */
+	n = read(tty, buf, sizeof(buf));
+	if (n < 0)
+		error_exit("waiting for START");
+	if (n == 0)
+		msg_exit("FAILED, broken x_char: not sent\n");
+	if (n != 1 || buf[0] != termios.c_cc[VSTART])
+		msg_exit("FAILED, broken x_char: n = %zu, ch = %hhx (expected = %hhx)\n",
+			 n, buf[0], termios.c_cc[VSTART]);
+
+	/* check for spurious write wakeup -
+	 * sending x_char should not cause a local write wakeup.
+	 * Check driver start_tx() and tx int handler for bad logic
+	 * which may perform a write wakeup.
+	 */
+	wake_test();
+
+	/* restore tty output */
+	if (tcflow(tty, TCOON) < 0)
+		error_exit("tcflow(TCOON)");
+
+	/* verify the pattern is now received unmangled */
+	read_verify(expected);
+}
+
+static int test(char *fname)
+{
+	printf("Test xchar on %s\n", fname);
+
+	tty = open(fname, O_RDWR);
+	if (tty < 0)
+		error_exit("opening %s", fname);
+
+	if (tcgetattr(tty, &termios) < 0)
+		error_exit("tcgetattr");
+
+	orig_termios = termios;
+	saved_termios = &orig_termios;
+
+	termios.c_lflag &= ~(ICANON | ISIG | IEXTEN | ECHO);
+	/* turn off IXON so the reflector doesn't trigger our flow control */
+	termios.c_iflag &= ~IXON;
+	termios.c_oflag &= ~OPOST;
+	termios.c_cc[VMIN] = 0;
+	termios.c_cc[VTIME] = 1;	/* 1/10th sec. */
+
+	if (tcsetattr(tty, TCSAFLUSH, &termios) < 0)
+		error_exit("tcsetattr");
+
+	printf("begin test1\n");
+	test1();
+
+	/* purge i/o buffers so next test starts with empty buffers */
+	if (tcflush(tty, TCIOFLUSH) < 0)
+		error_exit("tcflush() before test2\n");
+
+	printf("begin test2\n");
+	test2();
+
+	return 0;
+}
+
+static void usage(char *argv[]) {
+	msg_exit("Usage: %s tty\n"
+		 "\ttty   device filename (eg., /dev/ttyS0)\n",
+		 argv[0]);
+}
+
+int main(int argc, char* argv[])
+{
+	struct sigaction sa = { .sa_handler = sig_handler, .sa_flags = 0, };
+
+	setbuf(stdout, NULL);
+
+	if (argc < 2)
+		usage(argv);
+
+	if (atexit(restore_termios) < 0)
+		error_exit("atexit");
+
+	if (sigemptyset(&sa.sa_mask) < 0)
+		error_exit("sigemptyset");
+	if (sigaction(SIGINT, &sa, NULL) < 0)
+		error_exit("sigaction(SIGINT)");
+	if (sigaction(SIGTERM, &sa, NULL) < 0)
+		error_exit("sigaction(SIGTERM)");
+
+	test(argv[1]);
+
+	printf("PASSED\n");
+	return EXIT_SUCCESS;
+}

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Sören Brinkmann]

On Fri, 2015-11-20 at 12:16PM -0500, Peter Hurley wrote:
> On 11/20/2015 11:58 AM, Sören Brinkmann wrote:
> > On Fri, 2015-11-20 at 11:30AM -0500, Peter Hurley wrote:
> >> On 11/20/2015 10:28 AM, Sören Brinkmann wrote:
> >>> On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
> >>>> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
> >>>>> start_tx must start transmitting characters. Regardless of the state of
> >>>>> the circular buffer, always enable the transmitter hardware.
> >>>>
> >>>> Why?
> >>>>
> >>>> Does cdns_uart_stop_tx() actually stop the transmitter so that
> >>>> data remains in the transmitter?
> >>>
> >>> Well, I saw my system freezing and the cause seemed to be that the UART
> >>> receiver and/or transmitters were disabled while the system was trying
> >>> to print. Hence, I started questioning all locations touching the
> >>> transmitter/receiver enable. I read the docs in
> >>> https://www.kernel.org/doc/Documentation/serial/driver, which simply
> >>> says "Start transmitting characters." for start_tx(). Hence, I thought,
> >>> this function is probably supposed to just do that and start the
> >>> transmitter. I'll test whether this patch can be dropped.
> >>
> >> I don't think that patch would fix any freeze problems, but restarting
> >> the transmitter even if the circ buffer is empty may be necessary to
> >> push out remaining data when the port is restarted after being stopped.
> >>
> >> IOW, something like
> >>
> >> 	if (uart_tx_stopped(port))
> >> 		return;
> >>
> >> 	....
> >>
> >>
> >> 	if (uart_circ_empty(&port->state->xmit)
> >> 		return;
> > 
> > Thanks! I'll change the patch accordingly.
> > 
> >>
> >>
> >> Below is a (work-in-progress) serial driver validation test for flow
> >> control handling (it may need some tuning for slow line speeds).
> >> Usual caveats apply. Takes ~40secs @ 115200.
> > 
> > I'll try to get that running on my system.
> 
> The test below should pass too, but I know it won't because this xilinx
> driver isn't handling x_char at all.
> 
> Aside: does this h/w have rts driver/cts receiver?

I would have to look up the details. But, IIRC, the HW has all the
modem/flow control signals. But on our SoCs those signals are only
available when routing the UART through the FPGA part, which makes it a
rather unlikely configuration since most platforms want the UART as a
low level debug port that should not depend on any FPGA bistreams.
Hence, I suspect it might just be a limitation of the driver.

	Sören

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Michal Simek]

Hi Peter,

On 20.11.2015 18:16, Peter Hurley wrote:
> On 11/20/2015 11:58 AM, Sören Brinkmann wrote:
>> On Fri, 2015-11-20 at 11:30AM -0500, Peter Hurley wrote:
>>> On 11/20/2015 10:28 AM, Sören Brinkmann wrote:
>>>> On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
>>>>> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
>>>>>> start_tx must start transmitting characters. Regardless of the state of
>>>>>> the circular buffer, always enable the transmitter hardware.
>>>>>
>>>>> Why?
>>>>>
>>>>> Does cdns_uart_stop_tx() actually stop the transmitter so that
>>>>> data remains in the transmitter?
>>>>
>>>> Well, I saw my system freezing and the cause seemed to be that the UART
>>>> receiver and/or transmitters were disabled while the system was trying
>>>> to print. Hence, I started questioning all locations touching the
>>>> transmitter/receiver enable. I read the docs in
>>>> https://www.kernel.org/doc/Documentation/serial/driver, which simply
>>>> says "Start transmitting characters." for start_tx(). Hence, I thought,
>>>> this function is probably supposed to just do that and start the
>>>> transmitter. I'll test whether this patch can be dropped.
>>>
>>> I don't think that patch would fix any freeze problems, but restarting
>>> the transmitter even if the circ buffer is empty may be necessary to
>>> push out remaining data when the port is restarted after being stopped.
>>>
>>> IOW, something like
>>>
>>> 	if (uart_tx_stopped(port))
>>> 		return;
>>>
>>> 	....
>>>
>>>
>>> 	if (uart_circ_empty(&port->state->xmit)
>>> 		return;
>>
>> Thanks! I'll change the patch accordingly.
>>
>>>
>>>
>>> Below is a (work-in-progress) serial driver validation test for flow
>>> control handling (it may need some tuning for slow line speeds).
>>> Usual caveats apply. Takes ~40secs @ 115200.
>>
>> I'll try to get that running on my system.
> 
> The test below should pass too, but I know it won't because this xilinx
> driver isn't handling x_char at all.
> 
> Aside: does this h/w have rts driver/cts receiver?
> 
> --- >% ---
> --- /dev/null	2015-11-20 07:19:13.265468435 -0500
> +++ xchar.c	2015-11-20 11:55:26.210233102 -0500
> @@ -0,0 +1,354 @@
> +/*
> + * x_char unit test for tty drivers

All these tests looks very interesting. Do you have any any
work-in-progress repo with other tests? It will be good to run all of
them to validate our drivers.

Thanks,
Michal

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Peter Hurley]

Hi Michal,

On 11/23/2015 02:05 AM, Michal Simek wrote:
> All these tests looks very interesting. Do you have any any
> work-in-progress repo with other tests? It will be good to run all of
> them to validate our drivers.

I haven't upstreamed these yet, but I plan to do so soon, along with
tty core, pty, n_tty i/o, termios, and torture tests and some basic tools.

Regards,
Peter Hurley

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Michal Simek]

Hi Peter,

On 23.11.2015 21:00, Peter Hurley wrote:
> Hi Michal,
> 
> On 11/23/2015 02:05 AM, Michal Simek wrote:
>> All these tests looks very interesting. Do you have any any
>> work-in-progress repo with other tests? It will be good to run all of
>> them to validate our drivers.
> 
> I haven't upstreamed these yet, but I plan to do so soon, along with
> tty core, pty, n_tty i/o, termios, and torture tests and some basic tools.

great. Can you please keep me in the loop?

Thanks,
Michal

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Sören Brinkmann]

On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
> 
> 
> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
> > start_tx must start transmitting characters. Regardless of the state of
> > the circular buffer, always enable the transmitter hardware.
> 
> Why?
> 
> Does cdns_uart_stop_tx() actually stop the transmitter so that
> data remains in the transmitter?

Fixing up the patch, I looked at this one. It might actually do that.
Without having changed anything. The doc says: "The driver should
stop transmitting characters as soon as possible.". And the
implementation is really not draining any FIFO, but just disabling the
transmitter. I take your question as that this might not be this way?
Should stop_tx drain the FIFO first?

	Thanks,
	Sören

Re: [PATH RESEND v2 03/10] tty: xuartps: Always enable transmitter in start_tx

[Peter Hurley]

On 11/20/2015 12:05 PM, Sören Brinkmann wrote:
> On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
>>
>>
>> On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
>>> start_tx must start transmitting characters. Regardless of the state of
>>> the circular buffer, always enable the transmitter hardware.
>>
>> Why?
>>
>> Does cdns_uart_stop_tx() actually stop the transmitter so that
>> data remains in the transmitter?
> 
> Fixing up the patch, I looked at this one. It might actually do that.

Ok.

> Without having changed anything. The doc says: "The driver should
> stop transmitting characters as soon as possible.". And the
> implementation is really not draining any FIFO, but just disabling the
> transmitter. I take your question as that this might not be this way?
> Should stop_tx drain the FIFO first?

No.

Most h/w can't actually stop the transmitter (or not without losing
data), so that's why the expectation is only for "as soon as possible".
Stopping sooner is better.

Regards,
Peter Hurley

[PATH RESEND v2 04/10] tty: xuartps: Clear interrupt status register in shutdown

[Soren Brinkmann]

When shutting down the UART, clear the interrupt status register.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index df6778d17949..b7fc30639292 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -825,6 +825,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable interrupts */
 	status = readl(port->membase + CDNS_UART_IMR_OFFSET);
 	writel(status, port->membase + CDNS_UART_IDR_OFFSET);
+	writel(0xffffffff, port->membase + CDNS_UART_ISR_OFFSET);
 
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 05/10] tty: xuartps: Improve startup function

[Soren Brinkmann]

The startup function is supposed to initialize the UART for receiving.
Hence, don't enable the TX part. Also, protect HW accesses with the port
lock.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index b7fc30639292..167e0f4bcf7a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -755,6 +755,7 @@ static void cdns_uart_set_termios(struct uart_port *port,
  */
 static int cdns_uart_startup(struct uart_port *port)
 {
+	unsigned long flags;
 	unsigned int retval = 0, status = 0;
 
 	retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME,
@@ -762,6 +763,8 @@ static int cdns_uart_startup(struct uart_port *port)
 	if (retval)
 		return retval;
 
+	spin_lock_irqsave(&port->lock, flags);
+
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
@@ -772,15 +775,14 @@ static int cdns_uart_startup(struct uart_port *port)
 	writel(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST,
 			port->membase + CDNS_UART_CR_OFFSET);
 
-	status = readl(port->membase + CDNS_UART_CR_OFFSET);
-
-	/* Clear the RX disable and TX disable bits and then set the TX enable
-	 * bit and RX enable bit to enable the transmitter and receiver.
+	/*
+	 * Clear the RX disable bit and then set the RX enable bit to enable
+	 * the receiver.
 	 */
-	writel((status & ~(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS))
-			| (CDNS_UART_CR_TX_EN | CDNS_UART_CR_RX_EN |
-			CDNS_UART_CR_STOPBRK),
-			port->membase + CDNS_UART_CR_OFFSET);
+	status = readl(port->membase + CDNS_UART_CR_OFFSET);
+	status &= CDNS_UART_CR_RX_DIS;
+	status |= CDNS_UART_CR_RX_EN;
+	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
 	/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
 	 * no parity.
@@ -811,6 +813,8 @@ static int cdns_uart_startup(struct uart_port *port)
 		CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT,
 		port->membase + CDNS_UART_IER_OFFSET);
 
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	return retval;
 }
 
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 06/10] tty: xuartps: Keep lock for whole ISR

[Soren Brinkmann]

The RX path in the interrupt handler released a lock unnecessarily.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 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 167e0f4bcf7a..5efd1b9d220f 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -265,9 +265,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
 					data, status);
 		}
-		spin_unlock(&port->lock);
 		tty_flip_buffer_push(&port->state->port);
-		spin_lock(&port->lock);
 	}
 
 	/* Dispatch an appropriate handler */
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 07/10] tty: xuartps: Acquire port lock for shutdown

[Soren Brinkmann]

Shutting down the UART port can happen while console operations are in
progress. Holding the port lock serializes these operations and avoids
the UART HW to be disabled in the middle of console prints.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 5efd1b9d220f..7fd226d65496 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -823,6 +823,9 @@ static int cdns_uart_startup(struct uart_port *port)
 static void cdns_uart_shutdown(struct uart_port *port)
 {
 	int status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Disable interrupts */
 	status = readl(port->membase + CDNS_UART_IMR_OFFSET);
@@ -832,6 +835,9 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	free_irq(port->irq, port);
 }
 
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 08/10] tty: xuartps: Move RX path into helper function

[Soren Brinkmann]

Move RX-related IRQ handling into a helper function.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 50 +++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 7fd226d65496..aa1176e6661a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -172,28 +172,8 @@ struct cdns_uart {
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb);
 
-/**
- * cdns_uart_isr - Interrupt handler
- * @irq: Irq number
- * @dev_id: Id of the port
- *
- * Return: IRQHANDLED
- */
-static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 {
-	struct uart_port *port = (struct uart_port *)dev_id;
-	unsigned long flags;
-	unsigned int isrstatus, numbytes;
-	unsigned int data;
-	char status = TTY_NORMAL;
-
-	spin_lock_irqsave(&port->lock, flags);
-
-	/* Read the interrupt status register to determine which
-	 * interrupt(s) is/are active.
-	 */
-	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
-
 	/*
 	 * There is no hardware break detection, so we interpret framing
 	 * error with all-zeros data as a break sequence. Most of the time,
@@ -223,6 +203,9 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 		/* Receive Timeout Interrupt */
 		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
 					CDNS_UART_SR_RXEMPTY)) {
+			u32 data;
+			char status = TTY_NORMAL;
+
 			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
 
 			/* Non-NULL byte after BREAK is garbage (99%) */
@@ -263,10 +246,33 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 			}
 
 			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
-					data, status);
+					 data, status);
 		}
 		tty_flip_buffer_push(&port->state->port);
 	}
+}
+
+/**
+ * cdns_uart_isr - Interrupt handler
+ * @irq: Irq number
+ * @dev_id: Id of the port
+ *
+ * Return: IRQHANDLED
+ */
+static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+{
+	struct uart_port *port = (struct uart_port *)dev_id;
+	unsigned long flags;
+	unsigned int isrstatus, numbytes;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	/* Read the interrupt status register to determine which
+	 * interrupt(s) is/are active.
+	 */
+	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
+
+	cdns_uart_handle_rx(port, isrstatus);
 
 	/* Dispatch an appropriate handler */
 	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled

[Soren Brinkmann]

Ignore RX-related interrupts if RX is not enabled.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index aa1176e6661a..413229661f5d 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -161,6 +161,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
  * @pclk:		APB clock
  * @baud:		Current baud rate
  * @clk_rate_change_nb:	Notifier block for clock changes
+ * @flags:		Driver flags
  */
 struct cdns_uart {
 	struct uart_port	*port;
@@ -168,7 +169,11 @@ struct cdns_uart {
 	struct clk		*pclk;
 	unsigned int		baud;
 	struct notifier_block	clk_rate_change_nb;
+	u32			flags;
 };
+
+#define CDNS_FLAG_RX_ENABLED	BIT(0)
+
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb);
 
@@ -262,6 +267,7 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 {
 	struct uart_port *port = (struct uart_port *)dev_id;
+	struct cdns_uart *cdns_uart = port->private_data;
 	unsigned long flags;
 	unsigned int isrstatus, numbytes;
 
@@ -272,7 +278,8 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 	 */
 	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
 
-	cdns_uart_handle_rx(port, isrstatus);
+	if (cdns_uart->flags & CDNS_FLAG_RX_ENABLED)
+		cdns_uart_handle_rx(port, isrstatus);
 
 	/* Dispatch an appropriate handler */
 	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
@@ -576,11 +583,13 @@ static void cdns_uart_stop_tx(struct uart_port *port)
 static void cdns_uart_stop_rx(struct uart_port *port)
 {
 	unsigned int regval;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	regval = readl(port->membase + CDNS_UART_CR_OFFSET);
 	regval |= CDNS_UART_CR_RX_DIS;
 	/* Disable the receiver */
 	writel(regval, port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
 }
 
 /**
@@ -761,6 +770,7 @@ static int cdns_uart_startup(struct uart_port *port)
 {
 	unsigned long flags;
 	unsigned int retval = 0, status = 0;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME,
 								(void *)port);
@@ -787,6 +797,7 @@ static int cdns_uart_startup(struct uart_port *port)
 	status &= CDNS_UART_CR_RX_DIS;
 	status |= CDNS_UART_CR_RX_EN;
 	writel(status, port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags |= CDNS_FLAG_RX_ENABLED;
 
 	/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
 	 * no parity.
@@ -830,6 +841,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 {
 	int status;
 	unsigned long flags;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -841,6 +853,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
 
 	spin_unlock_irqrestore(&port->lock, flags);
 
-- 
2.6.3.3.g9bb996a

[PATH RESEND v2 10/10] tty: xuartps: Cleanup: Reformat if-else

[Soren Brinkmann]

Convert an if-else into the more common early return on error, reducing
the indent level of the happy path.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v2:
 - added this patch
---
 drivers/tty/serial/xilinx_uartps.c | 124 ++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 413229661f5d..aed8a31f3399 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -203,58 +203,55 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 	isrstatus &= port->read_status_mask;
 	isrstatus &= ~port->ignore_status_mask;
 
-	if ((isrstatus & CDNS_UART_IXR_TOUT) ||
-		(isrstatus & CDNS_UART_IXR_RXTRIG)) {
-		/* Receive Timeout Interrupt */
-		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
-					CDNS_UART_SR_RXEMPTY)) {
-			u32 data;
-			char status = TTY_NORMAL;
-
-			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
-
-			/* Non-NULL byte after BREAK is garbage (99%) */
-			if (data && (port->read_status_mask &
-						CDNS_UART_IXR_BRK)) {
-				port->read_status_mask &= ~CDNS_UART_IXR_BRK;
-				port->icount.brk++;
-				if (uart_handle_break(port))
-					continue;
-			}
+	if (!(isrstatus & (CDNS_UART_IXR_TOUT | CDNS_UART_IXR_RXTRIG)))
+		return;
+
+	while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
+				CDNS_UART_SR_RXEMPTY)) {
+		u32 data;
+		char status = TTY_NORMAL;
+
+		data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+
+		/* Non-NULL byte after BREAK is garbage (99%) */
+		if (data && (port->read_status_mask & CDNS_UART_IXR_BRK)) {
+			port->read_status_mask &= ~CDNS_UART_IXR_BRK;
+			port->icount.brk++;
+			if (uart_handle_break(port))
+				continue;
+		}
 
 #ifdef SUPPORT_SYSRQ
-			/*
-			 * uart_handle_sysrq_char() doesn't work if
-			 * spinlocked, for some reason
-			 */
-			 if (port->sysrq) {
-				spin_unlock(&port->lock);
-				if (uart_handle_sysrq_char(port,
-							(unsigned char)data)) {
-					spin_lock(&port->lock);
-					continue;
-				}
+		/*
+		 * uart_handle_sysrq_char() doesn't work if
+		 * spinlocked, for some reason
+		 */
+		 if (port->sysrq) {
+			spin_unlock(&port->lock);
+			if (uart_handle_sysrq_char(port, data)) {
 				spin_lock(&port->lock);
+				continue;
 			}
+			spin_lock(&port->lock);
+		}
 #endif
 
-			port->icount.rx++;
-
-			if (isrstatus & CDNS_UART_IXR_PARITY) {
-				port->icount.parity++;
-				status = TTY_PARITY;
-			} else if (isrstatus & CDNS_UART_IXR_FRAMING) {
-				port->icount.frame++;
-				status = TTY_FRAME;
-			} else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
-				port->icount.overrun++;
-			}
+		port->icount.rx++;
 
-			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
-					 data, status);
+		if (isrstatus & CDNS_UART_IXR_PARITY) {
+			port->icount.parity++;
+			status = TTY_PARITY;
+		} else if (isrstatus & CDNS_UART_IXR_FRAMING) {
+			port->icount.frame++;
+			status = TTY_FRAME;
+		} else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
+			port->icount.overrun++;
 		}
-		tty_flip_buffer_push(&port->state->port);
+
+		uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
+				 data, status);
 	}
+	tty_flip_buffer_push(&port->state->port);
 }
 
 /**
@@ -1431,27 +1428,30 @@ static int cdns_uart_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Cannot get uart_port structure\n");
 		rc = -ENODEV;
 		goto err_out_notif_unreg;
-	} else {
-		/* Register the port.
-		 * This function also registers this device with the tty layer
-		 * and triggers invocation of the config_port() entry point.
-		 */
-		port->mapbase = res->start;
-		port->irq = irq;
-		port->dev = &pdev->dev;
-		port->uartclk = clk_get_rate(cdns_uart_data->uartclk);
-		port->private_data = cdns_uart_data;
-		cdns_uart_data->port = port;
-		platform_set_drvdata(pdev, port);
-		rc = uart_add_one_port(&cdns_uart_uart_driver, port);
-		if (rc) {
-			dev_err(&pdev->dev,
-				"uart_add_one_port() failed; err=%i\n", rc);
-			goto err_out_notif_unreg;
-		}
-		return 0;
 	}
 
+	/*
+	 * Register the port.
+	 * This function also registers this device with the tty layer
+	 * and triggers invocation of the config_port() entry point.
+	 */
+	port->mapbase = res->start;
+	port->irq = irq;
+	port->dev = &pdev->dev;
+	port->uartclk = clk_get_rate(cdns_uart_data->uartclk);
+	port->private_data = cdns_uart_data;
+	cdns_uart_data->port = port;
+	platform_set_drvdata(pdev, port);
+
+	rc = uart_add_one_port(&cdns_uart_uart_driver, port);
+	if (rc) {
+		dev_err(&pdev->dev,
+			"uart_add_one_port() failed; err=%i\n", rc);
+		goto err_out_notif_unreg;
+	}
+
+	return 0;
+
 err_out_notif_unreg:
 #ifdef CONFIG_COMMON_CLK
 	clk_notifier_unregister(cdns_uart_data->uartclk,
-- 
2.6.3.3.g9bb996a