Message-ID: <20150919184743.GA2360@localhost>
Date:	Sat, 19 Sep 2015 11:47:43 -0700
From:	Johan Hovold <johan@...nel.org>
To:	Mathieu OTHACEHE <m.othacehe@...il.com>
Cc:	johan@...nel.org, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH] USB: serial: add Moxa UPORT 11x0 driver

On Fri, Sep 04, 2015 at 07:23:29AM +0200, Mathieu OTHACEHE wrote:
> Add a driver which supports :
> 
> - UPort 1110  : 1 port RS-232 USB to Serial Hub.
> - UPort 1130  : 1 port RS-422/485 USB to Serial Hub.
> - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
> - UPort 1150  : 1 port RS-232/422/485 USB to Serial Hub.
> - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
> 
> This driver is based on GPL MOXA driver written by Hen Huang and available
> on MOXA website. The original driver was based on io_ti serial driver.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@...il.com>

Thanks for submitting this driver. Code looks clean, but you should be
able to reuse a whole lot more of the generic usb-serial implementation,
something which should also give better throughput.

Specifically, most of the read and write code can simply be dropped in
favour of the generic implementation.

You must also get rid of the Moxa specific ioctl to set the port mode.
Have a look at the TIOCSRS485 ioctl as a basis for this change (but note
that we may need to extend that interface).

> ---
>  drivers/usb/serial/Kconfig   |   16 +
>  drivers/usb/serial/Makefile  |    1 +
>  drivers/usb/serial/mxu11x0.c | 1500 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/serial/mxu11x0.h |  199 ++++++

Just merge the header file with the implementation as it is not used
outside of it.

> +static int closing_wait = MXU1_DEFAULT_CLOSING_WAIT;

This should be configurable per device using TIOCSSERIAL, just drop the
module parameter.

