Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Apr 2023 11:03:33 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v1 15/18] printk: nobkl: Stop threads on
 shutdown/reboot
Message-ID: <ZDfFZckqBwchfib5@alley>
References: <20230302195618.156940-1-john.ogness@linutronix.de>
 <20230302195618.156940-16-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230302195618.156940-16-john.ogness@linutronix.de>
Precedence: bulk

On Thu 2023-03-02 21:02:15, John Ogness wrote:
> Register a syscore_ops shutdown function to stop all threaded
> printers on shutdown/reboot. This allows printk to transition back
> to atomic printing in order to provide a robust mechanism for
> outputting the final messages.
>
> --- a/kernel/printk/printk_nobkl.c
> +++ b/kernel/printk/printk_nobkl.c
> @@ -1763,3 +1764,33 @@ void cons_nobkl_cleanup(struct console *con)
>  	cons_state_set(con, CON_STATE_REQ, &state);
>  	cons_free_percpu_data(con);
>  }
> +
> +/**
> + * printk_kthread_shutdown - shutdown all threaded printers
> + *
> + * On system shutdown all threaded printers are stopped. This allows printk
> + * to transition back to atomic printing, thus providing a robust mechanism
> + * for the final shutdown/reboot messages to be output.
> + */
> +static void printk_kthread_shutdown(void)
> +{
> +	struct console *con;
> +
> +	console_list_lock();
> +	for_each_console(con) {
> +		if (con->flags & CON_NO_BKL)
> +			cons_kthread_stop(con);
> +	}
> +	console_list_unlock();

It would make sense to explicitly flush the consoles after stopping
the kthreads. There might be pending messages...

> +}
> +
> +static struct syscore_ops printk_syscore_ops = {
> +	.shutdown = printk_kthread_shutdown,
> +};
> +
> +static int __init printk_init_ops(void)
> +{
> +	register_syscore_ops(&printk_syscore_ops);
> +	return 0;
> +}
> +device_initcall(printk_init_ops);

Otherwise it looks good.

Best Regards,
Petr