Date:   Fri, 7 Aug 2020 09:18:57 +0000
From:   Johnson CH Chen (陳昭勳) 
        <JohnsonCH.Chen@...a.com>
To:     Jiri Slaby <jirislaby@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        Jason Chen (陳兆先) <Jason.Chen@...a.com>,
        Victor Yu (游勝義) <victor.yu@...a.com>,
        Danny Lin (林政易) <danny.lin@...a.com>
Subject: RE: [PATCH] tty: Add MOXA NPort Real TTY Driver

Hi Jiri,

Thanks for your good coding review for this patch, it helps us a lot!

> From: Jiri Slaby <jirislaby@...il.com>
> Sent: Tuesday, July 14, 2020 5:34 PM
> To: Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@...a.com>; Greg
> Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: linux-kernel@...r.kernel.org; linux-serial@...r.kernel.org
> 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@...a.com>
> > Signed-off-by: Jason Chen <jason.chen@...a.com>
> > Signed-off-by: Danny Lin <danny.lin@...a.com>
> > Signed-off-by: Victor Yu <victor.yu@...a.com>
> > ---
> ...
> > --- 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@...a.com>");
> > +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

Powered by blists - more mailing lists