linux-kernel - Re: [PATCH 1/2] serial: sh-sci: Fix early deassertion of dedicated RTS

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8834208.BLCpAM3tlx@avalon>
Date:   Fri, 06 Jan 2017 14:30:36 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Geert Uytterhoeven <geert+renesas@...der.be>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Christoph Baumann <cbaumann@...teon.com>,
        linux-serial@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] serial: sh-sci: Fix early deassertion of dedicated RTS

Hi Geert,

Thank you for the patch.

On Friday 02 Dec 2016 13:35:10 Geert Uytterhoeven wrote:
> If a UART has dedicated RTS/CTS pins, there are some issues:
> 
> 1. When changing hardware control flow, the new AUTORTS state is not
>    immediately reflected in the hardware, but only when RTS is raised.
>    However, the serial core doesn't call .set_mctrl() after
>    .set_termios(), hence AUTORTS may only become effective when the port
>    is closed, and reopened later.
>    Note that this problem does not happen when manually using stty to
>    change CRTSCTS, as AUTORTS will work fine on next open.
> 
> 2. When hardware control flow is disabled (or AUTORTS is not yet
>    effective), changing any serial port configuration deasserts RTS, as
>    .set_termios() calls sci_init_pins().

Isn't this still a problem with this patch applied ? Calling sci_set_mctrl() 
should reconfigure the pins properly, but won't there be a short window during 
which the configuration will be wrong ?

> To fix both issues, call .set_mctrl() from .set_termios() when dedicated
> RTS/CTS pins are present, to refresh the AUTORTS or RTS state.
> This is similar to what other drivers supporting AUTORTS do (e.g.
> omap-serial).
> 
> Reported-by: Baumann, Christoph (C.) <cbaumann@...teon.com> (issue 1)
> Fixes: 33f50ffc253854cf ("serial: sh-sci: Fix support for hardware-assisted
> RTS/CTS")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> ---
> Tested on r8a7791/koelsch with HSCIF1 (GPIO hardware flow control),
> and HSCIF2 and SCIFB0 (dedicated hardware flow control).
> 
> A simple test program (basically "cat" with CRTSCTS configuration
> capability) can be found at https://github.com/geertu/sercat
> 
> Without this patch the following will fail:
> 
>   1. sercat -f /dev/hscif2 &
>      seq 100 120 | sercat -f -w /dev/hscif1 # hangs
> 
>   2. seq 200 220 | sercat -f -w /dev/hscif2 &
>      sercat -f /dev/hscif1 # no data received
> ---
>  drivers/tty/serial/sh-sci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 91e7dddbf72cd3de..c503db1900f003ed 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2340,6 +2340,10 @@ static void sci_set_termios(struct uart_port *port,
> struct ktermios *termios,
> 
>  		serial_port_out(port, SCFCR, ctrl);
>  	}
> +	if (port->flags & UPF_HARD_FLOW) {
> +		/* Refresh (Auto) RTS */
> +		sci_set_mctrl(port, port->mctrl);
> +	}
> 
>  	scr_val |= s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0);
>  	dev_dbg(port->dev, "SCSCR 0x%x\n", scr_val);

-- 
Regards,

Laurent Pinchart