Re: [PATCHv2 4/4] printk: make sure we always print console disabled message

[Sergey Senozhatsky]

On (05/15/19 16:47), Petr Mladek wrote:
> On Fri 2019-04-26 14:44:45, Sergey Senozhatsky wrote:
> > 
> > Forgot to mention that the series is still in RFC phase.
> > 
> > 
> > On (04/26/19 14:33), Sergey Senozhatsky wrote:
> > [..]
> > > +++ b/kernel/printk/printk.c
> > > @@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
> > >  	pr_info("%sconsole [%s%d] disabled\n",
> > >  		(console->flags & CON_BOOT) ? "boot" : "",
> > >  		console->name, console->index);
> > > +	/*
> > > +	 * Print 'console disabled' on all the consoles, including the
> > > +	 * one we are about to unregister.
> > > +	 */
> > > +	console_unlock();
> > > +	console_lock();
> > >  
> > >  	res = _braille_unregister_console(console);
> > >  	if (res)
> > 
> > Need to think more if this is race free...
> 
> I am afraid that it is racy against for_each_console() when
> removing the boot consoles.

Can you explain? Do you mean that we can execute two paths unregistering
the same bcon?

	CPU0					CPU1

	console_lock();
	__unregister_console(bconA)		console_lock();
	console_unlock();

						__unregister_console(bconA);
						for (a = console_drivers->next ...)
							if (a == console)
								unregister();
							// console bconA is
							// not in the list
							// anymore
						console_unlock();

	for (a = console_drivers->next ...)
		if (a == console)
	console_unlock();


This CPU0 will never see bconA in the console drivers list.
But... it will try to do

	console->flags &= ~CON_ENABLED;

Which we need to fix.

Do not disable console which is not on the console_drivers list.

---
index 1177ea4b3fe1..e729992cb4e4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,8 @@ static int __unregister_console(struct console *console)
 	if (console_drivers != NULL && console->flags & CON_CONSDEV)
 		console_drivers->flags |= CON_CONSDEV;
 
-	console->flags &= ~CON_ENABLED;
+	if (!res)
+		console->flags &= ~CON_ENABLED;
 	return res;
 }
 
---
	-ss