Message-ID: <CAOMZO5AejtxU4hTMWa8PK9duXYAKUGzGm_mmeLuEW=tRk7GSCQ@mail.gmail.com>
Date: Tue, 25 Jun 2024 17:39:54 -0300
From: Fabio Estevam <festevam@...il.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>, 
	Christoph Niedermaier <cniedermaier@...electronics.com>, Marek Vasut <marex@...x.de>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, 
	Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2] serial: imx: ensure RTS signal is not left active
 after shutdown

[Adding Christoph and Marek]

On Tue, Jun 25, 2024 at 3:42 PM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> If a process is killed while writing to a /dev/ttymxc* device in RS485
> mode, we observe that the RTS signal is left high, thus making it
> impossible for other devices to transmit anything.
>
> Moreover, the ->tx_state variable is left in state SEND, which means
> that when one next opens the device and configures baud rate etc., the
> initialization code in imx_uart_set_termios dutifully ensures the RTS
> pin is pulled down, but since ->tx_state is already SEND, the logic in
> imx_uart_start_tx() does not in fact pull the pin high before
> transmitting, so nothing actually gets on the wire on the other side
> of the transceiver. Only when that transmission is allowed to complete
> is the state machine then back in a consistent state.
>
> This is completely reproducible by doing something as simple as
>
>   seq 10000 > /dev/ttymxc0
>
> and hitting ctrl-C, and watching with a logic analyzer.
>
> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
> ---
> v2: Use dev_warn() instead of dev_WARN_ONCE().
>
> v1: https://lore.kernel.org/lkml/20240524121246.1896651-1-linux@rasmusvillemoes.dk/
>
> A screen dump from a logic analyzer can be seen at:
>
>   https://ibb.co/xCcP7Jy
>
> This is on an imx8mp board, with /dev/ttymxc0 and /dev/ttymxc2 both
> configured for rs485 and connected to each other. I'm writing to
> /dev/ttymxc2. This demonstrates both bugs; that RTS is left high when
> a write is interrupted, and that a subsequent write actually fails to
> have RTS high while TX'ing.
>
> I'm not sure what commit to name as a Fixes:. This certainly happens
> on 6.6 and onwards, but I assume the problem exists since the tx_state
> machine was introduced in cb1a60923609 (serial: imx: implement rts
> delaying for rs485), and possibly even before that.
>
>
>  drivers/tty/serial/imx.c | 51 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 2eb22594960f..85c240e8c24e 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1551,6 +1551,7 @@ static void imx_uart_shutdown(struct uart_port *port)
>         struct imx_port *sport = (struct imx_port *)port;
>         unsigned long flags;
>         u32 ucr1, ucr2, ucr4, uts;
> +       int loops;
>
>         if (sport->dma_is_enabled) {
>                 dmaengine_terminate_sync(sport->dma_chan_tx);
> @@ -1613,6 +1614,56 @@ static void imx_uart_shutdown(struct uart_port *port)
>         ucr4 &= ~UCR4_TCEN;
>         imx_uart_writel(sport, ucr4, UCR4);
>
> +       /*
> +        * We have to ensure the tx state machine ends up in OFF. This
> +        * is especially important for rs485 where we must not leave
> +        * the RTS signal high, blocking the bus indefinitely.
> +        *
> +        * All interrupts are now disabled, so imx_uart_stop_tx() will
> +        * no longer be called from imx_uart_transmit_buffer(). It may
> +        * still be called via the hrtimers, and if those are in play,
> +        * we have to honour the delays.
> +        */
> +       if (sport->tx_state == WAIT_AFTER_RTS || sport->tx_state == SEND)
> +               imx_uart_stop_tx(port);
> +
> +       /*
> +        * In many cases (rs232 mode, or if tx_state was
> +        * WAIT_AFTER_RTS, or if tx_state was SEND and there is no
> +        * delay_rts_after_send), this will have moved directly to
> +        * OFF. In rs485 mode, tx_state might already have been
> +        * WAIT_AFTER_SEND and the hrtimer thus already started, or
> +        * the above imx_uart_stop_tx() call could have started it. In
> +        * those cases, we have to wait for the hrtimer to fire and
> +        * complete the transition to OFF.
> +        */
> +       loops = port->rs485.flags & SER_RS485_ENABLED ?
> +               port->rs485.delay_rts_after_send : 0;
> +       while (sport->tx_state != OFF && loops--) {
> +               uart_port_unlock_irqrestore(&sport->port, flags);
> +               msleep(1);
> +               uart_port_lock_irqsave(&sport->port, &flags);
> +       }
> +
> +       if (sport->tx_state != OFF) {
> +               dev_warn(sport->port.dev, "unexpected tx_state %d\n",
> +                        sport->tx_state);
> +               /*
> +                * This machine may be busted, but ensure the RTS
> +                * signal is inactive in order not to block other
> +                * devices.
> +                */
> +               if (port->rs485.flags & SER_RS485_ENABLED) {
> +                       ucr2 = imx_uart_readl(sport, UCR2);
> +                       if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +                               imx_uart_rts_active(sport, &ucr2);
> +                       else
> +                               imx_uart_rts_inactive(sport, &ucr2);
> +                       imx_uart_writel(sport, ucr2, UCR2);
> +               }
> +               sport->tx_state = OFF;
> +       }
> +
>         uart_port_unlock_irqrestore(&sport->port, flags);
>
>         clk_disable_unprepare(sport->clk_per);
> --
> 2.45.2
>

Powered by blists - more mailing lists