From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C12E1C2BD09 for ; Mon, 24 Jun 2024 13:33:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=//XF23zEHm/PyWCOR5lCi4S9MpUNUxr4ty7b/m9Hoq4=; b=NYoTb6S704arMRenuysqSwd+Fk SRtluaRRUNajxv9CXupC86ZBz419YM0Grt4961PnerK1vyUBZ5zIJSmFBWTHx6miRKxmIJV+KwfrR 2o3gNvQ86XKug6h8OlaLFPo7KffV4wz56751AyhfFU6bGJ9IEA8d+WyhpDJqOK5jcvJuv6QrWl0Tj EWpnwk27gSUBmmOCOcLw3tdYz6fj45ZNh3BMzscaK1rz/hMIUFJrC9c4MKi9hMkj6ZJgjnUG978Hc 56Ymosm15sz7pLDQuaC+OBs7781fMupPLEElArulPc3SEaHwvXI7VFEyLt5Bii7IdeDbOv/HZycmw TAyXLZSg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sLjpI-0000000GwhH-1COk; Mon, 24 Jun 2024 13:33:44 +0000 Received: from sin.source.kernel.org ([2604:1380:40e1:4800::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sLjpC-0000000Gwev-1wht for linux-arm-kernel@lists.infradead.org; Mon, 24 Jun 2024 13:33:40 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id D8532CE1277; Mon, 24 Jun 2024 13:33:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A801BC2BBFC; Mon, 24 Jun 2024 13:33:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1719236014; bh=9zFosSwVy43COA5LXpYQ2Dm9NEd+eTcGR2GIviXSK5M=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ILygvCUwZtEwOsboz813cE78GyGTV1+Q536rrkhlPHQ5+yLrn2GWhzLTMIuP+Arum U3ndDYX9c8/ks0zT8QwCcVoUCfVeR3VOjK/RMfZmee50qL5/Y/ROePNjN6JkQF7Kbu 4ZIGVuG0AkQKOXMzrX4zEOUvwWnZolPBaO00g44g= Date: Mon, 24 Jun 2024 15:33:31 +0200 From: Greg Kroah-Hartman To: Rasmus Villemoes Cc: Jiri Slaby , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] serial: imx: ensure RTS signal is not left active after shutdown Message-ID: <2024062427-cheating-cloning-3de1@gregkh> References: <20240524121246.1896651-1-linux@rasmusvillemoes.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240524121246.1896651-1-linux@rasmusvillemoes.dk> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240624_063338_884757_E4CC80D5 X-CRM114-Status: GOOD ( 37.26 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.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 > --- > > 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