| 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: Tue, 25 Feb 2020 10:47:51 +0100
From: Johan Hovold <johan@...nel.org>
To: Joel Stanley <joel@....id.au>
Cc: Johan Hovold <johan@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Ben Hutchings <ben@...adent.org.uk>,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
Carl Karsten <carl@...tdayvideo.com>, Tim Ansell <me@...h.ro>
Subject: Re: [PATCH] USB: serial: Add vizzini driver for Exar USB2 serial
adapters
On Fri, Jan 17, 2020 at 02:41:48PM +1000, Joel Stanley wrote:
> From: Ben Hutchings <ben@...adent.org.uk>
>
> This driver supports the one, two and four port variants of the USB
> serial adapters. These devices are present on the Digilent Atlys FPGA
> board.
>
> This driver was written by Ben in 2015. Minor cleanups and fixes for the
> 1410 by Joel.
>
> Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
> Signed-off-by: Joel Stanley <joel@....id.au>
> ---
> drivers/usb/serial/Kconfig | 9 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/vizzini.c | 370 +++++++++++++++++++++++++++++++++++
> 3 files changed, 380 insertions(+)
> create mode 100644 drivers/usb/serial/vizzini.c
>
> diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
> index 25d7e0c36d38..167992ad1d05 100644
> --- a/drivers/usb/serial/Kconfig
> +++ b/drivers/usb/serial/Kconfig
> @@ -654,4 +654,13 @@ config USB_SERIAL_DEBUG
> To compile this driver as a module, choose M here: the
> module will be called usb-debug.
>
> +config USB_SERIAL_VIZZINI
> + tristate "USB Exar XR21V141x 'Vizzini' Serial Driver"
> + help
> + Say Y here if you want to use the Exar XR21V1410/1412/141
> + USB 2 serial dapters.
typo: adapters
> +
> + To compile this driver as a module, choose M here: the
> + module will be called vizzini.
> +
> endif # USB_SERIAL
> diff --git a/drivers/usb/serial/Makefile b/drivers/usb/serial/Makefile
> index 2d491e434f11..9f65a4863f1d 100644
> --- a/drivers/usb/serial/Makefile
> +++ b/drivers/usb/serial/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_USB_SERIAL_WWAN) += usb_wwan.o
> obj-$(CONFIG_USB_SERIAL_TI) += ti_usb_3410_5052.o
> obj-$(CONFIG_USB_SERIAL_UPD78F0730) += upd78f0730.o
> obj-$(CONFIG_USB_SERIAL_VISOR) += visor.o
> +obj-$(CONFIG_USB_SERIAL_VIZZINI) += vizzini.o
> obj-$(CONFIG_USB_SERIAL_WISHBONE) += wishbone-serial.o
> obj-$(CONFIG_USB_SERIAL_WHITEHEAT) += whiteheat.o
> obj-$(CONFIG_USB_SERIAL_XIRCOM) += keyspan_pda.o
> diff --git a/drivers/usb/serial/vizzini.c b/drivers/usb/serial/vizzini.c
> new file mode 100644
> index 000000000000..f4d1b9a75d8a
> --- /dev/null
> +++ b/drivers/usb/serial/vizzini.c
> @@ -0,0 +1,370 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Based on vizzini driver:
> + *
> + * Copyright (c) 2013 Exar Corporation, Inc.
> + *
> + * Based on USB Serial "Simple" driver
> + *
> + * Copyright (C) 2001-2006,2008,2013 Greg Kroah-Hartman <greg@...ah.com>
> + * Copyright (C) 2005 Arthur Huillet (ahuillet@...rs.sf.net)
> + * Copyright (C) 2005 Thomas Hergenhahn <thomas.hergenhahn@...e.de>
> + * Copyright (C) 2009 Outpost Embedded, LLC
> + * Copyright (C) 2010 Zilogic Systems <code@...ogic.com>
> + * Copyright (C) 2013 Wei Shuai <cpuwolf@...il.com>
> + * Copyright (C) 2013 Linux Foundation
This looked weird, but I understand where it comes from now. With a
calc_num_ports() callback, you can drop this bit (see below).
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/tty.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/usb/serial.h>
> +
> +#define XR_SET_REG 0
> +
> +#define URM_REG_BLOCK 4
> +#define EPLOCALS_REG_BLOCK 0x66
> +
> +#define MEM_EP_LOCALS_SIZE_S 3
> +#define MEM_EP_LOCALS_SIZE (1 << MEM_EP_LOCALS_SIZE_S)
> +
> +#define EP_WIDE_MODE 0x03
> +
> +#define UART_GPIO_MODE 0x01a
> +
> +#define UART_GPIO_MODE_SEL_M 0x7
> +#define UART_GPIO_MODE_SEL_S 0
> +#define UART_GPIO_MODE_SEL 0x007
> +
> +#define UART_GPIO_MODE_SEL_GPIO (0x0 << UART_GPIO_MODE_SEL_S)
> +#define UART_GPIO_MODE_SEL_RTS_CTS (0x1 << UART_GPIO_MODE_SEL_S)
> +#define UART_GPIO_MODE_SEL_DTR_DSR (0x2 << UART_GPIO_MODE_SEL_S)
> +
> +#define UART_ENABLE 0x003
> +#define UART_ENABLE_TX_M 0x1
> +#define UART_ENABLE_TX_S 0
Looks like several of these mask/shift defines are unused.
> +#define UART_ENABLE_TX 0x001
> +#define UART_ENABLE_RX_M 0x1
> +#define UART_ENABLE_RX_S 1
> +#define UART_ENABLE_RX 0x002
> +
> +#define UART_CLOCK_DIVISOR_0 0x004
> +#define UART_CLOCK_DIVISOR_1 0x005
> +#define UART_CLOCK_DIVISOR_2 0x006
> +
> +#define UART_TX_CLOCK_MASK_0 0x007
> +#define UART_TX_CLOCK_MASK_1 0x008
> +
> +#define UART_RX_CLOCK_MASK_0 0x009
> +#define UART_RX_CLOCK_MASK_1 0x00a
> +
> +#define UART_FORMAT 0x00b
> +
> +#define UART_FORMAT_SIZE_M 0xf
> +#define UART_FORMAT_SIZE_S 0
> +#define UART_FORMAT_SIZE 0x00f
Same here.
> +#define UART_FORMAT_SIZE_7 (0x7 << UART_FORMAT_SIZE_S)
> +#define UART_FORMAT_SIZE_8 (0x8 << UART_FORMAT_SIZE_S)
> +#define UART_FORMAT_SIZE_9 (0x9 << UART_FORMAT_SIZE_S)
> +
> +#define UART_FORMAT_PARITY_M 0x7
> +#define UART_FORMAT_PARITY_S 4
> +#define UART_FORMAT_PARITY 0x070
> +
> +#define UART_FORMAT_PARITY_NONE (0x0 << UART_FORMAT_PARITY_S)
> +#define UART_FORMAT_PARITY_ODD (0x1 << UART_FORMAT_PARITY_S)
> +#define UART_FORMAT_PARITY_EVEN (0x2 << UART_FORMAT_PARITY_S)
> +#define UART_FORMAT_PARITY_1 (0x3 << UART_FORMAT_PARITY_S)
> +#define UART_FORMAT_PARITY_0 (0x4 << UART_FORMAT_PARITY_S)
> +
> +#define UART_FORMAT_STOP_M 0x1
> +#define UART_FORMAT_STOP_S 7
> +#define UART_FORMAT_STOP 0x080
> +
> +#define UART_FORMAT_STOP_1 (0x0 << UART_FORMAT_STOP_S)
> +#define UART_FORMAT_STOP_2 (0x1 << UART_FORMAT_STOP_S)
> +
> +#define UART_FLOW 0x00c
> +
> +#define UART_FLOW_MODE_M 0x7
> +#define UART_FLOW_MODE_S 0
> +#define UART_FLOW_MODE 0x007
> +
> +#define UART_FLOW_MODE_NONE (0x0 << UART_FLOW_MODE_S)
> +#define UART_FLOW_MODE_HW (0x1 << UART_FLOW_MODE_S)
> +#define UART_FLOW_MODE_SW (0x2 << UART_FLOW_MODE_S)
> +
> +#define UART_XON_CHAR 0x010
> +#define UART_XOFF_CHAR 0x011
> +
> +#define URM_ENABLE_BASE 0x010
> +#define URM_ENABLE_0 0x010
> +#define URM_ENABLE_0_TX 0x001
> +#define URM_ENABLE_0_RX 0x002
> +
> +struct vizzini_baud_rate {
> + unsigned int tx;
> + unsigned int rx0;
> + unsigned int rx1;
> +};
> +
> +static const struct vizzini_baud_rate vizzini_baud_rates[] = {
> + { 0x000, 0x000, 0x000 },
> + { 0x000, 0x000, 0x000 },
> + { 0x100, 0x000, 0x100 },
> + { 0x020, 0x400, 0x020 },
> + { 0x010, 0x100, 0x010 },
> + { 0x208, 0x040, 0x208 },
> + { 0x104, 0x820, 0x108 },
> + { 0x844, 0x210, 0x884 },
> + { 0x444, 0x110, 0x444 },
> + { 0x122, 0x888, 0x224 },
> + { 0x912, 0x448, 0x924 },
> + { 0x492, 0x248, 0x492 },
> + { 0x252, 0x928, 0x292 },
> + { 0x94A, 0x4A4, 0xA52 },
> + { 0x52A, 0xAA4, 0x54A },
> + { 0xAAA, 0x954, 0x4AA },
> + { 0xAAA, 0x554, 0xAAA },
> + { 0x555, 0xAD4, 0x5AA },
> + { 0xB55, 0xAB4, 0x55A },
> + { 0x6B5, 0x5AC, 0xB56 },
> + { 0x5B5, 0xD6C, 0x6D6 },
> + { 0xB6D, 0xB6A, 0xDB6 },
> + { 0x76D, 0x6DA, 0xBB6 },
> + { 0xEDD, 0xDDA, 0x76E },
> + { 0xDDD, 0xBBA, 0xEEE },
> + { 0x7BB, 0xF7A, 0xDDE },
> + { 0xF7B, 0xEF6, 0x7DE },
> + { 0xDF7, 0xBF6, 0xF7E },
> + { 0x7F7, 0xFEE, 0xEFE },
> + { 0xFDF, 0xFBE, 0x7FE },
> + { 0xF7F, 0xEFE, 0xFFE },
> + { 0xFFF, 0xFFE, 0xFFD },
> +};
> +
> +static int vizzini_set_reg(struct usb_serial_port *port,
> + unsigned int block, unsigned int regnum,
> + unsigned int value)
> +{
> + dev_dbg(&port->serial->dev->dev, "%s 0x%02x:0x%02x = 0x%02x\n",
> + __func__, block, regnum, value);
Use &port->dev instead.
> +
> + return usb_control_msg(port->serial->dev,
> + usb_sndctrlpipe(port->serial->dev, 0),
> + XR_SET_REG,
> + USB_DIR_OUT | USB_TYPE_VENDOR,
> + value, regnum | (block << 8),
> + NULL, 0,
> + 5000);
Use USB_CTRL_SET_TIMEOUT
Report errors?
> +}
> +
> +static void vizzini_disable(struct usb_serial_port *port)
> +{
> + unsigned int block = port->bulk_out_endpointAddress - 1;
You should probably use port->port_number instead of relying on fixed
endpoint addresses which you never verify.
> +
> + vizzini_set_reg(port, block, UART_ENABLE, 0);
> + vizzini_set_reg(port, URM_REG_BLOCK, URM_ENABLE_BASE + block, 0);
> +}
> +
> +static void vizzini_enable(struct usb_serial_port *port)
> +{
> + unsigned int block = port->bulk_out_endpointAddress - 1;
>
> + vizzini_set_reg(port, URM_REG_BLOCK, URM_ENABLE_BASE + block,
> + URM_ENABLE_0_TX);
Could you add comment about why this sequence is necessary, if it really
is?
> + vizzini_set_reg(port, block, UART_ENABLE,
> + UART_ENABLE_TX | UART_ENABLE_RX);
> + vizzini_set_reg(port, URM_REG_BLOCK, URM_ENABLE_BASE + block,
> + URM_ENABLE_0_TX | URM_ENABLE_0_RX);
No error handling?
> +}
> +
> +static void vizzini_set_baud_rate(struct usb_serial_port *port,
> + unsigned int rate)
> +{
> + int block = port->bulk_out_endpointAddress - 1;
> + unsigned int divisor = 48000000 / rate;
Use a define for the clock frequency?
> + unsigned int i = ((32 * 48000000) / rate) & 0x1f;
> + unsigned int tx_mask = vizzini_baud_rates[i].tx;
> + unsigned int rx_mask = ((divisor & 1) ? vizzini_baud_rates[i].rx1 :
> + vizzini_baud_rates[i].rx0);
Don't do non-trivial initialisations at declaration. And please avoid
the ternary operator.
> +
> + dev_dbg(&port->serial->dev->dev,
> + "Setting baud rate to %d: i=%u div=%u tx=%03x rx=%03x\n",
> + rate, i, divisor, tx_mask, rx_mask);
> +
> + vizzini_set_reg(port, block, UART_CLOCK_DIVISOR_0,
> + (divisor >> 0) & 0xff);
> + vizzini_set_reg(port, block, UART_CLOCK_DIVISOR_1,
> + (divisor >> 8) & 0xff);
> + vizzini_set_reg(port, block, UART_CLOCK_DIVISOR_2,
> + (divisor >> 16) & 0xff);
> +
> + vizzini_set_reg(port, block, UART_TX_CLOCK_MASK_0,
> + (tx_mask >> 0) & 0xff);
> + vizzini_set_reg(port, block, UART_TX_CLOCK_MASK_1,
> + (tx_mask >> 8) & 0xff);
> +
> + vizzini_set_reg(port, block, UART_RX_CLOCK_MASK_0,
> + (rx_mask >> 0) & 0xff);
> + vizzini_set_reg(port, block, UART_RX_CLOCK_MASK_1,
> + (rx_mask >> 8) & 0xff);
Error handling?
Can you add some comment about how the masks are used?
> +}
> +
> +static void vizzini_set_termios(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + struct ktermios *termios_old)
> +{
> + unsigned int cflag, block;
> + speed_t rate;
> + unsigned int format_size, format_parity, format_stop, flow, gpio_mode;
One declaration per line please, at least unless they are clearly
related (and stay under 80 cols).
Looks like you can get away with just a fmt variable.
> +
> + cflag = tty->termios.c_cflag;
> +
> + block = port->bulk_out_endpointAddress - 1;
> +
> + vizzini_disable(port);
Do you really need to disable the port?
> +
> + if ((cflag & CSIZE) == CS7)
Use C_CSIZE() and friends.
> + format_size = UART_FORMAT_SIZE_7;
> + else if ((cflag & CSIZE) == CS5)
> + /* Enabling 5-bit mode is really 9-bit mode! */
What does this mean? That you have redefined CS5?
> + format_size = UART_FORMAT_SIZE_9;
> + else
> + format_size = UART_FORMAT_SIZE_8;
You need to report back the settings actually used (update termios).
> +
> + if (cflag & PARENB) {
C_PARENB() etc.
> + if (cflag & PARODD) {
> + if (cflag & CMSPAR)
> + format_parity = UART_FORMAT_PARITY_1;
> + else
> + format_parity = UART_FORMAT_PARITY_ODD;
> + } else {
> + if (cflag & CMSPAR)
> + format_parity = UART_FORMAT_PARITY_0;
> + else
> + format_parity = UART_FORMAT_PARITY_EVEN;
> + }
> + } else {
> + format_parity = UART_FORMAT_PARITY_NONE;
> + }
> +
> + if (cflag & CSTOPB)
> + format_stop = UART_FORMAT_STOP_2;
> + else
> + format_stop = UART_FORMAT_STOP_1;
> +
> + vizzini_set_reg(port, block, UART_FORMAT,
> + format_size | format_parity | format_stop);
Error handling.
> +
> + if (cflag & CRTSCTS) {
> + flow = UART_FLOW_MODE_HW;
> + gpio_mode = UART_GPIO_MODE_SEL_RTS_CTS;
> + } else if (I_IXOFF(tty) || I_IXON(tty)) {
> + unsigned char start_char = START_CHAR(tty);
> + unsigned char stop_char = STOP_CHAR(tty);
> +
> + flow = UART_FLOW_MODE_SW;
> + gpio_mode = UART_GPIO_MODE_SEL_GPIO;
> +
> + vizzini_set_reg(port, block, UART_XON_CHAR, start_char);
> + vizzini_set_reg(port, block, UART_XOFF_CHAR, stop_char);
> + } else {
> + flow = UART_FLOW_MODE_NONE;
> + gpio_mode = UART_GPIO_MODE_SEL_GPIO;
> + }
> +
> + vizzini_set_reg(port, block, UART_FLOW, flow);
> + vizzini_set_reg(port, block, UART_GPIO_MODE, gpio_mode);
> +
> + vizzini_set_reg(port, EPLOCALS_REG_BLOCK,
> + (block * MEM_EP_LOCALS_SIZE) + EP_WIDE_MODE,
> + format_size == UART_FORMAT_SIZE_9);
What's this about?
> +
> + rate = tty_get_baud_rate(tty);
> + if (rate)
> + vizzini_set_baud_rate(port, rate);
> +
> + vizzini_enable(port);
> +}
> +
> +static int vizzini_probe(struct usb_serial *serial,
> + const struct usb_device_id *id)
> +{
> + struct usb_host_interface *iface = serial->interface->cur_altsetting;
> + int i;
> +
> + for (i = 0; i < iface->desc.bNumEndpoints; i++) {
> + if (usb_endpoint_is_bulk_out(&iface->endpoint[i].desc))
> + return 0;
> + }
Set num_bulk_out (and num_bulk_in?) in struct usb_serial_driver and let
core handle this for you, and you can drop probe() altogether.
> +
> + /* No bulk interfaces found */
> + dev_dbg(&serial->dev->dev, "Invalid interface, discarding\n");
> +
> + return -ENODEV;
> +}
> +
> +static const struct usb_device_id id_table[] = {
> + { USB_DEVICE(0x04e2, 0x1410), },
> + { USB_DEVICE(0x04e2, 0x1412), },
> + { USB_DEVICE(0x04e2, 0x1414), },
> + { },
> +};
> +MODULE_DEVICE_TABLE(usb, id_table);
> +
> +static const struct usb_device_id vizzini_1410_id_table[] = {
> + { USB_DEVICE(0x04e2, 0x1410), },
> + { },
> +};
> +static struct usb_serial_driver vizzini_1410_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "vizzini_1410",
> + },
> + .id_table = vizzini_1410_id_table,
> + .num_ports = 1,
> + .set_termios = vizzini_set_termios,
> + .probe = vizzini_probe,
> +};
> +
> +static const struct usb_device_id vizzini_1412_id_table[] = {
> + { USB_DEVICE(0x04e2, 0x1412), },
> + { },
> +};
> +static struct usb_serial_driver vizzini_1412_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "vizzini_1412",
> + },
> + .id_table = vizzini_1412_id_table,
> + .num_ports = 2,
> + .set_termios = vizzini_set_termios,
> +};
> +
> +static const struct usb_device_id vizzini_1414_id_table[] = {
> + { USB_DEVICE(0x04e2, 0x1414), },
> + { },
> +};
> +static struct usb_serial_driver vizzini_1414_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "vizzini_1414",
> + },
> + .id_table = vizzini_1414_id_table,
> + .num_ports = 4,
> + .set_termios = vizzini_set_termios,
> +};
> +
> +static struct usb_serial_driver * const serial_drivers[] = {
> + &vizzini_1410_device,
> + &vizzini_1412_device,
> + &vizzini_1414_device,
> + NULL
> +};
I think you should use a single struct usb_serial_driver for all three
models and implement the calc_num_ports() callback instead.
If you cannot determine the number of ports at runtime, you can encode
it in the device-id table (driver_info).
> +
> +module_usb_serial_driver(serial_drivers, id_table);
> +
> +MODULE_LICENSE("GPL");
Johan
Powered by blists - more mailing lists