| 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
| ||
|
Message-ID: <20150119112904.GQ30960@localhost>
Date: Mon, 19 Jan 2015 12:29:04 +0100
From: Johan Hovold <johan@...nel.org>
To: Peter Hung <hpeter@...il.com>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
tom_tsai@...tek.com.tw, peter_hong@...tek.com.tw,
Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH 1/1] usb: serial: Fintek F81232 driver improvement
On Mon, Jan 19, 2015 at 09:54:55AM +0800, Peter Hung wrote:
> The original driver completed with TX function, but RX/MSR/MCR/LSR is not
> workable with this driver. So we rewrite it to make this device workable.
>
> This patch is tested with PassMark BurnInTest with Cycle-to-115200 +
> MCR/MSR check for 15mins & checked with Suspend-To-RAM/DISK
>
> Signed-off-by: Peter Hung <hpeter+linux_kernel@...il.com>
Thanks for the patch.
You need to break these changes up into several patches adding the
various features and submit it as a series. The rule of thumb is one
self-contained, logical change per patch (e.g. "fix x", "refactor y",
"add function z").
A few initial comments follow inline below.
> ---
> drivers/usb/serial/f81232.c | 528 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 440 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index c5dc233..5ae6bc9 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,9 +23,16 @@
> #include <linux/uaccess.h>
> #include <linux/usb.h>
> #include <linux/usb/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/version.h>
> +
> +#define FINTEK_MAGIC 'F'
> +#define FINTEK_GET_ID _IOR(FINTEK_MAGIC, 3, int)
Adding new ioctls is hardly ever accepted, and definitely not for
retrieving static information that is already provided through sysfs
(idVendor, idProduct).
> +#define FINTEK_VENDOR_ID 0x1934
> +#define FINTEK_DEVICE_ID 0x0706 /* RS232 1 port */
>
> static const struct usb_device_id id_table[] = {
> - { USB_DEVICE(0x1934, 0x0706) },
> + { USB_DEVICE(FINTEK_VENDOR_ID, FINTEK_DEVICE_ID) },
So just drop these changes.
> { } /* Terminating entry */
> };
> MODULE_DEVICE_TABLE(usb, id_table);
> @@ -37,30 +44,257 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define UART_STATE_TRANSIENT_MASK 0x74
> #define UART_DCD 0x01
> #define UART_DSR 0x02
> -#define UART_BREAK_ERROR 0x04
> #define UART_RING 0x08
> -#define UART_FRAME_ERROR 0x10
> -#define UART_PARITY_ERROR 0x20
> -#define UART_OVERRUN_ERROR 0x40
> #define UART_CTS 0x80
>
> +
> +#define UART_BREAK_ERROR 0x10
> +#define UART_FRAME_ERROR 0x08
> +#define UART_PARITY_ERROR 0x04
> +#define UART_OVERRUN_ERROR 0x02
> +
> +
> +#define SERIAL_EVEN_PARITY (UART_LCR_PARITY | UART_LCR_EPAR)
> +
> +
> +#define REGISTER_REQUEST 0xA0
> +#define F81232_USB_TIMEOUT 1000
> +#define F81232_USB_RETRY 20
> +
> +
> +#define SERIAL_BASE_ADDRESS ((__u16)0x0120)
> +#define RECEIVE_BUFFER_REGISTER ((__u16)(0x00) + SERIAL_BASE_ADDRESS)
> +#define TRANSMIT_HOLDING_REGISTER ((__u16)(0x00) + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_ENABLE_REGISTER ((__u16)(0x01) + SERIAL_BASE_ADDRESS)
> +#define INTERRUPT_IDENT_REGISTER ((__u16)(0x02) + SERIAL_BASE_ADDRESS)
> +#define FIFO_CONTROL_REGISTER ((__u16)(0x02) + SERIAL_BASE_ADDRESS)
> +#define LINE_CONTROL_REGISTER ((__u16)(0x03) + SERIAL_BASE_ADDRESS)
> +#define MODEM_CONTROL_REGISTER ((__u16)(0x04) + SERIAL_BASE_ADDRESS)
> +#define LINE_STATUS_REGISTER ((__u16)(0x05) + SERIAL_BASE_ADDRESS)
> +#define MODEM_STATUS_REGISTER ((__u16)(0x06) + SERIAL_BASE_ADDRESS)
No need for casts.
> +
> +static int m_enable_debug;
> +
> +module_param(m_enable_debug, int, S_IRUGO);
> +MODULE_PARM_DESC(m_enable_debug, "Debugging mode enabled or not");
Don't add module parameters, use dynamic debugging.
> +
> +#define LOG_MESSAGE(x, y, ...) \
> + printk(x y, ##__VA_ARGS__)
> +
> +#define LOG_DEBUG_MESSAGE(level, y, ...) \
> + do { if (unlikely(m_enable_debug)) \
> + printk(level y, ##__VA_ARGS__); } while (0)
Don't add your own debug macros, use dev_dbg.
> +
> +
> struct f81232_private {
> spinlock_t lock;
> - u8 line_control;
> - u8 line_status;
> + u8 modem_control;
> + u8 modem_status;
> + struct usb_device *dev;
> +
> + struct work_struct int_worker;
> + struct usb_serial_port *port;
> };
>
> -static void f81232_update_line_status(struct usb_serial_port *port,
> - unsigned char *data,
> - unsigned int actual_length)
> +
> +static inline int calc_baud_divisor(u32 baudrate)
> {
> - /*
> - * FIXME: Update port->icount, and call
> - *
> - * wake_up_interruptible(&port->port.delta_msr_wait);
> - *
> - * on MSR changes.
> - */
> + u32 divisor, rem;
> +
> + divisor = 115200L / baudrate;
> + rem = 115200L % baudrate;
> +
> + /* Round to nearest divisor */
> + if (((rem * 2) >= baudrate) && (baudrate != 110))
> + divisor++;
> +
> + return divisor;
> +}
> +
> +
> +static inline int f81232_get_register(struct usb_device *dev,
> + u16 reg, u8 *data)
> +{
> + int status;
> + int i = 0;
> +timeout_get_repeat:
> +
> + status = usb_control_msg(dev,
> + usb_rcvctrlpipe(dev, 0),
> + REGISTER_REQUEST,
> + 0xc0,
Avoid magic constants, use defines with descriptive names.
> + reg,
> + 0,
> + data,
> + sizeof(*data),
> + F81232_USB_TIMEOUT);
> + if (status < 0) {
> + i++;
> +
> + if (i < F81232_USB_RETRY) {
> + mdelay(1);
> + goto timeout_get_repeat;
Why do you need to retry? You should probably just fail, otherwise
implement this a proper loop.
> + }
> + }
> + return status;
> +}
> +
> +
> +static inline int f81232_set_register(struct usb_device *dev,
> + u16 reg, u8 data)
> +{
> + int status;
> + int i = 0;
> +
> +timeout_set_repeat:
> + status = 0;
> +
> + status = usb_control_msg(dev,
> + usb_sndctrlpipe(dev, 0),
> + REGISTER_REQUEST,
> + 0x40,
> + reg,
> + 0,
> + &data,
> + 1,
> + F81232_USB_TIMEOUT);
> +
> + if (status < 0) {
> + i++;
> + if (i < F81232_USB_RETRY) {
> + mdelay(1);
> + goto timeout_set_repeat;
Same as above.
> + }
> + }
> +
> + return status;
> +}
[...]
> -static void f81232_process_read_urb(struct urb *urb)
> +static void f81232_read_bulk_callback(struct urb *urb)
Why are you renaming this function (hint: you shouldn't).
> {
> struct usb_serial_port *port = urb->context;
> - struct f81232_private *priv = usb_get_serial_port_data(port);
> unsigned char *data = urb->transfer_buffer;
> char tty_flag = TTY_NORMAL;
> - unsigned long flags;
> - u8 line_status;
> + u8 line_status = 0;
> int i;
>
> - /* update line status */
> - spin_lock_irqsave(&priv->lock, flags);
> - line_status = priv->line_status;
> - priv->line_status &= ~UART_STATE_TRANSIENT_MASK;
> - spin_unlock_irqrestore(&priv->lock, flags);
>
> if (!urb->actual_length)
> return;
>
> /* break takes precedence over parity, */
> /* which takes precedence over framing errors */
> +
> +#if 0
> if (line_status & UART_BREAK_ERROR)
> tty_flag = TTY_BREAK;
> else if (line_status & UART_PARITY_ERROR)
> @@ -129,28 +358,22 @@ static void f81232_process_read_urb(struct urb *urb)
> else if (line_status & UART_FRAME_ERROR)
> tty_flag = TTY_FRAME;
> dev_dbg(&port->dev, "%s - tty_flag = %d\n", __func__, tty_flag);
> +#endif
Either remove or fix code, don't keep it unless used.
> - /* overrun is special, not associated with a char */
> - if (line_status & UART_OVERRUN_ERROR)
> - tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> + if (urb->actual_length >= 2) {
>
> - if (port->port.console && port->sysrq) {
> - for (i = 0; i < urb->actual_length; ++i)
> - if (!usb_serial_handle_sysrq_char(port, data[i]))
> - tty_insert_flip_char(&port->port, data[i],
> - tty_flag);
> - } else {
> - tty_insert_flip_string_fixed_flag(&port->port, data, tty_flag,
> - urb->actual_length);
> - }
> + for (i = 0 ; i < urb->actual_length ; i += 2) {
> + line_status |= data[i+0];
> + tty_insert_flip_string_fixed_flag(&port->port,
> + &data[i+1], tty_flag, 1);
> + }
>
> - tty_flip_buffer_push(&port->port);
> -}
> + if (unlikely(line_status & UART_OVERRUN_ERROR))
> + tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> +
> + tty_flip_buffer_push(&port->port);
> + }
>
> -static int set_control_lines(struct usb_device *dev, u8 value)
> -{
> - /* FIXME - Stubbed out for now */
> - return 0;
> }
>
> static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> @@ -165,30 +388,117 @@ static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> }
>
> static void f81232_set_termios(struct tty_struct *tty,
> - struct usb_serial_port *port, struct ktermios *old_termios)
> + struct usb_serial_port *port,
> + struct ktermios *old_termios)
> {
> - /* FIXME - Stubbed out for now */
> + u16 divisor;
> + u16 new_lcr = 0;
> +/*
> +The following comment is for legacy 3.7.0- kernel, You
> +can uncomment and build it if toy need
> +*/
>
> - /* Don't change anything if nothing has changed */
> - if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> - return;
> +/*
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 7, 0)
> + struct ktermios *termios = &tty->termios;
> +#else
> + struct ktermios *termios = tty->termios;
> +#endif
> +*/
We don't want this. Don't use conditional compilation, and especially
not support older kernels like this.
> + struct ktermios *termios = &tty->termios;
> +
> + unsigned int cflag = termios->c_cflag;
> + int status;
> +
> + struct usb_device *dev = port->serial->dev;
> +
> + divisor = calc_baud_divisor(tty_get_baud_rate(tty));
> +
> + status = f81232_set_register(dev, LINE_CONTROL_REGISTER,
> + UART_LCR_DLAB); /* DLAB */
> + mdelay(1);
Why are you adding these delays?
> + status = f81232_set_register(dev, RECEIVE_BUFFER_REGISTER,
> + divisor & 0x00ff); /* low */
> + mdelay(1);
> + status = f81232_set_register(dev, INTERRUPT_ENABLE_REGISTER,
> + (divisor & 0xff00) >> 8); /* high */
> + mdelay(1);
> + status = f81232_set_register(dev, LINE_CONTROL_REGISTER, 0x00);
> + mdelay(1);
> +
> + if (cflag & PARENB) {
> + if (cflag & PARODD)
> + new_lcr |= UART_LCR_PARITY; /* odd */
> + else
> + new_lcr |= SERIAL_EVEN_PARITY; /* even */
> + }
> +
> +
> + if (cflag & CSTOPB)
> + new_lcr |= UART_LCR_STOP;
> + else
> + new_lcr &= ~UART_LCR_STOP;
> +
> + switch (cflag & CSIZE) {
> + case CS5:
> + new_lcr |= UART_LCR_WLEN5;
> + break;
> + case CS6:
> + new_lcr |= UART_LCR_WLEN6;
> + break;
> + case CS7:
> + new_lcr |= UART_LCR_WLEN7;
> + break;
> + default:
> + case CS8:
> + new_lcr |= UART_LCR_WLEN8;
> + break;
> + }
> +
> + status |= f81232_set_register(dev, LINE_CONTROL_REGISTER, new_lcr);
> +
> + status |= f81232_set_register(dev, FIFO_CONTROL_REGISTER,
> + 0x87); /* fifo, trigger8 */
> + status |= f81232_set_register(dev,
> + INTERRUPT_ENABLE_REGISTER, 0xf); /* IER */
> +
> + if (status < 0) {
> + LOG_MESSAGE(KERN_INFO,
> + "[Fintek]: LINE_CONTROL_REGISTER set error: %d\n"
> + , status);
> + }
>
> - /* Do the real work here... */
> - if (old_termios)
> - tty_termios_copy_hw(&tty->termios, old_termios);
> }
>
> static int f81232_tiocmget(struct tty_struct *tty)
> {
> - /* FIXME - Stubbed out for now */
> - return 0;
> + int r;
> + struct usb_serial_port *port = tty->driver_data;
> + struct f81232_private *port_priv = usb_get_serial_port_data(port);
> + unsigned long flags;
> +
> + LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_tiocmget in\n");
> + spin_lock_irqsave(&port_priv->lock, flags);
> + r = (port_priv->modem_control & UART_MCR_DTR ? TIOCM_DTR : 0) |
> + (port_priv->modem_control & UART_MCR_RTS ? TIOCM_RTS : 0) |
> + (port_priv->modem_status & UART_MSR_CTS ? TIOCM_CTS : 0) |
> + (port_priv->modem_status & UART_MSR_DCD ? TIOCM_CAR : 0) |
> + (port_priv->modem_status & UART_MSR_RI ? TIOCM_RI : 0) |
> + (port_priv->modem_status & UART_MSR_DSR ? TIOCM_DSR : 0);
> + spin_unlock_irqrestore(&port_priv->lock, flags);
Use a temporary variable for the status.
> + LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_tiocmget out\n");
> + return r;
> }
>
> static int f81232_tiocmset(struct tty_struct *tty,
> - unsigned int set, unsigned int clear)
> + unsigned int set,
> + unsigned int clear)
> {
> - /* FIXME - Stubbed out for now */
> - return 0;
> + struct usb_serial_port *port = tty->driver_data;
> + struct f81232_private *port_priv =
> + usb_get_serial_port_data(port);
> +
> + return update_mctrl(port_priv, set, clear);
> }
>
> static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
> @@ -201,12 +511,14 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>
> result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
> if (result) {
> - dev_err(&port->dev, "%s - failed submitting interrupt urb,"
> - " error %d\n", __func__, result);
> + dev_err(&port->dev,
> + "%s - failed submitting interrupt urb, error %d\n"
> + , __func__, result);
Fix this separately as well.
> return result;
> }
>
> result = usb_serial_generic_open(tty, port);
> +
Don't do random whitespace changes (here or elsewhere).
> if (result) {
> usb_kill_urb(port->interrupt_in_urb);
> return result;
> @@ -217,6 +529,7 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>
> static void f81232_close(struct usb_serial_port *port)
> {
> +
> usb_serial_generic_close(port);
> usb_kill_urb(port->interrupt_in_urb);
> }
> @@ -224,52 +537,89 @@ static void f81232_close(struct usb_serial_port *port)
> static void f81232_dtr_rts(struct usb_serial_port *port, int on)
> {
> struct f81232_private *priv = usb_get_serial_port_data(port);
> - unsigned long flags;
> - u8 control;
>
> - spin_lock_irqsave(&priv->lock, flags);
> - /* Change DTR and RTS */
> if (on)
> - priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
> + update_mctrl(priv, TIOCM_DTR | TIOCM_RTS, 0);
> else
> - priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
> - control = priv->line_control;
> - spin_unlock_irqrestore(&priv->lock, flags);
> - set_control_lines(port->serial->dev, control);
> + update_mctrl(priv, 0, TIOCM_DTR | TIOCM_RTS);
> }
>
> static int f81232_carrier_raised(struct usb_serial_port *port)
> {
> struct f81232_private *priv = usb_get_serial_port_data(port);
> - if (priv->line_status & UART_DCD)
> + unsigned long flags;
> + int modem_status;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + modem_status = priv->modem_status;
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + if (modem_status & UART_DCD)
> return 1;
> return 0;
> }
>
> +static int f81232_get_id(struct usb_serial_port *port, int __user *arg)
> +{
> + /* 0x19340706 */
> + int data = (FINTEK_VENDOR_ID << 16) | FINTEK_DEVICE_ID;
> +
> + if (copy_to_user((int __user *) arg, &data, sizeof(int)))
> + return -EFAULT;
> +
> + return 0;
> +}
So drop this.
> +
> +
> static int f81232_ioctl(struct tty_struct *tty,
> - unsigned int cmd, unsigned long arg)
> + unsigned int cmd,
> + unsigned long arg)
> {
> struct serial_struct ser;
> struct usb_serial_port *port = tty->driver_data;
>
> switch (cmd) {
> case TIOCGSERIAL:
> - memset(&ser, 0, sizeof ser);
> - ser.type = PORT_16654;
> + memset(&ser, 0, sizeof(ser));
> + ser.flags = ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ;
> + ser.xmit_fifo_size = port->bulk_out_size;
> + ser.close_delay = 5*HZ;
> + ser.closing_wait = 30*HZ;
> +
> + ser.type = PORT_16550A;
> ser.line = port->minor;
> ser.port = port->port_number;
> - ser.baud_base = 460800;
> + ser.baud_base = 115200;
>
> - if (copy_to_user((void __user *)arg, &ser, sizeof ser))
> + if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
> return -EFAULT;
>
> return 0;
> +
> + case FINTEK_GET_ID:
> + return f81232_get_id(port, (int __user *)arg);
> +
> default:
> break;
> }
> return -ENOIOCTLCMD;
> }
>
> +
> +
> +
> +static void f81232_int_work_wq(struct work_struct *work)
> +{
> + struct f81232_private *priv =
> + container_of(work, struct f81232_private, int_worker);
> +
> +
> + LOG_DEBUG_MESSAGE(KERN_INFO, "f81232_int_work_wq\n");
> + f81232_read_msr(priv);
> +
> +
> +}
> +
> static int f81232_port_probe(struct usb_serial_port *port)
> {
> struct f81232_private *priv;
> @@ -279,10 +629,12 @@ static int f81232_port_probe(struct usb_serial_port *port)
> return -ENOMEM;
>
> spin_lock_init(&priv->lock);
> + INIT_WORK(&priv->int_worker, f81232_int_work_wq);
>
> usb_set_serial_port_data(port, priv);
>
> - port->port.drain_delay = 256;
> + priv->dev = port->serial->dev;
> + priv->port = port;
No need to store either of these in the private data.
> return 0;
> }
> @@ -304,22 +656,21 @@ static struct usb_serial_driver f81232_device = {
> },
> .id_table = id_table,
> .num_ports = 1,
> - .bulk_in_size = 256,
> - .bulk_out_size = 256,
> + .bulk_in_size = 64,
> + .bulk_out_size = 64,
Why are you reducing the buffer sizes?
> .open = f81232_open,
> .close = f81232_close,
> - .dtr_rts = f81232_dtr_rts,
> + .dtr_rts = f81232_dtr_rts,
Again, don't include random whitespace changes.
> .carrier_raised = f81232_carrier_raised,
> .ioctl = f81232_ioctl,
> .break_ctl = f81232_break_ctl,
> .set_termios = f81232_set_termios,
> .tiocmget = f81232_tiocmget,
> .tiocmset = f81232_tiocmset,
> - .tiocmiwait = usb_serial_generic_tiocmiwait,
> - .process_read_urb = f81232_process_read_urb,
> + .process_read_urb = f81232_read_bulk_callback,
> .read_int_callback = f81232_read_int_callback,
> .port_probe = f81232_port_probe,
> - .port_remove = f81232_port_remove,
> + .port_remove = f81232_port_remove,
Ditto.
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