| Message ID | 20250606-printk-cleanup-part2-v1-4-f427c743dda0@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | printk cleanup - part 2 | expand |
Hi, On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza <mpdesouza@suse.com> wrote: > > All consoles found on for_each_console are registered, meaning that all of > them are CON_ENABLED. The code tries to find an active console, so check if the > console is not suspended instead. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > drivers/tty/serial/kgdboc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c > index 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b006b2923583a0d2 100644 > --- a/drivers/tty/serial/kgdboc.c > +++ b/drivers/tty/serial/kgdboc.c > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char *opt) > console_list_lock(); > for_each_console(con) { > if (con->write && con->read && > - (con->flags & (CON_BOOT | CON_ENABLED)) && > + (con->flags & CON_BOOT) && > + ((con->flags & CON_SUSPENDED) == 0) && I haven't tried running the code, so I could easily be mistaken, but... ...the above doesn't seem like the correct conversion. The old expression was: (con->flags & (CON_BOOT | CON_ENABLED)) That would evaluate to non-zero (true) if the console was _either_ "boot" or "enabled". The new expression is is: (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0) That's only true if the console is _both_ "boot" and "not suspended". -Doug
On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote: > Hi, > > On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza > <mpdesouza@suse.com> wrote: > > > > All consoles found on for_each_console are registered, meaning that > > all of > > them are CON_ENABLED. The code tries to find an active console, so > > check if the > > console is not suspended instead. > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > --- > > drivers/tty/serial/kgdboc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/tty/serial/kgdboc.c > > b/drivers/tty/serial/kgdboc.c > > index > > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b > > 006b2923583a0d2 100644 > > --- a/drivers/tty/serial/kgdboc.c > > +++ b/drivers/tty/serial/kgdboc.c > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char > > *opt) > > console_list_lock(); > > for_each_console(con) { > > if (con->write && con->read && > > - (con->flags & (CON_BOOT | CON_ENABLED)) && > > + (con->flags & CON_BOOT) && > > + ((con->flags & CON_SUSPENDED) == 0) && > > I haven't tried running the code, so I could easily be mistaken, > but... > > ...the above doesn't seem like the correct conversion. The old > expression was: > > (con->flags & (CON_BOOT | CON_ENABLED)) > > That would evaluate to non-zero (true) if the console was _either_ > "boot" or "enabled". > > The new expression is is: > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0) > > That's only true if the console is _both_ "boot" and "not suspended". My idea here was that the users of for_each_console would find the first available console, and by available I would expect them to be usable. In this case, is there any value for kgdboc to use a console that is suspended? Would it work in this case? I never really used kgdboc, but only checking if the console was enabled (which it's always the case here) was something that needed to be fixed. Maybe I'm missing something here as well, so please let me know if I should remove the new check. > > -Doug
Hi, On Tue, Jun 10, 2025 at 1:03 PM Marcos Paulo de Souza <mpdesouza@suse.com> wrote: > > On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote: > > Hi, > > > > On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza > > <mpdesouza@suse.com> wrote: > > > > > > All consoles found on for_each_console are registered, meaning that > > > all of > > > them are CON_ENABLED. The code tries to find an active console, so > > > check if the > > > console is not suspended instead. > > > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > > --- > > > drivers/tty/serial/kgdboc.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/tty/serial/kgdboc.c > > > b/drivers/tty/serial/kgdboc.c > > > index > > > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b > > > 006b2923583a0d2 100644 > > > --- a/drivers/tty/serial/kgdboc.c > > > +++ b/drivers/tty/serial/kgdboc.c > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char > > > *opt) > > > console_list_lock(); > > > for_each_console(con) { > > > if (con->write && con->read && > > > - (con->flags & (CON_BOOT | CON_ENABLED)) && > > > + (con->flags & CON_BOOT) && > > > + ((con->flags & CON_SUSPENDED) == 0) && > > > > I haven't tried running the code, so I could easily be mistaken, > > but... > > > > ...the above doesn't seem like the correct conversion. The old > > expression was: > > > > (con->flags & (CON_BOOT | CON_ENABLED)) > > > > That would evaluate to non-zero (true) if the console was _either_ > > "boot" or "enabled". > > > > The new expression is is: > > > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0) > > > > That's only true if the console is _both_ "boot" and "not suspended". > > My idea here was that the users of for_each_console would find the > first available console, and by available I would expect them to be > usable. In this case, is there any value for kgdboc to use a console > that is suspended? Would it work in this case? > > I never really used kgdboc, but only checking if the console was > enabled (which it's always the case here) was something that needed to > be fixed. > > Maybe I'm missing something here as well, so please let me know if I > should remove the new check. So it's been 5 years since I wrote the code, but reading that I was checking for: (con->flags & (CON_BOOT | CON_ENABLED)) Makes me believe that this was the case when I wrote the code: 1. Early boot consoles (earlycon) were not marked as CON_ENABLED but were instead marked as CON_BOOT. 2. Once consoles became non-early they were moved to CON_ENABLED. ...and the code was basically looking for any consoles with a matching name that were either boot consoles or normal/enabled consoles. Is that a plausible theory? It's also possible that I just was confused when I code things up and that I really meant to write: ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED)) ...AKA that I wanted consoles that were BOOT _and_ ENABLED. In any case, I booted up the current mainline and I put a printout here. I saw: [ 0.000000] kgdboc: DOUG: console qcom_geni has flags 0x0000000f So that means that both BOOT and ENABLED were set. That makes me feel like it's plausible that I was confused and really meant BOOT _and_ ENABLED. I didn't spend time trying to figure out how to build/boot an old kernel to check, though. In any case, given my test then I think your change should be fine. Given that it does change the boolean logic, it seems like that deserves a mention in the commit message. -Doug
On Tue 2025-06-10 16:18:22, Doug Anderson wrote: > Hi, > > On Tue, Jun 10, 2025 at 1:03 PM Marcos Paulo de Souza > <mpdesouza@suse.com> wrote: > > > > On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza > > > <mpdesouza@suse.com> wrote: > > > > > > > > All consoles found on for_each_console are registered, meaning that > > > > all of > > > > them are CON_ENABLED. The code tries to find an active console, so > > > > check if the > > > > console is not suspended instead. > > > > > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > > > --- > > > > drivers/tty/serial/kgdboc.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/tty/serial/kgdboc.c > > > > b/drivers/tty/serial/kgdboc.c > > > > index > > > > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b > > > > 006b2923583a0d2 100644 > > > > --- a/drivers/tty/serial/kgdboc.c > > > > +++ b/drivers/tty/serial/kgdboc.c > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char > > > > *opt) > > > > console_list_lock(); > > > > for_each_console(con) { > > > > if (con->write && con->read && > > > > - (con->flags & (CON_BOOT | CON_ENABLED)) && > > > > + (con->flags & CON_BOOT) && > > > > + ((con->flags & CON_SUSPENDED) == 0) && > > > > > > I haven't tried running the code, so I could easily be mistaken, > > > but... > > > > > > ...the above doesn't seem like the correct conversion. The old > > > expression was: > > > > > > (con->flags & (CON_BOOT | CON_ENABLED)) > > > > > > That would evaluate to non-zero (true) if the console was _either_ > > > "boot" or "enabled". > > > > > > The new expression is is: > > > > > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0) > > > > > > That's only true if the console is _both_ "boot" and "not suspended". > > > > My idea here was that the users of for_each_console would find the > > first available console, and by available I would expect them to be > > usable. In this case, is there any value for kgdboc to use a console > > that is suspended? Would it work in this case? > > > > I never really used kgdboc, but only checking if the console was > > enabled (which it's always the case here) was something that needed to > > be fixed. > > > > Maybe I'm missing something here as well, so please let me know if I > > should remove the new check. > > So it's been 5 years since I wrote the code, but reading that I was > checking for: > > (con->flags & (CON_BOOT | CON_ENABLED)) > > Makes me believe that this was the case when I wrote the code: > 1. Early boot consoles (earlycon) were not marked as CON_ENABLED but > were instead marked as CON_BOOT. > 2. Once consoles became non-early they were moved to CON_ENABLED. > > ...and the code was basically looking for any consoles with a matching > name that were either boot consoles or normal/enabled consoles. > > Is that a plausible theory? It's also possible that I just was > confused when I code things up and that I really meant to write: It is easy to get confused by the register_console() code. And it has been even worse some years ago. Anyway, the current code sets CON_ENABLED for all registered consoles, including CON_BOOT consoles. The flag is cleared only by console_suspend()[*] or unregister_console(). IMHO, kgdboc_earlycon_init() does not need to care about console_suspend() because the kernel could not be suspended during boot. Does this makes sense? Also it does not need to care about unregister_console() because the unregistered console won't be listed by for_each_console(). [*] The 1st patch in this patchset updates console_suspend() to set CON_SUSPENDED instead of clearing CON_ENABLED. It helps to make it clear that it is suspend-related. Resume: I would remove the check of CON_ENABLED or CON_SUSPENDED from kgdboc_earlycon_init() completely. IMHO, we should keep the check of CON_BOOT because we do not want to register "normal" console drivers as kgdboc_earlycon_io_ops. It is later removed by kgdboc_earlycon_deinit(). I guess that the code is not ready to handle a takeover from normal to normal (even the same) console driver. To make it clear, I would use: for_each_console(con) { if (con->write && con->read && (con->flags & CON_BOOT) && (!opt || !opt[0] || strcmp(con->name, opt) == 0)) break; } And I would do this change as the 1st patch in the patchset before we change the flag modified by console_suspend(). Best Regards, Petr
Hi, On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pmladek@suse.com> wrote: > > > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char > > > > > *opt) > > > > > console_list_lock(); > > > > > for_each_console(con) { > > > > > if (con->write && con->read && > > > > > - (con->flags & (CON_BOOT | CON_ENABLED)) && > > > > > + (con->flags & CON_BOOT) && > > > > > + ((con->flags & CON_SUSPENDED) == 0) && > > > > > > > > I haven't tried running the code, so I could easily be mistaken, > > > > but... > > > > > > > > ...the above doesn't seem like the correct conversion. The old > > > > expression was: > > > > > > > > (con->flags & (CON_BOOT | CON_ENABLED)) > > > > > > > > That would evaluate to non-zero (true) if the console was _either_ > > > > "boot" or "enabled". > > > > > > > > The new expression is is: > > > > > > > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0) > > > > > > > > That's only true if the console is _both_ "boot" and "not suspended". > > > > > > My idea here was that the users of for_each_console would find the > > > first available console, and by available I would expect them to be > > > usable. In this case, is there any value for kgdboc to use a console > > > that is suspended? Would it work in this case? > > > > > > I never really used kgdboc, but only checking if the console was > > > enabled (which it's always the case here) was something that needed to > > > be fixed. > > > > > > Maybe I'm missing something here as well, so please let me know if I > > > should remove the new check. > > > > So it's been 5 years since I wrote the code, but reading that I was > > checking for: > > > > (con->flags & (CON_BOOT | CON_ENABLED)) > > > > Makes me believe that this was the case when I wrote the code: > > 1. Early boot consoles (earlycon) were not marked as CON_ENABLED but > > were instead marked as CON_BOOT. > > 2. Once consoles became non-early they were moved to CON_ENABLED. > > > > ...and the code was basically looking for any consoles with a matching > > name that were either boot consoles or normal/enabled consoles. > > > > Is that a plausible theory? It's also possible that I just was > > confused when I code things up and that I really meant to write: > > It is easy to get confused by the register_console() code. And > it has been even worse some years ago. > > Anyway, the current code sets CON_ENABLED for all registered > consoles, including CON_BOOT consoles. The flag is cleared only > by console_suspend()[*] or unregister_console(). > > IMHO, kgdboc_earlycon_init() does not need to care about > console_suspend() because the kernel could not be suspended > during boot. Does this makes sense? Yeah, makes sense to me. > Also it does not need to care about unregister_console() because > the unregistered console won't be listed by for_each_console(). > > [*] The 1st patch in this patchset updates console_suspend() > to set CON_SUSPENDED instead of clearing CON_ENABLED. > It helps to make it clear that it is suspend-related. > > > Resume: > > I would remove the check of CON_ENABLED or CON_SUSPENDED > from kgdboc_earlycon_init() completely. > > IMHO, we should keep the check of CON_BOOT because we do not > want to register "normal" console drivers as kgdboc_earlycon_io_ops. > It is later removed by kgdboc_earlycon_deinit(). I guess > that the code is not ready to handle a takeover from normal > to normal (even the same) console driver. I'm not sure I understand your last sentence there. In general the code handling all of the possible handoff (or lack of handoff) cases between kgdboc_earlycon and regular kgdboc is pretty wacky. At one point I thought through it all and tried to test as many scenarios as I could and I seem to remember trying to model some of the behavior on how earlycon worked. If something looks broken here then let me know. > To make it clear, I would use: > > for_each_console(con) { > if (con->write && con->read && > (con->flags & CON_BOOT) && > (!opt || !opt[0] || strcmp(con->name, opt) == 0)) > break; > } > > And I would do this change as the 1st patch in the patchset > before we change the flag modified by console_suspend(). Seems OK to me. -Doug
Hi, On Fri, Jun 13, 2025 at 3:52 AM Petr Mladek <pmladek@suse.com> wrote: > > On Thu 2025-06-12 16:16:09, Doug Anderson wrote: > > Hi, > > > > On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pmladek@suse.com> wrote: > > > > > > > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char > > > > > > > *opt) > > > > > > > console_list_lock(); > > > > > > > for_each_console(con) { > > > > > > > if (con->write && con->read && > > > > > > > - (con->flags & (CON_BOOT | CON_ENABLED)) && > > > > > > > + (con->flags & CON_BOOT) && > > > > > > > + ((con->flags & CON_SUSPENDED) == 0) && > > > > > > > > > > > > I haven't tried running the code, so I could easily be mistaken, > > > > > > but... > > > > > > > > > > > > ...the above doesn't seem like the correct conversion. The old > > > > > > expression was: > > > > > > > > > > > > (con->flags & (CON_BOOT | CON_ENABLED)) > > > > > > > > > It is easy to get confused by the register_console() code. And > > > it has been even worse some years ago. > > > > > > Anyway, the current code sets CON_ENABLED for all registered > > > consoles, including CON_BOOT consoles. The flag is cleared only > > > by console_suspend()[*] or unregister_console(). > > > > > > IMHO, kgdboc_earlycon_init() does not need to care about > > > console_suspend() because the kernel could not be suspended > > > during boot. Does this makes sense? > > > > Yeah, makes sense to me. > > > > > Resume: > > > > > > I would remove the check of CON_ENABLED or CON_SUSPENDED > > > from kgdboc_earlycon_init() completely. > > > > > > IMHO, we should keep the check of CON_BOOT because we do not > > > want to register "normal" console drivers as kgdboc_earlycon_io_ops. > > > It is later removed by kgdboc_earlycon_deinit(). I guess > > > that the code is not ready to handle a takeover from normal > > > to normal (even the same) console driver. > > > > I'm not sure I understand your last sentence there. In general the > > code handling all of the possible handoff (or lack of handoff) cases > > between kgdboc_earlycon and regular kgdboc is pretty wacky. At one > > point I thought through it all and tried to test as many scenarios as > > I could and I seem to remember trying to model some of the behavior on > > how earlycon worked. If something looks broken here then let me know. > > Later update: The code is safe. The scenario below does not exist, > see the "WAIT:" section below. > > > I am not familiar with the kgdb init code. I thought about the > following scenario: > > 1. kgdboc_earlycon_init() registers some struct console via > > kgdboc_earlycon_io_ops.cons = con; > pr_info("Going to register kgdb with earlycon '%s'\n", con->name); > if (kgdb_register_io_module(&kgdboc_earlycon_io_ops) != 0) { > > and sets > > earlycon_orig_exit = con->exit; > con->exit = kgdboc_earlycon_deferred_exit; > > > 2. Later, configure_kgdboc() would find some "preferred" console > and register it via > > for_each_console_srcu(cons) { > int idx; > if (cons->device && cons->device(cons, &idx) == p && > idx == tty_line) { > kgdboc_io_ops.cons = cons; > [...] > err = kgdb_register_io_module(&kgdboc_io_ops); > > , where kgdb_register_io_module would call deinit for the > previously registered kgdboc_earlycon_io_ops: > > if (old_dbg_io_ops) { > old_dbg_io_ops->deinit(); > return 0; > } > > , where kgdboc_earlycon_deinit() might call the .exit() callback > > kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons); > > > BANG: If both "kgdboc_earlycon_io_ops" and "kgdboc_io_ops" pointed to > the same struct console then this might call .exit() callback > for a console which is still being used. > > But I am wrong, see below. > > WAIT: > > I have got all the pieces together when writing this mail). > I see that the code is safe, namely: > > static void kgdboc_earlycon_deinit(void) > { > if (!kgdboc_earlycon_io_ops.cons) > return; > > if (kgdboc_earlycon_io_ops.cons->exit == kgdboc_earlycon_deferred_exit) > /* > * kgdboc_earlycon is exiting but original boot console exit > * was never called (AKA kgdboc_earlycon_deferred_exit() > * didn't ever run). Undo our trap. > */ > kgdboc_earlycon_io_ops.cons->exit = earlycon_orig_exit; > else if (kgdboc_earlycon_io_ops.cons->exit) > /* > * We skipped calling the exit() routine so we could try to > * keep using the boot console even after it went away. We're > * finally done so call the function now. > */ > kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons); > > kgdboc_earlycon_io_ops.cons = NULL; > } > > It calls kgdboc_earlycon_io_ops.cons->exit() only when > unregister_console() tried to call the original con->exit() > which was reassigned to kgdboc_earlycon_deferred_exit()... > > Updated resume: > > It looks to me that even normal console can be used by > kgdboc_earlycon_io_ops and we could remove even the check > of the CON_BOOT flag. > > My expectation: > > If a "struct console" is registered then the driver is used > by printk() and it should be safe even for kgdboc_earlycon, > as long as it has both "con->write" and "con->read" callbacks. > > So the check in kgdboc_earlycon_init() might be: > > for_each_console(con) { > if (con->write && con->read && > (!opt || !opt[0] || strcmp(con->name, opt) == 0)) > break; > } > > Heh, I hope that you were able to follow my thoughts and I did not > create even bigger confusion. I didn't go back to the code and trace through every step you made, but it sounds like the code looks OK even if it's a bit convoluted (sorry about that! at least it's commented!). I agree with your final suggestion for the check. That has the side effect of also being less of a change from what we had before. Because of how the code was written before, it would have allowed any console because it checked for "enabled or boot" and all consoles were enabled. So just getting rid of the check for flags seems reasonable to me. -Doug
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c index 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b006b2923583a0d2 100644 --- a/drivers/tty/serial/kgdboc.c +++ b/drivers/tty/serial/kgdboc.c @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char *opt) console_list_lock(); for_each_console(con) { if (con->write && con->read && - (con->flags & (CON_BOOT | CON_ENABLED)) && + (con->flags & CON_BOOT) && + ((con->flags & CON_SUSPENDED) == 0) && (!opt || !opt[0] || strcmp(con->name, opt) == 0)) break; }
All consoles found on for_each_console are registered, meaning that all of them are CON_ENABLED. The code tries to find an active console, so check if the console is not suspended instead. Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> --- drivers/tty/serial/kgdboc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)