Message-ID: <20200130095807.GQ32742@smile.fi.intel.com>
Date:   Thu, 30 Jan 2020 11:58:07 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/5] console: Avoid positive return code from
 unregister_console()

On Thu, Jan 30, 2020 at 10:04:29AM +0100, Petr Mladek wrote:
> On Mon 2020-01-27 13:47:18, Andy Shevchenko wrote:
> > There are two callers which use the returned code from unregister_console().
> > In some cases, i.e. successfully unregistered Braille console or when console
> > has not been enabled the return code is 1. This code is ambiguous and also
> > prevents callers to distinguish successful operation.
> >
> > Replace this logic to return only negative error codes or 0 when console,
> > either enabled, disabled or Braille has been successfully unregistered.
> 
> I am quite confused by the above message. It is probably because
> the patched code is so confusing ;-)

True, and thanks for the elaboration. Some comments below, nevertheless.

> I would start with something like:
> 
> <begin>
> There are only two callers that use the returned code from
> unregister_console():
> 
>   + unregister_early_console() in arch/m68k/kernel/early_printk.c
>   + kgdb_unregister_nmi_console() in drivers/tty/serial/kgdb_nmi.c
> 
> They both expect to get "0" on success and a non-zero value on error.
> </end>

I'll rewrite commit message.

> The above is more or less clear. Now, the question is what behavior
> is considered as success and what is failure.
> 
> I started thinking about this in a paranoid mode. The console
> registration code is so tricky and it is easy to create
> regression.
> 
> But I think that it is actually not much important. There are only
> two callers that handle the return code:
> 
>    + The 1st one m68k is a late init call and the error code of
>      init calls is ignored.

That's not fully true. If you pass initcall_debug it will be helpful to see
what is failed and what is not.

>    + The 2nd one in kdb code is not much important. I wonder if anyone
>      is actually using kdb. If I remember correctly then Linus
>      prosed to remove it completely during the discussion about
>      lockless printk at Plumbers 2019 and nobody was against.

I agree with Linus, but It's not my area of expertise, for the scope of this
series I would rather ignore what it does with returned code and fix it later
if anybody complains (probably we won't see any complaint).

>      In fact, the kdb code is probably wrong. tty_register_driver()
>      is called before register_console() in
>      kgdb_register_nmi_console() =>
> 
>      kgdb_unregister_nmi_console() should probably call
>      tty_unregister_driver() even when unregister_console() fails.
> 
> unregister_console() is exported symbol but I doubt that the are
> more users of the error code.
> 
> So, I think that we do not need to care about regressions.
> But it is worth to define some resonable behavior, see
> below.

Agree.

> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index d40a316908da..da6a9bdf76b6 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2817,10 +2817,12 @@ int unregister_console(struct console *console)
> >  		console->name, console->index);
> >  
> >  	res = _braille_unregister_console(console);
> > -	if (res)
> > +	if (res < 0)
> >  		return res;
> > +	if (res > 0)
> > +		return 0;
> 
> Sigh, I wish that _braille_unregister_console() did not returned 1
> on success but ...
> 
> I would describe this as a bugfix. unregister_console() should return
> success (0) when _braille_unregister_console() succeeds.

You mean do a separate patch for it with Fixes tag?

> > -	res = 1;
> > +	res = -ENODEV;
> 
> I would describe this as using a regular "meaningful" error code.

In the commit message? Will do!

> >  	console_lock();
> >  	if (console_drivers == console) {
> >  		console_drivers=console->next;
> > @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> >  	if (!res && (console->flags & CON_EXTENDED))
> >  		nr_ext_console_drivers--;
> >  
> > +	if (res && !(console->flags & CON_ENABLED))
> > +		res = 0;
> 
> I personally think that success or failure of unregister_console()
> should not depend on the state of CON_ENABLED flag:
> 
>   + As it was discussed in the other thread. There are few consoles
>     that have set CON_ENABLED by default. unregister_console()
>     should not succeed when register_console() was not called
>     before.
> 
>   + This check would open a question if we should return error
>     when the console was in the list but CON_ENABLED was not set.
>     But consoles might be temporary disabled, see console_stop().
>     unregister_console() should succeed even when the console
>     was temporary stopped.
> 
> But I think that this is only theoretical discussion. IMHO, nobody
> really depends on the return code in reality. Alternative solution
> would be to make it symetric with register_console() and do not
> return the error code at all.

Okay, I understand that for time being it's matter of how eloquent
the commit message will be. (And maybe some comments in the code?)
Is it correct?

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists