From: Thomas Gleixner <tglx@linutronix.de>

To prepare for a new console infrastructure that is independent of the
console BKL, wrap the output mode into a descriptor struct so the new
infrastrucure can share the emit code that dumps the ringbuffer record
into the output text buffers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 15 +++++++
 kernel/printk/printk.c  | 88 ++++++++++++++++++++++++++++++-----------
 2 files changed, 79 insertions(+), 24 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index 05c7325e98f9..590ab62c01d9 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -187,6 +187,21 @@ struct cons_text_buf {
 	char		text[CONSOLE_LOG_MAX];
 };
 
+/**
+ * struct cons_outbuf_desc - console output buffer descriptor
+ * @txtbuf:		Pointer to buffer for storing the text
+ * @outbuf:		Pointer to the position in @buffer for
+ *			writing it out to the device
+ * @len:		Message length
+ * @extmsg:		Select extended format printing
+ */
+struct cons_outbuf_desc {
+	struct cons_text_buf	*txtbuf;
+	char			*outbuf;
+	unsigned int		len;
+	bool			extmsg;
+};
+
 /**
  * struct console - The console descriptor structure
  * @name:		The name of the console driver
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9cbd44e9fc45..c9207d81b90c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2689,40 +2689,39 @@ static void __console_unlock(void)
 	up_console_sem();
 }
 
-/*
- * Print one record for the given console. The record printed is whatever
- * record is the next available record for the given console.
- *
- * @text is a buffer of size CONSOLE_LOG_MAX.
- *
- * If extended messages should be printed, @ext_text is a buffer of size
- * CONSOLE_EXT_LOG_MAX. Otherwise @ext_text must be NULL.
+
+/**
+ * cons_fill_outbuf - Fill the output buffer with the next record
+ * @con:	The console to print on
+ * @desc:	Pointer to the output descriptor
  *
- * If dropped messages should be printed, @dropped_text is a buffer of size
- * DROPPED_TEXT_MAX. Otherwise @dropped_text must be NULL.
+ * The output descriptor contains all information which is necessary
+ * to print (buffer pointer, extended format selector...).
  *
- * @handover will be set to true if a printk waiter has taken over the
- * console_lock, in which case the caller is no longer holding the
- * console_lock. Otherwise it is set to false.
+ * Returns: False if there is no pending record in the ringbuffer
+ *	    True if there is a pending record in the ringbuffer.
  *
- * Returns false if the given console has no next record to print, otherwise
- * true.
+ * When the return value is true, then the caller has to check
+ * @desc->outbuf. If not NULL it points to the first character to write to
+ * the device and @desc->len contains the length of the message.
  *
- * Requires the console_lock.
+ * If it is NULL then records have been dropped or skipped and con->seq
+ * has been forwarded so the caller can try to print the next record.
  */
