diff mbox series

[printk,v3,24/40] tty: nfcon: use console_is_registered()

Message ID 20221107141638.3790965-25-john.ogness@linutronix.de
State New
Headers
Series reduce console_lock scope |

Commit Message

John Ogness Nov. 7, 2022, 2:16 p.m. UTC 
  Currently CON_ENABLED is being (mis)used to identify if the console
has been registered. This is not reliable because it can be set even
though registration failed or it can be unset, even though the console
is registered.

Instead, use console_is_registered().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/m68k/emu/nfcon.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Geert Uytterhoeven Nov. 8, 2022, 8:39 a.m. UTC | #1 
Hi John,

On Mon, Nov 7, 2022 at 3:16 PM John Ogness <john.ogness@linutronix.de> wrote:
> Currently CON_ENABLED is being (mis)used to identify if the console
> has been registered. This is not reliable because it can be set even
> though registration failed or it can be unset, even though the console
> is registered.
>
> Instead, use console_is_registered().
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Thanks for your patch!

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Petr Mladek Nov. 10, 2022, 1:58 p.m. UTC | #2 
On Mon 2022-11-07 15:22:22, John Ogness wrote:
> Currently CON_ENABLED is being (mis)used to identify if the console
> has been registered. This is not reliable because it can be set even
> though registration failed or it can be unset, even though the console
> is registered.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/m68k/emu/nfcon.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
> index 557d60867f98..292669fa480f 100644
> --- a/arch/m68k/emu/nfcon.c
> +++ b/arch/m68k/emu/nfcon.c
> @@ -49,14 +49,14 @@ static void nfcon_write(struct console *con, const char *str,
>  static struct tty_driver *nfcon_device(struct console *con, int *index)
>  {
>  	*index = 0;
> -	return (con->flags & CON_ENABLED) ? nfcon_tty_driver : NULL;
> +	return console_is_registered(con) ? nfcon_tty_driver : NULL;
>  }
>  
>  static struct console nf_console = {
>  	.name	= "nfcon",
>  	.write	= nfcon_write,
>  	.device	= nfcon_device,
> -	.flags	= CON_PRINTBUFFER,
> +	.flags	= CON_PRINTBUFFER | CON_ENABLED,

This causes that register_console() will always put the console into
console_list. It causes regression, see below.


>  	.index	= -1,
>  };
>  
> @@ -106,10 +106,8 @@ static int __init nf_debug_setup(char *arg)
>  		return 0;
>  
>  	stderr_id = nf_get_id("NF_STDERR");
> -	if (stderr_id) {
> -		nf_console.flags |= CON_ENABLED;
> +	if (stderr_id)
>  		register_console(&nf_console);

My understanding is that this should enable the console
when debug=nfcon kernel parameter is used.

It is a non-standard way. This is why CON_ENABLED flag
has to be explicitly set.

> -	}
>  
>  	return 0;
>  }
> @@ -151,7 +149,7 @@ static int __init nfcon_init(void)
>  
>  	nfcon_tty_driver = driver;
>  
> -	if (!(nf_console.flags & CON_ENABLED))
> +	if (!console_is_registered(&nf_console))
>  		register_console(&nf_console);

This calls register_console() when it was not already
registered by the debug=nfcon early parameter.

It is the standard way. It should enable the console only
when console=nfcon is defined on the commandline. Or when
there is no preferred console and no console enabled by default yet.

We should not pre-set CON_ENABLED in this case.

