LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Jason Wessel <jason.wessel@windriver.com>,
	Daniel Thompson <daniel.thompson@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	kgdb-bugreport@lists.sourceforge.net,
	LKML <linux-kernel@vger.kernel.org>, Jiri Slaby <jslaby@suse.com>,
	Jeremy Kerr <jk@ozlabs.org>,
	linux-serial@vger.kernel.org
Subject: Re: [PATCH 2/7] serial: core: Allow processing sysrq at port unlock time
Date: Mon, 29 Oct 2018 22:31:49 -0700	[thread overview]
Message-ID: <CAD=FV=WCocrN0ft3-VCsxF2-ftNpKNwmU33taWPkMb=rQF_U=Q@mail.gmail.com> (raw)
In-Reply-To: <20181029180707.207546-3-dianders@chromium.org>

Hi,

On Mon, Oct 29, 2018 at 11:08 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> Right now serial drivers process sysrq keys deep in their character
> receiving code.  This means that they've already grabbed their
> port->lock spinlock.  This can end up getting in the way if we've go
> to do serial stuff (especially kgdb) in response to the sysrq.
>
> Serial drivers have various hacks in them to handle this.  Looking at
> '8250_port.c' you can see that the console_write() skips locking if
> we're in the sysrq handler.  Looking at 'msm_serial.c' you can see
> that the port lock is dropped around uart_handle_sysrq_char().
>
> It turns out that these hacks aren't exactly perfect.  If you have
> lockdep turned on and use something like the 8250_port hack you'll get
> a splat that looks like:
>
>   WARNING: possible circular locking dependency detected
>   [...] is trying to acquire lock:
>   ... (console_owner){-.-.}, at: console_unlock+0x2e0/0x5e4
>
>   but task is already holding lock:
>   ... (&port_lock_key){-.-.}, at: serial8250_handle_irq+0x30/0xe4
>
>   which lock already depends on the new lock.
>
>   the existing dependency chain (in reverse order) is:
>
>   -> #1 (&port_lock_key){-.-.}:
>          _raw_spin_lock_irqsave+0x58/0x70
>          serial8250_console_write+0xa8/0x250
>          univ8250_console_write+0x40/0x4c
>          console_unlock+0x528/0x5e4
>          register_console+0x2c4/0x3b0
>          uart_add_one_port+0x350/0x478
>          serial8250_register_8250_port+0x350/0x3a8
>          dw8250_probe+0x67c/0x754
>          platform_drv_probe+0x58/0xa4
>          really_probe+0x150/0x294
>          driver_probe_device+0xac/0xe8
>          __driver_attach+0x98/0xd0
>          bus_for_each_dev+0x84/0xc8
>          driver_attach+0x2c/0x34
>          bus_add_driver+0xf0/0x1ec
>          driver_register+0xb4/0x100
>          __platform_driver_register+0x60/0x6c
>          dw8250_platform_driver_init+0x20/0x28
>          ...
>
>   -> #0 (console_owner){-.-.}:
>          lock_acquire+0x1e8/0x214
>          console_unlock+0x35c/0x5e4
>          vprintk_emit+0x230/0x274
>          vprintk_default+0x7c/0x84
>          vprintk_func+0x190/0x1bc
>          printk+0x80/0xa0
>          __handle_sysrq+0x104/0x21c
>          handle_sysrq+0x30/0x3c
>          serial8250_read_char+0x15c/0x18c
>          serial8250_rx_chars+0x34/0x74
>          serial8250_handle_irq+0x9c/0xe4
>          dw8250_handle_irq+0x98/0xcc
>          serial8250_interrupt+0x50/0xe8
>          ...
>
>   other info that might help us debug this:
>
>    Possible unsafe locking scenario:
>
>          CPU0                    CPU1
>          ----                    ----
>     lock(&port_lock_key);
>                                  lock(console_owner);
>                                  lock(&port_lock_key);
>     lock(console_owner);
>
>    *** DEADLOCK ***
>
> The hack used in 'msm_serial.c' doesn't cause the above splats but it
> seems a bit ugly to unlock / lock our spinlock deep in our irq
> handler.
>
> It seems like we could defer processing the sysrq until the end of the
> interrupt handler right after we've unlocked the port.  With this
> scheme if a whole batch of sysrq characters comes in one irq then we
> won't handle them all, but that seems like it should be a fine
> compromise.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  include/linux/serial_core.h | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 047fa67d039b..78de9d929762 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -175,6 +175,7 @@ struct uart_port {
>         struct console          *cons;                  /* struct console, if any */
>  #if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(SUPPORT_SYSRQ)
>         unsigned long           sysrq;                  /* sysrq timeout */
> +       unsigned int            sysrq_ch;               /* char for sysrq */
>  #endif
>
>         /* flags must be updated while holding port mutex */
> @@ -485,8 +486,42 @@ uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>         }
>         return 0;
>  }
> +static inline int
> +uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
> +{
> +       if (port->sysrq) {
> +               if (ch && time_before(jiffies, port->sysrq)) {
> +                       port->sysrq_ch = ch;
> +                       port->sysrq = 0;
> +                       return 1;
> +               }
> +               port->sysrq = 0;
> +       }
> +       return 0;
> +}
> +static inline void
> +uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
> +{
> +       int sysrq_ch;
> +
> +       sysrq_ch = port->sysrq_ch;
> +       port->sysrq_ch = 0;
> +
> +       spin_unlock_irqrestore(&port->lock, irqflags);
> +
> +       if (sysrq_ch)
> +               handle_sysrq(sysrq_ch);
> +}
>  #else
> -#define uart_handle_sysrq_char(port,ch) ({ (void)port; 0; })
> +static inline int
> +uart_handle_sysrq_char(struct uart_port *port, unsigned int ch) { return 0; }
> +static inline int
> +uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch) { return 0; }
> +static inline void
> +uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
> +{
> +       spin_unlock_irqrestore(&port->lock, irqflags);
> +}

