Message-ID: <CAD=FV=XwJ8b2Qd52dUG7DrbkVz2XdjCgXoThh2i3gi=+vGqFAw@mail.gmail.com>
Date: Fri, 13 Jun 2025 09:44:17 -0700
From: Doug Anderson <dianders@...omium.org>
To: Petr Mladek <pmladek@...e.com>
Cc: Marcos Paulo de Souza <mpdesouza@...e.com>, Steven Rostedt <rostedt@...dmis.org>, 
	John Ogness <john.ogness@...utronix.de>, Sergey Senozhatsky <senozhatsky@...omium.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Jason Wessel <jason.wessel@...driver.com>, Daniel Thompson <danielt@...nel.org>, 
	Richard Weinberger <richard@....at>, Anton Ivanov <anton.ivanov@...bridgegreys.com>, 
	Johannes Berg <johannes@...solutions.net>, linux-kernel@...r.kernel.org, 
	linux-serial@...r.kernel.org, kgdb-bugreport@...ts.sourceforge.net, 
	linux-um@...ts.infradead.org
Subject: Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead
 of CON_ENABLED

Hi,

On Fri, Jun 13, 2025 at 3:52 AM Petr Mladek <pmladek@...e.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@...e.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

Powered by blists - more mailing lists