On Thu 2025-06-12 16:16:09, Doug Anderson wrote:
> Hi,
>
> On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pmla...@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.
Best Regards,
Petr
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
