linux-kernel - Re: [PATCH] Proper RTS control when buffers fill

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 16 Dec 2019 15:28:03 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Brant Merryman <Brant.Merryman@...abs.com>
Cc:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Proper RTS control when buffers fill

First of all, please use the common "USB: serial: cp210x: " prefix for
your changes.

On Mon, Dec 09, 2019 at 07:17:26PM +0000, Brant Merryman wrote:
> Enables usb generic functions for throttle/unthrottle to prevent USB data
> loss. CP210x hardware disables RTS but leaves CTS when in hardware flow
> control mode and port is closed. When re-opening the serial port, if CTS
> is enabled, then RTS must be re-enabled inside the driver.

This took a while to parse, but makes a little more sense after looking
at the code and remembering that cp210x unlike other drivers is fetching
the termios settings from the device on every open rather than simply
reinitialising the device. Perhaps we should change that at some point.  

Please rephrase the above to say "auto-RTS" ("auto-CTS") so it doesn't
sound like we're controlling the state of the CTS input line (which
obviously makes no sense). Same for the comment in the code below.

I also think you should split this in two patches since these are
strictly two distinct issues; the first one fixing the auto-RTS settings
after reopening the port, and the second adding the throttle callbacks
so that the device can tell when the application can't keep up.

> Signed-off-by: Brant Merryman <brant.merryman@...abs.com>
> ---
>  drivers/usb/serial/cp210x.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index f5143eedbc48..fd54181e741b 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -272,6 +272,8 @@ static struct usb_serial_driver cp210x_device = {
>  	.break_ctl		= cp210x_break_ctl,
>  	.set_termios		= cp210x_set_termios,
>  	.tx_empty		= cp210x_tx_empty,
> +	.throttle		= usb_serial_generic_throttle,
> +	.unthrottle		= usb_serial_generic_unthrottle,
>  	.tiocmget		= cp210x_tiocmget,
>  	.tiocmset		= cp210x_tiocmset,
>  	.attach			= cp210x_attach,
> @@ -915,6 +917,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  	u32 baud;
>  	u16 bits;
>  	u32 ctl_hs;
> +	u32 flow_repl;
>  
>  	cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, &baud);
>  
> @@ -1013,8 +1016,24 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  	cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl,
>  			sizeof(flow_ctl));
>  	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> +	flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
> +	/* CP210x hardware disables RTS but leaves CTS when in hardware
> +	 * flow control mode and port is closed.
> +	 * This allows data to flow out, but new data will not come into
> +	 * the port. When re-opening the port, if CTS is enabled, then RTS
> +	 * must be re-enabled. in the driver
> +	 */

This isn't strictly true since we assert RTS in cp210x_dtr_rts() after
open() (but auto-RTS would incorrectly be disabled).

Also can you be a bit more specific here; is it the interface-disable
call in close() that unconditionally sets the RTS mode to "statically
inactive"?

Nits: Please fix the full-stop in the last sentence and use the right
format for multi-line comments, which is:

	/*
	 * blah, blah
	 */

Perhaps move the comment inside the if block (after dev_dbg()) as well.

>  	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
>  		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> +		flow_repl &= ~CP210X_SERIAL_RTS_MASK;
> +		flow_repl |= CP210X_SERIAL_RTS_SHIFT(
> +			CP210X_SERIAL_RTS_FLOW_CTL);

Indent continuation lines at least two tabs further, but here I'd
probably just break the 80-column rule if that's even an issue.

> +
> +		flow_ctl.ulControlHandshake = cpu_to_le32(ctl_hs);

Not needed, you're only changing flow_repl.

> +		flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
> +		cp210x_write_reg_block(port, CP210X_SET_FLOW,
> +			&flow_ctl, sizeof(flow_ctl));

At least two tabs here too.

> +
>  		cflag |= CRTSCTS;
>  	} else {
>  		dev_dbg(dev, "%s - flow control = NONE\n", __func__);

Johan