From patchwork Fri May 24 12:12:45 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 13673113 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 93ADBC25B74 for ; Fri, 24 May 2024 12:13:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-Id:Date:Subject:Cc :To:From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=WKPh5AqOXKlLCBUqAluErDmOtx1kgJuc//MIIv6ChB8=; b=KGGAQsK5t7taBL T9xp4nPEL7uxj3fOplJ3cIl/RLxbbVmUK+9PbRmHXDytzaJ5k8pRn2Uq+HYl80sTKyQeN12mst9Mq 2NvoVmfHzsmB4OKhPdv8k8wvJPKUl7kmb4pFn1+zfvTtPoMAWRVDJBaz7tifsq5hoMK16RRM769RQ wcM1EDgBwtagh9EfhiT7oaZjjZ2LFvDiO0ez3XBKtXaVyhZYGXnj9uIKeQZiLGMFVxoEg2QdUI8Fz CzUxsEDdUWZ0rT1DI4r33PTWamDKOFZl5OkDMW+IRjONpe8iWfLfKLSorb/csDgDocjQzvw8rXciX od43E8Vzhw/92o2GD4dA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sATn6-00000008q0N-3mRs; Fri, 24 May 2024 12:12:56 +0000 Received: from mail-lf1-x134.google.com ([2a00:1450:4864:20::134]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sATn3-00000008pzo-0Es7 for linux-arm-kernel@lists.infradead.org; Fri, 24 May 2024 12:12:55 +0000 Received: by mail-lf1-x134.google.com with SMTP id 2adb3069b0e04-52327368e59so9802354e87.1 for ; Fri, 24 May 2024 05:12:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; t=1716552770; x=1717157570; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=z/kQluXbrS5/xKa378gCLMmIP4qg7U9JjmWwPqCAy5U=; b=PiSy+ONTwnuT118WqHq2PULTdoDsLu4ipv4fIJBJS6d+GfQ3B+FdZe3cxqgz2Pfksu GAycOZWt1qKdwE4SkDrPuo7cvBWa2qSyCxyaLTSVfLFiogWIfYs11E2a0iJkrZI94+2u wnH2CKQo38ynK1XtfKS+F513ZppSwiL4MsOkU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716552770; x=1717157570; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=z/kQluXbrS5/xKa378gCLMmIP4qg7U9JjmWwPqCAy5U=; b=OdXekembcXBD3QcH6YbAyBkkX+sN8ywR6Y6dzUWTHaXx/RQCbdAS+h+10gV4hLMJRu y+M0YSEkI74rDVWcEt1EDOEUt4OXW+UOQ0Bxvh6TznriBE6ouuqRm6+M/+sZjIuTnjle PRwDwoeuJGG3wP1wGEFoHoNsn2MHyABbXgXmbqxpdXn8SXC57goPXwcgn9U5mYwYU9TL N3X/uiAWC0dIVGoBGCaM6GlCVkj9o5Lb32NKNOTc/3bzrFRUu41RQ02HI7eQryjZNCPP osseBlXQrTlwt4JCW2kG/XVWrDTK7BThZqREExBsoARbipBWplICDRPQrwodN5/mnbU1 W3Lg== X-Forwarded-Encrypted: i=1; AJvYcCXAUG4XoCQH9rfkSKaitkDqhACkP2lHO2fXzN7CDRNnOkBU8Yqi8iPs10dmonZcxablStPKg0YY5d1qOhSFU3NN8hFM0XbFW6zy9GZ7aYueubcPYSg= X-Gm-Message-State: AOJu0Yx4vbWrmjScKeu0Bb+YGTwigPZdHr7Ngr13hq5PWVaSOo6Ffar6 N7oBDdL6MrILti1UYejkcdcJ2063kSSbKZY9ZL1EGgpUtNLbw4sqJ1Uo3Epak+g= X-Google-Smtp-Source: AGHT+IFDzskSYUTxKkoP6SCgQgAYltT2X4VJOr36ZAkA0ExwWVFTGCSdWPvOlsTH+ENT8Hvd6LU7KA== X-Received: by 2002:a05:6512:10ca:b0:51c:b73f:950 with SMTP id 2adb3069b0e04-529661f315emr1807735e87.43.1716552769459; Fri, 24 May 2024 05:12:49 -0700 (PDT) Received: from prevas-ravi.prevas.se ([81.216.59.226]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-52970c3313fsm164559e87.200.2024.05.24.05.12.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 May 2024 05:12:49 -0700 (PDT) From: Rasmus Villemoes To: Greg Kroah-Hartman , Jiri Slaby , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam Cc: =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= , Rasmus Villemoes , linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org Subject: [PATCH] serial: imx: ensure RTS signal is not left active after shutdown Date: Fri, 24 May 2024 14:12:45 +0200 Message-Id: <20240524121246.1896651-1-linux@rasmusvillemoes.dk> X-Mailer: git-send-email 2.40.1.1.g1c60b9335d MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240524_051253_205644_AAB05C9A X-CRM114-Status: GOOD ( 24.04 ) 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 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)) { + /* + * 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);