diff mbox series

[printk,v2,06/38] tty: nfcon: use console_is_enabled()

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

Commit Message

John Ogness Oct. 19, 2022, 2:55 p.m. UTC 
  Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

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

Comments

Petr Mladek Oct. 21, 2022, 9:55 a.m. UTC | #1 
On Wed 2022-10-19 17:01:28, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/m68k/emu/nfcon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
> index 557d60867f98..c542fc2b121f 100644
> --- a/arch/m68k/emu/nfcon.c
> +++ b/arch/m68k/emu/nfcon.c
> @@ -49,7 +49,7 @@ 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_enabled(con) ? nfcon_tty_driver : NULL;
>  }
>  
>  static struct console nf_console = {
> @@ -151,7 +151,7 @@ static int __init nfcon_init(void)
>  
>  	nfcon_tty_driver = driver;
>  
> -	if (!(nf_console.flags & CON_ENABLED))
> +	if (!console_is_enabled(&nf_console))

Heh, the check of CON_ENABLED does not make much sense. IMHO, the important
thing is whether the console is in console_list or not.

I would personally add a check at the beginning of register_console()
whether the console is already registered and enabled() and do
nothing when already done.

I would be nice to fix this. But it might be done later.

The replacement is straightforward. Both checks are or might be racy.
Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

>  		register_console(&nf_console);
>  
>  	return 0;
John Ogness Oct. 31, 2022, 3:59 p.m. UTC | #2 
On 2022-10-21, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
>> index 557d60867f98..c542fc2b121f 100644
>> --- a/arch/m68k/emu/nfcon.c
>> +++ b/arch/m68k/emu/nfcon.c
>> @@ -151,7 +151,7 @@ static int __init nfcon_init(void)
>>  
>>  	nfcon_tty_driver = driver;
>>  
>> -	if (!(nf_console.flags & CON_ENABLED))
>> +	if (!console_is_enabled(&nf_console))
>
> Heh, the check of CON_ENABLED does not make much sense. IMHO, the
> important thing is whether the console is in console_list or not.
>
> I would personally add a check at the beginning of register_console()
> whether the console is already registered and enabled() and do
> nothing when already done.

Actually, register_console() already has this check, but it does a
WARN(). If the debug setup is used for nfcon, this situation is
normal. So probably to avoid the WARN, the CON_ENABLED code was added.

> I would be nice to fix this. But it might be done later.

I could add a console_is_registered() function that checks if the
console is in the console list and use that instead. If we are going to
touch this code, we might as well touch it correctly, right?

John
Petr Mladek Nov. 1, 2022, 8:57 a.m. UTC | #3 
On Mon 2022-10-31 17:05:54, John Ogness wrote:
> On 2022-10-21, Petr Mladek <pmladek@suse.com> wrote:
> >> diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
> >> index 557d60867f98..c542fc2b121f 100644
> >> --- a/arch/m68k/emu/nfcon.c
> >> +++ b/arch/m68k/emu/nfcon.c
> >> @@ -151,7 +151,7 @@ static int __init nfcon_init(void)
> >>  
> >>  	nfcon_tty_driver = driver;
> >>  
> >> -	if (!(nf_console.flags & CON_ENABLED))
> >> +	if (!console_is_enabled(&nf_console))
> >
> > Heh, the check of CON_ENABLED does not make much sense. IMHO, the
> > important thing is whether the console is in console_list or not.
> >
> > I would personally add a check at the beginning of register_console()
> > whether the console is already registered and enabled() and do
> > nothing when already done.
> 
> Actually, register_console() already has this check, but it does a
> WARN(). If the debug setup is used for nfcon, this situation is
> normal.

I see.

> So probably to avoid the WARN, the CON_ENABLED code was added.

I though more about it. I would keep the warning for the case
when the 2nd registration is not intentional.

This driver has two ways how the console can be registered.
It can happen either in nfcon_init() or in nf_debug_setup().
It is better to have an explicit check to show that it is intentional.


> > I would be nice to fix this. But it might be done later.
> 
> I could add a console_is_registered() function that checks if the
> console is in the console list and use that instead. If we are going to
> touch this code, we might as well touch it correctly, right?

It would be great. It is actually much easier after switching
to the hlist.

Best Regards,
Petr
diff mbox series

Patch

diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
index 557d60867f98..c542fc2b121f 100644
--- a/arch/m68k/emu/nfcon.c
+++ b/arch/m68k/emu/nfcon.c
@@ -49,7 +49,7 @@  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_enabled(con) ? nfcon_tty_driver : NULL;
 }
 
 static struct console nf_console = {
@@ -151,7 +151,7 @@  static int __init nfcon_init(void)
 
 	nfcon_tty_driver = driver;
 
-	if (!(nf_console.flags & CON_ENABLED))
+	if (!console_is_enabled(&nf_console))
 		register_console(&nf_console);
 
 	return 0;