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;
From:   John Ogness <john.ogness@linutronix.de>
To:     Petr Mladek <pmladek@suse.com>,
        Marek =?utf-8?Q?Beh=C3=BAn?= <kabel@kernel.org>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Jan Kara <jack@suse.cz>, Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] printk/console: Enable console kthreads only when there
 is no boot console left
In-Reply-To: <YrCO04oNncE1xF5K@alley>
References: <20220619204949.50d9154d@thinkpad>
 <87r13kwawb.fsf@jogness.linutronix.de> <20220620112936.48fcb2a4@thinkpad>
 <YrBdjVwBOVgLfHyb@alley>
 <CAHk-=wgdquXVVE37CZooVK4X+YdSa7XoGtjr71CEYh8UsdKUow@mail.gmail.com>
 <YrCDNqsPrY+Hs9ju@alley> <YrCO04oNncE1xF5K@alley>
Date:   Tue, 21 Jun 2022 00:57:09 +0206
Message-ID: <875ykvaqzm.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 2022-06-20, Petr Mladek <pmladek@suse.com> wrote:
> The console kthreads uncovered several races in console drivers.
> All problems were in situation when a console was being properly
> initialized and registered while an early console, using the same
> port, was being used.
>
> These problems are pretty hard to debug because they often result
> into silent boot crashes. It would be nice to fix them but it
> looks like a can of worms.
>
> Prevent these problems by delaying the use of console kthreads
> after all early consoles are gone. It might later be optimized.
> But let's close this can of worms with a big hammer for now
> so that they do not break first impression on the kthreads
> that solve other real problems.

The problem is that there are multiple consoles accessing the same
hardware. The threaded console printing model did not consider this. The
commit message should clearly state the actual problem.

Since early printing does not use the kernel driver model, I do not see
a clear solution at this point. But since using early printing is not a
typical use, I am OK with this patch.

For the commit message I would suggest something like:

Threaded console printing does not take into consideration that boot
consoles may be accessing the same hardware as normal consoles and thus
must not be called in parallel.

Since it is currently not possible to identify which consoles are
accessing the same hardware, delay threaded console printing activation
until it is known that there are no boot consoles registered.

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index b095fb5f5f61..c0c5e2b6b91d 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3597,23 +3614,18 @@ static int __init printk_late_init(void)
>  					console_cpu_notify, NULL);
>  	WARN_ON(ret < 0);
>  	printk_sysctl_init();
> -	return 0;
> -}
> -late_initcall(printk_late_init);
> -
> -static int __init printk_activate_kthreads(void)
> -{
> -	struct console *con;
>  
> -	console_lock();
> -	printk_kthreads_available = true;
> -	for_each_console(con)
> -		printk_start_kthread(con);
> -	console_unlock();
> +	/*
> +	 * Some console drivers are not ready to use the same port with
> +	 * boot (early) and normal console in parallel. Stay on the safe
> +	 * side and enable kthreads only when there is no boot console.
> +	 */

As with the commit message, this comment is not pointing out the real
issue. I suggest:

	/*
	 * Boot consoles may be accessing the same hardware as normal
	 * consoles and thus must not be called in parallel. Therefore
	 * only activate threaded console printing if it is known that
	 * there are no boot consoles registered.
	 */

> +	if (no_bootcon)
> +		printk_activate_kthreads();
>  
>  	return 0;
>  }
> -early_initcall(printk_activate_kthreads);
> +late_initcall(printk_late_init);
>  
>  #if defined CONFIG_PRINTK
>  /* If @con is specified, only wait for that console. Otherwise wait for all. */

John Ogness