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 X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INVALID_DATE_TZ_ABSURD, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 109A1C432BE for ; Thu, 5 Aug 2021 08:26:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DE68060EC0 for ; Thu, 5 Aug 2021 08:26:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239445AbhHEI04 (ORCPT ); Thu, 5 Aug 2021 04:26:56 -0400 Received: from Galois.linutronix.de ([193.142.43.55]:41152 "EHLO galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234559AbhHEI0z (ORCPT ); Thu, 5 Aug 2021 04:26:55 -0400 From: John Ogness DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1628152000; 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:references:references; bh=sDS1DpN+l+aAEkSP0+4dZetYVLkYHfUOpL7O0zwMLwc=; b=MImQslRh8bXrcBZeI2HDBMRTK4c/v9imWShtcDMCrlnPalgFEqSiy2FGo68lC1I7yc2RkE Em8eGKX0qzihnzwENOEW+Oh9efhvYi2oZJf6xdQ139MGwJguEVfmXcMJL2TxERNkPayZBy OtHmI8wIs5nobQLvfZS7PRnMzQtYNnCetnYyIQ5iFpuA6pWaJyOnHOT0oemivFz8+ZGHdk UHNiMhA9S69jhkOCxfC2Yzu79L7UdnADSap+2nvqBlEAyjCKqCIgpBoFQZEEgAoE2qNOOJ SpRxIr/lpvUnfbS992agNZC/gE7b35WvIK5Spo2m7hQw/axnaea8Z6xM7cCfag== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1628152000; 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:references:references; bh=sDS1DpN+l+aAEkSP0+4dZetYVLkYHfUOpL7O0zwMLwc=; b=/S1XK1SD2cNKm5oAne6Zd473q9DfZCgTAnfgMziAxO0ToB6fXXJLqRt7JMYFx4wKuVe2sm 7SczodRzMkLRhxBA== To: Jiri Slaby , Andy Shevchenko Cc: Petr Mladek , Sergey Senozhatsky , Steven Rostedt , Thomas Gleixner , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Paul Cercueil , Matthias Brugger , Andrew Jeffery , Christophe JAILLET , kuldip dwivedi , Wang Qing , Andrij Abyzov , Johan Hovold , Eddie Huang , Claire Chang , Hsin-Yi Wang , Zhang Qilong , "Maciej W. Rozycki" , Guenter Roeck , Sergey Senozhatsky , Serge Semin , "Gustavo A. R. Silva" , Al Cooper , linux-serial@vger.kernel.org, linux-mips@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org Subject: Re: [PATCH printk v1 10/10] serial: 8250: implement write_atomic In-Reply-To: <931ebc1a-3cc9-677c-44c3-7cbd645eb4f4@kernel.org> References: <20210803131301.5588-1-john.ogness@linutronix.de> <20210803131301.5588-11-john.ogness@linutronix.de> <931ebc1a-3cc9-677c-44c3-7cbd645eb4f4@kernel.org> Date: Thu, 05 Aug 2021 10:32:38 +0206 Message-ID: <87k0l0wb41.fsf@jogness.linutronix.de> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021-08-05, Jiri Slaby wrote: > On 03. 08. 21, 16:07, Andy Shevchenko wrote: >> On Tue, Aug 03, 2021 at 03:19:01PM +0206, John Ogness wrote: >>> Implement an NMI-safe write_atomic() console function in order to >>> support synchronous console printing. >>> >>> Since interrupts need to be disabled during transmit, all usage of >>> the IER register is wrapped with access functions that use the >>> printk cpulock to synchronize register access while tracking the >>> state of the interrupts. This is necessary because write_atomic() >>> can be called from an NMI context that has preempted write_atomic(). >> >> ... >> >>> +static inline void serial8250_set_IER(struct uart_8250_port *up, >>> + unsigned char ier) >>> +{ >>> + struct uart_port *port = &up->port; >>> + unsigned long flags; >>> + bool is_console; >> >>> + is_console = uart_console(port); >>> + >>> + if (is_console) >>> + console_atomic_cpu_lock(flags); >>> + >>> + serial_out(up, UART_IER, ier); >>> + >>> + if (is_console) >>> + console_atomic_cpu_unlock(flags); >> >> I would rewrite it as >> >> if (uart_console()) { >> console_atomic_cpu_lock(flags); >> serial_out(up, UART_IER, ier); >> console_atomic_cpu_unlock(flags); >> } else { >> serial_out(up, UART_IER, ier); >> } Some locations have more than just 1 line of code in between lock/unlock. I agree this looks better, but am unsure how much copy/paste code is acceptable. >> No additional variable, easier to get the algorithm on the first >> glance, less error prone. > > Yes, the original is terrible. > > Another option: > > bool locked = console_atomic_cpu_lock(flags, uart_console()); > serial_out(up, UART_IER, ier); > console_atomic_cpu_unlock(flags, locked); > > Which makes console_atomic_cpu_lock to lock only if second parameter > is true and return its value too. I am not sure how common such semantics for lock/unlock functions are. But since this pattern, using uart_console(), will most likely be a common pattern for atomic consoles, I can see how this will be useful. I will choose one of these 2 suggestions for v2. Thanks. > BTW I actually don't know what console_atomic_cpu_lock does to think > about it more as I was not CCed, and neither lore sees the other patches: > https://lore.kernel.org/linux-mips/20210803131301.5588-1-john.ogness@linutronix.de/ Only the lkml mailing list saw the full series: https://lore.kernel.org/lkml/20210803131301.5588-1-john.ogness@linutronix.de/ John Ogness