[PATCH 0/2] serial: sh-sci: Assorted flow control fixes

[PATCH 0/2] serial: sh-sci: Assorted flow control fixes

[Geert Uytterhoeven]

	Hi Greg, Jiri,

This patch series fixes two issues related to hardware flow control on
Renesas SCIF UARTs.

Thanks!

Geert Uytterhoeven (2):
  serial: sh-sci: Fix early deassertion of dedicated RTS
  serial: sh-sci: Fix hang in sci_reset()

 drivers/tty/serial/sh-sci.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

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

[Geert Uytterhoeven]

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().

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@visteon.com> (issue 1)
Fixes: 33f50ffc253854cf ("serial: sh-sci: Fix support for hardware-assisted RTS/CTS")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.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);
-- 
1.9.1

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

[Laurent Pinchart]

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@visteon.com> (issue 1)
> Fixes: 33f50ffc253854cf ("serial: sh-sci: Fix support for hardware-assisted
> RTS/CTS")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.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

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

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Jan 6, 2017 at 1:30 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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 ?

You mean in between the calls to sci_init_pins() and sci_set_mctrl()?
I don't think _de_asserting RTS for a few microseconds matters much, would it?
Asserting RTS wrongly would be worse.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

Hi Geert,

On Monday 09 Jan 2017 10:53:55 Geert Uytterhoeven wrote:
> On Fri, Jan 6, 2017 at 1:30 PM, Laurent Pinchart wrote:
> > 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 ?
> 
> You mean in between the calls to sci_init_pins() and sci_set_mctrl()?

That's what I mean, yes.

> I don't think _de_asserting RTS for a few microseconds matters much, would
> it? Asserting RTS wrongly would be worse.

It's not the end of the world, but as you're trying to fix this issue, maybe 
it would be a good time to refactor the code properly and fix the core problem 
(.set_termios() calling sci_init_pins()) rather than working around it :-)

-- 
Regards,

Laurent Pinchart

[PATCH 2/2] serial: sh-sci: Fix hang in sci_reset()

[Geert Uytterhoeven]

When the .set_termios() callback resets the UART, it first waits until
all characters in the transmit FIFO have been transmitted, to prevent a
port configuration change from impacting these characters.

However, if the previous user of the UART had dedicated RTS/CTS hardware
flow control enabled, these characters may have been stuck in the FIFO
due to CTS not being asserted. When the new user opens the port,
.set_termios() is called while transmission is still disabled, leading
to an infinite loop:

    NMI watchdog: BUG: soft lockup - CPU#0 stuck for 22s!

This has been observed with SCIFA (on r8a7740/armadillo) and SCIFB (on
r8a7791/koelsch).

The issue does not seem to happen when using:
  - GPIO RTS/CTS hardware flow control,
  - No hardware flow control,
  - HSCIF (on r8a7791/koelsch).

To fix this, wait for the draining of the transmit FIFO only if
transmission is already enabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
To reproduce:

    stty -echo < /dev/scifb0
    stty crtscts < /dev/scifb0
    echo hello > /dev/scifb0	# returns early (wrote to FIFO)
    echo hello > /dev/scifb0	# hangs

 drivers/tty/serial/sh-sci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index c503db1900f003ed..db5de80c5399a7bd 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2137,9 +2137,11 @@ static void sci_reset(struct uart_port *port)
 	const struct plat_sci_reg *reg;
 	unsigned int status;
 
-	do {
-		status = serial_port_in(port, SCxSR);
-	} while (!(status & SCxSR_TEND(port)));
+	if (serial_port_in(port, SCSCR) & SCSCR_TE) {
+		do {
+			status = serial_port_in(port, SCxSR);
+		} while (!(status & SCxSR_TEND(port)));
+	}
 
 	serial_port_out(port, SCSCR, 0x00);	/* TE=0, RE=0, CKE1=0 */
 
-- 
1.9.1

