lkml.org 
[lkml]   [2020]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2020-01-30 13:23    [W:0.080 / U:1.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site