Received-SPF: pass (google.com: domain of linux-kernel+bounces-29982-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: John Ogness <john.ogness@linutronix.de>
To: Sreenath Vijayan <sreenath.vijayan@sony.com>, corbet@lwn.net,
 gregkh@linuxfoundation.org, jirislaby@kernel.org, rdunlap@infradead.org,
 pmladek@suse.com
Cc: 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,
 sreenath.vijayan@sony.com
Subject: Re: [PATCH v3 1/2] printk: Add function to dump printk buffer
 directly to consoles
In-Reply-To: <402f0cbc3a573503c7cc794113aa5137ed7f276c.1705331453.git.sreenath.vijayan@sony.com>
References: <cover.1705331453.git.sreenath.vijayan@sony.com>
 <402f0cbc3a573503c7cc794113aa5137ed7f276c.1705331453.git.sreenath.vijayan@sony.com>
Date: Thu, 18 Jan 2024 10:55:20 +0106
Message-ID: <87le8nas4f.fsf@jogness.linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On 2024-01-17, Sreenath Vijayan <sreenath.vijayan@sony.com> 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 needs access to private items of printk like PRINTK_MESSAGE_MAX.
> Add function in printk.c to accomplish this.
>
> Suggested-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Sreenath Vijayan <sreenath.vijayan@sony.com>
> Signed-off-by: Shimoyashiki Taichi <taichi.shimoyashiki@sony.com>
> ---
>  include/linux/printk.h |  4 ++++
>  kernel/printk/printk.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 8ef499ab3c1e..0896745f31e2 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -192,6 +192,7 @@ void show_regs_print_info(const char *log_lvl);
>  extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
>  extern asmlinkage void dump_stack(void) __cold;
>  void printk_trigger_flush(void);
> +void dump_printk_buffer(void);
>  #else
>  static inline __printf(1, 0)
>  int vprintk(const char *s, va_list args)
> @@ -271,6 +272,9 @@ static inline void dump_stack(void)
>  static inline void printk_trigger_flush(void)
>  {
>  }
> +static inline void dump_printk_buffer(void)
> +{
> +}
>  #endif
>  
>  #ifdef CONFIG_SMP
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index f2444b581e16..5b11fb377f8f 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -4259,6 +4259,39 @@ 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)
> +{
> +	struct kmsg_dump_iter iter;
> +	struct console *con;
> +	char *buf;
> +	size_t len;
> +	int cookie;
> +
> +	buf = kmalloc(PRINTK_MESSAGE_MAX, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	kmsg_dump_rewind(&iter);
> +	while (kmsg_dump_get_line(&iter, 1, buf, PRINTK_MESSAGE_MAX, &len)) {

Although using the kmsg_dump interface will provide you the messages,
they will not necessarily be in the correct format. Consoles can be set
to use extended format.

We probably should respect that console setting.

> +		/*
> +		 * Since using printk() or pr_*() will append the message to the
> +		 * printk ring buffer, they cannot be used to display the retrieved
> +		 * message. Hence console_write() of serial drivers is used.
> +		 */
> +		console_lock();
> +		cookie = console_srcu_read_lock();
> +		for_each_console_srcu(con) {
> +			if ((console_srcu_read_flags(con) & CON_ENABLED) && con->write)

console_is_usable() should be used instead. It makes the correct checks.

> +				con->write(con, buf, len);
> +		}
> +		console_srcu_read_unlock(cookie);
> +		console_unlock();
> +	}
> +	kfree(buf);
> +}

We could do something like this:

void dump_printk_buffer(void)
{
	console_lock();
	console_flush_on_panic(CONSOLE_REPLAY_ALL);
	console_unlock();
}

This version respects all the console features (formatting, handovers),
but console_flush_on_panic() does not to allow cond_resched(), which we
would want in this case.

We could take the console sequence-resetting code out into its own
helper function. Then it would look like this (comments removed to keep
things short):

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
			c->seq = seq;
	}
	console_srcu_read_unlock(cookie);
}

void console_flush_on_panic(enum con_flush_mode mode)
{
	bool handover;
	u64 next_seq;

	console_may_schedule = 0;

	if (mode == CONSOLE_REPLAY_ALL)
		console_rewind_all();

	console_flush_all(false, &next_seq, &handover);
}

void dump_printk_buffer(void)
{
	bool handover;
	u64 next_seq;

	console_lock();
	console_rewind_all();
	console_flush_all(true, &next_seq, &handover);
	console_unlock();
}

Any thoughts?

John