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

[Doug Anderson <dianders@chromium.org>]

Hi,

On Mon, May 4, 2020 at 4:53 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Fri, May 01, 2020 at 10:36:14AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, May 1, 2020 at 6:32 AM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > >
> > > On Fri, May 01, 2020 at 12:49:43PM +0100, Daniel Thompson 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!
> > > >
> > > >
> > > > > 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?
> > > > > 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.
> > >
> > > ... so did a couple of experiments to avoid the workqueue.
> > >
> > > It occured to me that, since we have interfered with deinit() then the
> > > console hasn't actually been uninitialized meaning we could still use it.
> > > This does exposes us to risks similar to keep_bootcon but in exchange
> > > there is no window where kgdb is broken (and no need to panic).
> > >
> > > My prototype is minimal but I did wonder about ripping out all the
> > > code to defend against removal of the earlycon and simply keep the
> > > earlycon around until a new kgdbio handler is installed.
> >
> > It took me a little while, but I finally see what you're saying.
> > You're saying that we keep calling into the boot console even though
> > it's no longer in the list of consoles.  Then we temporarily disable
> > the boot console's exit routine until kgdb_earlycon() is done.  (side
> > note: the exit routine was recently added and probably most consoles
> > don't use it).
>
> Certainly none of the devices with a read() method have an exit().
>
>
> > OK, that doesn't seem totally insane.  It actually works OK for you?
>
> I tested on qemu/x86-64 (8250) and qemu/arm64 (pl011). In both cases it
> works well.
>
>
> > It's probably at least worth a warning in the log if we detect that
> > we're using the boot console and it's not in the console list anymore.
> > Then if kgdb starts misbehaving someone might have a clue.
>
> Yes, I'll add that.
>
>
> > If your solution is OK we might also want to remove the call to
> > cleanup_earlycon_if_invalid() in configure_kgdboc() too.
>
> That's what I thought, yes. Although it might be best to handle that
> by ripping it out of the original patch set.

I've incorporated ideas from my patch and yours into a v4 patchset of
the original series.  Note that I didn't include your earlycon
deferral patchset [1] in my series which means it'll need to be
rebased.  Hopefully this is OK since I think the rebase will be easy,
but yell if you want an extra pair of eyes on it.

[1] https://lore.kernel.org/r/20200430161741.1832050-1-daniel.thompson@linaro.org


-Doug