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; +} -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
