Re: [PATCH] tty: Add MOXA NPort Real TTY Driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> +	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);
> +
> +	ret = npreal_wait_command_completed(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_PORT_INIT,
> +					NPREAL_CMD_TIMEOUT, rsp_buf, rsp_len);
> +	if (ret)
> +		return -EIO;
> +
> +	if ((*rsp_len != 6) || (rsp_buf[2] != 3))
> +		return -EIO;
> +
> +	return 0;
> +}
...
> +static int npreal_set_generic_command_done(struct npreal_struct *info, int cmd)
> +{
> +	struct nd_struct *nd;
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +	int rsp_length = RSP_BUFFER_SIZE;
> +
> +	nd = info->net_node;
> +	if (!nd)
> +		return -EIO;
> +
> +	if (npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET, cmd) < 0)
> +		return -EIO;
> +
> +	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);
> +	if (npreal_wait_command_completed(nd, NPREAL_ASPP_COMMAND_SET, cmd, NPREAL_CMD_TIMEOUT,
> +						rsp_buffer, &rsp_length))
> +		return -EIO;
> +
> +	if ((rsp_length != 4) || (rsp_buffer[2] != 'O') || (rsp_buffer[3] != 'K'))

Too many parentheses in all these functions.

> +		return -EIO;
> +
> +	return 0;
> +}
...
> +static void npreal_flush_buffer(struct tty_struct *tty)
> +{
> +	struct npreal_struct *info = (struct npreal_struct *)tty->driver_data;
> +	struct nd_struct *nd;
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +	int rsp_length = RSP_BUFFER_SIZE;
> +	unsigned long flags;
> +
> +	if (!info)
> +		return;
> +
> +	spin_lock_irqsave(&info->tx_lock, flags);
> +	info->xmit_tail = 0;
> +	info->xmit_head = 0;
> +	info->xmit_cnt = 0;
> +	spin_unlock_irqrestore(&info->tx_lock, flags);
> +	wake_up_interruptible(&tty->write_wait);
> +
> +	if ((tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) && tty->ldisc->ops->write_wakeup)
> +		(tty->ldisc->ops->write_wakeup)(tty);

Why not tty_wakeup?

> +
> +	nd = info->net_node;
> +	if (!nd)
> +		return;
> +
> +	nd->tx_ready = 0;
> +	if (npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_FLUSH) < 0)
> +		return;
> +
> +	nd->cmd_buffer[2] = 1;
> +	nd->cmd_buffer[3] = ASPP_FLUSH_ALL_BUFFER;
> +	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);
> +
> +	npreal_wait_command_completed(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_FLUSH,
> +					NPREAL_CMD_TIMEOUT, rsp_buffer, &rsp_length);
> +}
> +
> +static long npreal_wait_oqueue(struct npreal_struct *info, long timeout)
> +{
> +	struct nd_struct *nd;
> +	long st = 0, tmp = 0;
> +	uint32_t tout;
> +
> +	nd = info->net_node;
> +	if (!nd)
> +		return -EIO;
> +
> +	if (npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_WAIT_OQUEUE) < 0)
> +		return -EIO;
> +
> +	if (timeout < HZ / 10)
> +		timeout = HZ / 10;
> +
> +	st = jiffies;
> +
> +	if (timeout != MAX_SCHEDULE_TIMEOUT)
> +		tout = (uint32_t)timeout;
> +	else
> +		tout = 0x7FFFFFFF;
> +
> +	nd->cmd_buffer[2] = 4;
> +	memcpy(&nd->cmd_buffer[3], (void *)&tout, 4);
> +	nd->wait_oqueue_responsed = 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);
> +
> +	while (nd->cmd_ready == 1) {
> +		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);
> +		} else {
> +			return -EIO;
> +		}
> +	}
> +
> +	nd->cmd_buffer[0] = 0;
> +	do {
> +		if (nd->wait_oqueue_responsed == 0) {
> +			if (wait_event_interruptible_timeout(nd->cmd_rsp_wait,
> +							nd->cmd_rsp_flag == 1, timeout)) {
> +				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;

Again this beast.

> +		} else {
> +			return nd->oqueue;
> +		}
> +	} while (timeout > 0);
> +
> +	return -EIO;
> +}
...
> +static void npreal_port_init_baud(struct npreal_struct *info, struct ktermios *termio,
> +				struct ktermios *old_termios, int32_t *baud_ret, int *index_ret)
> +{
> +	int baudIndex;
> +	int32_t baud;
> +
> +	switch (termio->c_cflag & (CBAUD | CBAUDEX)) {
> +	case B921600:
> +		baud = 921600L;

Why those L (long) suffixes when you assign to an int? Why is not the
int unsigned?

> +		baudIndex = ASPP_IOCTL_B921600;
> +		break;
> +
> +	case B460800:
> +		baud = 460800;
> +		baudIndex = ASPP_IOCTL_B460800;
> +		break;
> +
> +	case B230400:
> +		baud = 230400L;
> +		baudIndex = ASPP_IOCTL_B230400;
> +		break;
...
> +	default:
> +		baud = tty_termios_baud_rate(termio);
> +		baudIndex = 0xff;
> +	}
> +
> +#ifdef ASYNC_SPD_CUST
> +	if ((info->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST)
> +		baudIndex = 0xff;
> +#endif
> +
> +	if (baud > 921600L) {
> +		termio->c_cflag &= ~(CBAUD | CBAUDEX);
> +		termio->c_cflag |= old_termios->c_cflag & (CBAUD | CBAUDEX);
> +	}
> +
> +	*baud_ret = baud;
> +	*index_ret = baudIndex;
> +}
> +
> +static int npreal_port_init(struct npreal_struct *info, struct ktermios *old_termios)
> +{
> +	struct ktermios *termio;
> +	struct nd_struct *nd;
> +	int rsp_length = RSP_BUFFER_SIZE;
> +	int baudIndex, modem_status;
> +	int ret;
> +	int32_t baud, mode;
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +
> +	nd = info->net_node;
> +	if (!info->tty || !nd)
> +		return -EIO;
> +
> +	termio = &(info->tty->termios);
> +	mode = npreal_port_init_mode(termio);
> +	npreal_port_init_baud(info, termio, old_termios, &baud, &baudIndex);
> +	ret = npreal_set_port_command_done(info, nd, termio, rsp_buffer, &rsp_length, mode, baud,
> +						baudIndex);
> +	if (ret < 0)
> +		return ret;
> +
> +	modem_status = 0;
> +	if (((unsigned char)rsp_buffer[3] == 0xff) && ((unsigned char)rsp_buffer[4] == 0xff) &&
> +		((unsigned char)rsp_buffer[5] == 0xff)) {
> +		termio->c_cflag &= ~(CBAUD | CBAUDEX);
> +		termio->c_cflag |= old_termios->c_cflag & (CBAUD | CBAUDEX);
> +	} else {
> +		if (rsp_buffer[3])
> +			modem_status |= UART_MSR_DSR;
> +		if (rsp_buffer[4])
> +			modem_status |= UART_MSR_CTS;
> +		if (rsp_buffer[5])
> +			modem_status |= UART_MSR_DCD;
> +	}
> +
> +	npreal_check_modem_status(info, modem_status);
> +
> +	if ((baudIndex == 0xff) && (baud != 0)) {
> +		ret = npreal_set_baud_command_done(info, nd, rsp_buffer, &rsp_length, baud);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (termio->c_iflag & (IXON | IXOFF)) {
> +		ret = npreal_set_xonxoff_command_done(info, termio, nd, rsp_buffer, &rsp_length);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (termio->c_cflag & CLOCAL)
> +		info->flags &= ~ASYNC_CHECK_CD;
> +	else
> +		info->flags |= ASYNC_CHECK_CD;
> +
> +	if (!info->tty)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static void npreal_init_net_node(struct nd_struct *net_node, struct npreal_struct *tty_node,
> +								struct proc_dir_entry *de)
> +{
> +	net_node->tty_node = tty_node;
> +	net_node->node_entry = de;
> +	net_node->cmd_rsp_flag = 0;
> +	net_node->flag = 0;
> +
> +	sema_init(&net_node->cmd_semaphore, 1);
> +	sema_init(&net_node->semaphore, 1);
> +
> +	init_waitqueue_head(&net_node->initialize_wait);
> +	init_waitqueue_head(&net_node->select_in_wait);
> +	init_waitqueue_head(&net_node->select_out_wait);
> +	init_waitqueue_head(&net_node->select_ex_wait);
> +	init_waitqueue_head(&net_node->cmd_rsp_wait);
> +}
> +
> +static void npreal_init_tty_node(struct npreal_struct *tty_node, struct nd_struct *net_node, int i)
> +{
> +	tty_node->net_node = net_node;
> +	tty_node->port = i;
> +	tty_node->type = PORT_16550A;
> +	tty_node->flags = 0;
> +	tty_node->xmit_fifo_size = 16;
> +	tty_node->baud_base = 921600L;
> +	tty_node->close_delay = 5 * HZ / 10;
> +	tty_node->closing_wait = 30 * HZ;
> +	tty_node->normal_termios = npvar_sdriver->init_termios;
> +
> +	memset(&tty_node->icount, 0, sizeof(tty_node->icount));
> +	INIT_WORK(&tty_node->process_flip_tqueue, npreal_flush_to_ldisc);
> +	INIT_WORK(&tty_node->tqueue, npreal_do_softint);
> +	spin_lock_init(&tty_node->tx_lock);
> +	sema_init(&tty_node->rx_semaphore, 1);
> +
> +	init_waitqueue_head(&tty_node->open_wait);
> +	init_waitqueue_head(&tty_node->close_wait);
> +	init_waitqueue_head(&tty_node->delta_msr_wait);
> +}
> +
> +static int npreal_init(struct npreal_struct *tty_node, struct nd_struct *net_node)
> +{
> +	struct proc_dir_entry *de;
> +	char buf[4];
> +	int i;
> +
> +	npvar_proc_root = proc_mkdir("npreal2", NULL);
> +	if (!npvar_proc_root)
> +		return -ENOMEM;
> +
> +	tty_node = &npvar_table[0];
> +	net_node = &npvar_net_nodes[0];
> +
> +	for (i = 0; i < NPREAL_PORTS; i++, tty_node++, net_node++) {
> +		sprintf(buf, "%d", i);
> +		de = proc_create_data(buf, 0666 | S_IFREG, npvar_proc_root, &npreal_net_fops,
> +					(void *)net_node);
> +		if (!de)
> +			return -ENOMEM;
> +
> +		npreal_init_net_node(net_node, tty_node, de);
> +		npreal_init_tty_node(tty_node, net_node, i);
> +	}
> +
> +	return 0;
> +}
> +
> +static int npreal_chars_in_buffer(struct tty_struct *tty)
> +{
> +	struct npreal_struct *info = (struct npreal_struct *)tty->driver_data;
> +
> +	if (!info)
> +		return -EIO;
> +
> +	return info->xmit_cnt;
> +}
> +
> +static int npreal_block_till_ready(struct tty_struct *tty, struct file *filp,
> +						struct npreal_struct *info)

Why are you not using tty_port_block_til_ready?

> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +	struct nd_struct *nd;
> +	int do_clocal = 0;
> +	int retval = 0;
> +
> +	nd = info->net_node;
> +	if (!nd)
> +		return -EIO;
> +
> +	if ((filp->f_flags & O_NONBLOCK) || test_bit(TTY_IO_ERROR, &tty->flags)) {
> +		info->flags |= ASYNC_NORMAL_ACTIVE;
> +		return 0;
> +	}
> +
> +	if (tty->termios.c_cflag & CLOCAL)
> +		do_clocal = 1;
> +
> +	add_wait_queue(&info->open_wait, &wait);
> +	while (1) {
> +		if (tty_hung_up_p(filp))
> +			break;
> +		else if (info->flags & ASYNC_CLOSING) {
> +			if (SERIAL_DO_RESTART && !(info->flags & ASYNC_HUP_NOTIFY))
> +				retval = -ERESTARTSYS;
> +			else
> +				retval = -EAGAIN;
> +			break;
> +		}
> +
> +		if (!(info->flags & ASYNC_CLOSING) &&
> +			(do_clocal || (info->modem_status & UART_MSR_DCD)))
> +			break;
> +
> +		if (signal_pending(current)) {
> +			retval = -EIO;
> +			break;
> +		}
> +
> +		current->state = TASK_INTERRUPTIBLE;
> +		schedule();
> +	}
> +
> +	remove_wait_queue(&info->open_wait, &wait);
> +	if (!retval)
> +		info->flags |= ASYNC_NORMAL_ACTIVE;
> +
> +	return retval;
> +}
> +
> +static void set_common_xmit_fifo_size(struct npreal_struct *info, struct nd_struct *nd)
> +{
> +	if (info->type == PORT_16550A) {
> +		if (nd->server_type == CN2500)
> +			info->xmit_fifo_size = 64;
> +		else
> +			info->xmit_fifo_size = 16;
> +	} else {
> +		info->xmit_fifo_size = 1;
> +	}
> +}
> +
> +static int npreal_port_shutdown(struct npreal_struct *info)
> +{
> +	struct nd_struct *nd;
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +	int rsp_length = RSP_BUFFER_SIZE;
> +
> +	nd = info->net_node;
> +	if (!nd)
> +		return -EIO;
> +
> +	npreal_disconnect(nd, rsp_buffer, &rsp_length);
> +	nd->flag &= ~NPREAL_NET_TTY_INUSED;
> +	return 0;
> +}
> +
> +static int npreal_get_serial_info(struct npreal_struct *info, struct serial_struct *retinfo)
> +{
> +	struct serial_struct tmp;
> +
> +	if (!retinfo)
> +		return -EFAULT;
> +
> +	memset(&tmp, 0, sizeof(tmp));
> +	tmp.type = info->type;
> +	tmp.line = info->port;
> +	tmp.flags = info->flags;
> +	tmp.close_delay = info->close_delay;
> +	tmp.closing_wait = info->closing_wait;
> +	tmp.custom_divisor = info->custom_divisor;
> +	tmp.baud_base = info->baud_base;
> +	tmp.hub6 = 0;
> +
> +	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
> +		return -EFAULT;
> +	else
> +		return 0;
> +}
> +
> +static int npreal_set_serial_info(struct npreal_struct *info, struct serial_struct *new_info)
> +{
> +	struct serial_struct new_serial;
> +	int rsp_length = RSP_BUFFER_SIZE;
> +	int retval = 0;
> +	unsigned int flags;
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +
> +
> +	if ((!new_info) || copy_from_user(&new_serial, new_info, sizeof(new_serial)))
> +		return -EFAULT;
> +
> +	flags = info->flags & ASYNC_SPD_MASK;
> +
> +	if (!capable(CAP_SYS_ADMIN)) {
> +		if ((new_serial.close_delay != info->close_delay) ||
> +			((new_serial.flags & ~ASYNC_USR_MASK) != (info->flags & ~ASYNC_USR_MASK)))
> +			return -EPERM;
> +
> +		info->flags = ((info->flags & ~ASYNC_USR_MASK) |
> +				(new_serial.flags & ASYNC_USR_MASK));
> +	} else {
> +		info->flags = ((info->flags & ~ASYNC_FLAGS) | (new_serial.flags & ASYNC_FLAGS));
> +		info->close_delay = new_serial.close_delay * HZ / 100;
> +
> +		if (new_serial.closing_wait == ASYNC_CLOSING_WAIT_NONE)
> +			info->closing_wait = ASYNC_CLOSING_WAIT_NONE;
> +		else
> +			info->closing_wait = new_serial.closing_wait * HZ / 100;
> +	}
> +
> +	info->type = new_serial.type;
> +	set_common_xmit_fifo_size(info, info->net_node);
> +
> +	if (info->flags & ASYNC_INITIALIZED) {
> +		if (flags != (info->flags & ASYNC_SPD_MASK))
> +			retval = npreal_port_init(info, 0);
> +
> +		if (info->net_node)
> +			npreal_set_tx_fifo_command_done(info, info->net_node, rsp_buffer,
> +							&rsp_length);
> +
> +	}
> +
> +	info->custom_divisor = new_serial.custom_divisor;
> +
> +	if (info->custom_divisor == 0)
> +		info->baud_base = 921600L;
> +	else
> +		info->baud_base = new_serial.baud_base;
> +
> +	return retval;
> +}
> +
> +/**
> + * npreal_get_lsr_info() - get line status register info
> + *
> + * Let user call ioctl() to get info when the UART physically is emptied.
> + * On bus types like RS485, the transmitter must release the bus after
> + * transmitting. This must be done when the transmit shift register is
> + * empty, not be done when the transmit holding register is empty.
> + * This functionality allows an RS485 driver to be written in user space.
> + *
> + * Always return 0 when function is ended.
> + */
> +static int npreal_get_lsr_info(struct npreal_struct *info,
> +				unsigned int *value)
> +{
> +	unsigned int result = 0;
> +
> +	if (npreal_wait_oqueue(info, 0) == 0)
> +		result = TIOCSER_TEMT;
> +
> +	put_user(result, value);
> +
> +	return 0;
> +}
> +
> +static int npreal_start_break(struct nd_struct *nd)
> +{
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +	int rsp_length = RSP_BUFFER_SIZE;
> +
> +	npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_START_BREAK);
> +	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);
> +
> +	if (npreal_wait_command_completed(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_START_BREAK,
> +					NPREAL_CMD_TIMEOUT, rsp_buffer, &rsp_length))
> +		return -EIO;
> +
> +	if ((rsp_length != 4) || (rsp_buffer[2] != 'O') || (rsp_buffer[3] != 'K'))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int npreal_stop_break(struct nd_struct *nd)
> +{
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +	int rsp_length = RSP_BUFFER_SIZE;
> +
> +	npreal_wait_and_set_command(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_STOP_BREAK);
> +	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);
> +
> +	rsp_length = sizeof(rsp_buffer);
> +	if (npreal_wait_command_completed(nd, NPREAL_ASPP_COMMAND_SET, ASPP_CMD_STOP_BREAK,
> +					NPREAL_CMD_TIMEOUT, rsp_buffer, &rsp_length))
> +		return -EIO;
> +
> +	if (rsp_length != 4  || (rsp_buffer[2] != 'O') || (rsp_buffer[3] != 'K'))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +/**
> + * npreal_send_break() - Sends break characters out the serial port.
> + * @info: pointer for npread_struct
> + * @duration: the time during sending break signals
> + *
> + * This is called by npreal_ioctl() with case of TCSBRK or TCSBRKP
> + */
> +static void npreal_send_break(struct npreal_struct *info, unsigned int duration)
> +{
> +	struct nd_struct *nd;
> +
> +	nd = info->net_node;
> +	if (!nd)
> +		return;
> +
> +	npreal_start_break(nd);
> +	current->state = TASK_INTERRUPTIBLE;
> +	schedule_timeout(duration);
> +	npreal_stop_break(nd);
> +}
> +
> +static void npreal_remove_proc_entry(struct proc_dir_entry *pde, int idx)
> +{
> +	char tmp[10];
> +
> +	if (!pde)
> +		return;
> +
> +	sprintf(tmp, "%d", idx);
> +
> +	if (idx == 404)
> +		remove_proc_entry("npreal2", NULL);
> +	else
> +		remove_proc_entry(tmp, npvar_proc_root);
> +}
> +
> +static void npreal_process_notify(struct nd_struct *nd, char *rsp_buffer, int rsp_length)
> +{
> +	struct npreal_struct *info = nd->tty_node;
> +	int state;
> +
> +	if (!info || rsp_length != 5)
> +		return;
> +
> +	if (rsp_buffer[2] & ASPP_NOTIFY_MSR_CHG) {
> +		state = 0;
> +
> +		if (rsp_buffer[3] & 0x10)
> +			state |= UART_MSR_CTS;
> +		if (rsp_buffer[3] & 0x20)
> +			state |= UART_MSR_DSR;
> +		if (rsp_buffer[3] & 0x80)
> +			state |= UART_MSR_DCD;
> +		npreal_check_modem_status(info, state);
> +	}
> +
> +	if (rsp_buffer[2] & ASPP_NOTIFY_BREAK) {
> +		struct tty_struct *tty;
> +
> +		down(&info->rx_semaphore);
> +		tty = info->tty;
> +		if (!tty || !info->ttyPort.low_latency) {
> +			up(&info->rx_semaphore);
> +			return;
> +		}
> +
> +		tty_insert_flip_char(&info->ttyPort, 0, TTY_BREAK);
> +		up(&info->rx_semaphore);
> +		info->icount.rx++;
> +		info->icount.brk++;
> +		schedule_work(&info->process_flip_tqueue);
> +
> +		if (info->flags & ASYNC_SAK)
> +			do_SAK(info->tty);
> +	}
> +
> +	if (rsp_buffer[2] & ASPP_NOTIFY_PARITY)
> +		info->icount.parity++;
> +	if (rsp_buffer[2] & ASPP_NOTIFY_FRAMING)
> +		info->icount.frame++;
> +	if ((rsp_buffer[2] & ASPP_NOTIFY_SW_OVERRUN) || (rsp_buffer[2] & ASPP_NOTIFY_HW_OVERRUN))
> +		info->icount.overrun++;
> +}
> +
> +static int32_t npreal_do_session_mode(struct ktermios *termio)
> +{
> +	int32_t mode;
> +
> +	mode = termio->c_cflag & CSIZE;
> +	switch (mode) {
> +	case CS5:
> +		mode = ASPP_IOCTL_BITS5;
> +		break;
> +
> +	case CS6:
> +		mode = ASPP_IOCTL_BITS6;
> +		break;
> +
> +	case CS7:
> +		mode = ASPP_IOCTL_BITS7;
> +		break;
> +
> +	case CS8:
> +		mode = ASPP_IOCTL_BITS8;
> +		break;
> +
> +	}
> +
> +	if (termio->c_cflag & CSTOPB)
> +		mode |= ASPP_IOCTL_STOP2;
> +	else
> +		mode |= ASPP_IOCTL_STOP1;
> +
> +	if (termio->c_cflag & PARENB) {
> +		if (termio->c_cflag & PARODD)
> +			mode |= ASPP_IOCTL_ODD;
> +		else
> +			mode |= ASPP_IOCTL_EVEN;
> +	} else {
> +		mode |= ASPP_IOCTL_NONE;
> +	}
> +
> +	return mode;
> +}
> +
> +static void npreal_do_session_baud(struct npreal_struct *info, struct ktermios *termio,
> +							int32_t *baud_ret, int *baud_ind_ret)
> +{
> +	int32_t baud;
> +	int baudIndex;
> +
> +	switch (termio->c_cflag & (CBAUD | CBAUDEX)) {
> +	case B921600:
> +		baud = 921600L;
> +		baudIndex = ASPP_IOCTL_B921600;
> +		break;

Do I have deja-vu? Why all this duplicated code?

> +	case B460800:
> +		baud = 460800;
> +		baudIndex = ASPP_IOCTL_B460800;
> +		break;
...
> +static void npreal_do_session_buffer(struct nd_struct *nd, struct npreal_struct *info,
> +				struct ktermios *termio, int baud, int mode, int baudIndex)
> +{
> +	nd->cmd_buffer[0] = NPREAL_ASPP_COMMAND_SET;
> +	nd->cmd_buffer[1] = ASPP_CMD_PORT_INIT;
> +	nd->cmd_buffer[2] = 8;
> +	nd->cmd_buffer[3] = baudIndex;   /* baud rate */
> +	nd->cmd_buffer[4] = mode;       /* mode */
> +
> +	/* line control */
> +	if (info->modem_control & UART_MCR_DTR)
> +		nd->cmd_buffer[5] = 1;
> +	else
> +		nd->cmd_buffer[5] = 0;

And this duplicated code?

> +	if (info->modem_control & UART_MCR_RTS)
> +		nd->cmd_buffer[6] = 1;
> +	else
> +		nd->cmd_buffer[6] = 0;
> +
> +	/* flow control */
> +	if ((info->flags & ASYNC_INITIALIZED) && (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;
> +
> +	if ((baudIndex == 0xff) && (baud != 0)) {
> +		nd->cmd_buffer[11] = ASPP_CMD_SETBAUD;
> +		nd->cmd_buffer[12] = 4;
> +
> +		if (((info->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) && info->custom_divisor)
> +			baud = info->baud_base/info->custom_divisor;
> +
> +		memcpy(&nd->cmd_buffer[13], &baud, 4);
> +	}
> +
> +	if (termio->c_iflag & (IXON | IXOFF)) {
> +		nd->cmd_buffer[17] = ASPP_CMD_XONXOFF;
> +		nd->cmd_buffer[18] = 2;
> +		nd->cmd_buffer[19] = termio->c_cc[VSTART];
> +		nd->cmd_buffer[20] = termio->c_cc[VSTOP];
> +	}
> +
> +	nd->cmd_buffer[21] = ASPP_CMD_TX_FIFO;
> +	nd->cmd_buffer[22] = 1;
> +	nd->cmd_buffer[23] = info->xmit_fifo_size;
> +	nd->cmd_buffer[24] = ASPP_CMD_LINECTRL;
> +	nd->cmd_buffer[25] = 2;
> +
> +	if (info->modem_control & UART_MCR_DTR)
> +		nd->cmd_buffer[26] = 1;
> +	else
> +		nd->cmd_buffer[26] = 0;
> +	if (info->modem_control & UART_MCR_RTS)
> +		nd->cmd_buffer[27] = 1;
> +	else
> +		nd->cmd_buffer[27] = 0;
> +
> +	nd->do_session_recovery_len = 27 + 1;
> +}
> +
> +static void npreal_do_session_recovery(struct npreal_struct *info)
> +{
> +	struct tty_struct *tty;
> +	struct nd_struct *nd;
> +	struct ktermios *termio;
> +	int32_t baud, mode;
> +	int baudIndex;
> +
> +	tty = info->tty;
> +	nd = info->net_node;
> +
> +	if (!tty || !nd)
> +		return;
> +
> +	if (!(nd->flag & NPREAL_NET_NODE_OPENED) || !(nd->flag & NPREAL_NET_NODE_CONNECTED))
> +		return;
> +
> +	if (info->flags & ASYNC_INITIALIZED) {
> +		termio = &(info->tty->termios);
> +	} else {
> +		termio = &info->normal_termios;
> +
> +		if (!termio)
> +			return;
> +	}
> +
> +	down(&nd->cmd_semaphore);
> +	mode = npreal_do_session_mode(termio);
> +	npreal_do_session_baud(info, termio, &baud, &baudIndex);
> +	npreal_do_session_buffer(nd, info, termio, baud, mode, baudIndex);
> +	nd->flag |= NPREAL_NET_DO_SESSION_RECOVERY;
> +	nd->cmd_ready = 1;
> +	up(&nd->cmd_semaphore);
> +	/* waitqueue_active() is safe because nd->cmd_ready is in semaphore.*/
> +	if (waitqueue_active(&nd->select_ex_wait))
> +		wake_up_interruptible(&nd->select_ex_wait);
> +}
> +
> +static int npreal_startup_serial_port(struct npreal_struct *info, struct nd_struct *nd)
> +{
> +	int ret;
> +
> +	nd->flag &= ~NPREAL_NET_DO_SESSION_RECOVERY;
> +	info->modem_status = 0;
> +	info->modem_control = 0;
> +
> +	if (info->tty->termios.c_cflag & CBAUD)
> +		info->modem_control = UART_MCR_DTR | UART_MCR_RTS;
> +
> +	ret = npreal_port_init(info, 0);
> +	if (ret != 0)
> +		return ret;
> +
> +	set_common_xmit_fifo_size(info, nd);
> +	return 0;
> +}
> +
> +static int npreal_startup_init(struct npreal_struct *info, struct nd_struct *nd,
> +					struct tty_struct *tty, unsigned long *page)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +
> +	if (!nd)
> +		return -EIO;
> +
> +	add_wait_queue(&nd->initialize_wait, &wait);
> +	while (test_and_set_bit(NPREAL_NET_DO_INITIALIZE, &nd->flag)) {
> +		if (signal_pending(current))
> +			break;
> +
> +		schedule();

Nah. wait_event and friends.

> +	}
> +
> +	remove_wait_queue(&nd->initialize_wait, &wait);
> +
> +	info->tty = tty;
> +
> +	if (info->flags & ASYNC_INITIALIZED) {
> +		clear_bit(NPREAL_NET_DO_INITIALIZE, &nd->flag);
> +		smp_mb(); /* use smp_mb() with waitqueue_active() */
> +		/* used waitqueue_active() is safe because smp_mb() is used */
> +		if (waitqueue_active(&nd->initialize_wait))
> +			wake_up_interruptible(&nd->initialize_wait);
> +
> +		return 0;
> +	}
> +
> +	*page = __get_free_page(GFP_KERNEL);
> +	if (!(*page)) {
> +		clear_bit(NPREAL_NET_DO_INITIALIZE, &nd->flag);
> +		smp_mb(); /* use smp_mb() with waitqueue_active() */
> +		/* used waitqueue_active() is safe because smp_mb() is used */
> +		if (waitqueue_active(&nd->initialize_wait))
> +			wake_up_interruptible(&nd->initialize_wait);
> +
> +		return -ENOMEM;
> +	}
> +
> +	return 1;
> +}
> +
> +static int npreal_startup_tty_usage(struct npreal_struct *info, struct nd_struct *nd,
> +							char *rsp_buf, int *rsp_len)
> +{
> +	int ret;
> +
> +	ret = npreal_set_used_command_done(nd, rsp_buf, rsp_len);
> +	if (ret != 0) {
> +		npreal_set_unused_command_done(nd, rsp_buf, rsp_len);
> +		return ret;
> +	} else if (OFFLINE_POLLING) {
> +		if (!rsp_buf[2]) {
> +			npreal_set_unused_command_done(nd, rsp_buf, rsp_len);
> +			return ret;
> +		}
> +	}
> +
> +	nd->flag |= NPREAL_NET_TTY_INUSED;
> +	return 0;
> +}
> +
> +static int npreal_startup_disconnect(struct npreal_struct *info, struct nd_struct *nd,
> +								char *rsp_buf, int *rsp_len)
> +{
> +	int ret;
> +
> +	nd->flag &= ~NPREAL_NET_NODE_DISCONNECTED;
> +
> +	ret = npreal_set_unused_command_done(nd, rsp_buf, rsp_len);
> +	if (ret != 0)
> +		nd->flag |= NPREAL_NET_NODE_DISCONNECTED;
> +	else
> +		nd->flag &= ~NPREAL_NET_TTY_INUSED;
> +
> +	return ret;
> +}
> +
> +static int npreal_startup(struct npreal_struct *info, struct file *filp,
> +			struct tty_struct *tty)
> +{
> +	struct nd_struct *nd = info->net_node;
> +	unsigned long page;
> +	int rsp_length = RSP_BUFFER_SIZE;
> +	int cnt = 0, ret;
> +	char rsp_buffer[RSP_BUFFER_SIZE];
> +
> +	ret = npreal_startup_init(info, nd, tty, &page);
> +	if (ret < 1)
> +		return ret;
> +
> +	if (!(nd->flag & NPREAL_NET_TTY_INUSED)) {
> +		ret = npreal_startup_tty_usage(info, nd, rsp_buffer, &rsp_length);
> +		if (ret)
> +			goto startup_err;
> +	} else {
> +		if (nd->flag & NPREAL_NET_NODE_DISCONNECTED) {
> +			npreal_startup_disconnect(info, nd, rsp_buffer, &rsp_length);
> +			goto startup_err;
> +		}
> +
> +		while ((nd->cmd_ready == 1) && (cnt++ < 10)) {
> +			current->state = TASK_INTERRUPTIBLE;
> +			schedule_timeout(HZ / 100);

All these random schedules needs to go.

> +		}
> +	}
> +
> +	ret = npreal_startup_serial_port(info, nd);
> +	if (ret)
> +		goto startup_err;
> +
> +	ret = npreal_set_tx_fifo_command_done(info, nd, rsp_buffer, &rsp_length);
> +	if (ret)
> +		goto startup_err;
> +
> +	if (info->xmit_buf)
> +		free_page(page);
> +	else
> +		info->xmit_buf = (unsigned char *)page;
> +
> +	if (info->tty)
> +		test_and_clear_bit(TTY_IO_ERROR, &info->tty->flags);
> +
> +	info->xmit_tail = 0;
> +	info->xmit_head = 0;
> +	info->xmit_cnt = 0;
> +	info->flags |= ASYNC_INITIALIZED;
> +	clear_bit(NPREAL_NET_DO_INITIALIZE, &nd->flag);
> +	smp_mb(); /* use smp_mb() with waitqueue_active() */
> +	/* used waitqueue_active() is safe because smp_mb() is used */
> +	if (waitqueue_active(&nd->initialize_wait))
> +		wake_up_interruptible(&nd->initialize_wait);
> +
> +	return 0;
> +
> +startup_err:
> +	npreal_disconnect(nd, rsp_buffer, &rsp_length);
> +	free_page(page);
> +	clear_bit(NPREAL_NET_DO_INITIALIZE, &nd->flag);
> +	smp_mb(); /* use smp_mb() with waitqueue_active() */
> +	/* used waitqueue_active() is safe because smp_mb() is used */
> +	if (waitqueue_active(&nd->initialize_wait))
> +		wake_up_interruptible(&nd->initialize_wait);
> +
> +	return ret;
> +}
> +
> +static int npreal_open_startup(struct tty_struct *tty, struct npreal_struct *info,
> +								struct file *filp)
> +{
> +	long jiff_th;
> +	int ret, retry;
> +
> +	retry = NPREAL_CMD_TRY - 1;
> +	while (retry) {
> +		/* For some circumstance, device may reset the connection
> +		 * during the port opening. These code is to reopen the port
> +		 * without telling application. Considering a real situation
> +		 * of connection lost, we use -ETIME to exit the retry loop.
> +		 */
> +		ret = npreal_startup(info, filp, tty);
> +		if (ret == 0)
> +			break;
> +		else if (ret == -ETIME) {
> +			pr_err("npreal_startup failed(%d)\n", ret);
> +			return -EIO;
> +		}
> +
> +		jiff_th = (NPREAL_CMD_TRY-retry)*HZ/2;
> +		schedule_timeout_uninterruptible(jiff_th);

So msleep((NPREAL_CMD_TRY-retry)*500)?

> +		retry--;
> +		if (retry < 0) {
> +			pr_err("npreal_startup failed\n");
> +			return -EIO;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/**
> + * npreal_throttle() - Notify the tty driver input buffer is full
> + * @tty: pointer for struct tty_struct
> + *
> + * This routine is called by the upper-layer tty layer to signal that
> + * incoming characters should be throttled. (tty driver buffer is full)
> + *
> + */
> +static void npreal_throttle(struct tty_struct *tty)
> +{
> +	struct npreal_struct *info = (struct npreal_struct *)tty->driver_data;
> +	struct nd_struct *nd;
> +
> +	if (!info)
> +		return;
> +
> +	nd = info->net_node;
> +	if (!nd)
> +		return;
> +
> +	nd->rx_ready = 0;
> +	smp_mb(); /* use smp_mb() with waitqueue_active() */
> +	/* used waitqueue_active() is safe because smp_mb() is used */
> +	if (waitqueue_active(&nd->select_out_wait))

Why do you actully do these racy waitqueue_active checks all over the code?

> +		wake_up_interruptible(&nd->select_out_wait);
> +}
...
> +static void npreal_shutdown(struct npreal_struct *info)
> +{
> +	struct nd_struct *nd = info->net_node;
> +	unsigned long flags;
> +
> +	while (test_and_set_bit(NPREAL_NET_DO_INITIALIZE, &nd->flag)) {
> +		if (signal_pending(current))
> +			break;
> +		current->state = TASK_INTERRUPTIBLE;
> +		schedule_timeout(HZ / 100);

You already know what, right?

> +	}
> +
> +	if (!(info->flags & ASYNC_INITIALIZED))
> +		goto done;
> +
> +	spin_lock_irqsave(&info->tx_lock, flags);
> +	if (info->xmit_buf) {
> +		free_page((unsigned long)info->xmit_buf);
> +		info->xmit_buf = 0;
> +	}
> +	spin_unlock_irqrestore(&info->tx_lock, flags);
> +
> +	if (info->tty) {
> +		set_bit(TTY_IO_ERROR, &info->tty->flags);
> +		npreal_unthrottle(info->tty);
> +	}
> +
> +	if (!info->tty || (info->tty->termios.c_cflag & HUPCL))
> +		info->modem_control &= ~(UART_MCR_DTR | UART_MCR_RTS);
> +
> +done:
> +	npreal_port_shutdown(info);
> +	info->flags &= ~(ASYNC_NORMAL_ACTIVE |
> +	ASYNC_INITIALIZED | ASYNC_CLOSING);
> +	down(&info->rx_semaphore);
> +	info->tty = 0;

0 is not a pointer.

> +	up(&info->rx_semaphore);
> +	clear_bit(NPREAL_NET_DO_INITIALIZE, &nd->flag);
> +	wake_up_interruptible(&info->open_wait);
> +	wake_up_interruptible(&info->close_wait);
> +	smp_mb(); /* use smp_mb() with waitqueue_active() */
> +	/* used waitqueue_active() is safe because smp_mb() is used */
> +	if (waitqueue_active(&nd->initialize_wait))
> +		wake_up_interruptible(&nd->initialize_wait);
> +}
...
> +static int npreal_open(struct tty_struct *tty, struct file *filp)
> +{

Why not to use tty_port_open?

> +}
...
> +static void npreal_close(struct tty_struct *tty, struct file *filp)
> +{

And tty_port_close?

> +}
...
> +static int npreal_write(struct tty_struct *tty, const unsigned char *buf, int count)
> +{
> +	struct npreal_struct *info = (struct npreal_struct *)tty->driver_data;
> +	struct nd_struct *nd;
> +	unsigned long flags;
> +	int c, total = 0;
> +
> +	if ((!info) || !tty || !info->xmit_buf)
> +		return 0;
> +
> +	nd = info->net_node;
> +
> +	if (!nd)
> +		return 0;
> +	while (1) {
> +		c = min(count, min((int)(SERIAL_XMIT_SIZE - info->xmit_cnt - 1),
> +					(int)(SERIAL_XMIT_SIZE - info->xmit_head)));

Casts here only mean you have wrong types somewhere. If it must be (not
in this case), use min_t.

> +		if (c <= 0)
> +			break;
> +
> +		spin_lock_irqsave(&info->tx_lock, flags);
> +		memcpy(info->xmit_buf + info->xmit_head, buf, c);
> +		info->xmit_head = (info->xmit_head + c) & (SERIAL_XMIT_SIZE - 1);

If you used modulo (which is what you want here), you need not decrement
by one, right?

> +		info->xmit_cnt += c;
> +		spin_unlock_irqrestore(&info->tx_lock, flags);
> +
> +		buf += c;
> +		count -= c;
> +		total += c;
> +	}
> +
> +	if (info->xmit_cnt) {
> +		nd->tx_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_in_wait))
> +			wake_up_interruptible(&nd->select_in_wait);
> +	}
> +
> +	return total;
> +}
...
> +static int npreal_ioctl(struct tty_struct *tty, unsigned int cmd,
> +			unsigned long arg)
> +{
> +	struct npreal_struct *info = (struct npreal_struct *)tty->driver_data;
> +	struct serial_icounter_struct *p_cuser; /* user space */
> +	unsigned long templ;
> +	int ret = 0;
> +
> +	if (!info)
> +		return -ENODEV;
> +
> +	if ((cmd != TIOCGSERIAL) && (cmd != TIOCMIWAIT) && (cmd != TIOCGICOUNT) &&
> +		test_bit(TTY_IO_ERROR, &tty->flags))
> +		return -EIO;
> +
> +	switch (cmd) {
> +	case TCFLSH:

You should not handle ioctl like these in the driver, should you?

> +		ret = tty_check_change(tty);
> +		if (!ret) {
> +			switch (arg) {
> +			case TCIFLUSH:
> +				if (tty->ldisc->ops->flush_buffer)
> +					tty->ldisc->ops->flush_buffer(tty);
> +				break;
> +
> +			case TCIOFLUSH:
> +				if (tty->ldisc->ops->flush_buffer)
> +					tty->ldisc->ops->flush_buffer(tty);
> +				npreal_flush_buffer(tty);
> +				break;
> +
> +			case TCOFLUSH:
> +				npreal_flush_buffer(tty);
> +				break;
> +
> +			default:
> +				ret = -EINVAL;
> +			}
> +		}
> +		break;
...
> +static const struct tty_operations mpvar_ops = {
> +	.open               = npreal_open,
> +	.close              = npreal_close,
> +	.write              = npreal_write,
> +	.put_char           = npreal_put_char,
> +	.write_room         = npreal_write_room,
> +	.chars_in_buffer    = npreal_chars_in_buffer,
> +	.flush_buffer       = npreal_flush_buffer,
> +	.wait_until_sent    = npreal_wait_until_sent,
> +	.break_ctl          = npreal_break,
> +	.ioctl              = npreal_ioctl,
> +	.throttle           = npreal_throttle,
> +	.unthrottle         = npreal_unthrottle,
> +	.set_termios        = npreal_set_termios,
> +	.hangup             = npreal_hangup,
> +	.tiocmget           = npreal_tiocmget,
> +	.tiocmset           = npreal_tiocmset,
> +};
...
> +static ssize_t npreal_net_read(struct file *file, char *buf, size_t count, loff_t *ppos)
> +{
> +	struct nd_struct *nd = file->private_data;
> +	struct npreal_struct *info;
> +	struct tty_struct *tty;
> +	ssize_t rtn = 0;
> +	int min_val;
> +	unsigned long flags;
> +
> +
> +	info = (struct npreal_struct *)nd->tty_node;
> +	tty = info->tty;
> +
> +	if (!nd || !info || !tty) {
> +		rtn = -ENXIO;
> +		return rtn;
> +	}
> +
> +	if (info->x_char) {
> +		rtn = 1;
> +		if (copy_to_user(buf, &info->x_char, rtn)) {

I.e. put_user.

...
> +static ssize_t npreal_net_write(struct file *file, const char *buf, size_t count, loff_t *ppos)
> +{
> +	struct nd_struct *nd = file->private_data;
> +	struct npreal_struct *info;
> +	struct tty_struct *tty;
> +	unsigned char *k_buf = NULL;
> +	ssize_t rtn = 0;
> +	int cnt;
> +	unsigned long tmp;
> +
> +	if (!buf) {
> +		rtn = count;
> +		goto done;
> +	}
> +
> +	k_buf = kmalloc_array(count, sizeof(unsigned char), GFP_KERNEL);
> +	info = (struct npreal_struct *)nd->tty_node;
> +	tmp =  copy_from_user(k_buf, buf, count);

Is buf a user buffer? It is not marked as such. Ah, this is the proc
interface you should drop.

> +	if ((k_buf == NULL || tmp) || (!nd || !info) || (info->flags & ASYNC_CLOSING)) {
> +		rtn = count;
> +		goto done;
> +	}



Overall, you shall deduplicate the code and use tty_port helpers
wherever possible. This will simplify the code a lot. I wonder where you
get from this submission with "3194 insertions".

thanks,
-- 
js




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux