| Message ID | 20220601135528.3176471-1-michael@walle.cc |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | earlycon: prevent multiple register_console() | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Wed, Jun 1, 2022 at 8:55 AM Michael Walle <michael@walle.cc> wrote: > > If the earlycon parameter is given twice, the kernel will spit out a > WARN() in register_console() because it was already registered. The > non-dt variant setup_earlycon() already handles that gracefully. The dt > variant of_setup_earlycon() doesn't. Add the check there and propagate > it through early_init_dt_scan_chosen_stdout(). > > FWIW, this doesn't happen if CONFIG_ACPI_SPCR_TABLE is set. In that case > the registration is delayed until after earlycon parameter(s) are > parsed. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > > I'm not sure if this should have a Fixes tag or not. If so I guess it > should be the very first commit which introduced the support (commit > fb11ffe74c79 ("of/fdt: add FDT serial scanning for earlycon")). Considering no one noticed or cared in 8 years and that earlycon is supposed to be a debug option, I'd say no. > For the curious, here is the backtrace: > > [ 0.000000] ------------[ cut here ]------------ > [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/printk/printk.c:3328 register_console+0x2b4/0x364 > [ 0.000000] console 'atmel_serial0' already registered > [ 0.000000] Modules linked in: > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-next-20220601+ #652 > [ 0.000000] Hardware name: Generic DT based system > [ 0.000000] Backtrace: > [ 0.000000] dump_backtrace from show_stack+0x18/0x1c > [ 0.000000] show_stack from dump_stack_lvl+0x48/0x54 > [ 0.000000] dump_stack_lvl from dump_stack+0x18/0x1c > [ 0.000000] dump_stack from __warn+0xd0/0x148 > [ 0.000000] __warn from warn_slowpath_fmt+0x9c/0xc4 > [ 0.000000] warn_slowpath_fmt from register_console+0x2b4/0x364 > [ 0.000000] register_console from of_setup_earlycon+0x29c/0x2ac > [ 0.000000] of_setup_earlycon from early_init_dt_scan_chosen_stdout+0x154/0x18c > [ 0.000000] early_init_dt_scan_chosen_stdout from param_setup_earlycon+0x40/0x48 > [ 0.000000] param_setup_earlycon from do_early_param+0x88/0xc4 > [ 0.000000] do_early_param from parse_args+0x1a4/0x404 > [ 0.000000] parse_args from parse_early_options+0x40/0x48 > [ 0.000000] parse_early_options from parse_early_param+0x38/0x48 > [ 0.000000] parse_early_param from setup_arch+0x114/0x7a4 > [ 0.000000] setup_arch from start_kernel+0x74/0x6dc > [ 0.000000] start_kernel from 0x0 > [ 0.000000] ---[ end trace 0000000000000000 ]--- > > drivers/of/fdt.c | 6 ++++-- > drivers/tty/serial/earlycon.c | 5 ++++- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index a8f5b6532165..7f3524213b43 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -1025,6 +1025,7 @@ int __init early_init_dt_scan_chosen_stdout(void) > int l; > const struct earlycon_id *match; > const void *fdt = initial_boot_params; > + int ret; > > offset = fdt_path_offset(fdt, "/chosen"); > if (offset < 0) > @@ -1057,8 +1058,9 @@ int __init early_init_dt_scan_chosen_stdout(void) > if (fdt_node_check_compatible(fdt, offset, match->compatible)) > continue; > > - if (of_setup_earlycon(match, offset, options) == 0) > - return 0; > + ret = of_setup_earlycon(match, offset, options); > + if (!ret || ret == -EALREADY) > + return ret; Wouldn't just doing a 'return 0' here be simpler? The only other caller of this function doesn't check the error. Rob
Hi, Am 2022-06-01 18:39, schrieb Rob Herring: > On Wed, Jun 1, 2022 at 8:55 AM Michael Walle <michael@walle.cc> wrote: >> >> If the earlycon parameter is given twice, the kernel will spit out a >> WARN() in register_console() because it was already registered. The >> non-dt variant setup_earlycon() already handles that gracefully. The >> dt >> variant of_setup_earlycon() doesn't. Add the check there and propagate >> it through early_init_dt_scan_chosen_stdout(). >> >> FWIW, this doesn't happen if CONFIG_ACPI_SPCR_TABLE is set. In that >> case >> the registration is delayed until after earlycon parameter(s) are >> parsed. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- Scrap this version. Somehow I missed to add the actual check in of_setup_earlycon() to this patch. >> I'm not sure if this should have a Fixes tag or not. If so I guess it >> should be the very first commit which introduced the support (commit >> fb11ffe74c79 ("of/fdt: add FDT serial scanning for earlycon")). > > Considering no one noticed or cared in 8 years and that earlycon is > supposed to be a debug option, I'd say no. > >> For the curious, here is the backtrace: >> >> [ 0.000000] ------------[ cut here ]------------ >> [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/printk/printk.c:3328 >> register_console+0x2b4/0x364 >> [ 0.000000] console 'atmel_serial0' already registered >> [ 0.000000] Modules linked in: >> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted >> 5.18.0-next-20220601+ #652 >> [ 0.000000] Hardware name: Generic DT based system >> [ 0.000000] Backtrace: >> [ 0.000000] dump_backtrace from show_stack+0x18/0x1c >> [ 0.000000] show_stack from dump_stack_lvl+0x48/0x54 >> [ 0.000000] dump_stack_lvl from dump_stack+0x18/0x1c >> [ 0.000000] dump_stack from __warn+0xd0/0x148 >> [ 0.000000] __warn from warn_slowpath_fmt+0x9c/0xc4 >> [ 0.000000] warn_slowpath_fmt from register_console+0x2b4/0x364 >> [ 0.000000] register_console from of_setup_earlycon+0x29c/0x2ac >> [ 0.000000] of_setup_earlycon from >> early_init_dt_scan_chosen_stdout+0x154/0x18c >> [ 0.000000] early_init_dt_scan_chosen_stdout from >> param_setup_earlycon+0x40/0x48 >> [ 0.000000] param_setup_earlycon from do_early_param+0x88/0xc4 >> [ 0.000000] do_early_param from parse_args+0x1a4/0x404 >> [ 0.000000] parse_args from parse_early_options+0x40/0x48 >> [ 0.000000] parse_early_options from parse_early_param+0x38/0x48 >> [ 0.000000] parse_early_param from setup_arch+0x114/0x7a4 >> [ 0.000000] setup_arch from start_kernel+0x74/0x6dc >> [ 0.000000] start_kernel from 0x0 >> [ 0.000000] ---[ end trace 0000000000000000 ]--- >> >> drivers/of/fdt.c | 6 ++++-- >> drivers/tty/serial/earlycon.c | 5 ++++- >> 2 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index a8f5b6532165..7f3524213b43 100644 >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -1025,6 +1025,7 @@ int __init >> early_init_dt_scan_chosen_stdout(void) >> int l; >> const struct earlycon_id *match; >> const void *fdt = initial_boot_params; >> + int ret; >> >> offset = fdt_path_offset(fdt, "/chosen"); >> if (offset < 0) >> @@ -1057,8 +1058,9 @@ int __init >> early_init_dt_scan_chosen_stdout(void) >> if (fdt_node_check_compatible(fdt, offset, >> match->compatible)) >> continue; >> >> - if (of_setup_earlycon(match, offset, options) == 0) >> - return 0; >> + ret = of_setup_earlycon(match, offset, options); >> + if (!ret || ret == -EALREADY) >> + return ret; > > Wouldn't just doing a 'return 0' here be simpler? The only other > caller of this function doesn't check the error. Ok. -michael
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index a8f5b6532165..7f3524213b43 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -1025,6 +1025,7 @@ int __init early_init_dt_scan_chosen_stdout(void) int l; const struct earlycon_id *match; const void *fdt = initial_boot_params; + int ret; offset = fdt_path_offset(fdt, "/chosen"); if (offset < 0) @@ -1057,8 +1058,9 @@ int __init early_init_dt_scan_chosen_stdout(void) if (fdt_node_check_compatible(fdt, offset, match->compatible)) continue; - if (of_setup_earlycon(match, offset, options) == 0) - return 0; + ret = of_setup_earlycon(match, offset, options); + if (!ret || ret == -EALREADY) + return ret; } return -ENODEV; } diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c index 57c70851f22a..59fedf7be572 100644 --- a/drivers/tty/serial/earlycon.c +++ b/drivers/tty/serial/earlycon.c @@ -230,7 +230,10 @@ static int __init param_setup_earlycon(char *buf) earlycon_acpi_spcr_enable = true; return 0; } else if (!buf) { - return early_init_dt_scan_chosen_stdout(); + err = early_init_dt_scan_chosen_stdout(); + if (err == -EALREADY) + return 0; + return err; } }
If the earlycon parameter is given twice, the kernel will spit out a WARN() in register_console() because it was already registered. The non-dt variant setup_earlycon() already handles that gracefully. The dt variant of_setup_earlycon() doesn't. Add the check there and propagate it through early_init_dt_scan_chosen_stdout(). FWIW, this doesn't happen if CONFIG_ACPI_SPCR_TABLE is set. In that case the registration is delayed until after earlycon parameter(s) are parsed. Signed-off-by: Michael Walle <michael@walle.cc> --- I'm not sure if this should have a Fixes tag or not. If so I guess it should be the very first commit which introduced the support (commit fb11ffe74c79 ("of/fdt: add FDT serial scanning for earlycon")). For the curious, here is the backtrace: [ 0.000000] ------------[ cut here ]------------ [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/printk/printk.c:3328 register_console+0x2b4/0x364 [ 0.000000] console 'atmel_serial0' already registered [ 0.000000] Modules linked in: [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-next-20220601+ #652 [ 0.000000] Hardware name: Generic DT based system [ 0.000000] Backtrace: [ 0.000000] dump_backtrace from show_stack+0x18/0x1c [ 0.000000] show_stack from dump_stack_lvl+0x48/0x54 [ 0.000000] dump_stack_lvl from dump_stack+0x18/0x1c [ 0.000000] dump_stack from __warn+0xd0/0x148 [ 0.000000] __warn from warn_slowpath_fmt+0x9c/0xc4 [ 0.000000] warn_slowpath_fmt from register_console+0x2b4/0x364 [ 0.000000] register_console from of_setup_earlycon+0x29c/0x2ac [ 0.000000] of_setup_earlycon from early_init_dt_scan_chosen_stdout+0x154/0x18c [ 0.000000] early_init_dt_scan_chosen_stdout from param_setup_earlycon+0x40/0x48 [ 0.000000] param_setup_earlycon from do_early_param+0x88/0xc4 [ 0.000000] do_early_param from parse_args+0x1a4/0x404 [ 0.000000] parse_args from parse_early_options+0x40/0x48 [ 0.000000] parse_early_options from parse_early_param+0x38/0x48 [ 0.000000] parse_early_param from setup_arch+0x114/0x7a4 [ 0.000000] setup_arch from start_kernel+0x74/0x6dc [ 0.000000] start_kernel from 0x0 [ 0.000000] ---[ end trace 0000000000000000 ]--- drivers/of/fdt.c | 6 ++++-- drivers/tty/serial/earlycon.c | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-)