Thread (23 messages) 23 messages, 3 authors, 2015-11-24

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

From: Peter Hurley <hidden>
Date: 2015-11-20 17:16:42
Also in: linux-serial, lkml

On 11/20/2015 11:58 AM, S?ren Brinkmann wrote:
On Fri, 2015-11-20 at 11:30AM -0500, Peter Hurley wrote:
quoted
On 11/20/2015 10:28 AM, S?ren Brinkmann wrote:
quoted
On Fri, 2015-11-20 at 07:13AM -0500, Peter Hurley wrote:
quoted
On 11/19/2015 03:02 PM, Soren Brinkmann wrote:
quoted
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.
quoted

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;
+}
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help