Received-SPF: pass (google.com: domain of linux-kernel+bounces-78636-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 23 Feb 2024 16:47:53 +0100
From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v2 14/26] printk: nbcon: Provide function to flush
 using write_atomic()
Message-ID: <Zdi-KXfBScocqBR-@alley>
References: <20240218185726.1994771-1-john.ogness@linutronix.de>
 <20240218185726.1994771-15-john.ogness@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240218185726.1994771-15-john.ogness@linutronix.de>

On Sun 2024-02-18 20:03:14, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Provide nbcon_atomic_flush_all() to perform flushing of all
> registered nbcon consoles using their write_atomic() callback.
> Like with legacy consoles, the nbcon consoles are flushed one
> record per console. This allows all nbcon consoles to print
> lines pseudo-simultaneously, rather than one console waiting
> for the full ringbuffer to dump to another console before
> printing anything.
> 
> Unlike console_flush_all(), nbcon_atomic_flush_all() will only
> flush up through the newest record at the time of the call.
> This prevents a CPU from printing unbounded when other CPUs are
> adding records.

I think about using slightly different name to make the difference
more clear, for example nbcon_atomic_flush_pending() or so.

But I do not have a strong opinion.

> Perform nbcon console atomic flushing in
> console_flush_on_panic(). This function is not only used in
> panic() but also other locations where there may be stored
> messages that need to be flushed.

The above paragraph is a bit misleading. console_flush_on_panic()
is used only in panic(). I guess that you wanted to say something
like:

<proposal>
nbcon_atomic_flush_all() is safe in any context because it uses
write_atomic() and unsafe_takeover is disabled.

Use it in console_flush_on_panic() before flushing legacy consoles.
The legacy write() callbacks are not fully safe when oops_in_progress
is set.
</proposal>


> Co-developed-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>

The code looks good. After updating the commit message,
and eventually the function name, feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr