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

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

[Greg Kroah-Hartman]

On Tue, Jul 14, 2020 at 06:24:42AM +0000, 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.

A new serial driver, nice!

> 
> The following Moxa products are supported:
> * CN2600 Series
> * CN2500 Series
> * NPort DE Series
> * NPort 5000A-M12 Series
> * NPort 5100 Series
> * NPort 5200 Series
> * NPort 5400 Series
> * NPort 5600 Desktop Series
> * NPort 5600 Rackmount Series
> * NPort Wireless Series
> * NPort IA5000 Series
> * NPort 6000 Series
> * NPort S8000 Series
> * NPort S8455I Series
> * NPort S9000 Series
> * NE-4100 Series
> * MiiNePort Series
> 
> Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> Signed-off-by: Jason Chen <jason.chen@moxa.com>
> Signed-off-by: Danny Lin <danny.lin@moxa.com>
> Signed-off-by: Victor Yu <victor.yu@moxa.com>
> ---
>  drivers/tty/Kconfig   |   11 +
>  drivers/tty/Makefile  |    1 +
>  drivers/tty/npreal2.c | 3042 +++++++++++++++++++++++++++++++++++++++++
>  drivers/tty/npreal2.h |  140 ++
>  4 files changed, 3194 insertions(+)
>  create mode 100644 drivers/tty/npreal2.c
>  create mode 100644 drivers/tty/npreal2.h
> 
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 93fd984eb2f5..79b545269b71 100644
> --- 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"
> +	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
>  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 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * npreal2.c  -- MOXA NPort Server family Real TTY driver.
> + *
> + * Copyright (c) 1999-2020  Moxa Technologies (support@moxa.com)
> + *
> + * Supports the following Moxa Product:
> + * CN2600 Series
> + * CN2500 Series
> + * NPort DE Series
> + * NPort 5000A-M12 Series
> + * NPort 5100 Series
> + * NPort 5200 Series
> + * NPort 5400 Series
> + * NPort 5600 Desktop Series
> + * NPort 5600 Rackmount Series
> + * NPort Wireless Series
> + * NPort IA5000 Series
> + * NPort 6000 Series
> + * NPort S8000 Series
> + * NPort S8455I Series
> + * NPort S9000 Series
> + * NE-4100 Series
> + * MiiNePort Series
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/fcntl.h>
> +#include <linux/version.h>
> +#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;
> +static int verbose = 1;

Please do not do that, just use the normal dynamic debug logic that the
kernel has and that everyone uses.  No per-driver type of debugging
functionality, as obviously that does not scale at all.

> +
> +MODULE_AUTHOR("<support@moxa.com>");

We need a real author name here if you want to be the maintainer.
Otherwise a support address can't be an author :)

> +MODULE_DESCRIPTION("MOXA Async/NPort Server Family Real TTY Driver");
> +module_param(ttymajor, int, 0);
> +module_param(verbose, int, 0644);

No module parameters please, they should not be needed at all.

> +MODULE_VERSION(NPREAL_VERSION);

No need for a driver version once the code is in the kernel tree, as the
kernel version is what matters.

> +MODULE_LICENSE("GPL");
> +
> +struct server_setting_struct {
> +	int32_t server_type;
> +	int32_t disable_fifo;

Please use kernel types, like u32, instead of userspace types like
these.

> +};
> +
> +struct npreal_struct {
> +	struct tty_port ttyPort;
> +	struct work_struct tqueue;
> +	struct work_struct process_flip_tqueue;

None of these should be pointers?

> +	struct ktermios normal_termios;
> +	struct ktermios callout_termios;
> +	/* kernel counters for the 4 input interrupts */
> +	struct async_icount icount;
> +	struct semaphore rx_semaphore;
> +	struct nd_struct *net_node;
> +	struct tty_struct *tty;
> +	struct pid *session;
> +	struct pid *pgrp;
> +	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 */

enumerated 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;

Do you really need these?  Why not just use a ringbuffer structure from
the kernel?

> +	int xmit_cnt;
> +	unsigned char *xmit_buf;
> +
> +	/*
> +	 * We use spin_lock_irqsave instead of semaphonre here.
> +	 * 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
> +	 * 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;

2 locks in the same structure???

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

That's a lot of different queues a specific thing can be on at the same
time.  Are you sure you want that to happen?

> +	int32_t server_type;

Enumerated 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;

Serial port drivers should never create /proc files.  If you want
debugging stuff, use debugfs.

> +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() */

Huh?   Why is that needed?

> +	/* used waitqueue_active() is safe because smp_mb() is used */

Why?

Are you _sure_ you need that barrier?  If so, please document it really
really really well for you to be able to understand it in 10 years when
people have questions about it :)

> +static int npreal_chars_in_buffer(struct tty_struct *tty)
> +{
> +	struct npreal_struct *info = (struct npreal_struct *)tty->driver_data;
> +
> +	if (!info)
> +		return -EIO;

How can that ever happen?

> +
> +	return info->xmit_cnt;
> +}


> +/**

No need for kernel doc format for static functions.

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

Did you run sparse on this code?  Please do so and fix up the
errors/warnings it gives you for stuff like this.

> +static int npreal_net_open(struct inode *inode, struct file *file)
> +{
> +	struct nd_struct *nd;
> +	int rtn = 0;
> +
> +	try_module_get(THIS_MODULE);

That is racy and not needed and even wrong here.

Do not do this, it is not the way to correctly do it at all.

> +	if (!capable(CAP_SYS_ADMIN)) {
> +		rtn = -EPERM;
> +		goto done;
> +	}
> +
> +	if (file->private_data) {

How could that ever happen?

> +		rtn = -EINVAL;
> +		goto done;
> +	}
> +
> +	nd = (struct nd_struct *)PDE_DATA(inode);
> +	if (!nd) {
> +		rtn = -ENXIO;
> +		goto done;
> +	}
> +
> +	down(&nd->semaphore);
> +
> +	if (nd->flag & NPREAL_NET_NODE_OPENED) {
> +		rtn = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	nd->flag |= NPREAL_NET_NODE_OPENED;
> +	nd->tx_ready = 0;
> +	nd->rx_ready = 1;
> +	nd->cmd_ready = 0;
> +	tty_register_device(npvar_sdriver, nd->tty_node->port, NULL);
> +
> +unlock:
> +	up(&nd->semaphore);
> +	file->private_data = (void *)nd;
> +done:
> +	if (rtn)
> +		module_put(THIS_MODULE);
> +
> +	return rtn;
> +}
> +
> +static int npreal_net_close(struct inode *inode, struct file *file)
> +{
> +	struct nd_struct *nd;
> +
> +	nd = (struct nd_struct *)(file->private_data);

No need to cast.

> +	if (!nd)
> +		goto done;
> +
> +	/* This flag will be checked when npreal_net_open() is called again. */
> +	nd->flag &= ~NPREAL_NET_NODE_OPENED;

No locking?

> +	tty_unregister_device(npvar_sdriver, nd->tty_node->port);
> +
> +done:
> +	file->private_data = NULL;
> +	module_put(THIS_MODULE);

again, not needed.

