Messages in this thread | | | Date | Thu, 30 Jan 2020 11:58:07 +0200 | From | Andy Shevchenko <> | 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
| |