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@xxxxxxxxxxxxxxx
> > <linux-serial-owner@xxxxxxxxxxxxxxx> On Behalf Of Greg Kroah-Hartman
> > Sent: Tuesday, July 14, 2020 3:36 PM
> > To: Johnson CH Chen (陳昭勳) <JohnsonCH.Chen@xxxxxxxx>
> > Cc: Jiri Slaby <jirislaby@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > linux-serial@xxxxxxxxxxxxxxx
> > 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@xxxxxxxx>
> > > Signed-off-by: Jason Chen <jason.chen@xxxxxxxx>
> > > Signed-off-by: Danny Lin <danny.lin@xxxxxxxx>
> > > Signed-off-by: Victor Yu <victor.yu@xxxxxxxx>
> > > ---
> > > 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@xxxxxxxx)
> > > + *
> > > + * 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@xxxxxxxx>");
> > > +MODULE_DESCRIPTION("MOXA Async/NPort Server Family Real TTY
> > Driver");
> > > +module_param(ttymajor, int, 0); module_param(verbose, int, 0644);
> > > +MODULE_VERSION(NPREAL_VERSION); MODULE_LICENSE("GPL");
> > > +
> > > +struct server_setting_struct {
> > > + int32_t server_type;
> > > + int32_t disable_fifo;
> > > +};
> > > +
> > > +struct npreal_struct {
> > > + struct tty_port ttyPort;
> > > + struct work_struct tqueue;
> > > + struct work_struct process_flip_tqueue;
> > > + struct ktermios normal_termios;
> > > + struct ktermios callout_termios;
> > > + /* 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
