| 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
| ||
|
Date: Wed, 28 Aug 2019 16:56:47 +0200
From: Johan Hovold <johan@...nel.org>
To: "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
peter_hong@...tek.com.tw,
"Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V1 1/6] USB: serial: f81232: Add F81534A support
On Thu, Jun 06, 2019 at 10:54:11AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The Fintek F81532A/534A/535/536 is USB-to-2/4/8/12 serial ports device.
> It's most same with F81232, the UART device is difference as follow:
> 1. TX/RX bulk size is 128/512bytes
> 2. RX bulk layout change:
> F81232: [LSR(1Byte)+DATA(1Byte)][LSR(1Byte)+DATA(1Byte)]...
> F81534A:[LEN][Data.....][LSR]
>
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@...il.com>
> ---
> drivers/usb/serial/f81232.c | 153 +++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 144 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 43fa1f0716b7..84efcc66aa56 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> * Fintek F81232 USB to serial adaptor driver
> + * Fintek F81532A/534A/535/536 USB to 2/4/8/12 serial adaptor driver
> *
> * Copyright (C) 2012 Greg Kroah-Hartman (gregkh@...uxfoundation.org)
> * Copyright (C) 2012 Linux Foundation
> @@ -22,7 +23,20 @@
> #include <linux/serial_reg.h>
>
> static const struct usb_device_id id_table[] = {
> + /* F81232 */
> { USB_DEVICE(0x1934, 0x0706) },
> +
> + /* F81532A/534A/535/536 */
> + { USB_DEVICE(0x2c42, 0x1602) }, /* In-Box 2 port UART device */
> + { USB_DEVICE(0x2c42, 0x1604) }, /* In-Box 4 port UART device */
> + { USB_DEVICE(0x2c42, 0x1605) }, /* In-Box 8 port UART device */
> + { USB_DEVICE(0x2c42, 0x1606) }, /* In-Box 12 port UART device */
> + { USB_DEVICE(0x2c42, 0x1608) }, /* Non-Flash type */
> +
> + { USB_DEVICE(0x2c42, 0x1632) }, /* 2 port UART device */
> + { USB_DEVICE(0x2c42, 0x1634) }, /* 4 port UART device */
> + { USB_DEVICE(0x2c42, 0x1635) }, /* 8 port UART device */
> + { USB_DEVICE(0x2c42, 0x1636) }, /* 12 port UART device */
> { } /* Terminating entry */
> };
> MODULE_DEVICE_TABLE(usb, id_table);
> @@ -61,15 +75,25 @@ MODULE_DEVICE_TABLE(usb, id_table);
> #define F81232_CLK_14_77_MHZ (BIT(1) | BIT(0))
> #define F81232_CLK_MASK GENMASK(1, 0)
>
> +#define F81534A_MODE_CONF_REG 0x107
> +#define F81534A_TRIGGER_MASK GENMASK(3, 2)
> +#define F81534A_TRIGGER_MULTPILE_4X BIT(3)
> +#define F81534A_FIFO_128BYTE (BIT(1) | BIT(0))
> +
> +#define F81232_F81232_TYPE 1
> +#define F81232_F81534A_TYPE 2
> +
> struct f81232_private {
> struct mutex lock;
> u8 modem_control;
> u8 modem_status;
> u8 shadow_lcr;
> + u8 device_type;
> speed_t baud_base;
> struct work_struct lsr_work;
> struct work_struct interrupt_work;
> struct usb_serial_port *port;
> + void (*process_read_urb)(struct urb *urb);
> };
>
> static u32 const baudrate_table[] = { 115200, 921600, 1152000, 1500000 };
> @@ -376,6 +400,78 @@ static void f81232_process_read_urb(struct urb *urb)
> tty_flip_buffer_push(&port->port);
> }
>
> +static void f81534a_process_read_urb(struct urb *urb)
> +{
> + 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;
> + unsigned int i;
> + u8 lsr;
> + u8 len;
> +
> + if (urb->status) {
> + dev_err(&port->dev, "urb->status: %d\n", urb->status);
Please use proper error messages in English (not C) here and throughout.
But this one isn't needed since it should have been checked by the
completion handler.
> + return;
> + }
> +
> + if (!urb->actual_length) {
> + dev_err(&port->dev, "urb->actual_length == 0\n");
> + return;
> + }
> +
> + len = data[0];
> + if (len != urb->actual_length) {
> + dev_err(&port->dev, "len(%d) != urb->actual_length(%d)\n", len,
> + urb->actual_length);
> + return;
> + }
> +
> + /* bulk-in data: [LEN][Data.....][LSR] */
> + tty_flag = TTY_NORMAL;
> +
> + lsr = data[len - 1];
What if len == 1?
> + if (lsr & UART_LSR_BRK_ERROR_BITS) {
> + if (lsr & UART_LSR_BI) {
> + tty_flag = TTY_BREAK;
> + port->icount.brk++;
> + usb_serial_handle_break(port);
> + } else if (lsr & UART_LSR_PE) {
> + tty_flag = TTY_PARITY;
> + port->icount.parity++;
> + } else if (lsr & UART_LSR_FE) {
> + tty_flag = TTY_FRAME;
> + port->icount.frame++;
> + }
> +
> + if (lsr & UART_LSR_OE) {
> + port->icount.overrun++;
> + schedule_work(&priv->lsr_work);
> + tty_insert_flip_char(&port->port, 0, TTY_OVERRUN);
> + }
> + }
Isn't this something mostly shared with f81232r? Refactor?
> +
> + for (i = 1; i < urb->actual_length - 1; i++) {
> + if (port->port.console && port->sysrq) {
> + if (usb_serial_handle_sysrq_char(port, data[i]))
> + continue;
> + }
> +
> + tty_insert_flip_char(&port->port, data[i], tty_flag);
Use tty_insert_flip_string_fixed_char() when not a console.
> + }
> +
> + tty_flip_buffer_push(&port->port);
> +}
> +
> +static void f81232_read_urb_proxy(struct urb *urb)
> +{
> + struct usb_serial_port *port = urb->context;
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + if (priv->process_read_urb)
> + priv->process_read_urb(urb);
> +}
No, please add a proper usb-serial subdriver (to this file) rather
than reimplement this type abstraction yourself.
> +
> static void f81232_break_ctl(struct tty_struct *tty, int break_state)
> {
> struct usb_serial_port *port = tty->driver_data;
> @@ -487,13 +583,28 @@ static void f81232_set_baudrate(struct tty_struct *tty,
>
> static int f81232_port_enable(struct usb_serial_port *port)
> {
> + struct f81232_private *port_priv = usb_get_serial_port_data(port);
> u8 val;
> int status;
>
> - /* fifo on, trigger8, clear TX/RX*/
> - val = UART_FCR_TRIGGER_8 | UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> - UART_FCR_CLEAR_XMIT;
> + if (port_priv->device_type != F81232_F81232_TYPE) {
> + val = F81534A_TRIGGER_MULTPILE_4X | F81534A_FIFO_128BYTE;
> + status = f81232_set_mask_register(port, F81534A_MODE_CONF_REG,
> + F81534A_TRIGGER_MASK | F81534A_FIFO_128BYTE,
> + val);
> + if (status) {
> + dev_err(&port->dev, "failed to set MODE_CONF_REG: %d\n",
> + status);
> + return status;
> + }
> +
> + val = UART_FCR_TRIGGER_14;
> + } else {
> + val = UART_FCR_TRIGGER_8;
> + }
>
> + /* fifo on, clear TX/RX */
> + val |= UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
> status = f81232_set_register(port, FIFO_CONTROL_REGISTER, val);
> if (status) {
> dev_err(&port->dev, "%s failed to set FCR: %d\n",
> @@ -631,6 +742,16 @@ static int f81232_tiocmset(struct tty_struct *tty,
> return f81232_set_mctrl(port, set, clear);
> }
>
> +static void f81232_detect_device_type(struct usb_serial_port *port)
> +{
> + struct f81232_private *port_priv = usb_get_serial_port_data(port);
> +
> + if (port->serial->dev->descriptor.idProduct == 0x0706)
Missing le16_to_cpu()
Customers cannot set their own device ids?
Not needed with subdriver anyway.
> + port_priv->device_type = F81232_F81232_TYPE;
> + else
> + port_priv->device_type = F81232_F81534A_TYPE;
> +}
> +
> static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
> {
> int result;
> @@ -731,6 +852,7 @@ static void f81232_lsr_worker(struct work_struct *work)
> static int f81232_port_probe(struct usb_serial_port *port)
> {
> struct f81232_private *priv;
> + int status = 0;
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv)
> @@ -741,11 +863,26 @@ static int f81232_port_probe(struct usb_serial_port *port)
> INIT_WORK(&priv->lsr_work, f81232_lsr_worker);
>
> usb_set_serial_port_data(port, priv);
> + f81232_detect_device_type(port);
>
> port->port.drain_delay = 256;
> priv->port = port;
>
> - return 0;
> + switch (priv->device_type) {
> + case F81232_F81534A_TYPE:
> + priv->process_read_urb = f81534a_process_read_urb;
> + break;
> +
> + case F81232_F81232_TYPE:
> + priv->process_read_urb = f81232_process_read_urb;
> + break;
> +
> + default:
> + status = -ENODEV;
> + break;
> + }
> +
> + return status;
> }
>
> static int f81232_port_remove(struct usb_serial_port *port)
> @@ -797,12 +934,10 @@ static int f81232_resume(struct usb_serial *serial)
> static struct usb_serial_driver f81232_device = {
> .driver = {
> .owner = THIS_MODULE,
> - .name = "f81232",
> + .name = "f81232/f81534a",
> },
> .id_table = id_table,
> .num_ports = 1,
> - .bulk_in_size = 256,
> - .bulk_out_size = 256,
Why change this?
> .open = f81232_open,
> .close = f81232_close,
> .dtr_rts = f81232_dtr_rts,
> @@ -813,7 +948,7 @@ static struct usb_serial_driver f81232_device = {
> .tiocmget = f81232_tiocmget,
> .tiocmset = f81232_tiocmset,
> .tiocmiwait = usb_serial_generic_tiocmiwait,
> - .process_read_urb = f81232_process_read_urb,
> + .process_read_urb = f81232_read_urb_proxy,
> .read_int_callback = f81232_read_int_callback,
> .port_probe = f81232_port_probe,
> .port_remove = f81232_port_remove,
> @@ -828,7 +963,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
>
> module_usb_serial_driver(serial_drivers, id_table);
>
> -MODULE_DESCRIPTION("Fintek F81232 USB to serial adaptor driver");
> +MODULE_DESCRIPTION("Fintek F81232/532A/534A/535/536 USB to serial driver");
> MODULE_AUTHOR("Greg Kroah-Hartman <gregkh@...uxfoundation.org>");
> MODULE_AUTHOR("Peter Hong <peter_hong@...tek.com.tw>");
> MODULE_LICENSE("GPL v2");
Johan
Powered by blists - more mailing lists