Re: [PATCH 2/2] serial: sh-sci: Fix hang in sci_reset()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Friday 02 Dec 2016 13:35:11 Geert Uytterhoeven wrote:
> When the .set_termios() callback resets the UART, it first waits until
> all characters in the transmit FIFO have been transmitted, to prevent a
> port configuration change from impacting these characters.
> 
> However, if the previous user of the UART had dedicated RTS/CTS hardware
> flow control enabled, these characters may have been stuck in the FIFO
> due to CTS not being asserted. When the new user opens the port,
> .set_termios() is called while transmission is still disabled, leading
> to an infinite loop:
> 
>     NMI watchdog: BUG: soft lockup - CPU#0 stuck for 22s!
> 
> This has been observed with SCIFA (on r8a7740/armadillo) and SCIFB (on
> r8a7791/koelsch).
> 
> The issue does not seem to happen when using:
>   - GPIO RTS/CTS hardware flow control,
>   - No hardware flow control,
>   - HSCIF (on r8a7791/koelsch).
> 
> To fix this, wait for the draining of the transmit FIFO only if
> transmission is already enabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> To reproduce:
> 
>     stty -echo < /dev/scifb0
>     stty crtscts < /dev/scifb0
>     echo hello > /dev/scifb0	# returns early (wrote to FIFO)
>     echo hello > /dev/scifb0	# hangs
> 
>  drivers/tty/serial/sh-sci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index c503db1900f003ed..db5de80c5399a7bd 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2137,9 +2137,11 @@ static void sci_reset(struct uart_port *port)
>  	const struct plat_sci_reg *reg;
>  	unsigned int status;
> 
> -	do {
> -		status = serial_port_in(port, SCxSR);
> -	} while (!(status & SCxSR_TEND(port)));
> +	if (serial_port_in(port, SCSCR) & SCSCR_TE) {
> +		do {
> +			status = serial_port_in(port, SCxSR);
> +		} while (!(status & SCxSR_TEND(port)));

Won't we still loop forever if the remote side never asserts CTS ?

> +	}
> 
>  	serial_port_out(port, SCSCR, 0x00);	/* TE=0, RE=0, CKE1=0 */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] serial: sh-sci: Fix hang in sci_reset()

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Jan 6, 2017 at 1:31 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Friday 02 Dec 2016 13:35:11 Geert Uytterhoeven wrote:
>> When the .set_termios() callback resets the UART, it first waits until
>> all characters in the transmit FIFO have been transmitted, to prevent a
>> port configuration change from impacting these characters.
>>
>> However, if the previous user of the UART had dedicated RTS/CTS hardware
>> flow control enabled, these characters may have been stuck in the FIFO
>> due to CTS not being asserted. When the new user opens the port,
>> .set_termios() is called while transmission is still disabled, leading
>> to an infinite loop:
>>
>>     NMI watchdog: BUG: soft lockup - CPU#0 stuck for 22s!
>>
>> This has been observed with SCIFA (on r8a7740/armadillo) and SCIFB (on
>> r8a7791/koelsch).
>>
>> The issue does not seem to happen when using:
>>   - GPIO RTS/CTS hardware flow control,
>>   - No hardware flow control,
>>   - HSCIF (on r8a7791/koelsch).
>>
>> To fix this, wait for the draining of the transmit FIFO only if
>> transmission is already enabled.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> To reproduce:
>>
>>     stty -echo < /dev/scifb0
>>     stty crtscts < /dev/scifb0
>>     echo hello > /dev/scifb0  # returns early (wrote to FIFO)
>>     echo hello > /dev/scifb0  # hangs
>>
>>  drivers/tty/serial/sh-sci.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>> index c503db1900f003ed..db5de80c5399a7bd 100644
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -2137,9 +2137,11 @@ static void sci_reset(struct uart_port *port)
>>       const struct plat_sci_reg *reg;
>>       unsigned int status;
>>
>> -     do {
>> -             status = serial_port_in(port, SCxSR);
>> -     } while (!(status & SCxSR_TEND(port)));
>> +     if (serial_port_in(port, SCSCR) & SCSCR_TE) {
>> +             do {
>> +                     status = serial_port_in(port, SCxSR);
>> +             } while (!(status & SCxSR_TEND(port)));
>
> Won't we still loop forever if the remote side never asserts CTS ?

Thanks, you're right.

While my patch fixes the issue for a new user opening the port, I managed
to trigger the issue when changing the UART speed after writing some data
to an otherwise disconnected SCIFB0.

Now, how do other drivers handle this (if they handle it at all)?
As CTS is under control of the remote side, set_termios() may be blocked
for an arbitrary period of time. set_termios() also returns void, so it
cannot return e.g. -ERESTARTSYS from wait_event_interruptible().

Or is it considered a bug for userspace to change the terminal settings
without flushing the output buffer? In that case, we can just drop the
loop, and zap the FIFO regardless.

Thanks for your comments!

>
>> +     }
>>
>>       serial_port_out(port, SCSCR, 0x00);     /* TE=0, RE=0, CKE1=0 */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds