Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()

From: Andy Shevchenko
Date: Thu Jan 30 2020 - 04:58:09 EST


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