Received-SPF: pass (google.com: domain of linux-kernel+bounces-56639-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 7 Feb 2024 15:43:52 +0100
From: Petr Mladek <pmladek@suse.com>
To: Sreenath Vijayan <sreenath.vijayan@sony.com>
Cc: john.ogness@linutronix.de, corbet@lwn.net, gregkh@linuxfoundation.org,
	jirislaby@kernel.org, rdunlap@infradead.org, rostedt@goodmis.org,
	senozhatsky@chromium.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	taichi.shimoyashiki@sony.com, daniel.palmer@sony.com,
	anandakumar.balasubramaniam@sony.com
Subject: Re: [PATCH v4 1/2] printk: Add function to dump printk buffer
 directly to consoles
Message-ID: <ZcOXEyPsRnfewb4Y@alley>
References: <cover.1706772349.git.sreenath.vijayan@sony.com>
 <8cb5936021c5811bd03a6bc18300b1384009ac26.1706772349.git.sreenath.vijayan@sony.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8cb5936021c5811bd03a6bc18300b1384009ac26.1706772349.git.sreenath.vijayan@sony.com>

On Thu 2024-02-01 15:53:40, Sreenath Vijayan wrote:
> It is useful to be able to dump the printk buffer directly to
> consoles in some situations so as to not flood the buffer.

This is not longer true. I think that it was valid for
the previous versions which used separate buffers with
the kmsg_dump API.

> To do this, we reuse the CONSOLE_REPLAY_ALL mode code in
> console_flush_on_panic() by moving the code to a helper function
> console_rewind_all(). This is done because console_flush_on_panic()
> sets console_may_schedule to 0 but this should not be done in our
> case.

Also the "c->seq = seq;" is not safe in the panic version.
But it will be safe when called under the console_lock.

> Then console_rewind_all() is called from the new function
> dump_printk_buffer() with console lock held to set the console
> sequence number to oldest record in the buffer for all consoles.
> Releasing the console lock will flush the contents of printk buffer
> to the consoles.

My proposed commit message is:

<proposal>
Add a generic function for replaying the kernel log on consoles.
It would allow seeing the the log on an unresponsive terminal
via sysrq interface.

Reuse the existing code from console_flush_on_panic() for
reseting the sequence numbers. It will be safe when called
under console_lock(). Also the console_unlock() will
automatically flush the messages on the consoles.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3134,6 +3134,32 @@ void console_unblank(void)
>  		pr_flush(1000, true);
>  }
>  

I would call this function __console_rewind_all(void)
because it is not safe on its own. Also It would
deserve a comment, something like:

/*
 * Rewind all consoles to the oldest available record.
 *
 * IMPORTANT: The function is safe only when called under
 *            console_lock(). It is not enforced because
 *	      it is used as a best effort in panic().
 */
static void __console_rewind_all(void)


This would deserve a comment because it is not safe by
default.

> +static void console_rewind_all(void)
> +{
> +	struct console *c;
> +	short flags;
> +	int cookie;
> +	u64 seq;
> +
> +	seq = prb_first_valid_seq(prb);
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(c) {
> +		flags = console_srcu_read_flags(c);
> +
> +		if (flags & CON_NBCON) {
> +			nbcon_seq_force(c, seq);
> +		} else {
> +			/*
> +			 * This is an unsynchronized assignment. On
> +			 * panic legacy consoles are only best effort.
> +			 */

We should change this to something like:

			/*
			 * This assigment is safe only when called under
			 * console_lock(). */
			 */

> +			c->seq = seq;
> +		}
> +	}
> +	console_srcu_read_unlock(cookie);
> +}
> +
>  /**
>   * console_flush_on_panic - flush console content on panic
>   * @mode: flush all messages in buffer or just the pending ones
> @@ -3162,30 +3188,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
>  	 */
>  	console_may_schedule = 0;
>  
> -	if (mode == CONSOLE_REPLAY_ALL) {
> -		struct console *c;
> -		short flags;
> -		int cookie;
> -		u64 seq;
> -
> -		seq = prb_first_valid_seq(prb);
> -
> -		cookie = console_srcu_read_lock();
> -		for_each_console_srcu(c) {
> -			flags = console_srcu_read_flags(c);
> -
> -			if (flags & CON_NBCON) {
> -				nbcon_seq_force(c, seq);
> -			} else {
> -				/*
> -				 * This is an unsynchronized assignment. On
> -				 * panic legacy consoles are only best effort.
> -				 */
> -				c->seq = seq;
> -			}
> -		}
> -		console_srcu_read_unlock(cookie);
> -	}
> +	if (mode == CONSOLE_REPLAY_ALL)
> +		console_rewind_all();
>  
>  	console_flush_all(false, &next_seq, &handover);
>  }
> @@ -4259,6 +4263,15 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
>  }
>  EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>  
> +/**
> + * Dump the printk ring buffer directly to consoles
> + */
> +void dump_printk_buffer(void)

I would call this function console_replay_all(). IMHO, it better describes
what it does.

> +{
> +	console_lock();
> +	console_rewind_all();

I would add a comment:

	/* Consoles are flushed as part of console_unlock(). */

> +	console_unlock();
> +}
>  #endif

Best Regards,
Petr