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

[John Ogness <john.ogness@linutronix.de>]

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