Hi Jiri,

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

From: Jiri Slaby <redacted>
Sent: Tuesday, July 14, 2020 5:34 PM
To: Johnson CH Chen (陳昭勳) <redacted>; Greg
Kroah-Hartman [off-list ref]
Cc: linux-kernel@vger.kernel.org; linux-serial@vger.kernel.org
Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver

On 14. 07. 20, 8:24, Johnson CH Chen (陳昭勳) wrote:

quoted

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.

...

quoted

Signed-off-by: Johnson Chen <redacted>
Signed-off-by: Jason Chen <redacted>
Signed-off-by: Danny Lin <redacted>
Signed-off-by: Victor Yu <redacted>
---

...

quoted

--- 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.

quoted

+	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.

quoted

 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 @@

...

quoted

+#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?

quoted

+#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.

quoted

+static int verbose = 1;
+
+MODULE_AUTHOR("[off-list ref]");
+MODULE_DESCRIPTION("MOXA Async/NPort Server Family Real TTY

Driver");

quoted

+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?

quoted

+	/* 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.

quoted

+	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.

quoted

+	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.

quoted

+	/*
+	 * We use spin_lock_irqsave instead of semaphonre here.

"semaphonre"?

quoted

+	 * 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"?

quoted

+	 * 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?

quoted

+static void npreal_do_softint(struct work_struct *work) {

Well, this is the old way of doing things.

quoted

+	struct npreal_struct *info = container_of(work, struct npreal_struct,

tqueue);

quoted

+	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?

quoted

+			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.

quoted

+		}
+	}
+}
+
+/**
+ * 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.

quoted

+	struct npreal_struct *info = container_of(work, struct npreal_struct,

process_flip_tqueue);

quoted

+	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))

quoted

+		info->icount.dsr++;
+	if ((info->modem_status & UART_MSR_DCD) != (status &

UART_MSR_DCD)) {

quoted

+		info->icount.dcd++;
+		is_dcd_changed = 1;
+	}
+
+	if ((info->modem_status & UART_MSR_CTS) != (status &

UART_MSR_CTS))

quoted

+		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.

quoted

+		} 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,

quoted

+						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.

quoted

+			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.

quoted

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

quoted

+	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,

quoted

+						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);

quoted

+	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,

quoted

+						NPREAL_CMD_TIMEOUT, rsp_buf, rsp_len); }
+
+static int npreal_set_tx_fifo_command_done(struct npreal_struct *info,

struct nd_struct *nd,

quoted

+								char *rsp_buf, int *rsp_len)
+{
+	int ret;
+
+	ret = npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET,

ASPP_CMD_TX_FIFO);

quoted

+	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,

quoted

+						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,

quoted

+					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);

quoted

+	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:

quoted

+	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