From: Sumit Garg <sumit.garg@linaro.org>
To: Petr Mladek <pmladek@suse.com>
Cc: Daniel Thompson <daniel.thompson@linaro.org>,
kgdb-bugreport@lists.sourceforge.net,
Jason Wessel <jason.wessel@windriver.com>,
Douglas Anderson <dianders@chromium.org>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O
Date: Fri, 29 May 2020 11:16:04 +0530 [thread overview]
Message-ID: <CAFA6WYO6D-9dT46Zpmm9diW7QQPTWbT64K7XKXQasdwJ4xuZxw@mail.gmail.com> (raw)
In-Reply-To: <20200528145721.GE11286@linux-b0ei>
On Thu, 28 May 2020 at 20:27, Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2020-05-28 12:26:20, Daniel Thompson wrote:
> > On Thu, May 28, 2020 at 11:48:48AM +0530, Sumit Garg wrote:
> > > On Wed, 27 May 2020 at 19:01, Daniel Thompson
> > > <daniel.thompson@linaro.org> wrote:
> > > >
> > > > On Wed, May 27, 2020 at 11:55:59AM +0530, Sumit Garg wrote:
> > > > > In kgdb NMI context, calling console handlers isn't safe due to locks
> > > > > used in those handlers which could lead to a deadlock. Although, using
> > > > > oops_in_progress increases the chance to bypass locks in most console
> > > > > handlers but it might not be sufficient enough in case a console uses
> > > > > more locks (VT/TTY is good example).
> > > > >
> > > > > Currently when a driver provides both polling I/O and a console then kdb
> > > > > will output using the console. We can increase robustness by using the
> > > > > currently active polling I/O driver (which should be lockless) instead
> > > > > of the corresponding console. For several common cases (e.g. an
> > > > > embedded system with a single serial port that is used both for console
> > > > > output and debugger I/O) this will result in no console handler being
> > > > > used.
> > > >
> > > > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > > > > index b072aeb..05d165d 100644
> > > > > --- a/include/linux/kgdb.h
> > > > > +++ b/include/linux/kgdb.h
> > > > > @@ -275,6 +275,7 @@ struct kgdb_arch {
> > > > > * for the I/O driver.
> > > > > * @is_console: 1 if the end device is a console 0 if the I/O device is
> > > > > * not a console
> > > > > + * @tty_drv: Pointer to polling tty driver.
> > > > > */
> > > > > struct kgdb_io {
> > > > > const char *name;
> > > > > @@ -285,6 +286,7 @@ struct kgdb_io {
> > > > > void (*pre_exception) (void);
> > > > > void (*post_exception) (void);
> > > > > int is_console;
> > > > > + struct tty_driver *tty_drv;
> > > >
> > > > Should this be a struct tty_driver or a struct console?
> > > >
> > > > In other words if the lifetime the console structure is the same as the
> > > > tty_driver then isn't it better to capture the console instead
> > > > (easier to compare and works with non-tty devices such as the
> > > > USB debug mode).
> > > >
> > >
> > > IIUC, you mean to say we can easily replace "is_console" with "struct
> > > console". This sounds feasible and should be a straightforward
> > > comparison in order to prefer "dbg_io_ops" over console handlers. So I
> > > will switch to use "struct console" instead.
> >
> > My comment contains an if ("if the lifetime of the console structure is
> > the same") so you need to check that it is true before sharing a patch to
> > make the change.
>
> Honestly, I am not completely familiar with the console an tty drivers
> code.
>
> Anyway, struct console is typically statically defined by the console
> driver code. It is not must to have but I am not aware of any
> driver where it would be dynamically defined.
>
Yes this is mine understanding as well.
> On the other hand, struct tty_driver is dynamically allocated
> when the driver gets initialized.
>
> So I would say that it is pretty safe to store struct console.
Okay.
> Well, you need to call con->device() to see if the tty_driver
> is actually initialized.
Agree and con->device() is already invoked here [1]. So we only need
to store struct console if con->device() invocation returns success.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/kgdboc.c#n174
-Sumit
>
> Best Regards,
> Petr
prev parent reply other threads:[~2020-05-29 5:46 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-27 6:25 [PATCH v3 0/4] kdb: Improve console handling Sumit Garg
2020-05-27 6:25 ` [PATCH v3 1/4] kdb: Re-factor kdb_printf() message write code Sumit Garg
2020-05-27 8:29 ` Daniel Thompson
2020-05-27 10:01 ` Sumit Garg
2020-05-27 6:25 ` [PATCH v3 2/4] kdb: Check status of console prior to invoking handlers Sumit Garg
2020-05-27 14:26 ` Daniel Thompson
2020-05-27 6:25 ` [PATCH v3 3/4] kdb: Make kdb_printf robust to run in NMI context Sumit Garg
2020-05-27 14:26 ` Daniel Thompson
2020-05-28 7:42 ` Sumit Garg
2020-05-27 6:25 ` [PATCH v3 4/4] kdb: Switch kdb_msg_write() to use safer polling I/O Sumit Garg
2020-05-27 13:31 ` Daniel Thompson
2020-05-28 6:18 ` Sumit Garg
2020-05-28 11:26 ` Daniel Thompson
2020-05-28 14:57 ` Petr Mladek
2020-05-29 5:46 ` Sumit Garg [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFA6WYO6D-9dT46Zpmm9diW7QQPTWbT64K7XKXQasdwJ4xuZxw@mail.gmail.com \
--to=sumit.garg@linaro.org \
--cc=daniel.thompson@linaro.org \
--cc=dianders@chromium.org \
--cc=jason.wessel@windriver.com \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=pmladek@suse.com \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).