Date:	Sat, 5 Dec 2015 16:42:35 +0100
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 v4] USB: serial: add Moxa UPORT 11x0 driver

On Wed, Nov 11, 2015 at 10:35:47AM +0100, 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 the v4, and sorry about the late review.

> +struct mxu1_port {
> +	u8 mxp_msr;
> +	u8 mxp_mcr;
> +	u8 mxp_uart_types;
> +	u8 mxp_uart_mode;
> +	struct usb_serial_port *mxp_port;

You should not need a back pointer here. Pass the usb_serial_port as an
argument where needed instead.

> +	spinlock_t mxp_lock;    /* Protects mxp_msr */
> +	struct mutex mxp_mutex; /* Protects mxp_mcr */
> +	int mxp_send_break;

Why is this not a bool? You never use the value you assign to
mxp_send_break other than to compare it with a constant.

> +};

Please drop the mxp_ prefix from the field names.

> +struct mxu1_device {
> +	struct mutex mxd_lock;
> +	struct usb_serial *mxd_serial;

This lock and back pointer are not needed. You can access the usb_serial
from usb_serial_port.

> +	u16 mxd_model;
> +};
> +
> +static const struct usb_device_id mxu1_idtable[] = {
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1110_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1130_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1150_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1151_PRODUCT_ID) },
> +	{ USB_DEVICE(MXU1_VENDOR_ID, MXU1_1131_PRODUCT_ID) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mxu1_idtable);
> +
> +/* Write the given buffer out to the control pipe.  */
> +static int mxu1_send_ctrl_data_urb(struct usb_serial *serial,
> +				   u8 request,
> +				   u16 value, u16 index,
> +				   u8 *data, size_t size)

Use void * for the buffer and drop the casts at the call sites.

