Date: Tue, 29 Dec 2015 13:29:56 +0100
From: Johan Hovold <johan@kernel.org>
To: Mathieu OTHACEHE <m.othacehe@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org
Subject: Re: [PATCH v6] USB: serial: add Moxa UPORT 11x0 driver
Message-ID: <20151229122956.GB25531@localhost>
References: <20151228153238.GB18146@localhost>
 <1451334085-23464-1-git-send-email-m.othacehe@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1451334085-23464-1-git-send-email-m.othacehe@gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 8575
Lines: 296

On Mon, Dec 28, 2015 at 09:21:25PM +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@gmail.com>
> ---
> 
> Hi Johan,
> 
> Here is the v6 of the driver.
> 
> I understand the problems with the TIOCSRS485/TIOCGRS485 ioctls in
> this driver.
> For now, I prefer dropping mode switching support from the driver as
> you suggested.
>
> So UPORT 1110 and 1150 only support RS232 and UPORT 1130 only support
> RS-485-2W.
> If a new interface is developped, I'll restore mode switching code.

Sounds good.
 
> About firmware images, I just sent a patch to linux-firmware. Here is the link :
> https://lkml.org/lkml/2015/12/28/239

Thanks, I've done some quick tests using a 1150 now. When looking
through the code one last time I found a few issues that I fixed up. I'll
submit patches for these to the USB list.

But there are couple of things you could consider to do as follow ups as
well. Details below.

> +static int mxu1_port_probe(struct usb_serial_port *port)
> +{
> +	struct mxu1_port *mxport;
> +	struct mxu1_device *mxdev;
> +	struct urb *urb;
> +
> +	mxport = kzalloc(sizeof(struct mxu1_port), GFP_KERNEL);
> +	if (!mxport)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&mxport->spinlock);
> +	mutex_init(&mxport->mutex);
> +
> +	mxdev = usb_get_serial_data(port->serial);
> +
> +	urb = port->interrupt_in_urb;
> +	if (!urb) {

You are leaking the port private data here (fixed).

> +		dev_err(&port->dev, "%s - no interrupt urb\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	switch (mxdev->mxd_model) {
> +	case MXU1_1110_PRODUCT_ID:
> +	case MXU1_1150_PRODUCT_ID:
> +	case MXU1_1151_PRODUCT_ID:
> +		mxport->uart_mode = MXU1_UART_232;
> +		break;
> +	case MXU1_1130_PRODUCT_ID:
> +	case MXU1_1131_PRODUCT_ID:
> +		mxport->uart_mode = MXU1_UART_485_RECEIVER_DISABLED;
> +		break;
> +	}
> +
> +	usb_set_serial_port_data(port, mxport);
> +
> +	port->port.closing_wait =
> +			msecs_to_jiffies(MXU1_DEFAULT_CLOSING_WAIT * 10);
> +	port->port.drain_delay = 1;
> +
> +	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(&serial->interface->dev, "%s - product 0x%04X, num configurations %d, configuration value %d\n",
> +		__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)
> +		return -ENOMEM;
> +
> +	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, "failed to request firmware: %d\n",
> +				err);
> +			kfree(mxdev);
> +			return err;
> +		}
> +
> +		err = mxu1_download_firmware(serial, fw_p);
> +		if (err) {
> +			release_firmware(fw_p);
> +			kfree(mxdev);
> +			return err;
> +		}
> +
> +		status = -ENODEV;
> +		release_firmware(fw_p);

And you're leaking the interface private data here as well (also fixed).

What you could consider doing as a follow up it so move both the
interrupt-in urb test in port_probe and the firmware download to a new
probe callback. That way you avoid the unnecessary memory allocations
done by core before attach is called, and you can verify and refuse to
bind to a device in case the expected endpoints are missing.

> +	}
> +
> +	return status;
> +}

> +	if (C_BAUD(tty) == B0)
> +		mcr &= ~(MXU1_MCR_DTR | MXU1_MCR_RTS);
> +	else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
> +		mcr |= ~(MXU1_MCR_DTR | MXU1_MCR_RTS);

This should have been

	mcr |= MXU1_MCR_DTR | MXU1_MCR_RTS;

Also fixed.

> +static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> +	struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	int status;
> +	u16 open_settings;
> +
> +	open_settings = (MXU1_PIPE_MODE_CONTINUOUS |
> +			 MXU1_PIPE_TIMEOUT_ENABLE |
> +			 (MXU1_TRANSFER_TIMEOUT << 2));
> +
> +	mxport->msr = 0;
> +
> +	dev_dbg(&port->dev, "%s - start interrupt in urb\n", __func__);
> +	status = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
> +	if (status) {
> +		dev_err(&port->dev, "failed to submit interrupt urb: %d\n",
> +			status);
> +		return status;
> +	}
> +
> +	if (tty)
> +		mxu1_set_termios(tty, port, NULL);
> +
> +	status = mxu1_send_ctrl_urb(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(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(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(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(serial->dev, port->write_urb->pipe);
> +	usb_clear_halt(serial->dev, port->read_urb->pipe);
> +
> +	if (tty)
> +		mxu1_set_termios(tty, port, NULL);
> +
> +	status = mxu1_send_ctrl_urb(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(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;
> +	}

I noticed that you send OPEN and START twice during probe -- is that
really necessary?

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

And should there not be a matching STOP request at close?

> +static struct usb_serial_driver mxuport11_device = {
> +	.driver = {
> +		.owner		= THIS_MODULE,
> +		.name		= "mxuport11",

I also decided to rename the usb-serial driver mxu11x0 to match the
module and USB driver name.

I have applied the patch now and will post my fixes and a couple of
clean ups to the list.

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