Introduce FORCE_CON flag to printk. The new flag will make it possible to
create a context where printk messages will never be suppressed.

This mechanism will be used in the next patch to create a force_con
context on sysrq handling, removing an existing workaround on the
loglevel global variable. The workaround existed to make sure that sysrq
header messages were sent to all consoles, but this doesn't work with
deferred messages because the loglevel might be restored to its original
value before a console flushes the messages.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 include/linux/printk.h      |  3 +++
 kernel/printk/internal.h    |  3 +++
 kernel/printk/printk.c      | 21 ++++++++++++++++-----
 kernel/printk/printk_safe.c | 18 ++++++++++++++++++
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index eca9bb2ee637..232e5fd06701 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -166,6 +166,9 @@ __printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
 extern void __printk_deferred_enter(void);
 extern void __printk_deferred_exit(void);
 
+extern void printk_force_console_enter(void);
+extern void printk_force_console_exit(void);
+
 /*
  * The printk_deferred_enter/exit macros are available only as a hack for
  * some code paths that need to defer all printk console printing. Interrupts
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 3fcb48502adb..c6bb47666aef 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -53,6 +53,8 @@ int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write,
 
 /* Flags for a single printk record. */
 enum printk_info_flags {
+	/* always show on console, ignore console_loglevel */
+	LOG_FORCE_CON	= 1,
 	LOG_NEWLINE	= 2,	/* text ended with a newline */
 	LOG_CONT	= 8,	/* text is a fragment of a continuation line */
 };
@@ -90,6 +92,7 @@ bool printk_percpu_data_ready(void);
 
 void defer_console_output(void);
 bool is_printk_legacy_deferred(void);
+bool is_printk_force_console(void);
 
 u16 printk_parse_prefix(const char *text, int *level,
 			enum printk_info_flags *flags);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index beb808f4c367..911f2a32f1cb 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1319,11 +1319,11 @@ static void boot_delay_msec(int level)
 {
 	unsigned long long k;
 	unsigned long timeout;
+	bool suppress = !is_printk_force_console() &&
+			suppress_message_printing(level);
 
-	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
-		|| suppress_message_printing(level)) {
+	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING) || suppress)
 		return;
-	}
 
 	k = (unsigned long long)loops_per_msec * boot_delay;
 
