Message-ID: <aEwC81RhvveGP73Y@pathway.suse.cz>
Date: Fri, 13 Jun 2025 12:52:35 +0200
From: Petr Mladek <pmladek@...e.com>
To: Doug Anderson <dianders@...omium.org>
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

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.

Best Regards,
Petr

Powered by blists - more mailing lists