Messages in this thread |  | | Date | Thu, 30 Jan 2020 13:22:26 +0100 | From | Petr Mladek <> | 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
|  |