Re: RFC - printk handling more than one CON_BOOT

[Ingo Molnar]

* Robin Getz <rgetz@xxxxxxxxxxxxxxxxxxxx> wrote:

> Today, register_console() assumes the following usage (with 
> respect to boot consoles/early_printk).
> 
> The first console to register with register_console() with a flags 
> set to CON_BOOT is the one and only bootconsole.
> 
> If another register_console() is called with an additional 
> CON_BOOT, today it is silently rejected.
> 
> As soon as a console without the CON_BOOT set calls 
> register_console(), the one and only bootconsole is automatically 
> unregistered.
> 
> Once there is a "standard" console - register_console() will 
> silently reject any consoles with it's CON_BOOT, set.
> 
> 
> This changeset allows multiple boot consoles, and changes the 
> functionality to, be mostly the same as the above.
> 
> Any number of bootconsoles can be registered. A "real" console 
> will unregister all the bootconsoles. Once a "real" console is 
> registered, no more bootconsoles can be added.
> 
> Thoughts?
>
> The use case is to have a console buffer which goes to serial, and 
> a console buffer which goes to a fixed memory buffer at the same 
> time. (serial is what most people use, but if serial is hosed for 
> some reason, having things in a buffer (which gets printed out by 
> the bootloader) is the only way to tell what is going on).

Looks genuinely useful ...

> If you don't object I will send a properly formatted patch...

Find some minor comments below:

> -------------
> 
> 
> Index: kernel/printk.c
> ===================================================================
> --- kernel/printk.c	(revision 6860)
> +++ kernel/printk.c	(working copy)
> @@ -1126,9 +1126,24 @@
>  	unsigned long flags;
>  	struct console *bootconsole = NULL;
>  
> +	/* before we register a new CON_BOOT console, make sure we don't
> +	 * already have a valid console
> +	 */

please use the customary comment style:

  /*
   * Comment .....
   * ...... goes here:
   */

specified in Documentation/CodingStyle.

>  	if (console_drivers) {
> -		if (console->flags & CON_BOOT)
> -			return;
> +		if (console->flags & CON_BOOT) {
> +			for (bootconsole = console_drivers;
> +				bootconsole != NULL;
> +				bootconsole = bootconsole->next) {
> +				if (!(bootconsole->flags & CON_BOOT))
> +					break;
> +			}
> +			if ((console->flags & CON_BOOT) &&
> +				!(bootconsole->flags & CON_BOOT))
> +				return;
> +			if (!(bootconsole->flags & CON_BOOT))
> +				bootconsole = NULL;
> +		}

This is really ugly to read mainly because the meat of the function, 
the loop over all console drivers, is two indentation levels to the 
right which creates line break artifacts. I'd suggest to put this 
portion into a helper inline.

Also, the name 'bootconsole' is way too long for an iterator. 
Something like 'con' would be more than enough. (and the new console 
that is being registered could be new_con or so, to bring it in line 
with this naming.)

Also, the new semantics here seem overly complex. Why not have a 
state variable that tells us whether we are in the early boot phase 
or not and warn about early consoles that get registered too late 
and real consoles that get registered too early?

That way the CON_BOOT flag is really reduntant in terms of 
semantics: all consoles registered before console_init() are early 
ones, and all consoles registered after that are final consoles. (or 
something like that)

Is there any reason why we'd want to do something more complex than 
such simple rules?
 
> +
>  		if (console_drivers->flags & CON_BOOT)
>  			bootconsole = console_drivers;
>  	}
> @@ -1195,15 +1210,32 @@
>  	if (!(console->flags & CON_ENABLED))
>  		return;
>  
> -	if (bootconsole && (console->flags & CON_CONSDEV)) {
> -		printk(KERN_INFO "console handover: boot [%s%d] -> real [%s%d]\n",
> -		       bootconsole->name, bootconsole->index,
> -		       console->name, console->index);
> -		unregister_console(bootconsole);
> +	if (bootconsole && (console->flags & CON_CONSDEV) &&
> +		!(console->flags & CON_BOOT)) {
> +		/* we need to iterate through twice, to make sure we print
> +		 * everything out, before we unregister the console(s)
> +		 */

(please fix the comment.)

> +		printk(KERN_INFO "console handover: ");
> +		for (bootconsole = console_drivers; bootconsole != NULL;
> +			bootconsole = bootconsole->next) {
> +			if (bootconsole->flags & CON_BOOT)
> +				printk("boot [%s%d] ", bootconsole->name,
> +					bootconsole->index);
> +		}
> +		printk(" -> real [%s%d]\n", console->name, console->index);
> +		for (bootconsole = console_drivers; bootconsole != NULL;
> +			bootconsole = bootconsole->next) {
> +			if (bootconsole->flags & CON_BOOT)
> +				unregister_console(bootconsole);
> +		}

this too should probably move into a helper inline.

>  		console->flags &= ~CON_PRINTBUFFER;
>  	} else {
> -		printk(KERN_INFO "console [%s%d] enabled\n",
> -		       console->name, console->index);
> +		if (console->flags & CON_BOOT)
> +			printk(KERN_INFO "bootconsole [%s%d] enabled\n",
> +				console->name, console->index);
> +		else
> +			printk(KERN_INFO "console [%s%d] enabled\n",
> +				console->name, console->index);

multi-line branch statemens should preferably come with curly 
braces. And this could probably be written simpler as well, as:

	printk(KERN_INFO "%sconsole [%s%d] enabled\n",
		(con->flags & CON_BOOT) ? "boot" : "",
		 con->name, con->index);

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/