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