On Tue, Mar 05, 2019 at 11:12:31AM -0600, [email protected] wrote:
> From: Corey Minyard <[email protected]>
>
> This creates simulated serial ports, both as echo devices and pipe
> devices. The driver reasonably approximates the serial port speed
> and simulates some modem control lines. It allows error injection
> and direct control of modem control lines.
I like this, thanks for doing it!
Minor nits below:
> +You can modify null modem and control the lines individually
> +through an interface in /sys/class/tty/ttyECHO<n>/ctrl,
> +/sys/class/tty/ttyPipeA<n>/ctrl, and
> +/sys/class/tty/ttyPipeB<n>/ctrl. The following may be written to
> +those files:
> +
> +[+-]nullmodem
> + enable/disable null modem
> +
> +[+-]cd
> + enable/disable Carrier Detect (no effect if +nullmodem)
> +
> +[+-]dsr
> + enable/disable Data Set Ready (no effect if +nullmodem)
> +
> +[+-]cts
> + enable/disable Clear To Send (no effect if +nullmodem)
> +
> +[+-]ring
> + enable/disable Ring
> +
> +frame
> + inject a frame error on the next byte
> +
> +parity
> + inject a parity error on the next byte
> +
> +overrun
> + inject an overrun error on the next byte
> +
> +The contents of the above files has the following format:
> +
> +tty[Echo|PipeA|PipeB]<n>
> + <mctrl values>
> +
> +where <mctrl values> is the modem control values above (not frame,
> +parity, or overrun) with the following added:
> +
> +[+-]dtr
> + value of the Data Terminal Ready
> +
> +[+-]rts
> + value of the Request To Send
> +
> +The above values are not settable through this interface, they are
> +set through the serial port interface itself.
> +
> +So, for instance, ttyEcho0 comes up in the following state::
> +
> + # cat /sys/class/tty/ttyEcho0/ctrl
> + ttyEcho0: +nullmodem -cd -dsr -cts -ring -dtr -rts
> +
> +If something connects, it will become::
> +
> + ttyEcho0: +nullmodem +cd +dsr +cts -ring +dtr +rts
> +
> +To enable ring::
> +
> + # echo "+ring" >/sys/class/tty/ttyEcho0/ctrl
> + # cat /sys/class/tty/ttyEcho0/ctrl
> + ttyEcho0: +nullmodem +cd +dsr +cts +ring +dtr +rts
> +
> +Now disable NULL modem and the CD line::
> +
> + # echo "-nullmodem -cd" >/sys/class/tty/ttyEcho0/ctrl
> + # cat /sys/class/tty/ttyEcho0/ctrl
> + ttyEcho0: -nullmodem -cd -dsr -cts +ring -dtr -rts
> +
> +Note that these settings are for the side you are modifying. So if
> +you set nullmodem on ttyPipeA0, that controls whether the DTR/RTS
> +lines from ttyPipeB0 affect ttyPipeA0. It doesn't affect ttyPipeB's
> +modem control lines.
All of the sysfs stuff needs to also go in Documentation/ABI to describe
your new files.
> +config SERIAL_SIMULATOR
> + tristate "Serial port simulator"
> + default n
n is the default, no need to say it again here.
> --- /dev/null
> +++ b/drivers/tty/serial/serialsim.c
> @@ -0,0 +1,1101 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * serialsim - Emulate a serial device in a loopback and/or pipe
> + *
> + * See the docs for this for more details.
Pointer to the docs?
And no copyright notice? I don't object to it, but it is usually nice
to see.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/serial.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/device.h>
> +#include <linux/serial_core.h>
> +#include <linux/kthread.h>
> +#include <linux/hrtimer.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/ctype.h>
> +#include <linux/string.h>
> +#include <linux/uaccess.h>
> +
> +#include <linux/serialsim.h>
> +
> +#define PORT_SERIALECHO 72549
> +#define PORT_SERIALPIPEA 72550
> +#define PORT_SERIALPIPEB 72551
tabs?
> +
> +#ifdef CONFIG_HIGH_RES_TIMERS
> +#define SERIALSIM_WAKES_PER_SEC 1000
> +#else
> +#define SERIALSIM_WAKES_PER_SEC HZ
> +#endif
> +
> +#define SERIALSIM_XBUFSIZE 32
> +
> +/* For things to send on the line, in flags field. */
> +#define DO_FRAME_ERR (1 << TTY_FRAME)
> +#define DO_PARITY_ERR (1 << TTY_PARITY)
> +#define DO_OVERRUN_ERR (1 << TTY_OVERRUN)
> +#define DO_BREAK (1 << TTY_BREAK)
> +#define FLAGS_MASK (DO_FRAME_ERR | DO_PARITY_ERR | DO_OVERRUN_ERR | DO_BREAK)
> +
> +struct serialsim_intf {
> + struct uart_port port;
> +
> + /*
> + * This is my transmit buffer, my thread picks this up and
> + * injects them into the other port's uart.
> + */
> + unsigned char xmitbuf[SERIALSIM_XBUFSIZE];
> + struct circ_buf buf;
> +
> + /* Error flags to send. */
> + bool break_reported;
> + unsigned int flags;
> +
> + /* Modem state. */
> + unsigned int mctrl;
> + bool do_null_modem;
> + spinlock_t mctrl_lock;
> + struct tasklet_struct mctrl_tasklet;
> +
> + /* My transmitter is enabled. */
> + bool tx_enabled;
> +
> + /* I can receive characters. */
> + bool rx_enabled;
> +
> + /* Is the port registered with the uart driver? */
> + bool registered;
> +
> + /*
> + * The serial echo port on the other side of this pipe (or points
> + * to myself in loopback mode.
> + */
> + struct serialsim_intf *ointf;
> +
> + unsigned int div;
> + unsigned int bytes_per_interval;
> + unsigned int per_interval_residual;
> +
> + struct ktermios termios;
> +
> + const char *threadname;
> + struct task_struct *thread;
> +
> + struct serial_rs485 rs485;
> +};
> +
> +#define circ_sbuf_space(buf) CIRC_SPACE((buf)->head, (buf)->tail, \
> + SERIALSIM_XBUFSIZE)
> +#define circ_sbuf_empty(buf) ((buf)->head == (buf)->tail)
> +#define circ_sbuf_next(idx) (((idx) + 1) % SERIALSIM_XBUFSIZE)
Don't we have a ring buffer (or 3) structure already? Did you create
another one?
> +static void serialsim_thread_delay(void)
> +{
> +#ifdef CONFIG_HIGH_RES_TIMERS
> + ktime_t timeout;
> +
> + timeout = ns_to_ktime(1000000000 / SERIALSIM_WAKES_PER_SEC);
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_hrtimeout(&timeout, HRTIMER_MODE_REL);
> +#else
> + schedule_timeout_interruptible(1);
> +#endif
> +}
Why do you care about hires timers here? Why not always just sleep to
slow things down?
> +
> +static int serialsim_thread(void *data)
> +{
> + struct serialsim_intf *intf = data;
> + struct serialsim_intf *ointf = intf->ointf;
> + struct uart_port *port = &intf->port;
> + struct uart_port *oport = &ointf->port;
> + struct circ_buf *tbuf = &intf->buf;
> + unsigned int residual = 0;
> +
> + while (!kthread_should_stop()) {
Aren't we trying to get away from creating new kthreads?
Can you just use a workqueue entry?
> +static unsigned int nr_echo_ports = 4;
> +module_param(nr_echo_ports, uint, 0444);
> +MODULE_PARM_DESC(nr_echo_ports,
> + "The number of echo ports to create. Defaults to 4");
> +
> +static unsigned int nr_pipe_ports = 4;
> +module_param(nr_pipe_ports, uint, 0444);
> +MODULE_PARM_DESC(nr_pipe_ports,
> + "The number of pipe ports to create. Defaults to 4");
No way to just do this with configfs and not worry about module params?
> +static char *gettok(char **s)
> +{
> + char *t = skip_spaces(*s);
> + char *p = t;
> +
> + while (*p && !isspace(*p))
> + p++;
> + if (*p)
> + *p++ = '\0';
> + *s = p;
> +
> + return t;
> +}
We don't have this already in the tree?
> +static bool tokeq(const char *t, const char *m)
> +{
> + return strcmp(t, m) == 0;
> +}
same here.
> +
> +static unsigned int parse_modem_line(char op, unsigned int flag,
> + unsigned int *mctrl)
> +{
> + if (op == '+')
> + *mctrl |= flag;
> + else
> + *mctrl &= ~flag;
> + return flag;
> +}
> +
> +static void serialsim_ctrl_append(char **val, int *left, char *n, bool
> enabled)
> +{
> + int count;
> +
> + count = snprintf(*val, *left, " %c%s", enabled ? '+' : '-', n);
> + *left -= count;
> + *val += count;
> +}
sysfs files really should only be "one value per file", so this api is
troubling :(
> +static ssize_t serialsim_ctrl_read(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct tty_port *tport = dev_get_drvdata(dev);
> + struct uart_state *state = container_of(tport, struct uart_state, port);
> + struct uart_port *port = state->uart_port;
> + struct serialsim_intf *intf = serialsim_port_to_intf(port);
> + unsigned int mctrl = intf->mctrl;
> + char *val = buf;
> + int left = PAGE_SIZE;
> + int count;
> +
> + count = snprintf(val, left, "%s:", dev->kobj.name);
dev_name()?
And is it really needed? It's already known as this is the sysfs file
for that device. Don't make userspace parse it away.
> + val += count;
> + left -= count;
> + serialsim_ctrl_append(&val, &left, "nullmodem", intf->do_null_modem);
> + serialsim_ctrl_append(&val, &left, "cd", mctrl & TIOCM_CAR);
> + serialsim_ctrl_append(&val, &left, "dsr", mctrl & TIOCM_DSR);
> + serialsim_ctrl_append(&val, &left, "cts", mctrl & TIOCM_CTS);
> + serialsim_ctrl_append(&val, &left, "ring", mctrl & TIOCM_RNG);
> + serialsim_ctrl_append(&val, &left, "dtr", mctrl & TIOCM_DTR);
> + serialsim_ctrl_append(&val, &left, "rts", mctrl & TIOCM_RTS);
> + *val++ = '\n';
> +
> + return val - buf;
> +}
> +
> +static ssize_t serialsim_ctrl_write(struct device *dev,
> + struct device_attribute *attr,
> + const char *val, size_t count)
> +{
> + struct tty_port *tport = dev_get_drvdata(dev);
> + struct uart_state *state = container_of(tport, struct uart_state, port);
> + struct uart_port *port = state->uart_port;
> + struct serialsim_intf *intf = serialsim_port_to_intf(port);
> + char *str = kstrndup(val, count, GFP_KERNEL);
> + char *p, *s = str;
> + int rv = count;
> + unsigned int flags = 0;
> + unsigned int nullmodem = 0;
> + unsigned int mctrl_mask = 0, mctrl = 0;
> +
> + if (!str)
> + return -ENOMEM;
> +
> + p = gettok(&s);
> + while (*p) {
> + char op = '\0';
> + int err = 0;
> +
> + switch (*p) {
> + case '+':
> + case '-':
> + op = *p++;
> + break;
> + default:
> + break;
> + }
> +
> + if (tokeq(p, "frame"))
> + flags |= DO_FRAME_ERR;
> + else if (tokeq(p, "parity"))
> + flags |= DO_PARITY_ERR;
> + else if (tokeq(p, "overrun"))
> + flags |= DO_OVERRUN_ERR;
> + else if (tokeq(p, "nullmodem"))
> + nullmodem = op;
> + else if (tokeq(p, "dsr"))
> + mctrl_mask |= parse_modem_line(op, TIOCM_DSR, &mctrl);
> + else if (tokeq(p, "cts"))
> + mctrl_mask |= parse_modem_line(op, TIOCM_CTS, &mctrl);
> + else if (tokeq(p, "cd"))
> + mctrl_mask |= parse_modem_line(op, TIOCM_CAR, &mctrl);
> + else if (tokeq(p, "ring"))
> + mctrl_mask |= parse_modem_line(op, TIOCM_RNG, &mctrl);
> + else
> + err = -EINVAL;
> +
> + if (err) {
> + rv = err;
> + goto out;
> + }
> + p = gettok(&s);
> + }
> +
> + if (flags)
> + serialsim_set_flags(intf, flags);
> + if (nullmodem)
> + serialsim_set_null_modem(intf, nullmodem == '+');
> + if (mctrl_mask)
> + serialsim_set_modem_lines(intf, mctrl_mask, mctrl);
> +
> +out:
> + kfree(str);
> +
> + return rv;
> +}
This worries me, parsing sysfs files is ripe for problems. configfs
might be better here.
> +
> +static DEVICE_ATTR(ctrl, 0660, serialsim_ctrl_read, serialsim_ctrl_write);
DEVICE_ATTR_RW()?
> +
> +static struct attribute *serialsim_dev_attrs[] = {
> + &dev_attr_ctrl.attr,
> + NULL,
> +};
> +
> +static struct attribute_group serialsim_dev_attr_group = {
> + .attrs = serialsim_dev_attrs,
> +};
ATTRIBUTE_GROUPS()?
> +static int __init serialsim_init(void)
> +{
> + unsigned int i;
> + int rv;
> +
> + serialecho_ports = kcalloc(nr_echo_ports,
> + sizeof(*serialecho_ports),
> + GFP_KERNEL);
> + if (!serialecho_ports) {
> + pr_err("serialsim: Unable to allocate echo ports.\n");
No need to print error for memory failure.
> + rv = ENOMEM;
-ENOMEM?
> + goto out;
> + }
> +
> + serialpipe_ports = kcalloc(nr_pipe_ports * 2,
> + sizeof(*serialpipe_ports),
> + GFP_KERNEL);
> + if (!serialpipe_ports) {
> + kfree(serialecho_ports);
> + pr_err("serialsim: Unable to allocate pipe ports.\n");
Same here.
> + rv = ENOMEM;
-ENOMEM?
> + goto out;
Shouldn't out clean up stuff?
> + }
> +
> + serialecho_driver.nr = nr_echo_ports;
> + rv = uart_register_driver(&serialecho_driver);
> + if (rv) {
> + kfree(serialecho_ports);
> + kfree(serialpipe_ports);
> + pr_err("serialsim: Unable to register driver.\n");
> + goto out;
Again, out should clean up stuff for an error. Will make the code below
simpler.
> + }
> +
> + serialpipea_driver.nr = nr_pipe_ports;
> + rv = uart_register_driver(&serialpipea_driver);
> + if (rv) {
> + uart_unregister_driver(&serialecho_driver);
> + kfree(serialecho_ports);
> + kfree(serialpipe_ports);
> + pr_err("serialsim: Unable to register driver.\n");
> + goto out;
> + }
> +
> + serialpipeb_driver.nr = nr_pipe_ports;
> + rv = uart_register_driver(&serialpipeb_driver);
> + if (rv) {
> + uart_unregister_driver(&serialpipea_driver);
> + uart_unregister_driver(&serialecho_driver);
> + kfree(serialecho_ports);
> + kfree(serialpipe_ports);
> + pr_err("serialsim: Unable to register driver.\n");
> + goto out;
> + }
> +
> + for (i = 0; i < nr_echo_ports; i++) {
> + struct serialsim_intf *intf = &serialecho_ports[i];
> + struct uart_port *port = &intf->port;
> +
> + intf->buf.buf = intf->xmitbuf;
> + intf->ointf = intf;
> + intf->threadname = "serialecho";
> + intf->do_null_modem = true;
> + spin_lock_init(&intf->mctrl_lock);
> + tasklet_init(&intf->mctrl_tasklet, mctrl_tasklet, (long) intf);
> + /* Won't configure without some I/O or mem address set. */
> + port->iobase = 1;
> + port->line = i;
> + port->flags = UPF_BOOT_AUTOCONF;
> + port->ops = &serialecho_ops;
> + spin_lock_init(&port->lock);
> + port->attr_group = &serialsim_dev_attr_group;
> + rv = uart_add_one_port(&serialecho_driver, port);
> + if (rv)
> + pr_err("serialsim: Unable to add uart port %d: %d\n",
> + i, rv);
> + else
> + intf->registered = true;
> + }
> +
> + for (i = 0; i < nr_pipe_ports * 2; i += 2) {
> + struct serialsim_intf *intfa = &serialpipe_ports[i];
> + struct serialsim_intf *intfb = &serialpipe_ports[i + 1];
> + struct uart_port *porta = &intfa->port;
> + struct uart_port *portb = &intfb->port;
> +
> + intfa->buf.buf = intfa->xmitbuf;
> + intfb->buf.buf = intfb->xmitbuf;
> + intfa->ointf = intfb;
> + intfb->ointf = intfa;
> + intfa->threadname = "serialpipea";
> + intfb->threadname = "serialpipeb";
> + spin_lock_init(&intfa->mctrl_lock);
> + spin_lock_init(&intfb->mctrl_lock);
> + tasklet_init(&intfa->mctrl_tasklet, mctrl_tasklet,
> + (long) intfa);
> + tasklet_init(&intfb->mctrl_tasklet, mctrl_tasklet,
> + (long) intfb);
> +
> + /* Won't configure without some I/O or mem address set. */
> + porta->iobase = 1;
> + porta->line = i / 2;
> + porta->flags = UPF_BOOT_AUTOCONF;
> + porta->ops = &serialpipea_ops;
> + spin_lock_init(&porta->lock);
> + porta->attr_group = &serialsim_dev_attr_group;
> + porta->rs485_config = serialsim_rs485;
> +
> + /*
> + * uart_add_one_port() does an mctrl operation, which will
> + * claim the other port's lock. So everything needs to be
> + * full initialized, and we need null modem off until we
> + * get things added.
> + */
> + portb->iobase = 1;
> + portb->line = i / 2;
> + portb->flags = UPF_BOOT_AUTOCONF;
> + portb->ops = &serialpipeb_ops;
> + portb->attr_group = &serialsim_dev_attr_group;
> + spin_lock_init(&portb->lock);
> + portb->rs485_config = serialsim_rs485;
> +
> + rv = uart_add_one_port(&serialpipea_driver, porta);
> + if (rv) {
> + pr_err("serialsim: Unable to add uart pipe a port %d:
> %d\n",
> + i, rv);
> + continue;
> + } else {
> + intfa->registered = true;
> + }
> +
> + rv = uart_add_one_port(&serialpipeb_driver, portb);
> + if (rv) {
> + pr_err("serialsim: Unable to add uart pipe b port %d:
> %d\n",
> + i, rv);
> + intfa->registered = false;
> + uart_remove_one_port(&serialpipea_driver, porta);
> + } else {
> + intfb->registered = true;
> + }
> +
> + if (intfa->registered && intfb->registered) {
> + serialsim_set_null_modem(intfa, true);
> + serialsim_set_null_modem(intfb, true);
> + }
> + }
> + rv = 0;
> +
> + pr_info("serialsim ready\n");
Don't be noisy for when all is good. Drivers should be quiet.
> --- /dev/null
> +++ b/include/uapi/linux/serialsim.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +
> +/*
> + * serialsim - Emulate a serial device in a loopback and/or pipe
> + */
> +
> +/*
> + * TTY IOCTLs for controlling the modem control and for error injection.
> + * See drivers/tty/serial/serialsim.c and Documentation/serial/serialsim.rst
> + * for details.
> + */
> +
> +#ifndef LINUX_SERIALSIM_H
> +#define LINUX_SERIALSIM_H
> +
> +#include <linux/ioctl.h>
> +#include <asm/termbits.h>
> +
> +#define TIOCSERSREMNULLMODEM 0x54e4
> +#define TIOCSERSREMMCTRL 0x54e5
> +#define TIOCSERSREMERR 0x54e6
> +#ifdef TCGETS2
> +#define TIOCSERGREMTERMIOS _IOR('T', 0xe7, struct termios2)
> +#else
> +#define TIOCSERGREMTERMIOS _IOR('T', 0xe7, struct termios)
> +#endif
> +#define TIOCSERGREMNULLMODEM _IOR('T', 0xe8, int)
> +#define TIOCSERGREMMCTRL _IOR('T', 0xe9, unsigned int)
> +#define TIOCSERGREMERR _IOR('T', 0xea, unsigned int)
> +#define TIOCSERGREMRS485 _IOR('T', 0xeb, struct serial_rs485)
Wait, don't we have ioctls for these things in the tty layer already?
WHy add new ones?
thanks,
greg k-h
