[Adding Christoph and Marek] On Tue, Jun 25, 2024 at 3:42 PM Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> > --- > v2: Use dev_warn() instead of dev_WARN_ONCE(). > > v1: https://lore.kernel.org/lkml/20240524121246.1896651-1-linux@xxxxxxxxxxxxxxxxxx/ > > 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 >
