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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, May 24, 2024 at 02:12:45PM +0200, Rasmus Villemoes 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>
> ---
> 
> 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 | 50 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 2eb22594960f..35a47f4ab6ed 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,55 @@ 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 (dev_WARN_ONCE(sport->port.dev, sport->tx_state != OFF,
> +			  "unexpected tx_state %d\n", sport->tx_state)) {

Please don't reboot devices that have panic-on-warn enabled for
something like this, as you are handling it, but that didn't help for
those devices that had that option turned on :(

thanks,

greg k-h