| 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: <20200918094244.GR24441@localhost>
Date: Fri, 18 Sep 2020 11:42:44 +0200
From: Johan Hovold <johan@...nel.org>
To: Sheng Long Wang <china_shenglong@....com>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
jan.kiszka@...mens.com,
Wang Sheng Long <shenglong.wang.ext@...mens.com>
Subject: Re: [PATCH v4] usb-serial:cp210x: add support to software flow
control
On Wed, Sep 09, 2020 at 10:39:30AM +0800, Sheng Long Wang wrote:
> From: Wang Sheng Long <shenglong.wang.ext@...mens.com>
>
> When data is transmitted between two serial ports,
> the phenomenon of data loss often occurs. The two kinds
> of flow control commonly used in serial communication
> are hardware flow control and software flow control.
>
> In serial communication, If you only use RX/TX/GND Pins, you
> can't do hardware flow. So we often used software flow control
> and prevent data loss. The user sets the software flow control
> through the application program, and the application program
> sets the software flow control mode for the serial port
> chip through the driver.
>
> For the cp210 serial port chip, its driver lacks the
> software flow control setting code, so the user cannot set
> the software flow control function through the application
> program. This adds the missing software flow control.
>
> Signed-off-by: Wang Sheng Long <shenglong.wang.ext@...mens.com>
>
> Changes in v3:
> - fixed code style, It mainly adjusts the code style acccording
> to kernel specification.
>
> Changes in v4:
> - It mainly adjusts the patch based on the last usb-next branch
> of the usb-serial and optimized the relevant code.
"optimize code" is too hand-wavy, please be more specific on what you've
changed.
> ---
> drivers/usb/serial/cp210x.c | 125 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 120 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index d0c05aa8a0d6..bcbf8da99ebb 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -412,6 +412,15 @@ struct cp210x_comm_status {
> u8 bReserved;
> } __packed;
>
> +struct cp210x_special_chars {
> + u8 bEofChar;
> + u8 bErrorChar;
> + u8 bBreakChar;
> + u8 bEventChar;
> + u8 bXonChar;
> + u8 bXoffChar;
> +};
> +
> /*
> * CP210X_PURGE - 16 bits passed in wValue of USB request.
> * SiLabs app note AN571 gives a strange description of the 4 bits:
> @@ -675,6 +684,69 @@ static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
> return result;
> }
>
> +static int cp210x_get_chars(struct usb_serial_port *port, void *buf, int bufsize)
No need to pass in a length here. Just use a pointer to struct
cp210x_special_chars.
> +{
> + struct usb_serial *serial = port->serial;
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmemdup(buf, bufsize, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + result = usb_control_msg(serial->dev,
> + usb_sndctrlpipe(serial->dev, 0),
usb_rcvctrlpipe()
> + CP210X_GET_CHARS, REQTYPE_DEVICE_TO_HOST, 0,
> + port_priv->bInterfaceNumber,
> + dmabuf, bufsize, USB_CTRL_SET_TIMEOUT);
USB_CTRL_GET_TIMEOUT
> +
> + if (result == bufsize) {
> + memcpy(buf, dmabuf, bufsize);
> + result = 0;
> + } else {
> + dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
> + CP210X_GET_CHARS, bufsize, result);
Just spell out "failed to get special chars: %d\n"
> + if (result >= 0)
> + result = -EIO;
> + }
> +
> + kfree(dmabuf);
> +
> + return result;
> +}
> +
> +static int cp210x_set_chars(struct usb_serial_port *port, void *buf, int bufsize)
> +{
Same as above.
> + struct usb_serial *serial = port->serial;
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmemdup(buf, bufsize, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + result = usb_control_msg(serial->dev,
> + usb_sndctrlpipe(serial->dev, 0),
> + CP210X_SET_CHARS, REQTYPE_HOST_TO_INTERFACE, 0,
> + port_priv->bInterfaceNumber,
> + dmabuf, bufsize, USB_CTRL_SET_TIMEOUT);
> +
> + if (result == bufsize) {
> + result = 0;
> + } else {
> + dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
> + CP210X_SET_CHARS, bufsize, result);
"failed to set special chars: %d\n" (and not "get").
> + if (result >= 0)
> + result = -EIO;
> + }
> +
> + kfree(dmabuf);
> +
> + return result;
> +}
> +
> /*
> * Writes any 16-bit CP210X_ register (req) whose value is passed
> * entirely in the wValue field of the USB request.
> @@ -1356,11 +1428,17 @@ static void cp210x_set_termios(struct tty_struct *tty,
> struct usb_serial_port *port, struct ktermios *old_termios)
> {
> struct device *dev = &port->dev;
> - unsigned int cflag, old_cflag;
> + unsigned int cflag, old_cflag, iflag;
> + struct cp210x_special_chars charsres;
s/charsres/special_chars/
> + struct cp210x_flow_ctl flow_ctl;
> u16 bits;
> + int result;
> + u32 ctl_hs;
> + u32 flow_repl;
>
> cflag = tty->termios.c_cflag;
> old_cflag = old_termios->c_cflag;
> + iflag = tty->termios.c_iflag;
>
> if (tty->termios.c_ospeed != old_termios->c_ospeed)
> cp210x_change_speed(tty, port, old_termios);
> @@ -1434,10 +1512,6 @@ static void cp210x_set_termios(struct tty_struct *tty,
> }
>
> if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) {
> - struct cp210x_flow_ctl flow_ctl;
> - u32 ctl_hs;
> - u32 flow_repl;
> -
> cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
> sizeof(flow_ctl));
> ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> @@ -1474,6 +1548,47 @@ static void cp210x_set_termios(struct tty_struct *tty,
> sizeof(flow_ctl));
> }
>
> + if ((iflag & IXOFF) || (iflag & IXON)) {
Try to only do this on changes (of IXON/IXOFF/START_CHAR/STOP_CHAR).
> +
Stray newline.
> + result = cp210x_get_chars(port, &charsres, sizeof(charsres));
> + if (result < 0)
> + dev_err(dev, "failed to get character: %d\n", result);
Error already logged, you shouldn't proceed with set_chars if this
fails. Perhaps use a helper function for settings software flow
control?
> +
> + charsres.bXonChar = START_CHAR(tty);
> + charsres.bXoffChar = STOP_CHAR(tty);
> +
> + result = cp210x_set_chars(port, &charsres, sizeof(charsres));
> + if (result < 0)
> + dev_err(dev, "failed to set character: %d\n", result);
Same here.
> +
> + result = cp210x_read_reg_block(port,
> + CP210X_GET_FLOW,
> + &flow_ctl,
> + sizeof(flow_ctl));
> + if (result)
> + dev_err(dev, "failed to read flow_ctl reg: %d\n", result);
Don't continue updating flow control on errors.
> +
> + flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
> +
> + if (iflag & IXOFF)
> + flow_repl |= CP210X_SERIAL_AUTO_RECEIVE;
> + else
> + flow_repl &= ~CP210X_SERIAL_AUTO_RECEIVE;
> +
> + if (iflag & IXON)
> + flow_repl |= CP210X_SERIAL_AUTO_TRANSMIT;
> + else
> + flow_repl &= ~CP210X_SERIAL_AUTO_TRANSMIT;
> +
> + flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
> + result = cp210x_write_reg_block(port,
> + CP210X_SET_FLOW,
> + &flow_ctl,
> + sizeof(flow_ctl));
> + if (result)
> + dev_err(dev, "failed to write flow_ctl reg: %d\n", result);
> + }
> +
> /*
> * Enable event-insertion mode only if input parity checking is
> * enabled for now.
Finally, this driver is a bit weird in that it retrieves the termios
settings from the device on open. You need to handle IXON/IXOFF there as
well for now I'm afraid.
Johan
Powered by blists - more mailing lists