linux-kernel - Re: [PATCH v3 2/2] USB: serial: cp210x: 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]

Message-ID: <20200706101834.GI3453@localhost>
Date:   Mon, 6 Jul 2020 12:18:34 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Phu Luu <Phu.Luu@...abs.com>
Cc:     "johan@...nel.org" <johan@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brant Merryman <Brant.Merryman@...abs.com>,
        Richard Hendricks <Richard.Hendricks@...abs.com>,
        Phú Lưu An <luuanphu@...il.com>,
        "hungnd3@...ft.com.vn" <hungnd3@...ft.com.vn>
Subject: Re: [PATCH v3 2/2] USB: serial: cp210x: Proper RTS control when
 buffers fill

On Fri, Jun 26, 2020 at 09:42:06AM +0200, Johan Hovold wrote:
> On Fri, Jun 26, 2020 at 04:24:20AM +0000, Phu Luu wrote:
> > From: Brant Merryman <brant.merryman@...abs.com>
> > 
> > CP210x hardware disables auto-RTS but leaves auto-CTS when
> > in hardware flow control mode and UART on cp210x hardware
> > is disabled. This allows data to flow out, but new data
> > will not come into the port. When re-opening the port, if
> > auto-CTS is enabled on the cp210x, then auto-RTS must be
> > re-enabled in the driver.
> > 
> > Signed-off-by: Phu Luu <phu.luu@...abs.com>
> > Signed-off-by: Brant Merryman <brant.merryman@...abs.com>
> 
> Please revisit these tags as well.
> 
> > ---
> > 06/09/2020: Patch v3 2/2 Modified based on feedback from Johan Hovold <johan@...nel.org>
> > 12/18/2019: Patch v2 Broken into two patches and modified based on feedback from Johan Hovold <johan@...nel.org>
> > 12/09/2019: Initial submission of patch "Proper RTS control when buffers fill"
> > 
> >  drivers/usb/serial/cp210x.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> > index bcceb4ad8be0..091441b1c5b9 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -917,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);
> >  
> > @@ -1017,6 +1018,23 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
> >  	ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> >  	if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
> >  		dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__);
> > +		/*
> > +		 * When the port is closed, the CP210x hardware disables
> > +		 * auto-RTS and RTS is deasserted but it leaves auto-CTS when
> > +		 * in hardware flow control mode. This prevents new data from
> > +		 * being received by the port. When re-opening the port, if
> > +		 * auto-CTS is enabled on the cp210x, then auto-RTS must be
> > +		 * re-enabled in the driver.
> > +		 */
> 
> I already asked this of Brant, but could you please be more specific
> about which state the RTS-line end up in when disabling the UART (e.g.
> 0x00: statically inactive)?
> 
> The reason I ask is that after open() returns, the tty layer would raise
> RTS, which should again allow data to flow in in contrast to what the
> comment and changelog text claims.

I had another reason to look at this driver so I could determine that
the patch description and comment were indeed incorrect; the tty layer
asserts RTS when reopening the port just fine and data flows in.

> We still want to enable auto-RTS of course so the patch is otherwise
> correct.

I've fixed up the commit message and comment, reordered the SOB tags
and added your Co-developed-by tag before applying both of these now:

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-next

Johan