-static bool console_emit_next_record(struct console *con, struct cons_text_buf *txtbuf,
-				     bool *handover, bool extmsg)
+static bool cons_fill_outbuf(struct console *con, struct cons_outbuf_desc *desc)
 {
 	static int panic_console_dropped;
+
+	struct cons_text_buf *txtbuf = desc->txtbuf;
 	struct printk_info info;
 	struct printk_record r;
-	unsigned long flags;
 	char *write_text;
 	size_t len;
 
-	prb_rec_init_rd(&r, &info, txtbuf->text, CONSOLE_LOG_MAX);
+	desc->outbuf = NULL;
+	desc->len = 0;
 
-	*handover = false;
+	prb_rec_init_rd(&r, &info, txtbuf->text, CONSOLE_LOG_MAX);
 
 	if (!prb_read_valid(prb, con->seq, &r))
 		return false;
@@ -2739,10 +2738,10 @@ static bool console_emit_next_record(struct console *con, struct cons_text_buf *
 	/* Skip record that has level above the console loglevel. */
 	if (suppress_message_printing(r.info->level)) {
 		con->seq++;
-		goto skip;
+		return true;
 	}
 
-	if (extmsg) {
+	if (desc->extmsg) {
 		write_text = txtbuf->ext_text;
 		len = info_print_ext_header(write_text, CONSOLE_EXT_LOG_MAX, r.info);
 		len += msg_print_ext_body(write_text + len, CONSOLE_EXT_LOG_MAX - len,
@@ -2752,6 +2751,47 @@ static bool console_emit_next_record(struct console *con, struct cons_text_buf *
 		len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
 	}
 
+	desc->len = len;
+	desc->outbuf = write_text;
+	return true;
+}
+
+/**
+ * console_emit_next_record - Print one record for the given console
+ * @con:	The console to print on
+ * @txtbuf:	Pointer to the output buffer
+ * @handover:	Pointer to Handover handshake storage
+ * @extmsg:	Selects extended message format
+ *
+ * The record printed is whatever record is the next available record for
+ * the given console.
+ *
+ * @handover will be set to true if a printk waiter has taken over the
+ * console_lock, in which case the caller is no longer holding the
+ * console_lock. Otherwise it is set to false.
+ *
+ * Returns false if the given console has no next record to print, otherwise
+ * true.
+ *
+ * Requires the console_lock.
+ */
+static bool console_emit_next_record(struct console *con, struct cons_text_buf *txtbuf,
+				     bool *handover, bool extmsg)
+{
+	struct cons_outbuf_desc desc = {
+		.txtbuf	= txtbuf,
+		.extmsg = extmsg,
+	};
+	unsigned long flags;
+
+	*handover = false;
+
+	if (!cons_fill_outbuf(con, &desc))
+		return false;
+
+	if (!desc.outbuf)
+		goto skip;
+
 	/*
 	 * While actively printing out messages, if another printk()
 	 * were to occur on another CPU, it may wait for this one to
@@ -2766,7 +2806,7 @@ static bool console_emit_next_record(struct console *con, struct cons_text_buf *
 	console_lock_spinning_enable();
 
 	stop_critical_timings();	/* don't trace print latency */
-	call_console_driver(con, write_text, len, extmsg ? NULL : txtbuf->dropped_text);
+	call_console_driver(con, desc.outbuf, desc.len, extmsg ? NULL : txtbuf->dropped_text);
 	start_critical_timings();
 
 	con->seq++;
-- 
2.30.2

On Sat 2022-09-24 02:10:53, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To prepare for a new console infrastructure that is independent of the
> console BKL, wrap the output mode into a descriptor struct so the new
> infrastrucure can share the emit code that dumps the ringbuffer record
> into the output text buffers.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/console.h | 15 +++++++
>  kernel/printk/printk.c  | 88 ++++++++++++++++++++++++++++++-----------
>  2 files changed, 79 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 05c7325e98f9..590ab62c01d9 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -187,6 +187,21 @@ struct cons_text_buf {
>  	char		text[CONSOLE_LOG_MAX];
>  };
>  
> +/**
> + * struct cons_outbuf_desc - console output buffer descriptor
> + * @txtbuf:		Pointer to buffer for storing the text
> + * @outbuf:		Pointer to the position in @buffer for
> + *			writing it out to the device

This sounds like this pointer might point into the middle of
some buffer. It sounds scarry without providing the remaining
size of the buffer.

It seems that the pointer is actually used to point to
one of the buffers in txtbuf struct. Then it is again
a bit scarry without the size. I know that it is defined
by @len. But it is not obvious that it is related.

> + * @len:		Message length

It is not clear that it is length of the outbuf.

> + * @extmsg:		Select extended format printing

It would be nice to make it obvious (variable name)
that it is bool and not another buffer.

This actually defines which buffer will be used
in txtbuf.

> + */
> +struct cons_outbuf_desc {
> +	struct cons_text_buf	*txtbuf;
> +	char			*outbuf;
> +	unsigned int		len;
> +	bool			extmsg;
> +};

Sigh, I somehow do not like this structure. I think that the main
problem is that it combines both input and output values.

Also there is too much assignments here and there.

What about?

1. Storing "struct cons_text_buf *txtbuf" into struct console.
   Normal consoles might point to a global txtbuf.
   Atomic consoles might point to the allocated ones.

2. Create structure for writing the next record
   on the console, for example:

   struct console_record {	/* like struct printk_record */
	char *buf;
	int size;
	int len;
   }

Then we could implement:

bool console_get_record(struct console *con,
			struct console_record *cr)
{
	struct cons_text_buf *txtbuf = con->txtbuf;
	struct printk_info info;
	struct printk_record r;
	char *write_text;
	size_t len;

	cr->buf = NULL;
	cr->size = 0;
	cr->len = 0;

	prb_rec_init_rd(&r, &info, txtbuf->text, sizeof(txtbuf->text);

	if (!prb_read_valid(prb, desc->seq, &r))
		return false;

	/* Skip record that has level above the console loglevel. */
	if (suppress_message_printing(r.info->level)) {
		return true;
	}

	if (con->flags & CON_EXTENDED) {
		cr->buf = txtbuf->ext_text;
		cr->size = sizeof(txtbuf->ext_text);
		info_print_ext_header(cr, r.info);
		msg_print_ext_body(cr, &r);
	} else {
		cr->buf = txtbuf->text;
		cr->size = sizeof(txtbuf->text);
		record_print_text(cr, &r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);

		cons_print_dropped(cr, con);
	}

	return true;
}

and

static bool console_emit_next_record(struct console *con,
				     bool *handover)
{
	struct console_record cr;
	unsigned long flags;

	*handover = false;

	if (!console_get_next_record(con, cr))
		return false;

	/* supressed? */
	if (!cr->buf) {
		con->seq++;
		return true;
	}

	/*
	 * While actively printing out messages, if another printk()
	 * were to occur on another CPU, it may wait for this one to
	 * finish. This task can not be preempted if there is a
	 * waiter waiting to take over.
	 *
	 * Interrupts are disabled because the hand over to a waiter
	 * must not be interrupted until the hand over is completed
	 * (@console_waiter is cleared).
	 */
	printk_safe_enter_irqsave(flags);
	console_lock_spinning_enable();

	/* don't trace print latency */
	stop_critical_timings();
	/* Write everything out to the hardware */
	con->write(con, cr->buf, cr->len);
	start_critical_timings();

	con->seq++;

	*handover = console_lock_spinning_disable_and_check();
	printk_safe_exit_irqrestore(flags);

	return true;
}

Advantages:

	+ even less parameters
	+ less assignments (read/write directly in struct console)
	+ struct console_record has just output buffer =>
	  no confusion about the names

How does that sound, please?

Best Regards,
Petr