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 2BA79D729F0 for ; Fri, 29 Nov 2024 17:46:29 +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:Content-Type:MIME-Version: Message-ID:Date:In-Reply-To:Subject:Cc:To:From:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=TnmoU5TP+FLl7Ofb9grKV8LgLXGcc1efbm5QIQsFSMo=; b=gLBlcEVIEA5DHT GbWEAQLWie7I2LzKLrcjQOy1D+vqm6TLXApZmJ4jkbLxO01tXeB/UJ/Jf8xaNqsLrWydFMRPMgmYH EOjSl6St57v6hTrLIO+L+oeLROXElZWxkTbmomDOAJppomfvPwC4vBratqX/d1N8Od/maSVk6++LT 0RHtyKmJFJ8yYBFD/YZZ8j4ansIku+juC6ess1ekUOwA5Hd/zuvP1m8hJLiFVC9OQ2uGki2XlLtmD L8VqSXMMvXG3XppFG0P6DgoswAQS23J7apUZDY78LEDRXPaOZaIu9kH8uPUlw/zZh8lGM9L7O6+jD XTBwejO7Lnmjp0GkL6Rw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tH54I-00000000j5X-42gz; Fri, 29 Nov 2024 17:46:14 +0000 Received: from galois.linutronix.de ([2a0a:51c0:0:12e:550::1]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tH53J-00000000iz4-455P; Fri, 29 Nov 2024 17:45:15 +0000 From: John Ogness DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1732902309; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to; bh=TnmoU5TP+FLl7Ofb9grKV8LgLXGcc1efbm5QIQsFSMo=; b=R6SXnReSrE00cNIZqinb9hlhkaFAFNBbAY6yh/dAegTFNVE0pL5eQbQ2B4o3fAgiQXwmLj zgiUawZ0+OZevVoDPmH8jHUjjdfnVZ+mJMlureG1pyDeEf690m8gh5YRDm7yeGBoZmJftS ealh//fmdxdo/ujPuSkdu2OmuLr2pyO7G+l+SSnBid3mNax/Ip35Rx0Rema56X9WT1owuP pmfcpCxHkGLIYH6LMJR06AkqXM7W39LG0LnKlSk3LIC1Z4U2LJuYoBQyvlnaUIo2DNtozq LXeiConGjuzppjpCKHkgvqAJobrvpM5TTbXy4hCRfH03zbLd7aIdNrbInIs+rg== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1732902309; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to; bh=TnmoU5TP+FLl7Ofb9grKV8LgLXGcc1efbm5QIQsFSMo=; b=0MfM48tmlBjwXmDOX2EM6p7s4VVa3Ae0g+yn+nDsey53GY5bLZUZM54/w3a3dah9dd0VYU jODWcyT3qTV2iiBA== To: Petr Mladek Cc: Greg Kroah-Hartman , Jiri Slaby , Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , Esben Haabendal , linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Florian Fainelli , Broadcom internal kernel review list , Ray Jui , Scott Branden , Andy Shevchenko , "Paul E. McKenney" , Arnd Bergmann , Stefan Wahren , Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= , Kevin Hilman , Markus Schneider-Pargmann , Ronald Wahl , Udit Kumar , Griffin Kroah-Hartman , Rengarajan S , Lino Sanfilippo , Serge Semin , linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx In-Reply-To: Date: Fri, 29 Nov 2024 18:51:08 +0106 Message-ID: <84ed2url23.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241129_094514_295250_BB1A062A X-CRM114-Status: GOOD ( 26.40 ) 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 2024-11-06, Petr Mladek wrote: >> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the >> console write callback needs to enable/disable TX. It does this >> by calling the rs485_start/stop_tx() callbacks. However, these >> callbacks will disable/enable interrupts, which is a problem >> for console write, as it must be responsible for >> disabling/enabling interrupts. > > It is not clear to me what exactly is the problem. serial8250_em485_stop_tx() blindly sets the RX interrupt bits in IER, because it assumes they were cleared in serial8250_stop_rx(). This is fine for the driver in general, but it is wrong for the console ->write(), which restores those bits on its own later. > Is the main problem calling pm_runtime*() API because it uses extra > locks and can cause deadlocks? Or is it more complicated? pm_runtime*() is a second issue. In the v1 feeback we talked about it. tglx summarized it well here: https://lore.kernel.org/lkml/8734mbdwrf.ffs@tglx/ as well as explaining the need to split off the console-write code from the generic driver code. > IMHO, it would deserve some explanation. This commit message only talks about the first issue, which is enough to justify the patch. I will add that the callbacks are also not appropriate because they call into the PM code, which is not needed by console ->write() and is even unsafe in some contexts. > IMHO, one thing which makes things comlicated is that > serial8250_em485_start_tx() and serial8250_em485_stop_tx() > are not completely reversible operations. Especially, > the change done by __serial8250_stop_rx_mask_dr() is > not reverted in serial8250_em485_stop_tx(). It makes > things look tricky. But I think that it is beyond the scope > of this patchset to do anything about it. I agree that it is strange that the driver does not unmask DR later. I have now run tests and it seems the use of @read_status_mask is partially broken. I did some historical digging on it... For Linux 1.1.60 [0] the @read_status_mask usage was extended to support "stop listening to incoming characters" (text from the changelog [1]). Looking at that version, it is clear why and how it was used. For Linux 2.1.8 [2], the async handling was reworked, basically reverting the change from 1.1.60. However, that revert forgot the piece that clears the UART_LSR_DR bit in serial8250_stop_rx() (back then called rs_close()). And indeed, if you track the @read_status_mask value today, that bit remains cleared until serial8250_do_set_termios() happens to be called. But it didn't matter that the bit was not set again because that bit was not being evaluated at any call sites. For 4.6, RS485 support was added, but with a bug about re-enabling interrupts. When that bug was fixed [3], the fix did not set the UART_LSR_DR bit in @read_status_mask. Still that was not a problem because at that time, that bit still had no users. For 5.7, support was added to avoid reading characters when throttling. This re-introduced a user of the UART_LSR_DR bit in @read_status_mask. And thus now there _is_ a bug that the bit is not set when starting RX in __do_stop_tx_rs485(). Interestingly enough, the OMAP variant of the 8250 _did_ implement setting the bit when unthrottling [5] (also from the same series). So in summary, I will add a patch to my series that fixes [3] (or is it fixing [4]?) by setting the bit in __do_stop_tx_rs485() when re-enabling the RX interrupts. John [0] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893 [1] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/ChangeLog?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893 [2] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=0f9cac5b27076f801b29a0867868e1bce7310e00 [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=0c66940d584d1aac92f6a78460dc0ba2efd3b7ba [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f19c3f6c8109b8bab000afd35580929958e087a9 [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f4b042a050062b2dec456adfced13d61341939e2