lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 12 Dec 2014 09:01:03 -0600
From:	George McCollister <george.mccollister@...il.com>
To:	Johan Hovold <johan@...nel.org>
Cc:	linux-usb@...r.kernel.org,
	open list <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] USB: serial: add nt124 usb to serial driver

Johan,

Thanks for the thorough review.

On Wed, Dec 10, 2014 at 7:04 AM, Johan Hovold <johan@...nel.org> wrote:
> On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:
>> This driver is for the NovaTech 124 4x serial expansion board for the
>> NovaTech OrionLXm.
>>
>> Firmware source code can be found here:
>> https://github.com/novatechweb/nt124
>
> Great, and thanks for the patch!
>
>> Signed-off-by: George McCollister <george.mccollister@...il.com>
>> ---
>>  drivers/usb/serial/Kconfig  |   9 +
>>  drivers/usb/serial/Makefile |   1 +
>>  drivers/usb/serial/nt124.c  | 429 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 439 insertions(+)
>>  create mode 100644 drivers/usb/serial/nt124.c
>>
>> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
>> index a69f7cd..6dfc340 100644
>> --- a/drivers/usb/serial/Kconfig
>> +++ b/drivers/usb/serial/Kconfig
>> @@ -509,6 +509,15 @@ config USB_SERIAL_NAVMAN
>>         To compile this driver as a module, choose M here: the
>>         module will be called navman.
>>
>> +config USB_SERIAL_NT124
>> +     tristate "USB nt124 serial device"
>
> USB NovaTech 124 Serial Driver (or NovaTech nt124)
I'll use USB NovaTech 124 Serial Driver
>
>> +     help
>> +       Say Y here if you want to use the NovaTech 124 4x USB to serial
>> +       board.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called nt124.
>> +
>>  config USB_SERIAL_PL2303
>>       tristate "USB Prolific 2303 Single Port Serial Driver"
>>       help
>> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
>> index 349d9df..f88eaab 100644
>> --- a/drivers/usb/serial/Makefile
>> +++ b/drivers/usb/serial/Makefile
>> @@ -39,6 +39,7 @@ obj-$(CONFIG_USB_SERIAL_MOS7720)            += mos7720.o
>>  obj-$(CONFIG_USB_SERIAL_MOS7840)             += mos7840.o
>>  obj-$(CONFIG_USB_SERIAL_MXUPORT)             += mxuport.o
>>  obj-$(CONFIG_USB_SERIAL_NAVMAN)                      += navman.o
>> +obj-$(CONFIG_USB_SERIAL_NT124)                       += nt124.o
>>  obj-$(CONFIG_USB_SERIAL_OMNINET)             += omninet.o
>>  obj-$(CONFIG_USB_SERIAL_OPTICON)             += opticon.o
>>  obj-$(CONFIG_USB_SERIAL_OPTION)                      += option.o
>> diff --git a/drivers/usb/serial/nt124.c b/drivers/usb/serial/nt124.c
>> new file mode 100644
>> index 0000000..d7557ff
>> --- /dev/null
>> +++ b/drivers/usb/serial/nt124.c
>> @@ -0,0 +1,429 @@
>> +/*
>> + * nt124.c
>
> Put a brief description here instead.
Okay
>
>> + *
>> + * Copyright (c) 2014 NovaTech LLC
>> + *
>> + * Driver for nt124 4x serial board based on STM32F103
>
> For example use something like this above.
Okay
>
>> + *
>> + * Portions derived from the cdc-acm driver
>> + *
>> + * The original intention was to implement a cdc-acm compliant
>> + * 4x USB to serial converter in the STM32F103 however several problems arose.
>> + *   The STM32F103 didn't have enough end points to implement 4 ports.
>> + *   CTS control was required by the application.
>> + *   Accurate notification of transmission completion was required.
>> + *   RTSCTS flow control support was required.
>> + *
>> + * The interrupt endpoint was eliminated and the control line information
>> + * was moved to the first two bytes of the in endpoint message. CTS control
>
> bulk in endpoint
Okay
>
>> + * and mechanisms to enable RTSCTS flow control and deliver TXEMPTY
>> + * information were added.
>> + *
>> + * Firmware source code can be found here:
>> + * https://github.com/novatechweb/nt124
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_driver.h>
>> +#include <linux/tty_flip.h>
>> +#include <linux/module.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/serial.h>
>> +#include <asm/unaligned.h>
>> +
>> +#define NT124_VID            0x2aeb
>> +#define NT124_USB_PID                124
>> +
>> +#define DRIVER_AUTHOR "George McCollister <george.mccollister@...il.com>"
>> +#define DRIVER_DESC "nt124 USB serial driver"
>
> Just use the MODULE macros directly (at the end of the file), no need
> for the defines.
Okay
>
>> +
>> +static const struct usb_device_id id_table[] = {
>> +     { USB_DEVICE(NT124_VID, NT124_USB_PID) },
>> +     { },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(usb, id_table);
>> +
>> +/*
>> + * Output control lines.
>> + */
>> +
>
> No new line.
Okay
>
>> +#define NT124_CTRL_DTR               0x01
>> +#define NT124_CTRL_RTS               0x02
>> +
>> +/*
>> + * Input control lines and line errors.
>> + */
>> +
>
> Same here.
Okay
>
>> +#define NT124_CTRL_DCD               0x01
>> +#define NT124_CTRL_DSR               0x02
>> +#define NT124_CTRL_BRK               0x04
>> +#define NT124_CTRL_RI                0x08
>> +#define NT124_CTRL_FRAMING   0x10
>> +#define NT124_CTRL_PARITY    0x20
>> +#define NT124_CTRL_OVERRUN   0x40
>> +#define NT124_CTRL_TXEMPTY   0x80
>> +#define NT124_CTRL_CTS               0x100
>> +
>> +#define USB_NT124_REQ_SET_LINE_CODING                0x20
>> +#define USB_NT124_REQ_SET_CONTROL_LINE_STATE 0x22
>> +#define USB_NT124_REQ_SEND_BREAK             0x23
>> +#define USB_NT124_SET_FLOW_CONTROL           0x90
>> +
>> +struct nt124_line_coding {
>> +     __le32  dwDTERate;
>> +     u8      bCharFormat;
>> +     u8      bParityType;
>> +     u8      bDataBits;
>> +} __packed;
>
> __packed not needed.
Okay
>
>> +
>> +struct nt124_private {
>> +     /* USB interface */
>
> Superfluous comment.
Okay
>
>> +     u16 bInterfaceNumber;
>> +     /* input control lines (DCD, DSR, RI, break, overruns) */
>> +     unsigned int ctrlin;
>> +     /* output control lines (DTR, RTS) */
>> +     unsigned int ctrlout;
>
> Locking is missing for both of these (cdc-acm isn't always a great
> example).
>
> Use u16 for both?
Sure
>
>> +     /* termios CLOCAL */
>> +     unsigned char clocal;
>
> Not needed (see below).
Okay
>
>> +     /* bits, stop, parity */
>
> Superfluous comment.
Okay
>
>> +     struct nt124_line_coding line;
>> +     int serial_transmit;
>
> Comment on this one, or just call it tx_empty.
>
> And use bool.
I'll just replace it with bool tx_empty
>
>> +     unsigned int flowctrl;
>
> Don't think you need this one either (more below).
I'll remove it.
>
>> +};
>
>
>> +
>> +static int nt124_ctrl_msg(struct usb_serial_port *port, int request, int value,
>> +                       void *buf, int len)
>
> Use u16 (or unsigned int) for request and value, size_t for len.
Okay
>
>> +{
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +     int retval;
>> +
>> +     retval = usb_control_msg(port->serial->dev,
>> +                     usb_sndctrlpipe(port->serial->dev, 0),
>> +                     request, USB_TYPE_CLASS | USB_RECIP_INTERFACE, value,
>> +                     priv->bInterfaceNumber,
>> +                     buf, len, 5000);
>
> Use a define for the timeout (e.g. USB_CTRL_SET_TIMEOUT).
Okay
>
>> +
>> +     dev_dbg(&port->dev,
>> +                     "%s - rq 0x%02x, val %#x, len %#x, result %d\n",
>
> Request and val are 16-bit so use %04x for both, and %zu for len.
Okay
>
>> +                     __func__, request, value, len, retval);
>> +
>> +     return retval < 0 ? retval : 0;
>
> Avoid ?: constructs.
Okay
>
> Add proper error handling and add a dev_err for that case.
Okay
>
>> +}
>> +
>> +#define nt124_set_control(port, control) \
>> +     nt124_ctrl_msg(port, USB_NT124_REQ_SET_CONTROL_LINE_STATE, control, \
>> +     NULL, 0)
>> +#define nt124_set_line(port, line) \
>> +     nt124_ctrl_msg(port, USB_NT124_REQ_SET_LINE_CODING, 0, line, \
>> +     sizeof *(line))
>> +#define nt124_send_break(port, ms) \
>> +     nt124_ctrl_msg(port, USB_NT124_REQ_SEND_BREAK, ms, NULL, 0)
>> +#define nt124_set_flowctrl(port, flowctrl) \
>> +     nt124_ctrl_msg(port, USB_NT124_SET_FLOW_CONTROL, flowctrl, NULL, 0)
>
> Don't use macros for these. Either call the ctrl_msg helper directly or
> define proper inline functions.
Okay, I'll probably just call them directly.
>
>> +
>> +static void nt124_process_notify(struct usb_serial_port *port,
>
> Rename process_status?
Sure
>
>> +                              struct nt124_private *priv,
>> +                              unsigned char *data)
>> +{
>> +     int newctrl;
>
> Use an unsigned type.
I'll use u16
>
>> +     unsigned long flags;
>> +
>> +     newctrl = get_unaligned_le16(data);
>> +     if (newctrl & NT124_CTRL_TXEMPTY) {
>> +             spin_lock_irqsave(&port->lock, flags);
>> +             priv->serial_transmit = 0;
>> +             spin_unlock_irqrestore(&port->lock, flags);
>> +             tty_port_tty_wakeup(&port->port);
>> +     }
>
> Just store tx_empty. No need to do the wake up and not sure you'll need
> the locking (see below).
Okay, I'll drop the tty_port_tty_wakeup
>
>> +
>> +     if (!priv->clocal && (priv->ctrlin & ~newctrl & NT124_CTRL_DCD)) {
>> +             dev_dbg(&port->dev, "%s - calling hangup\n",
>> +                             __func__);
>> +             tty_port_tty_hangup(&port->port, false);
>> +     }
>
> Call usb_serial_handle_dcd_change() unconditionally when DCD has changed
> here instead.
Okay
>
>> +
>> +     priv->ctrlin = newctrl;
>
> Locking missing.
I'll add a spinlock and use it around ctrlin and ctrlout with irqsave.
>
>> +}
>> +
>> +static void nt124_process_read_urb(struct urb *urb)
>> +{
>> +     struct usb_serial_port *port = urb->context;
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +     unsigned char *data = (unsigned char *)urb->transfer_buffer;
>> +
>> +     if (urb->actual_length < 2)
>> +             return;
>> +
>> +     nt124_process_notify(port, priv, data);
>> +
>> +     if (urb->actual_length == 2)
>> +             return;
>> +
>> +     tty_insert_flip_string(&port->port, &data[2],
>> +                     urb->actual_length - 2);
>
> No need to break line.
Okay
>
> Use a temporary for length as well.
I'll make a temporary variable of type size_t named datalen.
>
>> +     tty_flip_buffer_push(&port->port);
>> +}
>> +
>> +static int nt124_tiocmget(struct tty_struct *tty)
>> +{
>> +     struct usb_serial_port *port = tty->driver_data;
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +
>> +     return (priv->ctrlout & NT124_CTRL_DTR ? TIOCM_DTR : 0) |
>> +            (priv->ctrlout & NT124_CTRL_RTS ? TIOCM_RTS : 0) |
>> +            (priv->ctrlin  & NT124_CTRL_DSR ? TIOCM_DSR : 0) |
>> +            (priv->ctrlin  & NT124_CTRL_RI  ? TIOCM_RI  : 0) |
>> +            (priv->ctrlin  & NT124_CTRL_DCD ? TIOCM_CD  : 0) |
>> +            (priv->ctrlin  & NT124_CTRL_CTS ? TIOCM_CTS : 0);
>
> Locking is missing. Use temporary variables.
Okay
>
>> +}
>> +
>> +static int nt124_port_tiocmset(struct usb_serial_port *port,
>> +                            unsigned int set, unsigned int clear)
>> +{
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +     unsigned int newctrl;
>> +
>> +     newctrl = priv->ctrlout;
>
> Locking throughout.
Okay
>
>> +     set = (set & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
>> +             (set & TIOCM_RTS ? NT124_CTRL_RTS : 0);
>> +     clear = (clear & TIOCM_DTR ? NT124_CTRL_DTR : 0) |
>> +             (clear & TIOCM_RTS ? NT124_CTRL_RTS : 0);
>
> Expand these without the ?:.
Okay
>
>> +
>> +     newctrl = (newctrl & ~clear) | set;
>> +
>> +     if (priv->ctrlout == newctrl)
>> +             return 0;
>
> Add newline.
Okay
>
>> +     return nt124_set_control(port, priv->ctrlout = newctrl);
>
> Don't do assignments in the argument list.
Okay, this needs to be changed to put locking around ctrlout anyway.
>
> Also use usb_translate_errors on any error returned from USB core before
> passing it to user space here.
Okay
>
>> +}
>> +
>> +static int nt124_tiocmset(struct tty_struct *tty,
>> +                       unsigned int set, unsigned int clear)
>> +{
>> +     struct usb_serial_port *port = tty->driver_data;
>> +
>> +     return nt124_port_tiocmset(port, set, clear);
>> +}
>> +
>> +static void nt124_dtr_rts(struct usb_serial_port *port, int on)
>> +{
>> +     if (on)
>> +             nt124_port_tiocmset(port, TIOCM_DTR|TIOCM_RTS, 0);
>
> Spaces around |.
Okay
>
>> +     else
>> +             nt124_port_tiocmset(port, 0, TIOCM_DTR|TIOCM_RTS);
>
> Ditto.
Okay
>
>> +}
>> +
>> +static void nt124_set_termios(struct tty_struct *tty,
>> +                           struct usb_serial_port *port,
>> +                           struct ktermios *termios_old)
>> +{
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +     struct ktermios *termios = &tty->termios;
>> +     struct nt124_line_coding newline;
>> +     int newctrl = priv->ctrlout;
>> +
>> +     newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty));
>> +     newline.bCharFormat = termios->c_cflag & CSTOPB ? 2 : 0;
>
> Do you support 1.5 stop bits (e.g. when using 5 data bits)?
No
>
>> +     newline.bParityType = termios->c_cflag & PARENB ?
>> +                             (termios->c_cflag & PARODD ? 1 : 2) +
>> +                             (termios->c_cflag & CMSPAR ? 2 : 0) : 0;
>
> This hardly readable. Don't use ?:
Okay
>
> Add new line.
Okay
>
>> +     switch (termios->c_cflag & CSIZE) {
>
> C_CSIZE(tty)
Okay
>
>> +     case CS5:
>> +             newline.bDataBits = 5;
>> +             break;
>> +     case CS6:
>> +             newline.bDataBits = 6;
>> +             break;
I should probably just remove CS5 and CS6 since I don't think they
will work anyway.
>> +     case CS7:
>> +             newline.bDataBits = 7;
>> +             break;
>> +     case CS8:
>> +     default:
>> +             newline.bDataBits = 8;
>> +             break;
>> +     }
>> +     priv->clocal = ((termios->c_cflag & CLOCAL) != 0);
>
> Not needed.
Okay
>
>> +
>> +     if (C_BAUD(tty) == B0) {
>> +             newline.dwDTERate = priv->line.dwDTERate;
>> +             newctrl &= ~NT124_CTRL_DTR;
>> +     } else if (termios_old && (termios_old->c_cflag & CBAUD) == B0) {
>> +             newctrl |=  NT124_CTRL_DTR;
>> +     }
>
> You need to raise or lower RTS here as well.
Oops, I missed that.
>
> And you might need to take flow control into account as well (e.g. see
> mxuport driver).
Okay
>
>> +
>> +     if (newctrl != priv->ctrlout)
>> +             nt124_set_control(port, priv->ctrlout = newctrl);
>
> No assignments in argument lists.
>
> Locking.
Okay
>
>> +
>> +     if (memcmp(&priv->line, &newline, sizeof(newline))) {
>> +             memcpy(&priv->line, &newline, sizeof(newline));
>> +             dev_dbg(&port->dev, "%s - set line: %d %d %d %d\n",
>> +                     __func__,
>
> %u
Okay
>
>> +                     le32_to_cpu(newline.dwDTERate),
>> +                     newline.bCharFormat, newline.bParityType,
>> +                     newline.bDataBits);
>> +             nt124_set_line(port, &priv->line);
>> +     }
>> +
>> +     if (termios->c_cflag & CRTSCTS && !priv->flowctrl) {
>
> C_CRTSCTS(tty), just compare with old termios for changes, no need to
> store flowctrl.
Okay
>
>> +             priv->flowctrl = 1;
>> +             nt124_set_flowctrl(port, priv->flowctrl);
>> +     } else if (!(termios->c_cflag & CRTSCTS) && priv->flowctrl) {
>> +             priv->flowctrl = 0;
>> +             nt124_set_flowctrl(port, priv->flowctrl);
>> +     }
>> +}
>> +
>> +static void nt124_write_bulk_callback(struct urb *urb)
>> +{
>> +     unsigned long flags;
>> +     struct usb_serial_port *port = urb->context;
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +     int i;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i) {
>> +             if (port->write_urbs[i] == urb)
>> +                     break;
>> +     }
>> +     spin_lock_irqsave(&port->lock, flags);
>> +     port->tx_bytes -= urb->transfer_buffer_length;
>> +     if (!urb->status)
>> +             priv->serial_transmit = 1;
>
> Why do you do this?
>
> This functions is just a copy of usb_serial_generic_write_bulk_callback
> apart from this, and you probably don't need it.
I'll remove this function and switch to using tx_empty.
>
>> +     set_bit(i, &port->write_urbs_free);
>> +     spin_unlock_irqrestore(&port->lock, flags);
>> +
>> +     switch (urb->status) {
>> +     case 0:
>> +             break;
>> +     case -ENOENT:
>> +     case -ECONNRESET:
>> +     case -ESHUTDOWN:
>> +             dev_dbg(&port->dev, "%s - urb stopped: %d\n",
>> +                                                     __func__, urb->status);
>> +             return;
>> +     case -EPIPE:
>> +             dev_err_console(port, "%s - urb stopped: %d\n",
>> +                                                     __func__, urb->status);
>> +             return;
>> +     default:
>> +             dev_err_console(port, "%s - nonzero urb status: %d\n",
>> +                                                     __func__, urb->status);
>> +             goto resubmit;
>> +     }
>> +
>> +resubmit:
>> +     usb_serial_generic_write_start(port, GFP_ATOMIC);
>> +     usb_serial_port_softint(port);
>> +}
>> +
>> +static int nt124_chars_in_buffer(struct tty_struct *tty)
>> +{
>> +     struct usb_serial_port *port = tty->driver_data;
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +     unsigned long flags;
>> +     int chars;
>> +
>> +     if (!port->bulk_out_size)
>> +             return 0;
>> +
>> +     spin_lock_irqsave(&port->lock, flags);
>> +     chars = kfifo_len(&port->write_fifo) + port->tx_bytes +
>> +             priv->serial_transmit;
>
> This is not correct.
>
> Implement the tx_empty() callback instead, and use the generic
> chars_in_buffer implementation.
I'll remove this function and switch to using tx_empty.
>
>> +     spin_unlock_irqrestore(&port->lock, flags);
>> +
>> +     dev_dbg(&port->dev, "%s - returns %d\n", __func__, chars);
>> +     return chars;
>> +}
>> +
>> +static void nt124_break_ctl(struct tty_struct *tty, int state)
>> +{
>> +     struct usb_serial_port *port = tty->driver_data;
>> +     int retval;
>> +
>> +     retval = nt124_send_break(port, state ? 0xffff : 0);
>
> No ?:
Okay
>
> Use defines for 0xffff and 0 (e.g. break on and off).
Okay
>
>> +     if (retval < 0)
>> +             dev_dbg(&port->dev, "%s - send break failed\n", __func__);
>
> dev_err
Okay
>
>> +}
>> +
>> +static int nt124_open(struct tty_struct *tty,
>> +                   struct usb_serial_port *port)
>> +{
>> +     struct nt124_private *priv = usb_get_serial_port_data(port);
>> +     int result = 0;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&port->lock, flags);
>> +     port->throttled = 0;
>> +     port->throttle_req = 0;
>> +     spin_unlock_irqrestore(&port->lock, flags);
>> +
>> +     priv->flowctrl = 0;
>
> Drop this and keep the current settings.
Okay
>
>> +     nt124_set_termios(tty, port, NULL);
>> +     nt124_set_flowctrl(port, priv->flowctrl);
>
> Drop this. This is handled by tty and dtr_rts().
Okay
>
>> +
>> +     if (port->bulk_in_size)
>> +             result = usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
>
> Call usb_serial_generic_open() instead (and skip the throttle-flags bit
> above).
Okay, so looks like the only thing I will do in this function is call
nt124_set_termios() followed by usb_serial_generic_open().
>
>> +
>> +     return result;
>> +}
>> +
>> +static int nt124_port_probe(struct usb_serial_port *port)
>> +{
>> +     struct usb_interface *interface = port->serial->interface;
>> +     struct usb_host_interface *cur_altsetting = interface->cur_altsetting;
>> +     struct nt124_private *priv;
>> +
>> +     priv = devm_kzalloc(&port->dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +
>> +     priv->bInterfaceNumber = cur_altsetting->desc.bInterfaceNumber;
>> +
>> +     usb_set_serial_port_data(port, priv);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct usb_serial_driver nt124_device = {
>> +     .driver = {
>> +             .owner =        THIS_MODULE,
>> +             .name =         "nt124",
>> +     },
>> +     .id_table =             id_table,
>> +     .num_ports =            1,
>> +     .bulk_in_size =         32,
>> +     .bulk_out_size =        32,
>
> Why do you reduce these buffer sizes? They can be bigger than the
> endpoint size for increased throughput.
I'll leave them out like most of the other drivers (and retest) unless
you have another suggestion.
>
>> +     .open =                 nt124_open,
>> +     .process_read_urb =     nt124_process_read_urb,
>> +     .write_bulk_callback =  nt124_write_bulk_callback,
>> +     .chars_in_buffer =      nt124_chars_in_buffer,
>> +     .throttle =             usb_serial_generic_throttle,
>> +     .unthrottle =           usb_serial_generic_unthrottle,
>> +     .set_termios =          nt124_set_termios,
>> +     .tiocmget =             nt124_tiocmget,
>> +     .tiocmset =             nt124_tiocmset,
>> +     .dtr_rts =              nt124_dtr_rts,
>> +     .break_ctl =            nt124_break_ctl,
>> +     .port_probe =           nt124_port_probe,
>> +};
>> +
>> +static struct usb_serial_driver * const serial_drivers[] = {
>> +     &nt124_device, NULL
>> +};
>> +
>> +module_usb_serial_driver(serial_drivers, id_table);
>> +
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_LICENSE("GPL");
>
> Thanks,
> Johan

-George
--
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