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 <[email protected]>
---
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,
.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;
--
2.30.2
Hi John,
On Mon, Nov 7, 2022 at 3:16 PM John Ogness <[email protected]> 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 <[email protected]>
Thanks for your patch!
LGTM, so
Reviewed-by: Geert Uytterhoeven <[email protected]>
Acked-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On 2022-11-10, Petr Mladek <[email protected]> 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
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 <[email protected]>
> ---
> 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
Hi,
On 10.11.2022 16.19, John Ogness wrote:
> On 2022-11-10, Petr Mladek <[email protected]> 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