| 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: <5697E03D.9040209@collabora.co.uk>
Date: Thu, 14 Jan 2016 17:51:57 +0000
From: Martyn Welch <martyn.welch@...labora.co.uk>
To: Konstantin Shkolnyy <Konstantin.Shkolnyy@...abs.com>,
Johan Hovold <johan@...nel.org>
CC: "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access
functions for large registers
On 02/01/16 03:12, Konstantin Shkolnyy wrote:
> Change to use new large register access functions instead of
> cp210x_get_config and cp210x_set_config and remove the old functions since
> they are now unused.
>
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy-Re5JQEeQqe8AvxtiuMwx3w@...lic.gmane.org>
> ---
> change in v3: Presented new function addition as a separate patch #1,
> to simplify code review.
>
> drivers/usb/serial/cp210x.c | 137 ++++++++------------------------------------
> 1 file changed, 24 insertions(+), 113 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 1339d77..ce80d5f 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -489,105 +489,6 @@ static int cp210x_write_u32_reg(struct usb_serial_port *port, u8 req, u32 val)
> }
>
> /*
> - * cp210x_get_config
> - * Reads from the CP210x configuration registers
> - * 'size' is specified in bytes.
> - * 'data' is a pointer to a pre-allocated array of integers large
> - * enough to hold 'size' bytes (with 4 bytes to each integer)
> - */
> -static int cp210x_get_config(struct usb_serial_port *port, u8 request,
> - unsigned int *data, int size)
> -{
> - struct usb_serial *serial = port->serial;
> - struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> - __le32 *buf;
> - int result, i, length;
> -
> - /* Number of integers required to contain the array */
> - length = (((size - 1) | 3) + 1) / 4;
> -
> - buf = kcalloc(length, sizeof(__le32), GFP_KERNEL);
> - if (!buf)
> - return -ENOMEM;
> -
> - /* Issue the request, attempting to read 'size' bytes */
> - result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
> - request, REQTYPE_INTERFACE_TO_HOST, 0x0000,
> - port_priv->bInterfaceNumber, buf, size,
> - USB_CTRL_GET_TIMEOUT);
> -
> - /* Convert data into an array of integers */
> - for (i = 0; i < length; i++)
> - data[i] = le32_to_cpu(buf[i]);
> -
> - kfree(buf);
> -
> - if (result != size) {
> - dev_dbg(&port->dev, "%s - Unable to send config request, request=0x%x size=%d result=%d\n",
> - __func__, request, size, result);
> - if (result > 0)
> - result = -EPROTO;
> -
> - return result;
> - }
> -
> - return 0;
> -}
> -
> -/*
> - * cp210x_set_config
> - * Writes to the CP210x configuration registers
> - * Values less than 16 bits wide are sent directly
> - * 'size' is specified in bytes.
> - */
> -static int cp210x_set_config(struct usb_serial_port *port, u8 request,
> - unsigned int *data, int size)
> -{
> - struct usb_serial *serial = port->serial;
> - struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> - __le32 *buf;
> - int result, i, length;
> -
> - /* Number of integers required to contain the array */
> - length = (((size - 1) | 3) + 1) / 4;
> -
> - buf = kmalloc(length * sizeof(__le32), GFP_KERNEL);
> - if (!buf)
> - return -ENOMEM;
> -
> - /* Array of integers into bytes */
> - for (i = 0; i < length; i++)
> - buf[i] = cpu_to_le32(data[i]);
> -
> - if (size > 2) {
> - result = usb_control_msg(serial->dev,
> - usb_sndctrlpipe(serial->dev, 0),
> - request, REQTYPE_HOST_TO_INTERFACE, 0x0000,
> - port_priv->bInterfaceNumber, buf, size,
> - USB_CTRL_SET_TIMEOUT);
> - } else {
> - result = usb_control_msg(serial->dev,
> - usb_sndctrlpipe(serial->dev, 0),
> - request, REQTYPE_HOST_TO_INTERFACE, data[0],
> - port_priv->bInterfaceNumber, NULL, 0,
> - USB_CTRL_SET_TIMEOUT);
> - }
> -
> - kfree(buf);
> -
> - if ((size > 2 && result != size) || result < 0) {
> - dev_dbg(&port->dev, "%s - Unable to send request, request=0x%x size=%d result=%d\n",
> - __func__, request, size, result);
> - if (result > 0)
> - result = -EPROTO;
> -
> - return result;
> - }
> -
> - return 0;
> -}
> -
> -/*
> * Detect CP2108 GET_LINE_CTL bug and activate workaround.
> * Write a known good value 0x800, read it back.
> * If it comes back swapped the bug is detected.
> @@ -786,7 +687,8 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> unsigned int *cflagp, unsigned int *baudp)
> {
> struct device *dev = &port->dev;
> - unsigned int cflag, modem_ctl[4];
> + unsigned int cflag;
> + u8 modem_ctl[16];
> u32 baud;
> u16 bits;
>
> @@ -884,8 +786,9 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> break;
> }
>
> - cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, 16);
> - if (modem_ctl[0] & 0x0008) {
> + cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
> + sizeof(modem_ctl));
> + if (modem_ctl[0] & 8) {
> dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> cflag |= CRTSCTS;
> } else {
> @@ -954,7 +857,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
> struct device *dev = &port->dev;
> unsigned int cflag, old_cflag;
> u16 bits;
> - unsigned int modem_ctl[4];
> + u8 modem_ctl[16];
>
> cflag = tty->termios.c_cflag;
> old_cflag = old_termios->c_cflag;
> @@ -1038,27 +941,35 @@ static void cp210x_set_termios(struct tty_struct *tty,
> }
>
> if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) {
> - cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, 16);
> - dev_dbg(dev, "%s - read modem controls = 0x%.4x 0x%.4x 0x%.4x 0x%.4x\n",
> - __func__, modem_ctl[0], modem_ctl[1],
> - modem_ctl[2], modem_ctl[3]);
> +
> + /* Only bytes 0, 4 and 7 out of first 8 have functional bits */
> +
> + cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl,
> + sizeof(modem_ctl));
> + dev_dbg(dev, "%s - read modem controls = %02x .. .. .. %02x .. .. %02x\n",
> + __func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
>
> if (cflag & CRTSCTS) {
> modem_ctl[0] &= ~0x7B;
> modem_ctl[0] |= 0x09;
> - modem_ctl[1] = 0x80;
> + modem_ctl[4] = 0x80;
> + /* FIXME - why clear reserved bits just read? */
> + modem_ctl[5] = 0;
> + modem_ctl[6] = 0;
> + modem_ctl[7] = 0;
> dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> } else {
> modem_ctl[0] &= ~0x7B;
> modem_ctl[0] |= 0x01;
> - modem_ctl[1] |= 0x40;
> + /* FIXME - OR here istead of assignment looks wrong */
s/istead/instead/
I'm a little unsure about FIXME comments being added rather than the
issue being addressed. If I'm reading this right, then this is the
control for the RTS/CTS lines, could the operation without these bits
being cleared/ORed be checked by using a serial cable (connected to
another serial port) and writing data with and without flow control
enabled through a terminal emulator?
Martyn
> + modem_ctl[4] |= 0x40;
> dev_dbg(dev, "%s - flow control = NONE\n", __func__);
> }
>
> - dev_dbg(dev, "%s - write modem controls = 0x%.4x 0x%.4x 0x%.4x 0x%.4x\n",
> - __func__, modem_ctl[0], modem_ctl[1],
> - modem_ctl[2], modem_ctl[3]);
> - cp210x_set_config(port, CP210X_SET_FLOW, modem_ctl, 16);
> + dev_dbg(dev, "%s - write modem controls = %02x .. .. .. %02x .. .. %02x\n",
> + __func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]);
> + cp210x_write_reg_block(port, CP210X_SET_FLOW, modem_ctl,
> + sizeof(modem_ctl));
> }
>
> }
>
Powered by blists - more mailing lists