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

[Petr Mladek]

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 ;-)

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>

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.

   + 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.

     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.


> 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.

>
> -	res = 1;
> +	res = -ENODEV;

I would describe this as using a regular "meaningful" error code.

>  	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.

Best Regards,
Petr