Best Regards,
Petr
John Ogness Nov. 10, 2022, 2:19 p.m. UTC | #3 
On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
>> index 557d60867f98..292669fa480f 100644
>> --- a/arch/m68k/emu/nfcon.c
>> +++ b/arch/m68k/emu/nfcon.c
>> @@ -49,14 +49,14 @@ static void nfcon_write(struct console *con, const char *str,
>>  static struct tty_driver *nfcon_device(struct console *con, int *index)
>>  {
>>  	*index = 0;
>> -	return (con->flags & CON_ENABLED) ? nfcon_tty_driver : NULL;
>> +	return console_is_registered(con) ? nfcon_tty_driver : NULL;
>>  }
>>  
>>  static struct console nf_console = {
>>  	.name	= "nfcon",
>>  	.write	= nfcon_write,
>>  	.device	= nfcon_device,
>> -	.flags	= CON_PRINTBUFFER,
>> +	.flags	= CON_PRINTBUFFER | CON_ENABLED,
>
> This causes that register_console() will always put the console into
> console_list. It causes regression, see below.

Agreed. Nice catch. I will remove initializing with CON_ENABLED.

>>  	.index	= -1,
>>  };
>>  
>> @@ -106,10 +106,8 @@ static int __init nf_debug_setup(char *arg)
>>  		return 0;
>>  
>>  	stderr_id = nf_get_id("NF_STDERR");
>> -	if (stderr_id) {
>> -		nf_console.flags |= CON_ENABLED;
>> +	if (stderr_id)
>>  		register_console(&nf_console);
>
> My understanding is that this should enable the console
> when debug=nfcon kernel parameter is used.
>
> It is a non-standard way. This is why CON_ENABLED flag
> has to be explicitly set.

Understood. I will add a comment explaining why CON_ENABLED is set here.

>> -	}
>>  
>>  	return 0;
>>  }
>> @@ -151,7 +149,7 @@ static int __init nfcon_init(void)
>>  
>>  	nfcon_tty_driver = driver;
>>  
>> -	if (!(nf_console.flags & CON_ENABLED))
>> +	if (!console_is_registered(&nf_console))
>>  		register_console(&nf_console);
>
> This calls register_console() when it was not already
> registered by the debug=nfcon early parameter.
>
> It is the standard way. It should enable the console only
> when console=nfcon is defined on the commandline. Or when
> there is no preferred console and no console enabled by default yet.
>
> We should not pre-set CON_ENABLED in this case.

Agreed. The hunk itself is OK.

John
Eero Tamminen Nov. 10, 2022, 5:50 p.m. UTC | #4 
Hi,

On 10.11.2022 16.19, John Ogness wrote:
> On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
...
>>> @@ -106,10 +106,8 @@ static int __init nf_debug_setup(char *arg)
>>>   		return 0;
>>>   
>>>   	stderr_id = nf_get_id("NF_STDERR");
>>> -	if (stderr_id) {
>>> -		nf_console.flags |= CON_ENABLED;
>>> +	if (stderr_id)
>>>   		register_console(&nf_console);
>>
>> My understanding is that this should enable the console
>> when debug=nfcon kernel parameter is used.
>>
>> It is a non-standard way. This is why CON_ENABLED flag
>> has to be explicitly set.
> 
> Understood. I will add a comment explaining why CON_ENABLED is set here.

NatFeats is emulator feature.  If you want to test the resulting kernel, 
you can use either Aranym or Hatari emulator.

Aranym NF docs are here:
https://github.com/aranym/aranym/wiki/natfeats-proposal

Hatari m68k linux docs are here:
https://hatari.tuxfamily.org/doc/m68k-linux.txt


	- Eero
diff mbox series

Patch

diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
index 557d60867f98..292669fa480f 100644
--- a/arch/m68k/emu/nfcon.c
+++ b/arch/m68k/emu/nfcon.c
@@ -49,14 +49,14 @@  static void nfcon_write(struct console *con, const char *str,
 static struct tty_driver *nfcon_device(struct console *con, int *index)
 {
 	*index = 0;
-	return (con->flags & CON_ENABLED) ? nfcon_tty_driver : NULL;
+	return console_is_registered(con) ? nfcon_tty_driver : NULL;
 }
 
 static struct console nf_console = {
 	.name	= "nfcon",
 	.write	= nfcon_write,
 	.device	= nfcon_device,
-	.flags	= CON_PRINTBUFFER,
+	.flags	= CON_PRINTBUFFER | CON_ENABLED,
 	.index	= -1,
 };
 
@@ -106,10 +106,8 @@  static int __init nf_debug_setup(char *arg)
 		return 0;
 
 	stderr_id = nf_get_id("NF_STDERR");
-	if (stderr_id) {
-		nf_console.flags |= CON_ENABLED;
+	if (stderr_id)
 		register_console(&nf_console);
-	}
 
 	return 0;
 }
@@ -151,7 +149,7 @@  static int __init nfcon_init(void)
 
 	nfcon_tty_driver = driver;
 
-	if (!(nf_console.flags & CON_ENABLED))
+	if (!console_is_registered(&nf_console))
 		register_console(&nf_console);
 
 	return 0;