Received-SPF: pass (google.com: domain of linux-kernel+bounces-21965-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: John Ogness <john.ogness@linutronix.de>
To: Sreenath Vijayan <sreenath.vijayan@sony.com>, sreenath.vijayan@sony.com,
 rdunlap@infradead.org
Cc: anandakumar.balasubramaniam@sony.com, taichi.shimoyashiki@sony.com,
	pmladek@suse.com, corbet@lwn.net, gregkh@linuxfoundation.org,
	jirislaby@kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        senozhatsky@chromium.org, rostedt@goodmis.org
Subject: Re: [PATCH v2] tty/sysrq: Dump printk ring buffer messages via sysrq
In-Reply-To: <20240110155634.1685656-1-sreenath.vijayan@sony.com>
References: <20231221133953.1507021-1-sreenath.vijayan@sony.com>
 <20240110155634.1685656-1-sreenath.vijayan@sony.com>
Date: Wed, 10 Jan 2024 11:26:45 +0106
Message-ID: <874jfllcaq.fsf@jogness.linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On 2024-01-10, Sreenath Vijayan <sreenath.vijayan@sony.com> wrote:
> When terminal is unresponsive, one cannot use dmesg to view printk
> ring buffer messages. Also, syslog services may be disabled,
> to check them after a reboot, especially on embedded systems.
> In this scenario, dump the printk ring buffer messages via sysrq
> by pressing sysrq+D.

Generally speaking, I like this idea. I also like that you do not
re-flood the kernel buffers and instead just print directly to the
consoles. (I wish sysrq+z with ftrace did that as well.)

Relying on a workqueue will really limit the usefulness of this
feature. Safe efforts could be made to print directly from the interrupt
context. But maybe this approach can be accepted for now as being
"better than nothing", which can be improved upon in the future (for
example, when atomic consoles are available).

Some more comments from me below...

> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 02217e3c916b..62b3911f03b5 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -450,6 +452,51 @@ static const struct sysrq_key_op sysrq_unrt_op = {
>  	.enable_mask	= SYSRQ_ENABLE_RTNICE,
>  };
>  
> +static void dmesg_dump_callback(struct work_struct *work)
> +{
> +	struct kmsg_dump_iter iter;
> +	size_t len;
> +	char *buf;
> +	struct console *con;
> +	int cookie;
> +
> +	/* Size to be updated if PRINTK_MESSAGE_MAX changes */
> +	buf = kzalloc(2048, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	kmsg_dump_rewind(&iter);
> +	while (kmsg_dump_get_line(&iter, 1, buf, 2048, &len)) {
> +		/*
> +		 * 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)
> +				con->write(con, buf, len);
> +		}
> +		console_srcu_read_unlock(cookie);
> +		console_unlock();
> +	}
> +	kfree(buf);
> +}

Rather than implementing all this in drivers/tty/sysrq.c it would
probably be better to just call a new function that is implemented in
kernel/printk/printk.c. Then you would have access to printk-private
items (such as the PRINTK_MESSAGE_MAX macro).

For example, sysrq+z just calls ftrace_dump(), which is implemented in
kernel/trace/trace.c.

John Ogness