| Message ID | 1480682111-9299-3-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Simon Horman |
| Headers | show |
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 */
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
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 */
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(-)