* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
[not found] <HK2PR01MB328134FB2EF5F9D1E381BDA3FA610@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
@ 2020-07-14 7:11 ` Greg Kroah-Hartman
2020-07-23 10:54 ` Johnson CH Chen (陳昭勳)
2020-07-14 7:36 ` Greg Kroah-Hartman
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-14 7:11 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳)
Cc: Jiri Slaby, linux-kernel, linux-serial
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
[not found] <HK2PR01MB328134FB2EF5F9D1E381BDA3FA610@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
2020-07-14 7:11 ` [PATCH] tty: Add MOXA NPort Real TTY Driver Greg Kroah-Hartman
@ 2020-07-14 7:36 ` Greg Kroah-Hartman
2020-07-16 7:19 ` Johnson CH Chen (陳昭勳)
2020-07-14 9:34 ` Jiri Slaby
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-14 7:36 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳)
Cc: Jiri Slaby, linux-kernel, linux-serial
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
[not found] <HK2PR01MB328134FB2EF5F9D1E381BDA3FA610@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
2020-07-14 7:11 ` [PATCH] tty: Add MOXA NPort Real TTY Driver Greg Kroah-Hartman
2020-07-14 7:36 ` Greg Kroah-Hartman
@ 2020-07-14 9:34 ` Jiri Slaby
2020-08-07 9:18 ` Johnson CH Chen (陳昭勳)
2020-07-15 8:27 ` kernel test robot
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Jiri Slaby @ 2020-07-14 9:34 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳), Greg Kroah-Hartman
Cc: linux-kernel, linux-serial
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
[not found] <HK2PR01MB328134FB2EF5F9D1E381BDA3FA610@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
` (2 preceding siblings ...)
2020-07-14 9:34 ` Jiri Slaby
@ 2020-07-15 8:27 ` kernel test robot
2020-07-24 5:32 ` kernel test robot
2020-07-28 20:40 ` Pavel Machek
5 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2020-07-15 8:27 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳),
Jiri Slaby, Greg Kroah-Hartman
Cc: kbuild-all, linux-kernel, linux-serial
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
2020-07-14 7:36 ` Greg Kroah-Hartman
@ 2020-07-16 7:19 ` Johnson CH Chen (陳昭勳)
2020-07-16 7:23 ` Greg Kroah-Hartman
0 siblings, 1 reply; 13+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-07-16 7:19 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-kernel, linux-serial
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
2020-07-16 7:19 ` Johnson CH Chen (陳昭勳)
@ 2020-07-16 7:23 ` Greg Kroah-Hartman
2020-07-22 7:04 ` Johnson CH Chen (陳昭勳)
0 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-16 7:23 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳)
Cc: Jiri Slaby, linux-kernel, linux-serial
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
2020-07-16 7:23 ` Greg Kroah-Hartman
@ 2020-07-22 7:04 ` Johnson CH Chen (陳昭勳)
2020-07-22 7:11 ` Greg Kroah-Hartman
0 siblings, 1 reply; 13+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-07-22 7:04 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-kernel, linux-serial,
Victor Yu (游勝義),
Danny Lin (林政易)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
2020-07-22 7:04 ` Johnson CH Chen (陳昭勳)
@ 2020-07-22 7:11 ` Greg Kroah-Hartman
0 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-22 7:11 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳)
Cc: Jiri Slaby, linux-kernel, linux-serial,
Victor Yu (游勝義),
Danny Lin (林政易)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
2020-07-14 7:11 ` [PATCH] tty: Add MOXA NPort Real TTY Driver Greg Kroah-Hartman
@ 2020-07-23 10:54 ` Johnson CH Chen (陳昭勳)
0 siblings, 0 replies; 13+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-07-23 10:54 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-kernel, linux-serial,
Victor Yu (游勝義),
Danny Lin (林政易)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
[not found] <HK2PR01MB328134FB2EF5F9D1E381BDA3FA610@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
` (3 preceding siblings ...)
2020-07-15 8:27 ` kernel test robot
@ 2020-07-24 5:32 ` kernel test robot
2020-07-28 20:40 ` Pavel Machek
5 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2020-07-24 5:32 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳),
Jiri Slaby, Greg Kroah-Hartman
Cc: kbuild-all, linux-kernel, linux-serial
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
[not found] <HK2PR01MB328134FB2EF5F9D1E381BDA3FA610@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
` (4 preceding siblings ...)
2020-07-24 5:32 ` kernel test robot
@ 2020-07-28 20:40 ` Pavel Machek
5 siblings, 0 replies; 13+ messages in thread
From: Pavel Machek @ 2020-07-28 20:40 UTC (permalink / raw)
To: Johnson CH Chen (???L??)
Cc: Jiri Slaby, Greg Kroah-Hartman, linux-kernel, linux-serial
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] tty: Add MOXA NPort Real TTY Driver
2020-07-14 9:34 ` Jiri Slaby
@ 2020-08-07 9:18 ` Johnson CH Chen (陳昭勳)
2020-08-07 9:48 ` Greg Kroah-Hartman
0 siblings, 1 reply; 13+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-08-07 9:18 UTC (permalink / raw)
To: Jiri Slaby, Greg Kroah-Hartman
Cc: linux-kernel, linux-serial, Jason Chen (陳兆先),
Victor Yu (游勝義),
Danny Lin (林政易)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tty: Add MOXA NPort Real TTY Driver
2020-08-07 9:18 ` Johnson CH Chen (陳昭勳)
@ 2020-08-07 9:48 ` Greg Kroah-Hartman
0 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-08-07 9:48 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳)
Cc: Jiri Slaby, linux-kernel, linux-serial,
Jason Chen (陳兆先),
Victor Yu (游勝義),
Danny Lin (林政易)
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-08-07 9:48 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <HK2PR01MB328134FB2EF5F9D1E381BDA3FA610@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
2020-07-14 7:11 ` [PATCH] tty: Add MOXA NPort Real TTY Driver Greg Kroah-Hartman
2020-07-23 10:54 ` Johnson CH Chen (陳昭勳)
2020-07-14 7:36 ` Greg Kroah-Hartman
2020-07-16 7:19 ` Johnson CH Chen (陳昭勳)
2020-07-16 7:23 ` Greg Kroah-Hartman
2020-07-22 7:04 ` Johnson CH Chen (陳昭勳)
2020-07-22 7:11 ` Greg Kroah-Hartman
2020-07-14 9:34 ` Jiri Slaby
2020-08-07 9:18 ` Johnson CH Chen (陳昭勳)
2020-08-07 9:48 ` Greg Kroah-Hartman
2020-07-15 8:27 ` kernel test robot
2020-07-24 5:32 ` kernel test robot
2020-07-28 20:40 ` Pavel Machek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).