Date:   Thu, 30 Jan 2020 13:22:26 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.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 2020-01-30 11:58:07, Andy Shevchenko wrote:
> 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?

Good question.

Please, remove the last hunk if Sergey is not against it.
I think that the success/error should not depend on the state
of CON_ENABLED flag.

The other two changes might stay in the same patch. We just need
to make the commit message easier to understand. I would write
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.
But the current behavior is confusing and buggy:

  + _braille_unregister_console() returns "1" on success
  + unregister_console() returns "1" on error

Fix and clean up the behavior:

  + Return success when _braille_unregister_console() succeeded.
  + Return a meaningful error code when the console was not
    registered before.
</end>

Best Regards,
Petr

Powered by blists - more mailing lists