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

[Peter Hurley <peter@hurleysoftware.com>]

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;
+}