> +{
> +	int status;
> +
> +	status = usb_control_msg(serial->dev,
> +				 usb_sndctrlpipe(serial->dev, 0),
> +				 request,
> +				 (USB_DIR_OUT | USB_TYPE_VENDOR |
> +				  USB_RECIP_DEVICE), value, index,
> +				 data, size,
> +				 USB_CTRL_SET_TIMEOUT);
> +	if (status < 0) {
> +		dev_err(&serial->interface->dev,
> +			"%s - usb_control_msg failed: %d\n",
> +			__func__, status);
> +		return status;
> +	}
> +
> +	if (status != size) {
> +		dev_err(&serial->interface->dev,
> +			"%s - short write (%d / %zd)\n",
> +			__func__, status, size);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Send a vendor request without any data */
> +static int mxu1_send_ctrl_urb(struct usb_serial *serial,
> +			      u8 request, u16 value, u16 index)
> +{
> +	return mxu1_send_ctrl_data_urb(serial, request, value, index,
> +				       NULL, 0);
> +}
> +
> +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;
> +	struct usb_device *dev = serial->dev;
> +	struct mxu1_firmware_header *header;
> +	unsigned int pipe;
> +
> +	pipe = usb_sndbulkpipe(dev, serial->port[0]->
> +				bulk_out_endpointAddress);

Odd line break that is not even needed.

> +
> +	buffer_size = fw_p->size + sizeof(*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(*header); pos < buffer_size; pos++)
> +		cs = (u8)(cs + buffer[pos]);
> +
> +	header = (struct mxu1_firmware_header *)buffer;
> +	header->wLength = cpu_to_le16(buffer_size - sizeof(*header));
> +	header->bCheckSum = cs;
> +
> +	dev_dbg(&dev->dev, "%s - downloading firmware\n", __func__);
> +
> +	for (pos = 0; pos < buffer_size; pos += done) {
> +		len = min(buffer_size - pos, MXU1_DOWNLOAD_MAX_PACKET_SIZE);
> +
> +		status = usb_bulk_msg(dev, pipe, buffer + pos, len, &done,
> +				MXU1_DOWNLOAD_TIMEOUT);
> +		if (status)
> +			break;
> +	}
> +
> +	kfree(buffer);
> +
> +	if (status) {
> +		dev_err(&dev->dev, "failed to download firmware: %d\n", status);
> +		return status;
> +	}
> +
> +	msleep_interruptible(100);
> +	usb_reset_device(dev);
> +
> +	dev_dbg(&dev->dev, "%s - download successful (%d)\n", __func__, status);

You can drop status from the message, it's always zero.

> +
> +	return 0;
> +}

> +static int mxu1_startup(struct usb_serial *serial)
> +{
> +	struct mxu1_device *mxdev;
> +	struct usb_device *dev = serial->dev;
> +	struct usb_host_interface *cur_altsetting;
> +	char fw_name[32];
> +	const struct firmware *fw_p = NULL;
> +	int err;
> +	int status = 0;
> +
> +	dev_dbg(&dev->dev, "%s - product 0x%4X, num configurations %d, configuration value %d\n",

Use &serial->interface->dev here too, and you want 0x%04x zero-padded.

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

Use !mxdev.

> +		return -ENOMEM;
> +
> +	mutex_init(&mxdev->mxd_lock);
> +	mxdev->mxd_serial = serial;

These two are not needed as mentioned above.

> +	usb_set_serial_data(serial, mxdev);
> +
> +	mxdev->mxd_model = le16_to_cpu(dev->descriptor.idProduct);
> +
> +	cur_altsetting = serial->interface->cur_altsetting;
> +
> +	/* if we have only 1 configuration, download firmware */
> +	if (cur_altsetting->desc.bNumEndpoints == 1) {
> +
> +		snprintf(fw_name,
> +			 sizeof(fw_name),
> +			 "moxa/moxa-%04x.fw",
> +			 mxdev->mxd_model);
> +
> +		err = request_firmware(&fw_p, fw_name, &serial->interface->dev);
> +		if (err) {
> +			dev_err(&serial->interface->dev, "firmware %s not found\n",
> +				fw_name);

Be a little less specific here (e.g. "failed to request firmware: %d\n" and
include the errno.

> +			kfree(mxdev);
> +			return err;
> +		}
> +
> +		err = mxu1_download_firmware(serial, fw_p);
> +		if (err) {
> +			kfree(mxdev);
> +			return err;

You're leaking the firmware memory here.

> +		}
> +
> +		status = -ENODEV;
> +		release_firmware(fw_p);
> +	}
> +
> +	return status;
> +}

> +static void mxu1_set_termios(struct tty_struct *tty,
> +			     struct usb_serial_port *port,
> +			     struct ktermios *old_termios)
> +{
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	struct mxu1_uart_config *config;
> +	tcflag_t cflag, iflag;
> +	speed_t baud;
> +	int status;
> +	unsigned int mcr;
> +
> +	dev_dbg(&port->dev, "%s\n", __func__);
> +
> +	cflag = tty->termios.c_cflag;
> +	iflag = tty->termios.c_iflag;
> +
> +	if (old_termios &&
> +	    !tty_termios_hw_change(&tty->termios, old_termios) &&
> +	    tty->termios.c_iflag == old_termios->c_iflag) {
> +		dev_dbg(&port->dev, "%s - nothing to change\n", __func__);
> +		return;
> +	}
> +
> +	dev_dbg(&port->dev,
> +		"%s - clfag %08x, iflag %08x\n", __func__, cflag, iflag);
> +
> +	if (old_termios) {
> +		dev_dbg(&port->dev, "%s - old clfag %08x, old iflag %08x\n",
> +			__func__,
> +			old_termios->c_cflag,
> +			old_termios->c_iflag);
> +	}
> +
> +	config = kzalloc(sizeof(*config), GFP_KERNEL);
> +	if (!config)
> +		return;
> +
> +	config->wFlags = 0;

You can rely on your kzalloc just above.

> +
> +	/* these flags must be set */
> +	config->wFlags |= MXU1_UART_ENABLE_MS_INTS;
> +	config->wFlags |= MXU1_UART_ENABLE_AUTO_START_DMA;
> +	if (mxport->mxp_send_break == MXU1_LCR_BREAK)
> +		config->wFlags |= MXU1_UART_SEND_BREAK_SIGNAL;
> +	config->bUartMode = (u8)(mxport->mxp_uart_mode);

No need to cast.

> +static int mxu1_ioctl_set_rs485(struct mxu1_port *mxport,
> +				struct serial_rs485 __user *rs485_user)
> +{
> +	struct serial_rs485 rs485;
> +	struct usb_serial_port *port = mxport->mxp_port;
> +	struct mxu1_device *mxdev;
> +
> +	mxdev = usb_get_serial_data(port->serial);
> +
> +	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
> +		return -EFAULT;
> +
> +	if (mxport->mxp_uart_types & (MXU1_TYPE_RS422 | MXU1_TYPE_RS485)) {
> +
> +		if (rs485.flags & SER_RS485_ENABLED) {
> +			mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_ENABLED;
> +		} else {
> +			if (mxport->mxp_uart_types & MXU1_TYPE_RS232)
> +				mxport->mxp_uart_mode = MXU1_UART_232;
> +			else
> +				mxport->mxp_uart_mode =
> +						MXU1_UART_485_RECEIVER_DISABLED;
> +		}
> +	} else {
> +		dev_err(&port->dev, "%s not handled by MOXA UPort %04x\n",
> +			__func__, mxdev->mxd_model);

Could you just spell this out as something like "RS485 not supported by
this device\n"?

> +		return  -EINVAL;
> +	}

You also need to revert any bits not supported and return the new config
to user space. Take a look at uart_set_rs485_config in serial_core.

> +
> +	return 0;
> +}

> +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;
> +	unsigned long flags;
> +
> +	dev_dbg(&port->dev, "%s\n", __func__);

Drop this.

> +
> +	mutex_lock(&mxport->mxp_mutex);
> +	spin_lock_irqsave(&mxport->mxp_lock, flags);
> +
> +	msr = mxport->mxp_msr;
> +	mcr = mxport->mxp_mcr;
> +
> +	spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> +	mutex_unlock(&mxport->mxp_mutex);
> +
> +	result = ((mcr & MXU1_MCR_DTR)	? TIOCM_DTR	: 0) |
> +		 ((mcr & MXU1_MCR_RTS)	? TIOCM_RTS	: 0) |
> +		 ((mcr & MXU1_MCR_LOOP) ? TIOCM_LOOP	: 0) |
> +		 ((msr & MXU1_MSR_CTS)	? TIOCM_CTS	: 0) |
> +		 ((msr & MXU1_MSR_CD)	? TIOCM_CAR	: 0) |
> +		 ((msr & MXU1_MSR_RI)	? TIOCM_RI	: 0) |
> +		 ((msr & MXU1_MSR_DSR)	? TIOCM_DSR	: 0);
> +
> +	dev_dbg(&port->dev, "%s - 0x%04X\n", __func__, result);
> +
> +	return result;
> +}
> +
> +static int mxu1_tiocmset(struct tty_struct *tty,
> +			 unsigned int set, unsigned int clear)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	int err;
> +	unsigned int mcr;
> +
> +	dev_dbg(&port->dev, "%s\n", __func__);

Ditto.

> +
> +	mutex_lock(&mxport->mxp_mutex);
> +	mcr = mxport->mxp_mcr;
> +
> +	if (set & TIOCM_RTS)
> +		mcr |= MXU1_MCR_RTS;
> +	if (set & TIOCM_DTR)
> +		mcr |= MXU1_MCR_DTR;
> +	if (set & TIOCM_LOOP)
> +		mcr |= MXU1_MCR_LOOP;
> +
> +	if (clear & TIOCM_RTS)
> +		mcr &= ~MXU1_MCR_RTS;
> +	if (clear & TIOCM_DTR)
> +		mcr &= ~MXU1_MCR_DTR;
> +	if (clear & TIOCM_LOOP)
> +		mcr &= ~MXU1_MCR_LOOP;
> +
> +	err = mxu1_set_mcr(port, mcr);
> +	if (!err)
> +		mxport->mxp_mcr = mcr;
> +
> +	mutex_unlock(&mxport->mxp_mutex);
> +
> +	return err;
> +}

> +static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	struct mxu1_device *mxdev;
> +	struct mxu1_port *mxport;
> +	struct usb_device *dev;
> +	struct urb *urb;
> +	int status;
> +	u16 open_settings;
> +
> +	open_settings = (MXU1_PIPE_MODE_CONTINUOUS |
> +			 MXU1_PIPE_TIMEOUT_ENABLE |
> +			 (MXU1_TRANSFER_TIMEOUT << 2));
> +
> +	mxdev = usb_get_serial_data(port->serial);

You don't need mxdev; you already have port->serial, and the interrupt
urb already has the usb_serial_port set as context.

> +	mxport = usb_get_serial_port_data(port);
> +
> +	dev = port->serial->dev;
> +
> +	mxport->mxp_msr = 0;
> +	mxport->mxp_mcr |= MXU1_MCR_RTS | MXU1_MCR_DTR;

These will be raised by tty core using dtr_rts, so drop this.

> +
> +	dev_dbg(&port->dev, "%s - start interrupt in urb\n", __func__);
> +	urb = port->interrupt_in_urb;
> +	urb->context = mxdev;
> +	status = usb_submit_urb(urb, GFP_KERNEL);
> +	if (status) {
> +		dev_err(&port->dev, "failed to submit interrupt urb: %d\n",
> +			status);
> +		return status;
> +	}
> +
> +	mxu1_set_termios(tty, port, NULL);

tty may be NULL if this port used as a console, so only call set_termios
if tty is non-NULL.

> +
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> +				    open_settings, MXU1_UART1_PORT);
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send open command: %d\n",
> +			__func__, status);
> +		goto unlink_int_urb;
> +	}
> +
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> +				    0, MXU1_UART1_PORT);
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send start command: %d\n",
> +			__func__, status);
> +		goto unlink_int_urb;
> +	}
> +
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> +				    MXU1_PURGE_INPUT, MXU1_UART1_PORT);
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot clear input buffers: %d\n",
> +			__func__, status);
> +
> +		goto unlink_int_urb;
> +	}
> +
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> +				    MXU1_PURGE_OUTPUT, MXU1_UART1_PORT);
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot clear output buffers: %d\n",
> +			__func__, status);
> +
> +		goto unlink_int_urb;
> +	}
> +
> +	/*
> +	 * reset the data toggle on the bulk endpoints to work around bug in
> +	 * host controllers where things get out of sync some times
> +	 */
> +	usb_clear_halt(dev, port->write_urb->pipe);
> +	usb_clear_halt(dev, port->read_urb->pipe);
> +
> +	mxu1_set_termios(tty, port, NULL);
> +
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> +				    open_settings, MXU1_UART1_PORT);
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send open command: %d\n",
> +			__func__, status);
> +		goto unlink_int_urb;
> +	}
> +
> +	status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> +				    0, MXU1_UART1_PORT);
> +	if (status) {
> +		dev_err(&port->dev, "%s - cannot send start command: %d\n",
> +			__func__, status);
> +		goto unlink_int_urb;
> +	}
> +
> +	status = usb_serial_generic_open(tty, port);
> +	if (status) {
> +		dev_err(&port->dev, "%s - submit read urb failed: %d\n",
> +			__func__, status);
> +		goto unlink_int_urb;
> +	}
> +
> +	return status;
> +
> +unlink_int_urb:
> +	usb_kill_urb(port->interrupt_in_urb);
> +
> +	return status;
> +}
> +
> +static void mxu1_close(struct usb_serial_port *port)
> +{
> +	int status;
> +
> +	dev_dbg(&port->dev, "%s\n", __func__);

Drop this.

> +
> +	usb_serial_generic_close(port);
> +	usb_kill_urb(port->interrupt_in_urb);
> +
> +	status = mxu1_send_ctrl_urb(port->serial, MXU1_CLOSE_PORT,
> +				    0, MXU1_UART1_PORT);
> +	if (status)
> +		dev_err(&port->dev, "failed to send close port command: %d\n",
> +			status);
> +}
> +
> +static void mxu1_handle_new_msr(struct usb_serial_port *port,
> +				struct mxu1_port *mxport, u8 msr)
> +{
> +	struct async_icount *icount;
> +	unsigned long flags;
> +
> +	dev_dbg(&port->dev, "%s - msr 0x%02X\n", __func__, msr);
> +
> +	if (msr & MXU1_MSR_DELTA_MASK) {
> +		icount = &mxport->mxp_port->icount;

You already have the port structure.

> +		if (msr & MXU1_MSR_DELTA_CTS)
> +			icount->cts++;
> +		if (msr & MXU1_MSR_DELTA_DSR)
> +			icount->dsr++;
> +		if (msr & MXU1_MSR_DELTA_CD)
> +			icount->dcd++;
> +		if (msr & MXU1_MSR_DELTA_RI)
> +			icount->rng++;
> +
> +		wake_up_interruptible(&port->port.delta_msr_wait);
> +	}
> +
> +	spin_lock_irqsave(&mxport->mxp_lock, flags);
> +	mxport->mxp_msr = msr & MXU1_MSR_MASK;
> +	spin_unlock_irqrestore(&mxport->mxp_lock, flags);

You want to do this before testing the delta and waking up any waiters.

> +}
> +
> +static void mxu1_interrupt_callback(struct urb *urb)
> +{
> +	struct mxu1_device *mxdev = urb->context;
> +	struct usb_serial_port *port;
> +	struct usb_serial *serial = mxdev->mxd_serial;
> +	struct mxu1_port *mxport;
> +	unsigned char *data = urb->transfer_buffer;
> +	int length = urb->actual_length;
> +	int function;
> +	int status;
> +	u8 msr;
> +
> +	port = serial->port[0];
> +	mxport = usb_get_serial_port_data(port);
> +
> +	dev_dbg(&port->dev, "%s\n", __func__);

Drop this.

> +
> +	switch (urb->status) {
> +	case 0:
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		dev_dbg(&port->dev, "%s - urb shutting down: %d\n",
> +			__func__, urb->status);
> +		return;
> +	default:
> +		dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
> +			__func__, urb->status);
> +		goto exit;
> +	}
> +
> +	if (length != 2) {
> +		dev_dbg(&port->dev, "%s - bad packet size: %d\n",
> +			__func__, length);
> +		goto exit;
> +	}
> +
> +	if (data[0] == MXU1_CODE_HARDWARE_ERROR) {
> +		dev_err(&port->dev, "%s - hardware error: %d\n",
> +			__func__, data[1]);
> +		goto exit;
> +	}
> +
> +	function = mxu1_get_func_from_code(data[0]);
> +
> +	dev_dbg(&port->dev, "%s - function %d, data 0x%02X\n",
> +		 __func__, function, data[1]);
> +
> +	switch (function) {
> +	case MXU1_CODE_DATA_ERROR:
> +		dev_dbg(&port->dev, "%s - DATA ERROR, data 0x%02X\n",
> +			 __func__, data[1]);
> +		break;
> +
> +	case MXU1_CODE_MODEM_STATUS:
> +		msr = data[1];
> +		dev_dbg(&port->dev, "%s - msr 0x%02X\n", __func__, msr);

You print the same debug info in mxu1_handle_new_msr.

> +		mxu1_handle_new_msr(port, mxport, msr);
> +		break;
> +
> +	default:
> +		dev_err(&port->dev, "%s - unknown interrupt code: 0x%02X\n",
> +			__func__, data[1]);
> +		break;
> +	}
> +
> +exit:
> +	status = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (status)
> +		dev_err(&port->dev, "%s - resubmit interrupt urb failed: %d\n",
> +			__func__, status);

No need to include the function name.

> +}

Overall this looks really good now.

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