diff mbox series

[v2] serial: imx: ensure RTS signal is not left active after shutdown

Message ID 20240625184206.508837-1-linux@rasmusvillemoes.dk (mailing list archive)
State New
Headers show
Series [v2] serial: imx: ensure RTS signal is not left active after shutdown | expand

Commit Message

Rasmus Villemoes June 25, 2024, 6:42 p.m. UTC
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@rasmusvillemoes.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(+)

Comments

Fabio Estevam June 25, 2024, 8:39 p.m. UTC | #1
[Adding Christoph and Marek]

On Tue, Jun 25, 2024 at 3:42 PM Rasmus Villemoes
<linux@rasmusvillemoes.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@rasmusvillemoes.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
>
Marek Vasut July 2, 2024, 4:02 a.m. UTC | #2
On 6/25/24 10:39 PM, Fabio Estevam wrote:
> [Adding Christoph and Marek]
> 
> On Tue, Jun 25, 2024 at 3:42 PM Rasmus Villemoes
> <linux@rasmusvillemoes.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@rasmusvillemoes.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.

Wow, thank you for the detailed analysis of the issue.

Reviewed-by: Marek Vasut <marex@denx.de>
diff mbox series

Patch

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);