Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 18 Sep 2020 11:42:44 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Sheng Long Wang <china_shenglong@163.com>
Cc:     johan@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        jan.kiszka@siemens.com,
        Wang Sheng Long <shenglong.wang.ext@siemens.com>
Subject: Re: [PATCH v4] usb-serial:cp210x: add support to software flow
 control
Message-ID: <20200918094244.GR24441@localhost>
References: <20200909023931.19530-1-china_shenglong@163.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200909023931.19530-1-china_shenglong@163.com>
Precedence: bulk

On Wed, Sep 09, 2020 at 10:39:30AM +0800, Sheng Long Wang wrote:
> From: Wang Sheng Long <shenglong.wang.ext@siemens.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@siemens.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