Jeremy wrote me to point out that I messed up and didn't get this
patch posted to the linux-serial list.  Sorry about that.  :(  I guess
get_mainatiners doesn't notice that this include file is relevant to
serial and I didn't notice either since I was too focused on trying to
figure out if it was really the right call to Cc all the arch
maintainers on the cover letter and the last patch...  Sigh.

If/when I need to repost, I'll make sure to get linux-serial.  For now
at least they are on LKML so probably the easiest place to find all
the patches is:

https://lore.kernel.org/patchwork/cover/1004280/

...if you clock on the "show" next to "Related" you get the whole
series.  Using the message ID from there you can also find them at:

https://lkml.kernel.org/r/20181029180707.207546-1-dianders@chromium.org

Both places allow you to grab 'mbox' files which (which a bit of a
hassle--sorry) can allow you to reply /apply patches.

-Doug

  reply	other threads:[~2018-10-30  5:32 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-29 18:07 [PATCH 0/7] serial: Finish kgdb on qcom_geni; fix many lockdep splats w/ kgdb Douglas Anderson
2018-10-29 18:07 ` [PATCH 1/7] serial: qcom_geni_serial: Finish supporting sysrq Douglas Anderson
2018-11-02 16:47   ` Stephen Boyd
2018-10-29 18:07 ` [PATCH 2/7] serial: core: Allow processing sysrq at port unlock time Douglas Anderson
2018-10-30  5:31   ` Doug Anderson [this message]
2018-10-29 18:07 ` [PATCH 3/7] serial: qcom_geni_serial: Process " Douglas Anderson
2018-10-29 18:07 ` [PATCH 4/7] serial: core: Include console.h from serial_core.h Douglas Anderson
2018-10-29 18:07 ` [PATCH 5/7] serial: 8250: Process sysrq at port unlock time Douglas Anderson
2018-10-29 18:07 ` [PATCH 6/7] smp: Don't yell about IRQs disabled in kgdb_roundup_cpus() Douglas Anderson
2018-10-30  8:25   ` Peter Zijlstra
2018-10-30  9:41   ` Daniel Thompson
2018-10-30 22:21     ` Doug Anderson
2018-10-29 18:07 ` [PATCH 7/7] kgdb: Remove irq flags and local_irq_enable/disable from roundup Douglas Anderson
2018-10-30 11:46   ` Daniel Thompson
2018-10-30 22:22     ` Doug Anderson
2018-10-30 11:56 ` [PATCH 0/7] serial: Finish kgdb on qcom_geni; fix many lockdep splats w/ kgdb Daniel Thompson
2018-10-30 12:31   ` Russell King - ARM Linux
2018-10-30 12:36 ` Andy Shevchenko

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='CAD=FV=WCocrN0ft3-VCsxF2-ftNpKNwmU33taWPkMb=rQF_U=Q@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=daniel.thompson@linaro.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jason.wessel@windriver.com \
    --cc=jk@ozlabs.org \
    --cc=jslaby@suse.com \
    --cc=kgdb-bugreport@lists.sourceforge.net \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH 2/7] serial: core: Allow processing sysrq at port unlock time' \
    /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

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).