Re: [PATCH v6] USB: serial: add Moxa UPORT 11x0 driver

From: Johan Hovold
Date: Tue Dec 29 2015 - 07:29:41 EST


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@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/