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:   Mon, 20 Jun 2022 01:28:25 +0200
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Petr Mladek <pmladek@suse.com>, Marco Elver <elver@google.com>,
        linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH v2] printk: allow direct console printing to be enabled
 always
Message-ID: <Yq+xGcBO06ILMUFy@zx2c4.com>
References: <Yq+Jjoyn/wj7yzeQ@zx2c4.com>
 <20220619204326.556923-1-Jason@zx2c4.com>
 <87letsw8en.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <87letsw8en.fsf@jogness.linutronix.de>
Precedence: bulk

Hi John,

On Mon, Jun 20, 2022 at 01:23:04AM +0206, John Ogness wrote:
> On 2022-06-19, "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
> > diff --git a/init/Kconfig b/init/Kconfig
> > index c7900e8975f1..47466aa2b0e8 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> 
> Sorry, I missed this in your v1. But I think this config belongs in
> lib/Kconfig.debug under the "printk and dmesg options" menu.

That's better. It helps drive home that it's a debug thing.

> > +static bool printk_direct = IS_ENABLED(CONFIG_PRINTK_DIRECT);
> 
> I understand why you would name the variable to match the boot arg. But
> I would prefer the _internal_ variable had a name that makes it clear
> (to us developers) that it is a permanent setting. Perhaps
> printk_direct_only or printk_direct_always?

Sure, I'll do that. The variable can also be __initdata, since it's only
used inside of an __init function.

> > +	if (printk_direct)
> 
> I'm wondering if we should output a message here. My suggestion is:
> 
> pr_info("printing threads disabled, using direct printing\n");

That seems a bit heavy to me, and just adds more log spam. Moving it
into the debug kconfig zone seems like the right way to handle this
instead.

> > +		return 0;
> > +
> >  	console_lock();
> >  	printk_kthreads_available = true;
> >  	for_each_console(con)
> 
> Otherwise it looks OK to me. But you may want to wait on a response from
> Petr, Sergey, or Steven before sending a v3. You are adding a kernel
> config and a boot argument. Both of these are sensitive topics that
> require more feedback from others.

Fair enough. It's easy enough to send a v3, and I agree with most of
your suggestions, so I'll send that now, and we'll wait to hear if the
others think it's fine too. 

Jason