> +static int mxu1_download_firmware(struct usb_serial *serial,
> +				  const struct firmware *fw_p)
> +{
> +	int status = 0;
> +	int buffer_size;
> +	int pos;
> +	int len;
> +	int done;
> +	__u8 cs = 0;
> +	__u8 *buffer;

Only use __u8 when implementing a user-space API (use u8).

> +	struct usb_device *dev = serial->dev;
> +	struct mxu1_firmware_header *header;
> +	unsigned int pipe = usb_sndbulkpipe(dev, serial->port[0]->
> +					    bulk_out_endpointAddress);
> +
> +	buffer_size = fw_p->size +
> +		sizeof(struct mxu1_firmware_header);
> +	buffer = kmalloc(buffer_size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	memcpy(buffer, fw_p->data, fw_p->size);
> +	memset(buffer + fw_p->size, 0xff, buffer_size - fw_p->size);
> +
> +	for (pos = sizeof(struct mxu1_firmware_header);
> +	     pos < buffer_size; pos++)
> +		cs = (__u8)(cs + buffer[pos]);
> +
> +	header = (struct mxu1_firmware_header *)buffer;
> +	header->wLength = cpu_to_le16(
> +		(__u16)(buffer_size - sizeof(struct mxu1_firmware_header)));
> +	header->bCheckSum = cs;
> +
> +	pr_debug("%s - downloading firmware", __func__);

Drop all pr_debug (and friends) in favour of the more descriptive
dev_dbg.

> +static int mxu1_startup(struct usb_serial *serial)
> +{
> +	struct mxu1_device *mxdev;
> +	struct usb_device *dev = serial->dev;
> +	char fw_name[32];
> +	const struct firmware *fw_p = NULL;
> +	u16 product_id;
> +	int err;
> +
> +	pr_debug("%s - product 0x%4X, num configurations %d, configuration value %d",
> +		 __func__, le16_to_cpu(dev->descriptor.idProduct),
> +		 dev->descriptor.bNumConfigurations,
> +		 dev->actconfig->desc.bConfigurationValue);
> +
> +	/* create device structure */
> +	mxdev = kzalloc(sizeof(struct mxu1_device), GFP_KERNEL);
> +	if (mxdev == NULL)
> +		return -ENOMEM;
> +
> +	mutex_init(&mxdev->mxd_lock);
> +	mxdev->mxd_serial = serial;
> +	usb_set_serial_data(serial, mxdev);
> +
> +	/* determine device type */
> +	if (!usb_match_id(serial->interface, mxuport11_idtable))
> +		return 0;

This would already have been taken care of by usb-serial core.

> +
> +	product_id = dev->descriptor.idProduct;

Missing le16_to_cpu().

> +static int mxu1_set_mcr(struct mxu1_port *mxport, unsigned int mcr)
> +{
> +	int status = 0;
> +
> +	status = mxu1_write_byte(mxport->mxp_mxdev,
> +				 mxport->mxp_uart_base_addr +
> +				 MXU1_UART_OFFSET_MCR,
> +				 MXU1_MCR_RTS | MXU1_MCR_DTR | MXU1_MCR_LOOP,
> +				 mcr);
> +
> +	if (!status)
> +		mxport->mxp_shadow_mcr = mcr;

No locking?

> +static int mxu1_set_serial_info(struct mxu1_port *mxport,
> +				struct serial_struct __user *new_arg)
> +{
> +	struct serial_struct new_serial;
> +
> +	if (copy_from_user(&new_serial, new_arg, sizeof(new_serial)))
> +		return -EFAULT;
> +
> +	mxport->mxp_flags = new_serial.flags & MXU1_SET_SERIAL_FLAGS;
> +
> +	if (mxport->mxp_mxdev->mxd_model_name != MXU1_MODEL_1110) {
> +		/* UPort 1130, 1150, 1150I */
> +		switch (new_serial.port) {

Hmm, this is not how to switch the port mode, use the ioctl mentioned
above.

> +		case MXU1_RS232: /* UPort 1150, 1150I */
> +			if (mxport->mxp_mxdev->mxd_model_name ==
> +			    MXU1_MODEL_1130 ||
> +			    mxport->mxp_mxdev->mxd_model_name ==
> +			    MXU1_MODEL_1131) {
> +				return -EINVAL;
> +			}

Instead of sprinkling the driver with model conditionals, consider
abstracting this into device type-flags that you can test for instead
(e.g. MXU1_TYPE_RS232_ONLY).

> +static int mxu1_ioctl(struct tty_struct *tty,
> +		      unsigned int cmd, unsigned long arg)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	struct async_icount cnow;
> +	struct async_icount cprev;
> +
> +	pr_debug("%s - port %d, cmd = 0x%04X",
> +		 __func__, port->port_number, cmd);
> +
> +	if (mxport == NULL)
> +		return -ENODEV;
> +
> +	switch (cmd) {
> +	case TIOCGSERIAL:
> +		pr_debug("%s - (%d) TIOCGSERIAL", __func__, port->port_number);
> +		return mxu1_get_serial_info(mxport,
> +					    (struct serial_struct __user *)arg);
> +
> +	case TIOCSSERIAL:
> +		pr_debug("%s - (%d) TIOCSSERIAL", __func__, port->port_number);
> +		return mxu1_set_serial_info(mxport,
> +					    (struct serial_struct __user *)arg);
> +
> +	case TIOCMIWAIT:
> +		pr_debug("%s - (%d) TIOCMIWAIT", __func__, port->port_number);
> +		cprev = mxport->mxp_icount;
> +		mxport->mxp_msr_wait_flags = 1;
> +		while (1) {
> +			wait_event_interruptible_timeout
> +				(mxport->mxp_msr_wait,
> +				 (mxport->mxp_msr_wait_flags == 0),
> +				 MXU1_MSR_WAIT_TIMEOUT);
> +
> +			if (signal_pending(current))
> +				return -ERESTARTSYS;
> +			cnow = mxport->mxp_icount;
> +			if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
> +			    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
> +				return -EIO; /* no change => error */
> +			if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
> +			    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
> +			    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
> +			    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
> +				return 0;
> +			}
> +			cprev = cnow;
> +		}
> +		break;

Use the generic implementation of this.

> +
> +	case TIOCGICOUNT:
> +		pr_debug("%s - (%d) TIOCGICOUNT RX=%d, TX=%d",
> +			 __func__,
> +			 port->port_number, mxport->mxp_icount.rx,
> +			 mxport->mxp_icount.tx);
> +
> +		if (copy_to_user((void __user *)arg,
> +				 &mxport->mxp_icount,
> +				 sizeof(mxport->mxp_icount)))
> +			return -EFAULT;
> +		return 0;

And this.

> +
> +	case MOXA_SET_INTERFACE:
> +		pr_debug("%s - port%d MOXA_SET_INTERFACE=%d", __func__,
> +			 port->port_number, (int)arg);
> +
> +		if (mxport->mxp_mxdev->mxd_model_name != MXU1_MODEL_1110) {
> +			/* UPort 1130, 1150, 1150I */
> +			switch (arg) {
> +			case MXU1_RS232: /* UPort 1150, 1150I */
> +				if (mxport->mxp_mxdev->mxd_model_name
> +				    == MXU1_MODEL_1130 ||
> +				    mxport->mxp_mxdev->mxd_model_name
> +				    == MXU1_MODEL_1131) {
> +					return -EINVAL;
> +				}
> +				mxport->mxp_uart_mode = MXU1_UART_232;
> +				break;
> +			case MXU1_RS422: /* UPort 1130, 1150, 1150I */
> +			case MXU1_RS4854W: /* UPort 1130, 1150, 1150I */
> +				mxport->mxp_uart_mode =
> +					MXU1_UART_485_RECEIVER_ENABLED;
> +				break;
> +
> +			case MXU1_RS4852W: /* UPort 1130, 1150, 1150I */
> +				mxport->mxp_uart_mode =
> +					MXU1_UART_485_RECEIVER_DISABLED;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		} else { /* UPort 1110 */
> +			if (arg != MXU1_RS232)
> +				return -EINVAL;
> +		}

And drop this one in favour of the standard interface.

> +static int mxu1_tiocmget(struct tty_struct *tty)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	unsigned int result;
> +	unsigned int msr;
> +	unsigned int mcr;
> +
> +	pr_debug("%s - port %d", __func__, port->port_number);
> +
> +	if (mxport == NULL)
> +		return -ENODEV;
> +
> +	msr = mxport->mxp_msr;
> +	mcr = mxport->mxp_shadow_mcr;

Again, no locking.

> +static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	struct mxu1_device *mxdev;
> +	struct usb_device *dev;
> +	struct urb *urb;
> +	int port_number;
> +	int status;
> +	__u16 open_settings = (__u8)(MXU1_PIPE_MODE_CONTINUOUS |
> +				     MXU1_PIPE_TIMEOUT_ENABLE |
> +				     (MXU1_TRANSFER_TIMEOUT << 2));
> +	if (!mxport)
> +		return -ENODEV;
> +
> +	dev = port->serial->dev;
> +	mxdev = mxport->mxp_mxdev;
> +
> +	if (port->port.tty)
> +		port->port.low_latency = MXU1_DEFAULT_LOW_LATENCY;

This flag should not be set.

> +
> +	port_number = port->port_number - port->minor;
> +
> +	mxport->mxp_msr = 0;
> +	mxport->mxp_shadow_mcr |= (MXU1_MCR_RTS | MXU1_MCR_DTR);
> +
> +	if (mutex_lock_interruptible(&mxdev->mxd_lock))
> +		return -ERESTARTSYS;
> +
> +	/* start interrupt urb the first time a port is opened on this device */
> +	if (mxdev->mxd_open_port_count == 0) {
> +		pr_debug("%s - start interrupt in urb", __func__);
> +		urb = mxdev->mxd_serial->port[0]->interrupt_in_urb;
> +		if (!urb) {
> +			dev_err(&port->dev,
> +				"%s - no interrupt urb\n",
> +				__func__);
> +			status = -EINVAL;
> +			goto release_mxd_lock;
> +		}
> +		urb->context = mxdev;
> +		status = usb_submit_urb(urb, GFP_KERNEL);
> +		if (status) {
> +			dev_err(&port->dev,
> +				"%s - submit interrupt urb failed, %d\n",
> +				__func__,
> +				status);
> +			goto release_mxd_lock;
> +		}
> +	}

This does not seem to make sense, since this is a single port device.
Just submit the interrupt urb unconditionally at open.

> +module_param(closing_wait, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(closing_wait, "Maximum wait for data to drain, in .01 secs");

And as mentioned above, just drop this.

> diff --git a/drivers/usb/serial/mxu11x0.h b/drivers/usb/serial/mxu11x0.h
> new file mode 100644
> index 0000000..6c7ac10
> --- /dev/null
> +++ b/drivers/usb/serial/mxu11x0.h
> +/* Config struct */

> +struct mxu1_uart_config {
> +	__u16	wBaudRate;
> +	__u16	wFlags;

Use __be16 (or __le16).

> +	__u8	bDataBits;
> +	__u8	bParity;
> +	__u8	bStopBits;
> +	char	cXon;
> +	char	cXoff;
> +	__u8	bUartMode;
> +} __packed;

> +struct mxu1_write_data_bytes {
> +	__u8	bAddrType;
> +	__u8	bDataType;
> +	__u8	bDataCounter;
> +#ifdef __CHECK_ENDIAN__
> +	__be16	wBaseAddrHi;
> +	__be16	wBaseAddrLo;
> +#else
> +	__u16	wBaseAddrHi;
> +	__u16	wBaseAddrLo;
> +#endif

Drop the ifdef.

> +	__u8	bData[0];
> +} __packed;
> +
> +/* Interrupt codes */
> +#define MXU1_GET_PORT_FROM_CODE(c)		(((c) >> 4) - 3)
> +#define MXU1_GET_FUNC_FROM_CODE(c)		((c) & 0x0f)
> +#define MXU1_CODE_HARDWARE_ERROR		0xFF
> +#define MXU1_CODE_DATA_ERROR			0x03
> +#define MXU1_CODE_MODEM_STATUS			0x04
> +
> +/* Download firmware max packet size */
> +#define MXU1_DOWNLOAD_MAX_PACKET_SIZE		64
> +
> +/* Firmware image header */
> +struct mxu1_firmware_header {
> +#ifdef __CHECK_
> +	__le16 wLength;
> +#else
> +	__u16 wLength;
> +#endif

Ditto.

> +	__u8 bCheckSum;
> +} __packed;

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists