Hi Jiri,
Thanks for your good coding review for this patch, it helps us a lot!
> From: Jiri Slaby <jirislaby@xxxxxxxxx>
> Sent: Tuesday, July 14, 2020 5:34 PM
> To: Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@xxxxxxxx>; Greg
> Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
>
> On 14. 07. 20, 8:24, Johnson CH Chen (陳昭勳) wrote:
> > This driver supports tty functions for all of MOXA's NPort series with
> > v5.0. Using this driver, host part can use tty to connect NPort device
> > server by ethernet.
> ...
> > Signed-off-by: Johnson Chen <johnsonch.chen@xxxxxxxx>
> > Signed-off-by: Jason Chen <jason.chen@xxxxxxxx>
> > Signed-off-by: Danny Lin <danny.lin@xxxxxxxx>
> > Signed-off-by: Victor Yu <victor.yu@xxxxxxxx>
> > ---
> ...
> > --- a/drivers/tty/Kconfig
> > +++ b/drivers/tty/Kconfig
> > @@ -259,6 +259,17 @@ config MOXA_SMARTIO
> > This driver can also be built as a module. The module will be called
> > mxser. If you want to do that, say M here.
> >
> > +config MOXA_NPORT_REAL_TTY
> > + tristate "Moxa NPort Real TTY support v5.0"
>
> MOXA_SMARTIO is lexicographically after MOXA_NPORT_REAL_TTY. So move
> this before MOXA_SMARTIO.
>
> > + help
> > + Say Y here if you have a Moxa NPort serial device server.
> > +
> > + The purpose of this driver is to map NPort serial port to host tty
> > + port. Using this driver, you can use NPort serial port as local tty port.
> > +
> > + This driver can also be built as a module. The module will be called
> > + npreal2 by setting M.
> > +
> > config SYNCLINK
> > tristate "Microgate SyncLink card support"
> > depends on SERIAL_NONSTANDARD && PCI && ISA_DMA_API diff --git
> > a/drivers/tty/Makefile b/drivers/tty/Makefile index
> > 020b1cd9294f..6d07985d6962 100644
> > --- a/drivers/tty/Makefile
> > +++ b/drivers/tty/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_CYCLADES) += cyclades.o
> > obj-$(CONFIG_ISI) += isicom.o
> > obj-$(CONFIG_MOXA_INTELLIO) += moxa.o
> > obj-$(CONFIG_MOXA_SMARTIO) += mxser.o
> > +obj-$(CONFIG_MOXA_NPORT_REAL_TTY) += npreal2.o
>
> The same.
>
> > obj-$(CONFIG_NOZOMI) += nozomi.o
> > obj-$(CONFIG_NULL_TTY) += ttynull.o
> > obj-$(CONFIG_ROCKETPORT) += rocket.o
> > diff --git a/drivers/tty/npreal2.c b/drivers/tty/npreal2.c new file
> > mode 100644 index 000000000000..65c773420755
> > --- /dev/null
> > +++ b/drivers/tty/npreal2.c
> > @@ -0,0 +1,3042 @@
> ...
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/fcntl.h>
> > +#include <linux/version.h>
>
> What do you need version.h for? Are you sure, you need all these headers?
>
> > +#include <linux/init.h>
> > +#include <linux/ioport.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/major.h>
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/ptrace.h>
> > +#include <linux/poll.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/serial.h>
> > +#include <linux/serial_reg.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/signal.h>
> > +#include <linux/sched.h>
> > +#include <linux/tty.h>
> > +#include <linux/tty_flip.h>
> > +#include <linux/timer.h>
> > +#include "npreal2.h"
> > +
> > +static int ttymajor = NPREALMAJOR;
>
> You want dynamic major anyway. So kill this all.
>
> > +static int verbose = 1;
> > +
> > +MODULE_AUTHOR("<support@xxxxxxxx>");
> > +MODULE_DESCRIPTION("MOXA Async/NPort Server Family Real TTY
> Driver");
> > +module_param(ttymajor, int, 0); module_param(verbose, int, 0644);
> > +MODULE_VERSION(NPREAL_VERSION); MODULE_LICENSE("GPL");
> > +
> > +struct server_setting_struct {
> > + int32_t server_type;
> > + int32_t disable_fifo;
> > +};
> > +
> > +struct npreal_struct {
> > + struct tty_port ttyPort;
> > + struct work_struct tqueue;
> > + struct work_struct process_flip_tqueue;
> > + struct ktermios normal_termios;
> > + struct ktermios callout_termios;
>
> callout in 2020?
>
> > + /* kernel counters for the 4 input interrupts */
> > + struct async_icount icount;
> > + struct semaphore rx_semaphore;
>
> semaphores in new code? You have to explain why are you not using simpler
> and faset mutexes.
>
> > + struct nd_struct *net_node;
> > + struct tty_struct *tty;
> > + struct pid *session;
> > + struct pid *pgrp;
>
> Why does a driver need to care about pgrp? And session? You should kill all
> these set, but unused fields. Note that you should also use fields from struct
> tty_port instead of the duplicates here.
>
> > + wait_queue_head_t open_wait;
> > + wait_queue_head_t close_wait;
> > + wait_queue_head_t delta_msr_wait;
> > + unsigned long baud_base;
> > + unsigned long event;
> > + unsigned short closing_wait;
> > + int port;
> > + int flags;
> > + int type; /* UART type */
> > + int xmit_fifo_size;
> > + int custom_divisor;
> > + int x_char; /* xon/xoff character */
> > + int close_delay;
> > + int modem_control; /* Modem control register */
> > + int modem_status; /* Line status */
> > + int count; /* # of fd on device */
> > + int xmit_head;
> > + int xmit_tail;
> > + int xmit_cnt;
> > + unsigned char *xmit_buf;
>
> ringbuf (as Greg suggests) or kfifo.
>
> > + /*
> > + * We use spin_lock_irqsave instead of semaphonre here.
>
> "semaphonre"?
>
> > + * Reason: When we use pppd to dialout via Real TTY driver,
> > + * some driver functions, such as npreal_write(), would be
> > + * invoked under interrpute mode which causes warning in
>
> "interrpute"?
>
> > + * down/up tx_semaphore.
> > + */
> > + spinlock_t tx_lock;
> > +};
> > +
> > +struct nd_struct {
> > + struct semaphore cmd_semaphore;
> > + struct proc_dir_entry *node_entry;
> > + struct npreal_struct *tty_node;
> > + struct semaphore semaphore;
> > + wait_queue_head_t initialize_wait;
> > + wait_queue_head_t select_in_wait;
> > + wait_queue_head_t select_out_wait;
> > + wait_queue_head_t select_ex_wait;
> > + wait_queue_head_t cmd_rsp_wait;
> > + int32_t server_type;
> > + int do_session_recovery_len;
> > + int cmd_rsp_flag;
> > + int tx_ready;
> > + int rx_ready;
> > + int cmd_ready;
> > + int wait_oqueue_responsed;
> > + int oqueue;
> > + int rsp_length;
> > + unsigned long flag;
> > + unsigned char cmd_buffer[84];
> > + unsigned char rsp_buffer[84];
> > +};
> > +
> > +static const struct proc_ops npreal_net_fops; static const struct
> > +tty_operations mpvar_ops; static struct proc_dir_entry
> > +*npvar_proc_root; static struct tty_driver *npvar_sdriver; static
> > +struct npreal_struct *npvar_table; static struct nd_struct
> > +*npvar_net_nodes;
>
> Could you reorder the code, so that you don't need these forward declarations?
>
> > +static void npreal_do_softint(struct work_struct *work) {
>
> Well, this is the old way of doing things.
>
> > + struct npreal_struct *info = container_of(work, struct npreal_struct,
> tqueue);
> > + struct tty_struct *tty;
> > +
> > + if (!info)
> > + return;
> > +
> > + tty = info->tty;
> > + if (tty) {
> > + if (test_and_clear_bit(NPREAL_EVENT_TXLOW, &info->event)) {
>
> Do you ever set that flag?
>
> > + if ((tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) &&
> > + tty->ldisc->ops->write_wakeup)
> > + (tty->ldisc->ops->write_wakeup)(tty);
> > + wake_up_interruptible(&tty->write_wait);
> > + }
> > +
> > + if (test_and_clear_bit(NPREAL_EVENT_HANGUP, &info->event)) {
> > + /* Do it when entering npreal_hangup() */
> > + tty_hangup(tty);
>
> Have you checked what tty_hangup actually does? Drop this whole function.
>
> > + }
> > + }
> > +}
> > +
> > +/**
> > + * npreal_flush_to_ldisc() - Read data from tty device to line
> > +discipline
> > + * @tty: pointer for struct tty_struct
> > + * @filp: pointer for struct file
> > + *
> > + * This routine is called out of the software interrupt to flush data
> > + * from the flip buffer to the line discipline.
> > + *
> > + */
> > +
> > +static void npreal_flush_to_ldisc(struct work_struct *work) {
>
> Ugh, the same as above, drop this and call flip directly.
>
> > + struct npreal_struct *info = container_of(work, struct npreal_struct,
> process_flip_tqueue);
> > + struct tty_struct *tty;
> > + struct tty_port *port;
> > +
> > + if (info == NULL)
> > + return;
> > +
> > + tty = info->tty;
> > + if (tty == NULL)
> > + return;
> > +
> > + port = tty->port;
> > + tty_flip_buffer_push(port);
> > +}
> > +
> > +static inline void npreal_check_modem_status(struct npreal_struct
> > +*info, int status) {
> > + int is_dcd_changed = 0;
> > +
> > + if ((info->modem_status & UART_MSR_DSR) != (status &
> UART_MSR_DSR))
> > + info->icount.dsr++;
> > + if ((info->modem_status & UART_MSR_DCD) != (status &
> UART_MSR_DCD)) {
> > + info->icount.dcd++;
> > + is_dcd_changed = 1;
> > + }
> > +
> > + if ((info->modem_status & UART_MSR_CTS) != (status &
> UART_MSR_CTS))
> > + info->icount.cts++;
> > +
> > + info->modem_status = status;
> > + wake_up_interruptible(&info->delta_msr_wait);
> > +
> > + if ((info->flags & ASYNC_CHECK_CD) && (is_dcd_changed)) {
> > + if (status & UART_MSR_DCD) {
> > + wake_up_interruptible(&info->open_wait);
> > + } else {
> > + set_bit(NPREAL_EVENT_HANGUP, &info->event);
> > + schedule_work(&info->tqueue);
> > + }
> > + }
> > +}
> > +
> > +static int npreal_wait_and_set_command(struct nd_struct *nd, char
> > +command_set, char command) {
> > + unsigned long et;
> > +
> > + if ((command_set != NPREAL_LOCAL_COMMAND_SET) &&
> > + ((nd->flag & NPREAL_NET_DO_SESSION_RECOVERY) ||
> > + (nd->flag & NPREAL_NET_NODE_DISCONNECTED))) {
> > +
> > + if (nd->flag & NPREAL_NET_DO_SESSION_RECOVERY)
> > + return -EAGAIN;
> > +
> > + return -EIO;
> > + }
> > +
> > + down(&nd->cmd_semaphore);
> > + nd->cmd_rsp_flag = 0;
> > + up(&nd->cmd_semaphore);
> > +
> > + et = jiffies + NPREAL_CMD_TIMEOUT;
> > + while (1) {
> > + down(&nd->cmd_semaphore);
> > + if (!(nd->cmd_buffer[0] == 0 || ((jiffies - et >= 0) ||
> > + signal_pending(current)))) {
> > + up(&nd->cmd_semaphore);
> > + current->state = TASK_INTERRUPTIBLE;
> > + schedule_timeout(1);
>
> This is very bad.
>
> This calls for wait_event_interruptible or alike. "jiffies - et >= 0"
> is broken in any case. time_after() is your friend.
>
> > + } else {
> > + nd->cmd_buffer[0] = command_set;
> > + nd->cmd_buffer[1] = command;
> > + up(&nd->cmd_semaphore);
> > + return 0;
> > + }
> > + }
> > +}
> > +
> > +static int npreal_wait_command_completed(struct nd_struct *nd, char
> command_set, char command,
> > + long timeout, char *rsp_buf, int *rsp_len) {
> > + long st = 0, tmp = 0;
> > +
> > + if ((command_set != NPREAL_LOCAL_COMMAND_SET) &&
> > + ((nd->flag & NPREAL_NET_DO_SESSION_RECOVERY) ||
> > + (nd->flag & NPREAL_NET_NODE_DISCONNECTED))) {
> > +
> > + if (nd->flag & NPREAL_NET_DO_SESSION_RECOVERY)
> > + return -EAGAIN;
> > + else
> > + return -EIO;
> > + }
> > +
> > + if (*rsp_len <= 0)
> > + return -EIO;
> > +
> > + while (1) {
> > + down(&nd->cmd_semaphore);
> > +
> > + if ((nd->rsp_length) && (nd->rsp_buffer[0] == command_set) &&
> > + (nd->rsp_buffer[1] == command)) {
>
> You should break the loop here and do the processing below after the loop.
> Making thuse the loop minimalistic.
>
> > + if (nd->rsp_length > *rsp_len)
> > + return -1;
> > +
> > + *rsp_len = nd->rsp_length;
> > + memcpy(rsp_buf, nd->rsp_buffer, *rsp_len);
> > + nd->rsp_length = 0;
> > + up(&nd->cmd_semaphore);
> > + return 0;
> > +
> > + } else if (timeout > 0) {
> > + up(&nd->cmd_semaphore);
> > + if (signal_pending(current))
> > + return -EIO;
> > +
> > + st = jiffies;
> > + if (wait_event_interruptible_timeout(nd->cmd_rsp_wait,
> > + nd->cmd_rsp_flag == 1, timeout) != 0) {
> > + down(&nd->cmd_semaphore);
> > + nd->cmd_rsp_flag = 0;
> > + up(&nd->cmd_semaphore);
> > + }
> > +
> > + tmp = abs((long)jiffies - (long)st);
> > +
> > + if (tmp >= timeout)
> > + timeout = 0;
> > + else
> > + timeout -= tmp;
>
> wait_event_interruptible_timeout already returns what you compute here in a
> complicated way, IIUC.
>
> > + } else {
> > + up(&nd->cmd_semaphore);
> > + return -ETIME;
> > + }
> > + }
> > +}
> > +
> > +static int npreal_set_unused_command_done(struct nd_struct *nd, char
> > +*rsp_buf, int *rsp_len) {
> > + npreal_wait_and_set_command(nd, NPREAL_LOCAL_COMMAND_SET,
> LOCAL_CMD_TTY_UNUSED);
> > + nd->cmd_buffer[2] = 0;
> > + nd->cmd_ready = 1;
> > + smp_mb(); /* use smp_mb() with waitqueue_active() */
> > + /* used waitqueue_active() is safe because smp_mb() is used */
> > + if (waitqueue_active(&nd->select_ex_wait))
> > + wake_up_interruptible(&nd->select_ex_wait);
> > +
> > + return npreal_wait_command_completed(nd,
> NPREAL_LOCAL_COMMAND_SET, LOCAL_CMD_TTY_UNUSED,
> > + NPREAL_CMD_TIMEOUT, rsp_buf, rsp_len); }
> > +
> > +static int npreal_set_used_command_done(struct nd_struct *nd, char
> > +*rsp_buf, int *rsp_len) {
> > + nd->cmd_buffer[0] = 0;
> > + npreal_wait_and_set_command(nd, NPREAL_LOCAL_COMMAND_SET,
> LOCAL_CMD_TTY_USED);
> > + nd->cmd_buffer[2] = 0;
> > + nd->cmd_ready = 1;
> > + smp_mb(); /* use smp_mb() with waitqueue_active() */
> > + /* used waitqueue_active() is safe because smp_mb() is used */
> > + if (waitqueue_active(&nd->select_ex_wait))
> > + wake_up_interruptible(&nd->select_ex_wait);
> > +
> > + return npreal_wait_command_completed(nd,
> NPREAL_LOCAL_COMMAND_SET, LOCAL_CMD_TTY_USED,
> > + NPREAL_CMD_TIMEOUT, rsp_buf, rsp_len); }
> > +
> > +static int npreal_set_tx_fifo_command_done(struct npreal_struct *info,
> struct nd_struct *nd,
> > + char *rsp_buf, int *rsp_len)
> > +{
> > + int ret;
> > +
> > + ret = npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET,
> ASPP_CMD_TX_FIFO);
> > + if (ret < 0)
> > + return ret;
> > +
> > + nd->cmd_buffer[2] = 1;
> > + nd->cmd_buffer[3] = info->xmit_fifo_size;
> > + nd->cmd_ready = 1;
> > + smp_mb(); /* use smp_mb() with waitqueue_active() */
> > + /* used waitqueue_active() is safe because smp_mb() is used */
> > + if (waitqueue_active(&nd->select_ex_wait))
> > + wake_up_interruptible(&nd->select_ex_wait);
> > +
> > + *rsp_len = RSP_BUFFER_SIZE;
> > + ret = npreal_wait_command_completed(nd,
> NPREAL_ASPP_COMMAND_SET, ASPP_CMD_TX_FIFO,
> > + NPREAL_CMD_TIMEOUT, rsp_buf, rsp_len);
> > + if (ret)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static int npreal_set_port_command_done(struct npreal_struct *info, struct
> nd_struct *nd,
> > + struct ktermios *termio, char *rsp_buf, int *rsp_len,
> > + int32_t mode, int32_t baud, int baudIndex) {
> > + int ret;
> > +
> > + ret = npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET,
> ASPP_CMD_PORT_INIT);
> > + if (ret < 0)
> > + return ret;
> > +
> > + nd->cmd_buffer[2] = 8;
> > + nd->cmd_buffer[3] = baudIndex;
> > + nd->cmd_buffer[4] = mode;
> > +
> > + if (info->modem_control & UART_MCR_DTR)
> > + nd->cmd_buffer[5] = 1;
> > + else
> > + nd->cmd_buffer[5] = 0;
>
> Or simply:
> nd->cmd_buffer[5] = !!(info->modem_control & UART_MCR_DTR);
> In all of them below:
>
> > + if (info->modem_control & UART_MCR_RTS)
> > + nd->cmd_buffer[6] = 1;
> > + else
> > + nd->cmd_buffer[6] = 0;
> > +
> > + if (termio->c_cflag & CRTSCTS) {
> > + nd->cmd_buffer[7] = 1;
> > + nd->cmd_buffer[8] = 1;
> > + } else {
> > + nd->cmd_buffer[7] = 0;
> > + nd->cmd_buffer[8] = 0;
> > + }
> > +
> > + if (termio->c_iflag & IXON)
> > + nd->cmd_buffer[9] = 1;
> > + else
> > + nd->cmd_buffer[9] = 0;
> > +
> > + if (termio->c_iflag & IXOFF)
> > + nd->cmd_buffer[10] = 1;
> > + else
> > + nd->cmd_buffer[10] = 0;
>
> What is this cmd_buffer good for actually? Only to let the user know?
> Then -- drop it.
Because detailed iterations for cmd_buffer and cmd_rsp with Nport server device are regarded proprietary for our company, is it good to reveal value of cmd_buffer[] with macros only for upstream this driver?
Best regards,
Johnson