> +	return 0;
> +}
> +
> +static unsigned int npreal_net_select(struct file *file, struct poll_table_struct *table)
> +{
> +	struct nd_struct *nd = file->private_data;
> +	unsigned int retval = 0;
> +
> +	if (!nd)
> +		return retval;
> +
> +	poll_wait(file, &nd->select_in_wait, table);
> +	poll_wait(file, &nd->select_out_wait, table);
> +	poll_wait(file, &nd->select_ex_wait, table);
> +
> +	if (nd->tx_ready)
> +		retval |= POLLIN | POLLRDNORM;
> +	if (nd->rx_ready)
> +		retval |= POLLOUT | POLLWRNORM;
> +	if (nd->cmd_ready)
> +		retval |= POLLPRI;
> +
> +	return retval;
> +}
> +
> +static long npreal_net_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct nd_struct *nd = file->private_data;
> +	int ret = 0;
> +	int size, len;
> +
> +	if (!nd) {
> +		ret = -ENXIO;
> +		return ret;
> +	}
> +
> +	size = _IOC_SIZE(cmd);
> +
> +	switch (_IOC_NR(cmd)) {
> +	case NPREAL_NET_CMD_RETRIEVE:

Do not make up custom ioctls for a single serial driver, just use the
default ones, that should be all that is needed, correct?

If not, what do you need to do that the serial layer does not provide
for you?

> +static int __init npreal2_module_init(void)
> +{
> +	int i, retval;
> +
> +	npvar_sdriver = alloc_tty_driver(NPREAL_PORTS);
> +	if (!npvar_sdriver)
> +		return -ENOMEM;
> +
> +	npvar_sdriver->name = "ttyr";
> +	npvar_sdriver->major = ttymajor;
> +	npvar_sdriver->minor_start = 0;
> +	npvar_sdriver->type = TTY_DRIVER_TYPE_SERIAL;
> +	npvar_sdriver->subtype = SERIAL_TYPE_NORMAL;
> +	npvar_sdriver->init_termios = tty_std_termios;
> +	npvar_sdriver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL;
> +	npvar_sdriver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> +
> +	npvar_table = kmalloc_array(NPREAL_PORTS, sizeof(struct npreal_struct), GFP_KERNEL);
> +	if (npvar_table == NULL)
> +		return -1;
> +
> +	npvar_net_nodes = kmalloc_array(NPREAL_PORTS, sizeof(struct nd_struct), GFP_KERNEL);
> +	if (npvar_net_nodes == NULL) {
> +		kfree(npvar_table);
> +		return -1;
> +	}
> +
> +	tty_set_operations(npvar_sdriver, &mpvar_ops);
> +	memset(npvar_table, 0, NPREAL_PORTS * sizeof(struct npreal_struct));
> +
> +	for (i = 0; i < NPREAL_PORTS; i++) {
> +		tty_port_init(&npvar_table[i].ttyPort);
> +		tty_port_link_device(&npvar_table[i].ttyPort, npvar_sdriver, i);
> +	}
> +
> +	retval = tty_register_driver(npvar_sdriver);
> +	if (retval) {
> +		pr_err("Couldn't install MOXA Async/NPort server family driver !\n");
> +		put_tty_driver(npvar_sdriver);
> +		return -1;
> +	}
> +
> +	retval = npreal_init(npvar_table, npvar_net_nodes);
> +	if (retval) {
> +		tty_unregister_driver(npvar_sdriver);
> +		pr_err("Couldn't install MOXA Async/NPort server family Real TTY driver !\n");
> +		return -1;
> +	}
> +
> +	pr_info("MOXA Nport driver version %s\n", NPREAL_VERSION);

No need to be noisy if all works properly, a driver should be silent.

But why are you doing all of this initialization without actually
checking if your hardware is present in the system?  Please use the
bus-specific logic that your hardware is on to properly set things up in
the probe function, not all in the module init function, as this will
probably cause failures if loaded on a system without the hardware,
right?

And what causes the module to properly load automatically?  I missed
that logic somewhere in here, where is it?

> --- /dev/null
> +++ b/drivers/tty/npreal2.h

No need for a .h file for a single .c file.

thanks,

greg k-h

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

[Johnson CH Chen (陳昭勳)]

Hi Greg,

Thanks for your detailed good review!

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, July 14, 2020 3:12 PM
> To: Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@moxa.com>
> Cc: Jiri Slaby <jirislaby@gmail.com>; linux-kernel@vger.kernel.org;
> linux-serial@vger.kernel.org
> Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
> 
> On Tue, Jul 14, 2020 at 06:24:42AM +0000, 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.
> 
> A new serial driver, nice!
> 
> >
> > The following Moxa products are supported:
> > * CN2600 Series
> > * CN2500 Series
> > * NPort DE Series
> > * NPort 5000A-M12 Series
> > * NPort 5100 Series
> > * NPort 5200 Series
> > * NPort 5400 Series
> > * NPort 5600 Desktop Series
> > * NPort 5600 Rackmount Series
> > * NPort Wireless Series
> > * NPort IA5000 Series
> > * NPort 6000 Series
> > * NPort S8000 Series
> > * NPort S8455I Series
> > * NPort S9000 Series
> > * NE-4100 Series
> > * MiiNePort Series
> >
> > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> > Signed-off-by: Jason Chen <jason.chen@moxa.com>
> > Signed-off-by: Danny Lin <danny.lin@moxa.com>
> > Signed-off-by: Victor Yu <victor.yu@moxa.com>
> > ---
> >  drivers/tty/Kconfig   |   11 +
> >  drivers/tty/Makefile  |    1 +
> >  drivers/tty/npreal2.c | 3042
> > +++++++++++++++++++++++++++++++++++++++++
> >  drivers/tty/npreal2.h |  140 ++
> >  4 files changed, 3194 insertions(+)
> >  create mode 100644 drivers/tty/npreal2.c  create mode 100644
> > drivers/tty/npreal2.h
> >
> > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig index
> > 93fd984eb2f5..79b545269b71 100644
> > --- 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"
> > +	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
> >  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 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * npreal2.c  -- MOXA NPort Server family Real TTY driver.
> > + *
> > + * Copyright (c) 1999-2020  Moxa Technologies (support@moxa.com)
> > + *
> > + * Supports the following Moxa Product:
> > + * CN2600 Series
> > + * CN2500 Series
> > + * NPort DE Series
> > + * NPort 5000A-M12 Series
> > + * NPort 5100 Series
> > + * NPort 5200 Series
> > + * NPort 5400 Series
> > + * NPort 5600 Desktop Series
> > + * NPort 5600 Rackmount Series
> > + * NPort Wireless Series
> > + * NPort IA5000 Series
> > + * NPort 6000 Series
> > + * NPort S8000 Series
> > + * NPort S8455I Series
> > + * NPort S9000 Series
> > + * NE-4100 Series
> > + * MiiNePort Series
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/fcntl.h>
> > +#include <linux/version.h>
> > +#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;
> > +static int verbose = 1;
> 
> Please do not do that, just use the normal dynamic debug logic that the kernel
> has and that everyone uses.  No per-driver type of debugging functionality, as
> obviously that does not scale at all.
> 
> > +
> > +MODULE_AUTHOR("<support@moxa.com>");
> 
> We need a real author name here if you want to be the maintainer.
> Otherwise a support address can't be an author :)
> 
After discussion, I will put my email, thanks!

> > +MODULE_DESCRIPTION("MOXA Async/NPort Server Family Real TTY
> Driver");
> > +module_param(ttymajor, int, 0); module_param(verbose, int, 0644);
> 
> No module parameters please, they should not be needed at all.
> 
> > +MODULE_VERSION(NPREAL_VERSION);
> 
> No need for a driver version once the code is in the kernel tree, as the kernel
> version is what matters.
> 
> > +MODULE_LICENSE("GPL");
> > +
> > +struct server_setting_struct {
> > +	int32_t server_type;
> > +	int32_t disable_fifo;
> 
> Please use kernel types, like u32, instead of userspace types like these.
> 
> > +};
> > +
> > +struct npreal_struct {
> > +	struct tty_port ttyPort;
> > +	struct work_struct tqueue;
> > +	struct work_struct process_flip_tqueue;
> 
> None of these should be pointers?

tqueue and process_flip_tqueue could be not used, like Jiri refers.
> 
> > +	struct ktermios normal_termios;
> > +	struct ktermios callout_termios;
> > +	/* kernel counters for the 4 input interrupts */
> > +	struct async_icount icount;
> > +	struct semaphore rx_semaphore;
> > +	struct nd_struct *net_node;
> > +	struct tty_struct *tty;
> > +	struct pid *session;
> > +	struct pid *pgrp;
> > +	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 */
> 
> enumerated 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;
> 
> Do you really need these?  Why not just use a ringbuffer structure from the
> kernel?
> 
> > +	int xmit_cnt;
> > +	unsigned char *xmit_buf;
> > +
> > +	/*
> > +	 * We use spin_lock_irqsave instead of semaphonre here.
> > +	 * 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
> > +	 * 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;
> 
> 2 locks in the same structure???
> 
I think " struct semaphore semaphore" can be removed, just use cmd_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;
> 
> That's a lot of different queues a specific thing can be on at the same time.
> Are you sure you want that to happen?

Above some queue can be decreased, I will try to do that.
> 
> > +	int32_t server_type;
> 
> Enumerated 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;
> 
> Serial port drivers should never create /proc files.  If you want debugging stuff,
> use debugfs.
> 
> > +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() */
> 
> Huh?   Why is that needed?
> 
> > +	/* used waitqueue_active() is safe because smp_mb() is used */
> 
> Why?
> 
> Are you _sure_ you need that barrier?  If so, please document it really really
> really well for you to be able to understand it in 10 years when people have
> questions about it :)
> 
It seems it's good to avoid usage of waitqueue_active() that Jiri refers, too. So smp_mb() is not needed because of warning of checkpatch.py.

> > +static int npreal_chars_in_buffer(struct tty_struct *tty) {
> > +	struct npreal_struct *info = (struct npreal_struct
> > +*)tty->driver_data;
> > +
> > +	if (!info)
> > +		return -EIO;
> 
> How can that ever happen?
> 
It seems a to protect a situation that reading/writing while upload driver unexpectedly. I think this kind of protections can be decreased, I'll try to do this.

> > +
> > +	return info->xmit_cnt;
> > +}
> 
> 
> > +/**
> 
> No need for kernel doc format for static functions.
> 
> > + * 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);
> 
> Did you run sparse on this code?  Please do so and fix up the errors/warnings
> it gives you for stuff like this.
> 
Thanks your nice remind, I'll use sparse to check code, thanks!

> > +static int npreal_net_open(struct inode *inode, struct file *file) {
> > +	struct nd_struct *nd;
> > +	int rtn = 0;
> > +
> > +	try_module_get(THIS_MODULE);
> 
> That is racy and not needed and even wrong here.
> 
> Do not do this, it is not the way to correctly do it at all.
> 
> > +	if (!capable(CAP_SYS_ADMIN)) {
> > +		rtn = -EPERM;
> > +		goto done;
> > +	}
> > +
> > +	if (file->private_data) {
> 
> How could that ever happen?
> 
This protects a situation that npreal_net_close() isn't called but npreal_net_open() is called again.

> > +		rtn = -EINVAL;
> > +		goto done;
> > +	}
> > +
> > +	nd = (struct nd_struct *)PDE_DATA(inode);
> > +	if (!nd) {
> > +		rtn = -ENXIO;
> > +		goto done;
> > +	}
> > +
> > +	down(&nd->semaphore);
> > +
> > +	if (nd->flag & NPREAL_NET_NODE_OPENED) {
> > +		rtn = -EBUSY;
> > +		goto unlock;
> > +	}
> > +
> > +	nd->flag |= NPREAL_NET_NODE_OPENED;
> > +	nd->tx_ready = 0;
> > +	nd->rx_ready = 1;
> > +	nd->cmd_ready = 0;
> > +	tty_register_device(npvar_sdriver, nd->tty_node->port, NULL);
> > +
> > +unlock:
> > +	up(&nd->semaphore);
> > +	file->private_data = (void *)nd;
> > +done:
> > +	if (rtn)
> > +		module_put(THIS_MODULE);
> > +
> > +	return rtn;
> > +}
> > +
> > +static int npreal_net_close(struct inode *inode, struct file *file) {
> > +	struct nd_struct *nd;
> > +
> > +	nd = (struct nd_struct *)(file->private_data);
> 
> No need to cast.
> 
> > +	if (!nd)
> > +		goto done;
> > +
> > +	/* This flag will be checked when npreal_net_open() is called again. */
> > +	nd->flag &= ~NPREAL_NET_NODE_OPENED;
> 
> No locking?
> 
It should be locked.

> > +	tty_unregister_device(npvar_sdriver, nd->tty_node->port);
> > +
> > +done:
> > +	file->private_data = NULL;
> > +	module_put(THIS_MODULE);
> 
> again, not needed.
> 
> > +	return 0;
> > +}
> > +
> > +static unsigned int npreal_net_select(struct file *file, struct
> > +poll_table_struct *table) {
> > +	struct nd_struct *nd = file->private_data;
> > +	unsigned int retval = 0;
> > +
> > +	if (!nd)
> > +		return retval;
> > +
> > +	poll_wait(file, &nd->select_in_wait, table);
> > +	poll_wait(file, &nd->select_out_wait, table);
> > +	poll_wait(file, &nd->select_ex_wait, table);
> > +
> > +	if (nd->tx_ready)
> > +		retval |= POLLIN | POLLRDNORM;
> > +	if (nd->rx_ready)
> > +		retval |= POLLOUT | POLLWRNORM;
> > +	if (nd->cmd_ready)
> > +		retval |= POLLPRI;
> > +
> > +	return retval;
> > +}
> > +
> > +static long npreal_net_ioctl(struct file *file, unsigned int cmd,
> > +unsigned long arg) {
> > +	struct nd_struct *nd = file->private_data;
> > +	int ret = 0;
> > +	int size, len;
> > +
> > +	if (!nd) {
> > +		ret = -ENXIO;
> > +		return ret;
> > +	}
> > +
> > +	size = _IOC_SIZE(cmd);
> > +
> > +	switch (_IOC_NR(cmd)) {
> > +	case NPREAL_NET_CMD_RETRIEVE:
> 
> Do not make up custom ioctls for a single serial driver, just use the default
> ones, that should be all that is needed, correct?
> 
> If not, what do you need to do that the serial layer does not provide for you?
> 
I will try to use default ioctl, thanks!

> > +static int __init npreal2_module_init(void) {
> > +	int i, retval;
> > +
> > +	npvar_sdriver = alloc_tty_driver(NPREAL_PORTS);
> > +	if (!npvar_sdriver)
> > +		return -ENOMEM;
> > +
> > +	npvar_sdriver->name = "ttyr";
> > +	npvar_sdriver->major = ttymajor;
> > +	npvar_sdriver->minor_start = 0;
> > +	npvar_sdriver->type = TTY_DRIVER_TYPE_SERIAL;
> > +	npvar_sdriver->subtype = SERIAL_TYPE_NORMAL;
> > +	npvar_sdriver->init_termios = tty_std_termios;
> > +	npvar_sdriver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL |
> CLOCAL;
> > +	npvar_sdriver->flags = TTY_DRIVER_REAL_RAW |
> TTY_DRIVER_DYNAMIC_DEV;
> > +
> > +	npvar_table = kmalloc_array(NPREAL_PORTS, sizeof(struct npreal_struct),
> GFP_KERNEL);
> > +	if (npvar_table == NULL)
> > +		return -1;
> > +
> > +	npvar_net_nodes = kmalloc_array(NPREAL_PORTS, sizeof(struct
> nd_struct), GFP_KERNEL);
> > +	if (npvar_net_nodes == NULL) {
> > +		kfree(npvar_table);
> > +		return -1;
> > +	}
> > +
> > +	tty_set_operations(npvar_sdriver, &mpvar_ops);
> > +	memset(npvar_table, 0, NPREAL_PORTS * sizeof(struct npreal_struct));
> > +
> > +	for (i = 0; i < NPREAL_PORTS; i++) {
> > +		tty_port_init(&npvar_table[i].ttyPort);
> > +		tty_port_link_device(&npvar_table[i].ttyPort, npvar_sdriver, i);
> > +	}
> > +
> > +	retval = tty_register_driver(npvar_sdriver);
> > +	if (retval) {
> > +		pr_err("Couldn't install MOXA Async/NPort server family
> driver !\n");
> > +		put_tty_driver(npvar_sdriver);
> > +		return -1;
> > +	}
> > +
> > +	retval = npreal_init(npvar_table, npvar_net_nodes);
> > +	if (retval) {
> > +		tty_unregister_driver(npvar_sdriver);
> > +		pr_err("Couldn't install MOXA Async/NPort server family Real TTY
> driver !\n");
> > +		return -1;
> > +	}
> > +
> > +	pr_info("MOXA Nport driver version %s\n", NPREAL_VERSION);
> 
> No need to be noisy if all works properly, a driver should be silent.
> 
> But why are you doing all of this initialization without actually checking if your
> hardware is present in the system?  Please use the bus-specific logic that your
> hardware is on to properly set things up in the probe function, not all in the
> module init function, as this will probably cause failures if loaded on a system
> without the hardware, right?
> 
NPort driver creates virtual COM and communicates with Nport daemon, and let Nport daemon talks with HW,
so probe() for this driver may not be needed.

> And what causes the module to properly load automatically?  I missed that
> logic somewhere in here, where is it?
> 
It doesn't load automatically for original design. We load this driver by init.d or other daemons.

> > --- /dev/null
> > +++ b/drivers/tty/npreal2.h
> 
> No need for a .h file for a single .c file.
> 
I will try to follow above good suggestions, thanks!

Best regards,
Johnson

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

[Greg Kroah-Hartman]

On Tue, Jul 14, 2020 at 06:24:42AM +0000, 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.
> 
> The following Moxa products are supported:
> * CN2600 Series
> * CN2500 Series
> * NPort DE Series
> * NPort 5000A-M12 Series
> * NPort 5100 Series
> * NPort 5200 Series
> * NPort 5400 Series
> * NPort 5600 Desktop Series
> * NPort 5600 Rackmount Series
> * NPort Wireless Series
> * NPort IA5000 Series
> * NPort 6000 Series
> * NPort S8000 Series
> * NPort S8455I Series
> * NPort S9000 Series
> * NE-4100 Series
> * MiiNePort Series
> 
> Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> Signed-off-by: Jason Chen <jason.chen@moxa.com>
> Signed-off-by: Danny Lin <danny.lin@moxa.com>
> Signed-off-by: Victor Yu <victor.yu@moxa.com>
> ---
>  drivers/tty/Kconfig   |   11 +
>  drivers/tty/Makefile  |    1 +
>  drivers/tty/npreal2.c | 3042 +++++++++++++++++++++++++++++++++++++++++
>  drivers/tty/npreal2.h |  140 ++
>  4 files changed, 3194 insertions(+)
>  create mode 100644 drivers/tty/npreal2.c
>  create mode 100644 drivers/tty/npreal2.h
> 
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 93fd984eb2f5..79b545269b71 100644
> --- 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"
> +	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
>  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 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * npreal2.c  -- MOXA NPort Server family Real TTY driver.
> + *
> + * Copyright (c) 1999-2020  Moxa Technologies (support@moxa.com)
> + *
> + * Supports the following Moxa Product:
> + * CN2600 Series
> + * CN2500 Series
> + * NPort DE Series
> + * NPort 5000A-M12 Series
> + * NPort 5100 Series
> + * NPort 5200 Series
> + * NPort 5400 Series
> + * NPort 5600 Desktop Series
> + * NPort 5600 Rackmount Series
> + * NPort Wireless Series
> + * NPort IA5000 Series
> + * NPort 6000 Series
> + * NPort S8000 Series
> + * NPort S8455I Series
> + * NPort S9000 Series
> + * NE-4100 Series
> + * MiiNePort Series
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/fcntl.h>
> +#include <linux/version.h>
> +#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;
> +static int verbose = 1;
> +
> +MODULE_AUTHOR("<support@moxa.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;
> +	/* kernel counters for the 4 input interrupts */
> +	struct async_icount icount;
> +	struct semaphore rx_semaphore;
> +	struct nd_struct *net_node;
> +	struct tty_struct *tty;
> +	struct pid *session;
> +	struct pid *pgrp;
> +	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;
> +
> +	/*
> +	 * We use spin_lock_irqsave instead of semaphonre here.
> +	 * 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
> +	 * 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];

You seem to have two "static" buffers here, for your device, that you
semi-randomly write to all over the place, but I can't find any locking
or coordination between things that prevents multiple commands from not
just overwritting each other.

Also, how does the data get sent to the hardware at all?  I see
cmd_buffer[] being written to, but what reads from it and how does the
hardware get the data?

What am I missing here?

thanks,

greg k-h

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

[Johnson CH Chen (陳昭勳)]

Hi Greg,

Thanks for detailed and good suggestions!

> From: linux-serial-owner@vger.kernel.org
> <linux-serial-owner@vger.kernel.org> On Behalf Of Greg Kroah-Hartman
> Sent: Tuesday, July 14, 2020 3:36 PM
> To: Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@moxa.com>
> Cc: Jiri Slaby <jirislaby@gmail.com>; linux-kernel@vger.kernel.org;
> linux-serial@vger.kernel.org
> Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
> 
> On Tue, Jul 14, 2020 at 06:24:42AM +0000, 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.
> >
> > The following Moxa products are supported:
> > * CN2600 Series
> > * CN2500 Series
> > * NPort DE Series
> > * NPort 5000A-M12 Series
> > * NPort 5100 Series
> > * NPort 5200 Series
> > * NPort 5400 Series
> > * NPort 5600 Desktop Series
> > * NPort 5600 Rackmount Series
> > * NPort Wireless Series
> > * NPort IA5000 Series
> > * NPort 6000 Series
> > * NPort S8000 Series
> > * NPort S8455I Series
> > * NPort S9000 Series
> > * NE-4100 Series
> > * MiiNePort Series
> >
> > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> > Signed-off-by: Jason Chen <jason.chen@moxa.com>
> > Signed-off-by: Danny Lin <danny.lin@moxa.com>
> > Signed-off-by: Victor Yu <victor.yu@moxa.com>
> > ---
> >  drivers/tty/Kconfig   |   11 +
> >  drivers/tty/Makefile  |    1 +
> >  drivers/tty/npreal2.c | 3042
> > +++++++++++++++++++++++++++++++++++++++++
> >  drivers/tty/npreal2.h |  140 ++
> >  4 files changed, 3194 insertions(+)
> >  create mode 100644 drivers/tty/npreal2.c  create mode 100644
> > drivers/tty/npreal2.h
> >
> > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig index
> > 93fd984eb2f5..79b545269b71 100644
> > --- 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"
> > +	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
> >  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 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * npreal2.c  -- MOXA NPort Server family Real TTY driver.
> > + *
> > + * Copyright (c) 1999-2020  Moxa Technologies (support@moxa.com)
> > + *
> > + * Supports the following Moxa Product:
> > + * CN2600 Series
> > + * CN2500 Series
> > + * NPort DE Series
> > + * NPort 5000A-M12 Series
> > + * NPort 5100 Series
> > + * NPort 5200 Series
> > + * NPort 5400 Series
> > + * NPort 5600 Desktop Series
> > + * NPort 5600 Rackmount Series
> > + * NPort Wireless Series
> > + * NPort IA5000 Series
> > + * NPort 6000 Series
> > + * NPort S8000 Series
> > + * NPort S8455I Series
> > + * NPort S9000 Series
> > + * NE-4100 Series
> > + * MiiNePort Series
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/fcntl.h>
> > +#include <linux/version.h>
> > +#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;
> > +static int verbose = 1;
> > +
> > +MODULE_AUTHOR("<support@moxa.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;
> > +	/* kernel counters for the 4 input interrupts */
> > +	struct async_icount icount;
> > +	struct semaphore rx_semaphore;
> > +	struct nd_struct *net_node;
> > +	struct tty_struct *tty;
> > +	struct pid *session;
> > +	struct pid *pgrp;
> > +	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;
> > +
> > +	/*
> > +	 * We use spin_lock_irqsave instead of semaphonre here.
> > +	 * 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
> > +	 * 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];
> 
> You seem to have two "static" buffers here, for your device, that you
> semi-randomly write to all over the place, but I can't find any locking or
> coordination between things that prevents multiple commands from not just
> overwritting each other.
> 
For cmd_buffer[], we use npreal_wait_and_set_command() to make sure cmd_buffer[] is safe to be written by checking "cmd_buffer[0] == 0".

For rsp_buffer[], we use npreal_wait_command_completed() to make sure rsp_buffer[] is desired by checking rsp_buffer[0] and rsp_buffer[1]. Command_set and command should be checked. Besides, rsp_buffer[] is got from user space by "NPREAL_NET_CMD_RESPONSE" in npreal_net_ioctl().

> Also, how does the data get sent to the hardware at all?  I see cmd_buffer[]
> being written to, but what reads from it and how does the hardware get the
> data?

Actually we need to both NPort driver (this driver) and Npreal daemon (userspace) to let HW work. Npreal daemon can communicate with HW by socket, and Npreal deamon communicates with Nport driver by "npreal_net_fops". When commands are ready for driver part, it will wake up poll event to let Nport daemon know.

Best regards,
Johnson
> 
> What am I missing here?
> 
> thanks,
> 
> greg k-h

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

[Greg Kroah-Hartman]

On Thu, Jul 16, 2020 at 07:19:02AM +0000, Johnson CH Chen (陳昭勳) wrote:
> Hi Greg,
> 
> Thanks for detailed and good suggestions!
> 
> > From: linux-serial-owner@vger.kernel.org
> > <linux-serial-owner@vger.kernel.org> On Behalf Of Greg Kroah-Hartman
> > Sent: Tuesday, July 14, 2020 3:36 PM
> > To: Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@moxa.com>
> > Cc: Jiri Slaby <jirislaby@gmail.com>; linux-kernel@vger.kernel.org;
> > linux-serial@vger.kernel.org
> > Subject: Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
> > 
> > On Tue, Jul 14, 2020 at 06:24:42AM +0000, 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.
> > >
> > > The following Moxa products are supported:
> > > * CN2600 Series
> > > * CN2500 Series
> > > * NPort DE Series
> > > * NPort 5000A-M12 Series
> > > * NPort 5100 Series
> > > * NPort 5200 Series
> > > * NPort 5400 Series
> > > * NPort 5600 Desktop Series
> > > * NPort 5600 Rackmount Series
> > > * NPort Wireless Series
> > > * NPort IA5000 Series
> > > * NPort 6000 Series
> > > * NPort S8000 Series
> > > * NPort S8455I Series
> > > * NPort S9000 Series
> > > * NE-4100 Series
> > > * MiiNePort Series
> > >
> > > Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> > > Signed-off-by: Jason Chen <jason.chen@moxa.com>
> > > Signed-off-by: Danny Lin <danny.lin@moxa.com>
> > > Signed-off-by: Victor Yu <victor.yu@moxa.com>
> > > ---
> > >  drivers/tty/Kconfig   |   11 +
> > >  drivers/tty/Makefile  |    1 +
> > >  drivers/tty/npreal2.c | 3042
> > > +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/tty/npreal2.h |  140 ++
> > >  4 files changed, 3194 insertions(+)
> > >  create mode 100644 drivers/tty/npreal2.c  create mode 100644
> > > drivers/tty/npreal2.h
> > >
> > > diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig index
> > > 93fd984eb2f5..79b545269b71 100644
> > > --- 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"
> > > +	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
> > >  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 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * npreal2.c  -- MOXA NPort Server family Real TTY driver.
> > > + *
> > > + * Copyright (c) 1999-2020  Moxa Technologies (support@moxa.com)
> > > + *
> > > + * Supports the following Moxa Product:
> > > + * CN2600 Series
> > > + * CN2500 Series
> > > + * NPort DE Series
> > > + * NPort 5000A-M12 Series
> > > + * NPort 5100 Series
> > > + * NPort 5200 Series
> > > + * NPort 5400 Series
> > > + * NPort 5600 Desktop Series
> > > + * NPort 5600 Rackmount Series
> > > + * NPort Wireless Series
> > > + * NPort IA5000 Series
> > > + * NPort 6000 Series
> > > + * NPort S8000 Series
> > > + * NPort S8455I Series
> > > + * NPort S9000 Series
> > > + * NE-4100 Series
> > > + * MiiNePort Series
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/fcntl.h>
> > > +#include <linux/version.h>
> > > +#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;
> > > +static int verbose = 1;
> > > +
> > > +MODULE_AUTHOR("<support@moxa.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;
> > > +	/* kernel counters for the 4 input interrupts */
> > > +	struct async_icount icount;
> > > +	struct semaphore rx_semaphore;
> > > +	struct nd_struct *net_node;
> > > +	struct tty_struct *tty;
> > > +	struct pid *session;
> > > +	struct pid *pgrp;
> > > +	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;
> > > +
> > > +	/*
> > > +	 * We use spin_lock_irqsave instead of semaphonre here.
> > > +	 * 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
> > > +	 * 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];
> > 
> > You seem to have two "static" buffers here, for your device, that you
> > semi-randomly write to all over the place, but I can't find any locking or
> > coordination between things that prevents multiple commands from not just
> > overwritting each other.
> > 
> For cmd_buffer[], we use npreal_wait_and_set_command() to make sure
> cmd_buffer[] is safe to be written by checking "cmd_buffer[0] == 0".

And what locks are protecting you there?

> For rsp_buffer[], we use npreal_wait_command_completed() to make sure
> rsp_buffer[] is desired by checking rsp_buffer[0] and rsp_buffer[1].
> Command_set and command should be checked. Besides, rsp_buffer[] is
> got from user space by "NPREAL_NET_CMD_RESPONSE" in
> npreal_net_ioctl().

Again, what locking is really handling this?

> > Also, how does the data get sent to the hardware at all?  I see cmd_buffer[]
> > being written to, but what reads from it and how does the hardware get the
> > data?
> 
> Actually we need to both NPort driver (this driver) and Npreal daemon
> (userspace) to let HW work. Npreal daemon can communicate with HW by
> socket, and Npreal deamon communicates with Nport driver by
> "npreal_net_fops". When commands are ready for driver part, it will
> wake up poll event to let Nport daemon know.

That is not obvious at all, and needs to be really really really
documented here.  Why not put the userspace chunk in the tree too?  At
the least, you need to point at it.

And why is a userspace part needed?  We have tty-over-ethernet drivers
that do not require such a thing in the tree somewhere...

thanks,

greg k-h

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

[Johnson CH Chen (陳昭勳)]

Hi Greg,

Thanks for your response!

> > > > +	unsigned long flag;
> > > > +	unsigned char cmd_buffer[84];
> > > > +	unsigned char rsp_buffer[84];
> > >
> > > You seem to have two "static" buffers here, for your device, that 
> > > you semi-randomly write to all over the place, but I can't find 
> > > any locking or coordination between things that prevents multiple 
> > > commands from not just overwritting each other.
> > >
> > For cmd_buffer[], we use npreal_wait_and_set_command() to make sure 
> > cmd_buffer[] is safe to be written by checking "cmd_buffer[0] == 0".
> 
> And what locks are protecting you there?
> 
> > For rsp_buffer[], we use npreal_wait_command_completed() to make 
> > sure rsp_buffer[] is desired by checking rsp_buffer[0] and rsp_buffer[1].
> > Command_set and command should be checked. Besides, rsp_buffer[] is 
> > got from user space by "NPREAL_NET_CMD_RESPONSE" in 
> > npreal_net_ioctl().
> 
> Again, what locking is really handling this?
> 

It's better to protect cmd_buffer[84] and rsp_buffer[84] by locking completely. They are safe because NPort driver should be worked with NPort daemon before, and NPort daemon is designed to be simple.

> > > Also, how does the data get sent to the hardware at all?  I see 
> > > cmd_buffer[] being written to, but what reads from it and how does 
> > > the hardware get the data?
> >
> > Actually we need to both NPort driver (this driver) and Npreal 
> > daemon
> > (userspace) to let HW work. Npreal daemon can communicate with HW by 
> > socket, and Npreal deamon communicates with Nport driver by 
> > "npreal_net_fops". When commands are ready for driver part, it will 
> > wake up poll event to let Nport daemon know.
> 
> That is not obvious at all, and needs to be really really really documented here.
> Why not put the userspace chunk in the tree too?  At the least, you 
> need to point at it.
> 
> And why is a userspace part needed?  We have tty-over-ethernet drivers 
> that do not require such a thing in the tree somewhere...
>

Because we need hardware serial to Ethernet converter (NPort device server) to manage some serial devices, so we still need to use MOXA Nport's commands and responses between host computer and converter. We will have an internal discussion about release of Nport daemon and related document, or using free tty to Ethernet daemon such as (ser2net/ socat/ remtty) and improved nport driver later. Thanks a lot!

Best regards,
Johnson

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

[Greg Kroah-Hartman]

On Wed, Jul 22, 2020 at 07:04:00AM +0000, Johnson CH Chen (陳昭勳) wrote:
> Hi Greg,
> 
> Thanks for your response!
> 
> > > > > +	unsigned long flag;
> > > > > +	unsigned char cmd_buffer[84];
> > > > > +	unsigned char rsp_buffer[84];
> > > >
> > > > You seem to have two "static" buffers here, for your device, that 
> > > > you semi-randomly write to all over the place, but I can't find 
> > > > any locking or coordination between things that prevents multiple 
> > > > commands from not just overwritting each other.
> > > >
> > > For cmd_buffer[], we use npreal_wait_and_set_command() to make sure 
> > > cmd_buffer[] is safe to be written by checking "cmd_buffer[0] == 0".
> > 
> > And what locks are protecting you there?
> > 
> > > For rsp_buffer[], we use npreal_wait_command_completed() to make 
> > > sure rsp_buffer[] is desired by checking rsp_buffer[0] and rsp_buffer[1].
> > > Command_set and command should be checked. Besides, rsp_buffer[] is 
> > > got from user space by "NPREAL_NET_CMD_RESPONSE" in 
> > > npreal_net_ioctl().
> > 
> > Again, what locking is really handling this?
> > 
> 
> It's better to protect cmd_buffer[84] and rsp_buffer[84] by locking completely. They are safe because NPort driver should be worked with NPort daemon before, and NPort daemon is designed to be simple.

I'm sorry, but I do not understand this answer at all.  Something can be
"simple" and still be totally wrong :)

Without locking, this code is broken.

thanks,

greg k-h

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

[Jiri Slaby]

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@moxa.com>
> Signed-off-by: Jason Chen <jason.chen@moxa.com>
> Signed-off-by: Danny Lin <danny.lin@moxa.com>
> Signed-off-by: Victor Yu <victor.yu@moxa.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@moxa.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.

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

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

[Johnson CH Chen (陳昭勳)]

Hi Jiri,

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

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

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

[Greg Kroah-Hartman]

On Fri, Aug 07, 2020 at 09:18:57AM +0000, Johnson CH Chen (陳昭勳) wrote:
> > > +	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?

There is nothing "proprietary" for Linux kernel code, sorry.  Please
document this properly so we can understand, review, and maintain it
over time correctly.

thanks,

greg k-h

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 13972 bytes --]

Hi "Johnson,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on v5.8-rc5 next-20200714]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Johnson-CH-Chen/tty-Add-MOXA-NPort-Real-TTY-Driver/20200714-142712
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: sparc64-randconfig-s032-20200715 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-49-g707c5017-dirty
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=sparc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/tty/npreal2.c:1107:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got struct serial_struct *retinfo @@
>> drivers/tty/npreal2.c:1107:26: sparse:     expected void [noderef] __user *to
>> drivers/tty/npreal2.c:1107:26: sparse:     got struct serial_struct *retinfo
>> drivers/tty/npreal2.c:1122:56: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got struct serial_struct *new_info @@
>> drivers/tty/npreal2.c:1122:56: sparse:     expected void const [noderef] __user *from
>> drivers/tty/npreal2.c:1122:56: sparse:     got struct serial_struct *new_info
>> drivers/tty/npreal2.c:1149:57: sparse: sparse: Using plain integer as NULL pointer
>> drivers/tty/npreal2.c:1186:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got unsigned int *value @@
>> drivers/tty/npreal2.c:1186:9: sparse:     expected void const volatile [noderef] __user *
>> drivers/tty/npreal2.c:1186:9: sparse:     got unsigned int *value
   drivers/tty/npreal2.c:1624:38: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:1897:34: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:1914:21: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:1984:46: sparse: sparse: Using plain integer as NULL pointer
>> drivers/tty/npreal2.c:2261:17: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got unsigned long * @@
   drivers/tty/npreal2.c:2261:17: sparse:     expected void const volatile [noderef] __user *
>> drivers/tty/npreal2.c:2261:17: sparse:     got unsigned long *
   drivers/tty/npreal2.c:2265:17: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got unsigned long * @@
   drivers/tty/npreal2.c:2265:17: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2265:17: sparse:     got unsigned long *
>> drivers/tty/npreal2.c:2319:21: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2319:21: sparse:     expected void const volatile [noderef] __user *
>> drivers/tty/npreal2.c:2319:21: sparse:     got int *
   drivers/tty/npreal2.c:2319:62: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2319:62: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2319:62: sparse:     got int *
   drivers/tty/npreal2.c:2320:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2320:25: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2320:25: sparse:     got int *
   drivers/tty/npreal2.c:2321:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2321:25: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2321:25: sparse:     got int *
   drivers/tty/npreal2.c:2322:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2322:25: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2322:25: sparse:     got int *
   drivers/tty/npreal2.c:2323:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2323:25: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2323:25: sparse:     got int *
   drivers/tty/npreal2.c:2324:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2324:25: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2324:25: sparse:     got int *
   drivers/tty/npreal2.c:2329:17: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2329:17: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2329:17: sparse:     got int *
   drivers/tty/npreal2.c:2330:17: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2330:17: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2330:17: sparse:     got int *
   drivers/tty/npreal2.c:2331:17: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2331:17: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2331:17: sparse:     got int *
   drivers/tty/npreal2.c:2332:17: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __user * @@     got int * @@
   drivers/tty/npreal2.c:2332:17: sparse:     expected void const volatile [noderef] __user *
   drivers/tty/npreal2.c:2332:17: sparse:     got int *
>> drivers/tty/npreal2.c:2570:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got void * @@
   drivers/tty/npreal2.c:2570:35: sparse:     expected void [noderef] __user *to
>> drivers/tty/npreal2.c:2570:35: sparse:     got void *
>> drivers/tty/npreal2.c:2591:57: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got void * @@
   drivers/tty/npreal2.c:2591:57: sparse:     expected void const [noderef] __user *from
   drivers/tty/npreal2.c:2591:57: sparse:     got void *
   drivers/tty/npreal2.c:2720:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got void * @@
   drivers/tty/npreal2.c:2720:35: sparse:     expected void [noderef] __user *to
   drivers/tty/npreal2.c:2720:35: sparse:     got void *
   drivers/tty/npreal2.c:2734:56: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got void * @@
   drivers/tty/npreal2.c:2734:56: sparse:     expected void const [noderef] __user *from
   drivers/tty/npreal2.c:2734:56: sparse:     got void *
>> drivers/tty/npreal2.c:2803:34: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got char *buf @@
   drivers/tty/npreal2.c:2803:34: sparse:     expected void [noderef] __user *to
>> drivers/tty/npreal2.c:2803:34: sparse:     got char *buf
>> drivers/tty/npreal2.c:2831:38: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got char * @@
   drivers/tty/npreal2.c:2831:38: sparse:     expected void [noderef] __user *to
>> drivers/tty/npreal2.c:2831:38: sparse:     got char *
>> drivers/tty/npreal2.c:2899:38: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got char const *buf @@
   drivers/tty/npreal2.c:2899:38: sparse:     expected void const [noderef] __user *from
>> drivers/tty/npreal2.c:2899:38: sparse:     got char const *buf
>> drivers/tty/npreal2.c:2950:22: sparse: sparse: incorrect type in initializer (incompatible argument 2 (different address spaces)) @@     expected long ( *proc_read )( ... ) @@     got long ( * )( ... ) @@
>> drivers/tty/npreal2.c:2950:22: sparse:     expected long ( *proc_read )( ... )
>> drivers/tty/npreal2.c:2950:22: sparse:     got long ( * )( ... )
>> drivers/tty/npreal2.c:2951:23: sparse: sparse: incorrect type in initializer (incompatible argument 2 (different address spaces)) @@     expected long ( *proc_write )( ... ) @@     got long ( * )( ... ) @@
>> drivers/tty/npreal2.c:2951:23: sparse:     expected long ( *proc_write )( ... )
   drivers/tty/npreal2.c:2951:23: sparse:     got long ( * )( ... )
>> drivers/tty/npreal2.c:2954:22: sparse: sparse: incorrect type in initializer (different base types) @@     expected restricted __poll_t ( *proc_poll )( ... ) @@     got unsigned int ( * )( ... ) @@
>> drivers/tty/npreal2.c:2954:22: sparse:     expected restricted __poll_t ( *proc_poll )( ... )
>> drivers/tty/npreal2.c:2954:22: sparse:     got unsigned int ( * )( ... )

vim +1107 drivers/tty/npreal2.c

  1089	
  1090	static int npreal_get_serial_info(struct npreal_struct *info, struct serial_struct *retinfo)
  1091	{
  1092		struct serial_struct tmp;
  1093	
  1094		if (!retinfo)
  1095			return -EFAULT;
  1096	
  1097		memset(&tmp, 0, sizeof(tmp));
  1098		tmp.type = info->type;
  1099		tmp.line = info->port;
  1100		tmp.flags = info->flags;
  1101		tmp.close_delay = info->close_delay;
  1102		tmp.closing_wait = info->closing_wait;
  1103		tmp.custom_divisor = info->custom_divisor;
  1104		tmp.baud_base = info->baud_base;
  1105		tmp.hub6 = 0;
  1106	
> 1107		if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
  1108			return -EFAULT;
  1109		else
  1110			return 0;
  1111	}
  1112	
  1113	static int npreal_set_serial_info(struct npreal_struct *info, struct serial_struct *new_info)
  1114	{
  1115		struct serial_struct new_serial;
  1116		int rsp_length = RSP_BUFFER_SIZE;
  1117		int retval = 0;
  1118		unsigned int flags;
  1119		char rsp_buffer[RSP_BUFFER_SIZE];
  1120	
  1121	
> 1122		if ((!new_info) || copy_from_user(&new_serial, new_info, sizeof(new_serial)))
  1123			return -EFAULT;
  1124	
  1125		flags = info->flags & ASYNC_SPD_MASK;
  1126	
  1127		if (!capable(CAP_SYS_ADMIN)) {
  1128			if ((new_serial.close_delay != info->close_delay) ||
  1129				((new_serial.flags & ~ASYNC_USR_MASK) != (info->flags & ~ASYNC_USR_MASK)))
  1130				return -EPERM;
  1131	
  1132			info->flags = ((info->flags & ~ASYNC_USR_MASK) |
  1133					(new_serial.flags & ASYNC_USR_MASK));
  1134		} else {
  1135			info->flags = ((info->flags & ~ASYNC_FLAGS) | (new_serial.flags & ASYNC_FLAGS));
  1136			info->close_delay = new_serial.close_delay * HZ / 100;
  1137	
  1138			if (new_serial.closing_wait == ASYNC_CLOSING_WAIT_NONE)
  1139				info->closing_wait = ASYNC_CLOSING_WAIT_NONE;
  1140			else
  1141				info->closing_wait = new_serial.closing_wait * HZ / 100;
  1142		}
  1143	
  1144		info->type = new_serial.type;
  1145		set_common_xmit_fifo_size(info, info->net_node);
  1146	
  1147		if (info->flags & ASYNC_INITIALIZED) {
  1148			if (flags != (info->flags & ASYNC_SPD_MASK))
> 1149				retval = npreal_port_init(info, 0);
  1150	
  1151			if (info->net_node)
  1152				npreal_set_tx_fifo_command_done(info, info->net_node, rsp_buffer,
  1153								&rsp_length);
  1154	
  1155		}
  1156	
  1157		info->custom_divisor = new_serial.custom_divisor;
  1158	
  1159		if (info->custom_divisor == 0)
  1160			info->baud_base = 921600L;
  1161		else
  1162			info->baud_base = new_serial.baud_base;
  1163	
  1164		return retval;
  1165	}
  1166	
  1167	/**
  1168	 * npreal_get_lsr_info() - get line status register info
  1169	 *
  1170	 * Let user call ioctl() to get info when the UART physically is emptied.
  1171	 * On bus types like RS485, the transmitter must release the bus after
  1172	 * transmitting. This must be done when the transmit shift register is
  1173	 * empty, not be done when the transmit holding register is empty.
  1174	 * This functionality allows an RS485 driver to be written in user space.
  1175	 *
  1176	 * Always return 0 when function is ended.
  1177	 */
  1178	static int npreal_get_lsr_info(struct npreal_struct *info,
  1179					unsigned int *value)
  1180	{
  1181		unsigned int result = 0;
  1182	
  1183		if (npreal_wait_oqueue(info, 0) == 0)
  1184			result = TIOCSER_TEMT;
  1185	
> 1186		put_user(result, value);
  1187	
  1188		return 0;
  1189	}
  1190	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29498 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 15045 bytes --]

Hi "Johnson,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on v5.8-rc6 next-20200723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Johnson-CH-Chen/tty-Add-MOXA-NPort-Real-TTY-Driver/20200714-142712
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: mips-randconfig-s032-20200723 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-93-g4c6cbe55-dirty
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   drivers/tty/npreal2.c:1107:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got struct serial_struct *retinfo @@
   drivers/tty/npreal2.c:1107:26: sparse:     expected void [noderef] __user *to
   drivers/tty/npreal2.c:1107:26: sparse:     got struct serial_struct *retinfo
   drivers/tty/npreal2.c:1122:56: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got struct serial_struct *new_info @@
   drivers/tty/npreal2.c:1122:56: sparse:     expected void const [noderef] __user *from
   drivers/tty/npreal2.c:1122:56: sparse:     got struct serial_struct *new_info
   drivers/tty/npreal2.c:1149:57: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:1186:9: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned int [noderef] __user *__pu_addr @@     got unsigned int *value @@
   drivers/tty/npreal2.c:1186:9: sparse:     expected unsigned int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:1186:9: sparse:     got unsigned int *value
   drivers/tty/npreal2.c:1624:38: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:1897:34: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:1914:21: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:1984:46: sparse: sparse: Using plain integer as NULL pointer
   drivers/tty/npreal2.c:2261:17: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned long [noderef] __user *__pu_addr @@     got unsigned long * @@
   drivers/tty/npreal2.c:2261:17: sparse:     expected unsigned long [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2261:17: sparse:     got unsigned long *
>> drivers/tty/npreal2.c:2265:17: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned long const [noderef] __user *__gu_ptr @@     got unsigned long * @@
>> drivers/tty/npreal2.c:2265:17: sparse:     expected unsigned long const [noderef] __user *__gu_ptr
   drivers/tty/npreal2.c:2265:17: sparse:     got unsigned long *
   drivers/tty/npreal2.c:2319:21: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2319:21: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2319:21: sparse:     got int *
   drivers/tty/npreal2.c:2319:62: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2319:62: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2319:62: sparse:     got int *
   drivers/tty/npreal2.c:2320:25: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2320:25: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2320:25: sparse:     got int *
   drivers/tty/npreal2.c:2321:25: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2321:25: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2321:25: sparse:     got int *
   drivers/tty/npreal2.c:2322:25: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2322:25: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2322:25: sparse:     got int *
   drivers/tty/npreal2.c:2323:25: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2323:25: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2323:25: sparse:     got int *
   drivers/tty/npreal2.c:2324:25: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2324:25: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2324:25: sparse:     got int *
   drivers/tty/npreal2.c:2329:17: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2329:17: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2329:17: sparse:     got int *
   drivers/tty/npreal2.c:2330:17: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2330:17: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2330:17: sparse:     got int *
   drivers/tty/npreal2.c:2331:17: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2331:17: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2331:17: sparse:     got int *
   drivers/tty/npreal2.c:2332:17: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected int [noderef] __user *__pu_addr @@     got int * @@
   drivers/tty/npreal2.c:2332:17: sparse:     expected int [noderef] __user *__pu_addr
   drivers/tty/npreal2.c:2332:17: sparse:     got int *
   drivers/tty/npreal2.c:2570:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got void * @@
   drivers/tty/npreal2.c:2570:35: sparse:     expected void [noderef] __user *to
   drivers/tty/npreal2.c:2570:35: sparse:     got void *
   drivers/tty/npreal2.c:2591:57: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got void * @@
   drivers/tty/npreal2.c:2591:57: sparse:     expected void const [noderef] __user *from
   drivers/tty/npreal2.c:2591:57: sparse:     got void *
   drivers/tty/npreal2.c:2720:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got void * @@
   drivers/tty/npreal2.c:2720:35: sparse:     expected void [noderef] __user *to
   drivers/tty/npreal2.c:2720:35: sparse:     got void *
   drivers/tty/npreal2.c:2734:56: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got void * @@
   drivers/tty/npreal2.c:2734:56: sparse:     expected void const [noderef] __user *from
   drivers/tty/npreal2.c:2734:56: sparse:     got void *
   drivers/tty/npreal2.c:2803:34: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got char *buf @@
   drivers/tty/npreal2.c:2803:34: sparse:     expected void [noderef] __user *to
   drivers/tty/npreal2.c:2803:34: sparse:     got char *buf
   drivers/tty/npreal2.c:2831:38: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __user *to @@     got char * @@
   drivers/tty/npreal2.c:2831:38: sparse:     expected void [noderef] __user *to
   drivers/tty/npreal2.c:2831:38: sparse:     got char *
   drivers/tty/npreal2.c:2899:38: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got char const *buf @@
   drivers/tty/npreal2.c:2899:38: sparse:     expected void const [noderef] __user *from
   drivers/tty/npreal2.c:2899:38: sparse:     got char const *buf
>> drivers/tty/npreal2.c:2950:22: sparse: sparse: incorrect type in initializer (incompatible argument 2 (different address spaces)) @@     expected int ( *proc_read )( ... ) @@     got int ( * )( ... ) @@
>> drivers/tty/npreal2.c:2950:22: sparse:     expected int ( *proc_read )( ... )
>> drivers/tty/npreal2.c:2950:22: sparse:     got int ( * )( ... )
>> drivers/tty/npreal2.c:2951:23: sparse: sparse: incorrect type in initializer (incompatible argument 2 (different address spaces)) @@     expected int ( *proc_write )( ... ) @@     got int ( * )( ... ) @@
>> drivers/tty/npreal2.c:2951:23: sparse:     expected int ( *proc_write )( ... )
   drivers/tty/npreal2.c:2951:23: sparse:     got int ( * )( ... )
   drivers/tty/npreal2.c:2954:22: sparse: sparse: incorrect type in initializer (different base types) @@     expected restricted __poll_t ( *proc_poll )( ... ) @@     got unsigned int ( * )( ... ) @@
   drivers/tty/npreal2.c:2954:22: sparse:     expected restricted __poll_t ( *proc_poll )( ... )
   drivers/tty/npreal2.c:2954:22: sparse:     got unsigned int ( * )( ... )

vim +2265 drivers/tty/npreal2.c

  2201	
  2202	static int npreal_ioctl(struct tty_struct *tty, unsigned int cmd,
  2203				unsigned long arg)
  2204	{
  2205		struct npreal_struct *info = (struct npreal_struct *)tty->driver_data;
  2206		struct serial_icounter_struct *p_cuser; /* user space */
  2207		unsigned long templ;
  2208		int ret = 0;
  2209	
  2210		if (!info)
  2211			return -ENODEV;
  2212	
  2213		if ((cmd != TIOCGSERIAL) && (cmd != TIOCMIWAIT) && (cmd != TIOCGICOUNT) &&
  2214			test_bit(TTY_IO_ERROR, &tty->flags))
  2215			return -EIO;
  2216	
  2217		switch (cmd) {
  2218		case TCFLSH:
  2219			ret = tty_check_change(tty);
  2220			if (!ret) {
  2221				switch (arg) {
  2222				case TCIFLUSH:
  2223					if (tty->ldisc->ops->flush_buffer)
  2224						tty->ldisc->ops->flush_buffer(tty);
  2225					break;
  2226	
  2227				case TCIOFLUSH:
  2228					if (tty->ldisc->ops->flush_buffer)
  2229						tty->ldisc->ops->flush_buffer(tty);
  2230					npreal_flush_buffer(tty);
  2231					break;
  2232	
  2233				case TCOFLUSH:
  2234					npreal_flush_buffer(tty);
  2235					break;
  2236	
  2237				default:
  2238					ret = -EINVAL;
  2239				}
  2240			}
  2241			break;
  2242	
  2243		case TCSBRK: /* SVID version: non-zero arg --> no break */
  2244			ret = tty_check_change(tty);
  2245			if (!ret) {
  2246				tty_wait_until_sent(tty, 0);
  2247				if (!arg)
  2248					npreal_send_break(info, HZ / 4);
  2249			}
  2250			break;
  2251	
  2252		case TCSBRKP: /* support for POSIX tcsendbreak() */
  2253			ret = tty_check_change(tty);
  2254			if (!ret) {
  2255				tty_wait_until_sent(tty, 0);
  2256				npreal_send_break(info, arg ? arg * (HZ / 10) : HZ / 4);
  2257			}
  2258			break;
  2259	
  2260		case TIOCGSOFTCAR:
  2261			put_user(C_CLOCAL(tty) ? 1 : 0, (unsigned long *)arg);
  2262			break;
  2263	
  2264		case TIOCSSOFTCAR:
> 2265			get_user(templ, (unsigned long *)arg);
  2266			tty->termios.c_cflag = ((tty->termios.c_cflag & ~CLOCAL) | (arg ? CLOCAL : 0));
  2267			break;
  2268	
  2269		case TIOCGSERIAL:
  2270			ret = (npreal_get_serial_info(info, (struct serial_struct *)arg));
  2271			break;
  2272	
  2273		case TIOCSSERIAL:
  2274			ret = (npreal_set_serial_info(info, (struct serial_struct *)arg));
  2275			break;
  2276	
  2277		case TIOCSERGETLSR: /* Get line status register */
  2278			ret = (npreal_get_lsr_info(info, (unsigned int *)arg));
  2279			break;
  2280	
  2281		case TIOCMIWAIT: {
  2282			struct async_icount cprev;
  2283			DECLARE_WAITQUEUE(wait, current);
  2284	
  2285			cprev = info->icount;
  2286			add_wait_queue(&info->delta_msr_wait, &wait);
  2287			while (1) {
  2288				struct async_icount cnow;
  2289	
  2290				cnow = info->icount;
  2291				if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
  2292					((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
  2293					((arg & TIOCM_CD) && (cnow.dcd != cprev.dcd)) ||
  2294					((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
  2295					ret = 0;
  2296					break;
  2297				}
  2298	
  2299				if (signal_pending(current)) {
  2300					ret = -ERESTARTSYS;
  2301					break;
  2302				}
  2303	
  2304				cprev = cnow;
  2305				current->state = TASK_INTERRUPTIBLE;
  2306				schedule();
  2307			}
  2308	
  2309			remove_wait_queue(&info->delta_msr_wait, &wait);
  2310			break;
  2311		}
  2312	
  2313		case TIOCGICOUNT:{
  2314			struct async_icount cnow;
  2315	
  2316			cnow = info->icount;
  2317			p_cuser = (struct serial_icounter_struct *)arg;
  2318	
  2319			if (put_user(cnow.frame, &p_cuser->frame) || put_user(cnow.brk, &p_cuser->brk) ||
  2320				put_user(cnow.overrun, &p_cuser->overrun) ||
  2321				put_user(cnow.buf_overrun, &p_cuser->buf_overrun) ||
  2322				put_user(cnow.parity, &p_cuser->parity) ||
  2323				put_user(cnow.rx, &p_cuser->rx) ||
  2324				put_user(cnow.tx, &p_cuser->tx)) {
  2325				ret = -EFAULT;
  2326				break;
  2327			}
  2328	
  2329			put_user(cnow.cts, &p_cuser->cts);
  2330			put_user(cnow.dsr, &p_cuser->dsr);
  2331			put_user(cnow.rng, &p_cuser->rng);
  2332			put_user(cnow.dcd, &p_cuser->dcd);
  2333			break;
  2334		}
  2335		case TCXONC:
  2336			ret = tty_check_change(tty);
  2337			if (!ret) {
  2338				switch (arg) {
  2339				case TCOOFF:
  2340					ret = npreal_set_generic_command_done(info, ASPP_CMD_SETXOFF);
  2341					break;
  2342	
  2343				case TCOON:
  2344					ret = npreal_set_generic_command_done(info, ASPP_CMD_SETXON);
  2345					break;
  2346	
  2347				default:
  2348					ret = -EINVAL;
  2349				}
  2350			}
  2351			break;
  2352	
  2353		default:
  2354			ret = -ENOIOCTLCMD;
  2355		}
  2356		return ret;
  2357	}
  2358	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24970 bytes --]

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

[Pavel Machek]

Hi!

> +	  This driver can also be built as a module. The module will be called
> +	  npreal2 by setting M.

Odd wording...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html