Re: [PATCH printk v1 1/1] printk: Avoid scheduling irq_work on suspend

[Petr Mladek <pmladek@suse.com>]

On Tue 2025-11-11 15:49:22, John Ogness wrote:
> Allowing irq_work to be scheduled while trying to suspend has shown
> to cause problems as some architectures interpret the pending
> interrupts as a reason to not suspend. This became a problem for
> printk() with the introduction of NBCON consoles. With every
> printk() call, NBCON console printing kthreads are woken by queueing
> irq_work. This means that irq_work continues to be queued due to
> printk() calls late in the suspend procedure.
> 
> Avoid this problem by preventing printk() from queueing irq_work
> once console suspending has begun. This applies to triggering NBCON
> and legacy deferred printing as well as klogd waiters.
> 
> Since triggering of NBCON threaded printing relies on irq_work, the
> pr_flush() within console_suspend_all() is used to perform the final
> flushing before suspending consoles and blocking irq_work queueing.
> NBCON consoles that are not suspended (due to the usage of the
> "no_console_suspend" boot argument) transition to atomic flushing.
> 
> Introduce a new global variable @console_offload_blocked to flag

s/console_offload_blocked/console_irqwork_blocked/

> when irq_work queueing is to be avoided. The flag is used by
> printk_get_console_flush_type() to avoid allowing deferred printing
> and switch NBCON consoles to atomic flushing. It is also used by
> vprintk_emit() to avoid klogd waking.
> 
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -230,6 +230,8 @@ struct console_flush_type {
>  	bool	legacy_offload;
>  };
>  
> +extern bool console_irqwork_blocked;
> +
>  /*
>   * Identify which console flushing methods should be used in the context of
>   * the caller.
> @@ -241,7 +243,7 @@ static inline void printk_get_console_flush_type(struct console_flush_type *ft)
>  	switch (nbcon_get_default_prio()) {
>  	case NBCON_PRIO_NORMAL:
>  		if (have_nbcon_console && !have_boot_console) {
> -			if (printk_kthreads_running)
> +			if (printk_kthreads_running && !console_irqwork_blocked)
>  				ft->nbcon_offload = true;
>  			else
>  				ft->nbcon_atomic = true;
> @@ -251,7 +253,7 @@ static inline void printk_get_console_flush_type(struct console_flush_type *ft)
>  		if (have_legacy_console || have_boot_console) {
>  			if (!is_printk_legacy_deferred())
>  				ft->legacy_direct = true;
> -			else
> +			else if (!console_irqwork_blocked)
>  				ft->legacy_offload = true;
>  		}
>  		break;

This is one possibility.

Another possibility would be to block the irq work
directly in defer_console_output() and wake_up_klogd().
It would handle all situations, including printk_trigger_flush()
or defer_console_output().

Or is there any reason, why these two call paths are not handled?

I do not have strong opinion. This patch makes it more explicit
when defer_console_output() or wake_up_klogd() is called.

If we move the check into defer_console_output() or wake_up_klogd(),
it would hide the behavior. But it will make the API more safe
to use. And wake_up_klogd() is even exported via <linux/printk.h>.


> @@ -264,7 +266,7 @@ static inline void printk_get_console_flush_type(struct console_flush_type *ft)
>  		if (have_legacy_console || have_boot_console) {
>  			if (!is_printk_legacy_deferred())
>  				ft->legacy_direct = true;
> -			else
> +			else if (!console_irqwork_blocked)
>  				ft->legacy_offload = true;

This change won't be needed if we move the check into
defer_console_output() and wake_up_klogd().

>  		}
>  		break;
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 5aee9ffb16b9a..94fc4a8662d4b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2426,7 +2429,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  
>  	if (ft.legacy_offload)
>  		defer_console_output();
> -	else
> +	else if (!console_irqwork_blocked)
>  		wake_up_klogd();

Same here.

>  
>  	return printed_len;


The rest of the patch looks good to me.

Best Regards,
Petr