@@ -2273,6 +2273,9 @@ int vprintk_store(int facility, int level,
 	if (dev_info)
 		flags |= LOG_NEWLINE;
 
+	if (is_printk_force_console())
+		flags |= LOG_FORCE_CON;
+
 	if (flags & LOG_CONT) {
 		prb_rec_init_wr(&r, reserve_size);
 		if (prb_reserve_in_last(&e, prb, &r, caller_id, PRINTKRB_RECORD_MAX)) {
@@ -2280,6 +2283,9 @@ int vprintk_store(int facility, int level,
 						 facility, &flags, fmt, args);
 			r.info->text_len += text_len;
 
+			if (flags & LOG_FORCE_CON)
+				r.info->flags |= LOG_FORCE_CON;
+
 			if (flags & LOG_NEWLINE) {
 				r.info->flags |= LOG_NEWLINE;
 				prb_final_commit(&e);
@@ -2947,6 +2953,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 	struct printk_info info;
 	struct printk_record r;
 	size_t len = 0;
+	bool force_con;
 
 	/*
 	 * Formatting extended messages requires a separate buffer, so use the
@@ -2965,9 +2972,13 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 
 	pmsg->seq = r.info->seq;
 	pmsg->dropped = r.info->seq - seq;
+	force_con = r.info->flags & LOG_FORCE_CON;
 
-	/* Skip record that has level above the console loglevel. */
-	if (may_suppress && suppress_message_printing(r.info->level))
+	/*
+	 * Skip records that are not forced to be printed on consoles and that
+	 * has level above the console loglevel.
+	 */
+	if (!force_con && may_suppress && suppress_message_printing(r.info->level))
 		goto out;
 
 	if (is_extended) {
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 2b35a9d3919d..6f94418d53ff 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -12,6 +12,24 @@
 
 #include "internal.h"
 
+/* Context where printk messages are never suppressed */
+static atomic_t force_con;
+
+void printk_force_console_enter(void)
+{
+	atomic_inc(&force_con);
+}
+
+void printk_force_console_exit(void)
+{
+	atomic_dec(&force_con);
+}
+
+bool is_printk_force_console(void)
+{
+	return atomic_read(&force_con);
+}
+
 static DEFINE_PER_CPU(int, printk_context);
 
 /* Can be preempted by NMI. */

-- 
2.47.0

On Tue 2024-11-05 16:45:08, Marcos Paulo de Souza wrote:
> Introduce FORCE_CON flag to printk. The new flag will make it possible to
> create a context where printk messages will never be suppressed.
> 
> This mechanism will be used in the next patch to create a force_con
> context on sysrq handling, removing an existing workaround on the
> loglevel global variable. The workaround existed to make sure that sysrq
> header messages were sent to all consoles, but this doesn't work with
> deferred messages because the loglevel might be restored to its original
> value before a console flushes the messages.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Looks good:

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

Just a nit below.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1319,11 +1319,11 @@ static void boot_delay_msec(int level)
>  {
>  	unsigned long long k;
>  	unsigned long timeout;
> +	bool suppress = !is_printk_force_console() &&
> +			suppress_message_printing(level);
>  
> -	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
> -		|| suppress_message_printing(level)) {
> +	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING) || suppress)
>  		return;

These spaghetti conditions are hard to follow. I would personally
prefer:

	if ((boot_delay == 0)
		return;

	if (system_state >= SYSTEM_RUNNING)
		return;

	if (suppress_message_printing(level) &&	!is_printk_force_console())
		return;

But I do not resist on it.


Best Regards,
Petr

On 2024-11-05, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> @@ -2947,6 +2953,7 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  	struct printk_info info;
>  	struct printk_record r;
>  	size_t len = 0;
> +	bool force_con;
>  
>  	/*
>  	 * Formatting extended messages requires a separate buffer, so use the
> @@ -2965,9 +2972,13 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  
>  	pmsg->seq = r.info->seq;
>  	pmsg->dropped = r.info->seq - seq;
> +	force_con = r.info->flags & LOG_FORCE_CON;
>  
> -	/* Skip record that has level above the console loglevel. */
> -	if (may_suppress && suppress_message_printing(r.info->level))
> +	/*
> +	 * Skip records that are not forced to be printed on consoles and that
> +	 * has level above the console loglevel.
> +	 */
> +	if (!force_con && may_suppress && suppress_message_printing(r.info->level))
>  		goto out;

Rather than adding a new local variable, setting it, and expanding the
condition, it might be cleaner to just update @may_suppress before the
condition check?

	/* Records forced to be printed on consoles must not be skipped. */
	may_suppress &= !(r.info->flags & LOG_FORCE_CON);

Feel free to ignore this suggestion if you think having an extra
variable is easier to follow.

With or without suggested change:

Reviewed-by: John Ogness <john.ogness@linutronix.de>

On Tue, 2024-11-05 at 22:40 +0106, John Ogness wrote:
> On 2024-11-05, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> > @@ -2947,6 +2953,7 @@ bool printk_get_next_message(struct
> > printk_message *pmsg, u64 seq,
> >  	struct printk_info info;
> >  	struct printk_record r;
> >  	size_t len = 0;
> > +	bool force_con;
> >  
> >  	/*
> >  	 * Formatting extended messages requires a separate
> > buffer, so use the
> > @@ -2965,9 +2972,13 @@ bool printk_get_next_message(struct
> > printk_message *pmsg, u64 seq,
> >  
> >  	pmsg->seq = r.info->seq;
> >  	pmsg->dropped = r.info->seq - seq;
> > +	force_con = r.info->flags & LOG_FORCE_CON;
> >  
> > -	/* Skip record that has level above the console loglevel.
> > */
> > -	if (may_suppress && suppress_message_printing(r.info-
> > >level))
> > +	/*
> > +	 * Skip records that are not forced to be printed on
> > consoles and that
> > +	 * has level above the console loglevel.
> > +	 */
> > +	if (!force_con && may_suppress &&
> > suppress_message_printing(r.info->level))
> >  		goto out;
> 
> Rather than adding a new local variable, setting it, and expanding
> the
> condition, it might be cleaner to just update @may_suppress before
> the
> condition check?
> 
> 	/* Records forced to be printed on consoles must not be
> skipped. */
> 	may_suppress &= !(r.info->flags & LOG_FORCE_CON);

Well, your suggestion seems clever than what I did :)

IHMO, I would prefer the new variable as it's easier to read (for me at
least), but I can change if you think it's better..

> 
> Feel free to ignore this suggestion if you think having an extra
> variable is easier to follow.
> 
> With or without suggested change:
> 
> Reviewed-by: John Ogness <john.ogness@linutronix.de>

Thanks John!