From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752637AbdFOPbj (ORCPT ); Thu, 15 Jun 2017 11:31:39 -0400 Received: from mx2.suse.de ([195.135.220.15]:50264 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751016AbdFOPbi (ORCPT ); Thu, 15 Jun 2017 11:31:38 -0400 Date: Thu, 15 Jun 2017 17:31:36 +0200 From: Petr Mladek To: Sergey Senozhatsky Cc: Sergey Senozhatsky , Steven Rostedt , Andrew Morton , Peter Zijlstra , Aleksey Makarov , Sabrina Dubroca , Sudeep Holla , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] printk/console: Clean up logic around fallback console Message-ID: <20170615153136.GD23337@pathway.suse.cz> References: <1497358444-30736-1-git-send-email-pmladek@suse.com> <1497358444-30736-3-git-send-email-pmladek@suse.com> <20170614083803.GB3011@jagdpanzerIV.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170614083803.GB3011@jagdpanzerIV.localdomain> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 2017-06-14 17:38:03, Sergey Senozhatsky wrote: > Thanks for taking a look at this. > > On (06/13/17 14:54), Petr Mladek wrote: > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > > index 8ebc480fdbc6..6e651f68bffd 100644 > > --- a/kernel/printk/printk.c > > +++ b/kernel/printk/printk.c > > @@ -2413,51 +2413,45 @@ void register_console(struct console *newcon) > > unsigned long flags; > > struct console *bcon = NULL; > > I was going to ask if we can also rename `bcon' to just `con', because not > every console we see in for_each_console() is a boot console, that's why we > test for CON_BOOT bit set and then 'if boot_console->flags & CON_BOOT' looks > a bit odd; but looking at the bottom of register_console() we probably better > keep it the way it is. but who knows... suggestions? Yeah, the variable is temporary used for iterating in a cycle where the name is confusing. The real value is assigned and used later. I though about cleaning this as well but I did not get a good idea and let it for future ;-) > > > struct console_cmdline *c; > > - static bool has_preferred; > > + bool have_real_console = false; > > ok, `real console' probably can work. boot console can also be real, > right? it's just it's not a fully featured one. may be there is a better > naming here, can't immediately think of any tho. ideas? There is mentioned 'normal console' in Documentation/admin-guide/kernel-parameters.txt. But it is related to earlyprintk. The same document uses 'real console' several times when it talks about the 'keep' and 'keep_bootcon' options. I am not much happy with the name but it seems to be in sync with the existing documentation. > dunno, looking at this `newcon->flags |= CON_CONSDEV' I think I'd > probably also add the following patch to the series > > --- > > diff --git a/include/linux/console.h b/include/linux/console.h > index b8920a031a3e..a2525e0d7605 100644 > --- a/include/linux/console.h > +++ b/include/linux/console.h > @@ -127,7 +127,7 @@ static inline int con_debug_leave(void) > */ > > #define CON_PRINTBUFFER (1) > -#define CON_CONSDEV (2) /* Last on the command line */ > +#define CON_CONSDEV (2) The description is wrong. The name and the use suggest that it should be something like: #define CON_CONSDEV (2) /* Driver used by /dev/console */ There are some situations (bugs) where this is not true. But I will try to fix the code to fit this description. > so, as far as I can see, the patch shouldn't change the logic of > registration. just a clean up. need to run tests/etc. etc. I just > read the patch so far. Thanks a lot for great review. Best Regards, Petr