Re: [PATCH] kgdboc: Be a bit more robust about handling earlycon leaving

[Doug Anderson <dianders@chromium.org>]

Hi,

On Fri, May 1, 2020 at 4:49 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Apr 30, 2020 at 09:59:09AM -0700, Douglas Anderson wrote:
> > The original implementation of kgdboc_earlycon included a comment
> > about how it was impossible to get notified about the boot console
> > going away without making changes to the Linux core.  Since folks
> > often don't want to change the Linux core for kgdb's purposes, the
> > kgdboc_earlycon implementation did a bit of polling to figure out when
> > the boot console went away.
> >
> > It turns out, though, that it is possible to get notified about the
> > boot console going away.  The solution is either clever or a hack
> > depending on your viewpoint.  ...or, perhaps, a clever hack.  All we
> > need to do is head-patch the "exit" routine of the boot console.  We
> > know that "struct console" must be writable because it has a "next"
> > pointer in it, so we can just put our own exit routine in, do our
> > stuff, and then call back to the original.
>
> I think I'm in the hack camp on this one!

LOL.  We can drop it if need be and wait to see if someone actually
ends up in the dead-zone.  Though I gotta earn the name "kernel
hacker" somehow, no?  ;-)


> > This works great to get notified about the boot console going away.
> > The (slight) problem is that in the context of the boot console's exit
> > routine we can't call tty_find_polling_driver().
>
> I presume this is something to do with the tty_mutex?

Actually, no.

trying to acquire lock:
ffffff81b34436a8 (&port->mutex){+.+.}-{3:3}, at: uart_poll_init+0x70/0x164
but task is already holding lock:
ffffff81b34436a8 (&port->mutex){+.+.}-{3:3}, at: uart_add_one_port+0x84/0x49c

Here's the stack (I just put a call to kgdboc_earlycon_exit_work_fn()
straight into kgdboc_earlycon_trap_exit() to reproduce which is why it
still looks like there's a worker:

uart_poll_init+0x70/0x164
tty_find_polling_driver+0x130/0x18c
configure_kgdboc+0x9c/0x164
kgdboc_earlycon_exit_work_fn+0x30/0x5c
kgdboc_earlycon_trap_exit+0x1c/0x4c
unregister_console+0xd4/0x100
register_console+0x374/0x39c
uart_add_one_port+0x418/0x49c
qcom_geni_serial_probe+0x230/0x330

If we continue on this patch I'll add it to the commit message.


> > We solve this by
> > kicking off some work on the system_wq when we get notified and this
> > works pretty well.
>
> There are some problems with the workqueue approach.
>
> Firstly, its runs too early on many systems (esp. those that register
> the console from a console initcall. kgdboc cannot find the tty, I think
> because the console is registered a long time before the corresponding
> tty comes up.

Ah, I see.  So the normal console comes along an inits and kicks the
boot console out but the tty isn't there yet.  :(  I guess my solution
only works if uart_add_one_port() is used and we hit this case in
uart_configure_port():

/*
 * If this driver supports console, and it hasn't been
 * successfully registered yet, try to re-register it.
 * It may be that the port was not available.
 */

...if you've done something to register the console earlier then
you're outta luck.  I'm not sure there's a ton we can do here.  I
don't think we can really transition over to running kgdb on the main
console because, if nothing else, the write() function for those tend
to grab "uport->lock" which doesn't seem like it's always safe.  Then
there'd be the problem of adding read() to all these consoles and
assuming it's non-blocking.

I feel like maybe the best that can be done is, now that earlycon
works as well as it does, to suggest that people _stop_ trying to
register their main console so early.  ;-)

(OK, now that I've read your 2nd message I guess there is something we
can do: keep using the boot console).


In general, though, my patch shouldn't necessarily make anything
_worse_ for you.  When the boot console's exit() routine is called
then you really can't use it any more.  It shouldn't hurt to check if
the TTY is available so we can switch right away.  I guess if your
case is common, though, we'd maybe want to get rid of the
cleanup_earlycon() in kgdboc_earlycon_exit_work_fn() because if you
attached gdb in kgdb_earlycon the debug_core will throw its hissyfit.
We can just cross our fingers and hope the TTY comes along before we
end up in the debugger again.


> Secondly I am seeing warnings related to the tty_mutex where the
> might_sleep() machinery ends up flushing the active workqueue.
>
> [   39.298332] ------------[ cut here ]------------
> [   39.298332] WARNING: CPU: 0 PID: 5 at kernel/workqueue.c:3033 __flush_work+00
> [   39.298332] Modules linked in:
> [   39.298332] CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.7.0-rc3+ #47
> [   39.298332] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-204
> [   39.298332] Workqueue: events kgdboc_earlycon_exit_work_fn
> [   39.298332] RIP: 0010:__flush_work+0x19c/0x1c0
> [   39.298332] Code: 4c 8b 6d 20 e9 06 ff ff ff 41 c6 04 24 00 fb 45 31 f6 eb 8f
> [   39.298332] RSP: 0018:ffff993500033dd0 EFLAGS: 00010246
> [   39.298332] RAX: 0000000000000000 RBX: ffffffffadcd0720 RCX: 0000000000000001
> [   39.298332] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffadcd0820
> [   39.298332] RBP: ffff8a633ec299c0 R08: 0000000000000000 R09: 0000000000000001
> [   39.298332] R10: 000000000000000a R11: f000000000000000 R12: 00000000ffffffed
> [   39.298332] R13: ffff8a633e408840 R14: 0000000000000000 R15: ffff8a633e408840
> [   39.298332] FS:  0000000000000000(0000) GS:ffff8a633ec00000(0000) knlGS:00000
> [   39.298332] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   39.298332] CR2: ffff8a6333201000 CR3: 0000000032a0a000 CR4: 00000000000006f0
> [   39.298332] Call Trace:
> [   39.298332]  ? _cond_resched+0x10/0x20
> [   39.298332]  ? mutex_lock+0x9/0x30
> [   39.298332]  ? tty_find_polling_driver+0x134/0x1a0
> [   39.298332]  configure_kgdboc+0x12d/0x1c0
> [   39.298332]  kgdboc_earlycon_exit_work_fn+0x1a/0x40
> [   39.298332]  process_one_work+0x1d3/0x380
> [   39.298332]  worker_thread+0x45/0x3c0
> [   39.298332]  kthread+0xf6/0x130
> [   39.298332]  ? process_one_work+0x380/0x380
> [   39.298332]  ? kthread_park+0x80/0x80
> [   39.298332]  ret_from_fork+0x22/0x40
> [   39.298332] ---[ end trace 1190f578d6e11204 ]---
> [   39.298338] KGDB: Unregistered I/O driver kgdboc_earlycon, debugger disabled

This is weird.  Why don't I see this?  Oh, I see.  It's because your
console is replacing the boot console so early so "workqueue_init"
hasn't run yet.  Mine happens much, much later.

...I can try to dig more if we want to continue going down this path,
but in general it should be fine to grab a mutex on a worker, right?
...and the workers shouldn't start running until it's safe to run?  Is
this just a race where we don't set "wq_online = true" early enough
and pending work has run or something?  Maybe if you drop into the
debugger at the time of this warning you'll find that some other task
is running and somewhere midway though workqueue_init()?

-Doug