* [PATCH 3/7] serial: qcom_geni_serial: Process sysrq at port unlock time
From: Douglas Anderson @ 2018-10-29 18:07 UTC (permalink / raw)
To: Jason Wessel, Daniel Thompson, tglx, mingo, gregkh
Cc: linux-arm-msm, kgdb-bugreport, Douglas Anderson, linux-kernel,
linux-serial, jslaby
In-Reply-To: <20181029180707.207546-1-dianders@chromium.org>
Let's take advantage of the new ("serial: core: Allow processing sysrq
at port unlock time") to handle sysrqs more cleanly.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/tty/serial/qcom_geni_serial.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3c8e0202da8b..20edce1e222e 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -499,9 +499,7 @@ static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
continue;
}
- spin_unlock(&uport->lock);
- sysrq = uart_handle_sysrq_char(uport, buf[c]);
- spin_lock(&uport->lock);
+ sysrq = uart_prepare_sysrq_char(uport, buf[c]);
if (!sysrq)
tty_insert_flip_char(tport, buf[c], TTY_NORMAL);
@@ -811,7 +809,8 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
qcom_geni_serial_handle_rx(uport, drop_rx);
out_unlock:
- spin_unlock_irqrestore(&uport->lock, flags);
+ uart_unlock_and_check_sysrq(uport, flags);
+
return IRQ_HANDLED;
}
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* [PATCH 5/7] serial: 8250: Process sysrq at port unlock time
From: Douglas Anderson @ 2018-10-29 18:07 UTC (permalink / raw)
To: Jason Wessel, Daniel Thompson, tglx, mingo, gregkh
Cc: linux-arm-msm, kgdb-bugreport, Douglas Anderson, nm, marex,
vigneshr, linux-aspeed, jk, andrew, rolf.evers.fischer,
linux-kernel, tony, Jisheng.Zhang, joel, linux-serial, jslaby,
asierra, andriy.shevchenko, dan.carpenter, linux-arm-kernel
In-Reply-To: <20181029180707.207546-1-dianders@chromium.org>
Let's take advantage of the new ("serial: core: Allow processing sysrq
at port unlock time") to handle sysrqs more cleanly.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I don't have a great way to test all 8250 variants, but I've at least
tested rk3288 / rk3399 and they seem to work. Hopefully I got the
aspeed_vuart / fsl / omap variants right (I only compile tested
those).
drivers/tty/serial/8250/8250_aspeed_vuart.c | 6 +++++-
drivers/tty/serial/8250/8250_fsl.c | 6 +++++-
drivers/tty/serial/8250/8250_omap.c | 6 +++++-
drivers/tty/serial/8250/8250_port.c | 8 +++-----
4 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 435bec40dee6..0438d9a905ce 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -5,6 +5,10 @@
* Copyright (C) 2016 Jeremy Kerr <jk@ozlabs.org>, IBM Corp.
* Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>, IBM Corp.
*/
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
#include <linux/device.h>
#include <linux/module.h>
#include <linux/of_address.h>
@@ -293,7 +297,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
if (lsr & UART_LSR_THRE)
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_unlock_and_check_sysrq(port, flags);
return 1;
}
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 6640a4c7ddd1..ff3dcaea5d93 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -1,4 +1,8 @@
// SPDX-License-Identifier: GPL-2.0
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
#include <linux/serial_reg.h>
#include <linux/serial_8250.h>
@@ -54,7 +58,7 @@ int fsl8250_handle_irq(struct uart_port *port)
serial8250_tx_chars(up);
up->lsr_saved_flags = orig_lsr;
- spin_unlock_irqrestore(&up->port.lock, flags);
+ uart_unlock_and_check_sysrq(&up->port, flags);
return 1;
}
EXPORT_SYMBOL_GPL(fsl8250_handle_irq);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index a019286f8bb6..ad7ba7d0f28d 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -8,6 +8,10 @@
*
*/
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
#include <linux/device.h>
#include <linux/io.h>
#include <linux/module.h>
@@ -1085,7 +1089,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
}
}
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_unlock_and_check_sysrq(port, flags);
serial8250_rpm_put(up);
return 1;
}
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f776b3eafb96..c39482b96111 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1755,7 +1755,7 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
else if (lsr & UART_LSR_FE)
flag = TTY_FRAME;
}
- if (uart_handle_sysrq_char(port, ch))
+ if (uart_prepare_sysrq_char(port, ch))
return;
uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);
@@ -1897,7 +1897,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE))
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_unlock_and_check_sysrq(port, flags);
return 1;
}
EXPORT_SYMBOL_GPL(serial8250_handle_irq);
@@ -3258,9 +3258,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
serial8250_rpm_get(up);
- if (port->sysrq)
- locked = 0;
- else if (oops_in_progress)
+ if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
else
spin_lock_irqsave(&port->lock, flags);
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* Re: [PATCH 2/7] serial: core: Allow processing sysrq at port unlock time
From: Doug Anderson @ 2018-10-30 5:31 UTC (permalink / raw)
To: Jason Wessel, Daniel Thompson, Thomas Gleixner, Ingo Molnar,
Greg Kroah-Hartman
Cc: linux-arm-msm, kgdb-bugreport, LKML, Jiri Slaby, Jeremy Kerr,
linux-serial
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
^ permalink raw reply
* [PATCH] serial: sh-sci: Fix could not remove dev_attr_rx_fifo_timeout
From: Yoshihiro Shimoda @ 2018-10-30 6:13 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial; +Cc: linux-renesas-soc, stable, Yoshihiro Shimoda
This patch fixes an issue that the sci_remove() could not remove
dev_attr_rx_fifo_timeout because uart_remove_one_port() set
the port->port.type to PORT_UNKNOWN.
Reported-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Fixes: 5d23188a473d ("serial: sh-sci: make RX FIFO parameters tunable via sysfs")
Cc: <stable@vger.kernel.org> # v4.11+
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
drivers/tty/serial/sh-sci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index ab3f6e91..3649b83 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3102,6 +3102,7 @@ static inline int sci_probe_earlyprintk(struct platform_device *pdev)
static int sci_remove(struct platform_device *dev)
{
struct sci_port *port = platform_get_drvdata(dev);
+ unsigned int type = port->port.type; /* uart_remove_... clears it */
sci_ports_in_use &= ~BIT(port->port.line);
uart_remove_one_port(&sci_uart_driver, &port->port);
@@ -3112,8 +3113,7 @@ static int sci_remove(struct platform_device *dev)
sysfs_remove_file(&dev->dev.kobj,
&dev_attr_rx_fifo_trigger.attr);
}
- if (port->port.type == PORT_SCIFA || port->port.type == PORT_SCIFB ||
- port->port.type == PORT_HSCIF) {
+ if (type == PORT_SCIFA || type == PORT_SCIFB || type == PORT_HSCIF) {
sysfs_remove_file(&dev->dev.kobj,
&dev_attr_rx_fifo_timeout.attr);
}
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] serial: sh-sci: Fix could not remove dev_attr_rx_fifo_timeout
From: Ulrich Hecht @ 2018-10-30 7:01 UTC (permalink / raw)
To: Yoshihiro Shimoda, gregkh, jslaby, linux-serial; +Cc: linux-renesas-soc, stable
In-Reply-To: <1540880015-11877-1-git-send-email-yoshihiro.shimoda.uh@renesas.com>
Thanks for your patch!
> On October 30, 2018 at 7:13 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
>
>
> This patch fixes an issue that the sci_remove() could not remove
> dev_attr_rx_fifo_timeout because uart_remove_one_port() set
> the port->port.type to PORT_UNKNOWN.
>
> Reported-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> Fixes: 5d23188a473d ("serial: sh-sci: make RX FIFO parameters tunable via sysfs")
> Cc: <stable@vger.kernel.org> # v4.11+
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> drivers/tty/serial/sh-sci.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index ab3f6e91..3649b83 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3102,6 +3102,7 @@ static inline int sci_probe_earlyprintk(struct platform_device *pdev)
> static int sci_remove(struct platform_device *dev)
> {
> struct sci_port *port = platform_get_drvdata(dev);
> + unsigned int type = port->port.type; /* uart_remove_... clears it */
>
> sci_ports_in_use &= ~BIT(port->port.line);
> uart_remove_one_port(&sci_uart_driver, &port->port);
> @@ -3112,8 +3113,7 @@ static int sci_remove(struct platform_device *dev)
> sysfs_remove_file(&dev->dev.kobj,
> &dev_attr_rx_fifo_trigger.attr);
> }
> - if (port->port.type == PORT_SCIFA || port->port.type == PORT_SCIFB ||
> - port->port.type == PORT_HSCIF) {
> + if (type == PORT_SCIFA || type == PORT_SCIFB || type == PORT_HSCIF) {
> sysfs_remove_file(&dev->dev.kobj,
> &dev_attr_rx_fifo_timeout.attr);
> }
> --
> 1.9.1
>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
CU
Uli
^ permalink raw reply
* [PATCH v2 1/2] serial: set suppress_bind_attrs flag only if builtin
From: Anders Roxell @ 2018-10-30 11:35 UTC (permalink / raw)
To: linux, gregkh, akpm
Cc: linux-serial, linux-kernel, linux-arm-kernel, linux-mm, tj,
Anders Roxell, Arnd Bergmann
When the test 'CONFIG_DEBUG_TEST_DRIVER_REMOVE=y' is enabled,
arch_initcall(pl011_init) came before subsys_initcall(default_bdi_init).
devtmpfs gets killed because we try to remove a file and decrement the
wb reference count before the noop_backing_device_info gets initialized.
[ 0.332075] Serial: AMBA PL011 UART driver
[ 0.485276] 9000000.pl011: ttyAMA0 at MMIO 0x9000000 (irq = 39, base_baud = 0) is a PL011 rev1
[ 0.502382] console [ttyAMA0] enabled
[ 0.515710] Unable to handle kernel paging request at virtual address 0000800074c12000
[ 0.516053] Mem abort info:
[ 0.516222] ESR = 0x96000004
[ 0.516417] Exception class = DABT (current EL), IL = 32 bits
[ 0.516641] SET = 0, FnV = 0
[ 0.516826] EA = 0, S1PTW = 0
[ 0.516984] Data abort info:
[ 0.517149] ISV = 0, ISS = 0x00000004
[ 0.517339] CM = 0, WnR = 0
[ 0.517553] [0000800074c12000] user address but active_mm is swapper
[ 0.517928] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[ 0.518305] Modules linked in:
[ 0.518839] CPU: 0 PID: 13 Comm: kdevtmpfs Not tainted 4.19.0-rc5-next-20180928-00002-g2ba39ab0cd01-dirty #82
[ 0.519307] Hardware name: linux,dummy-virt (DT)
[ 0.519681] pstate: 80000005 (Nzcv daif -PAN -UAO)
[ 0.519959] pc : __destroy_inode+0x94/0x2a8
[ 0.520212] lr : __destroy_inode+0x78/0x2a8
[ 0.520401] sp : ffff0000098c3b20
[ 0.520590] x29: ffff0000098c3b20 x28: 00000000087a3714
[ 0.520904] x27: 0000000000002000 x26: 0000000000002000
[ 0.521179] x25: ffff000009583000 x24: 0000000000000000
[ 0.521467] x23: ffff80007bb52000 x22: ffff80007bbaa7c0
[ 0.521737] x21: ffff0000093f9338 x20: 0000000000000000
[ 0.522033] x19: ffff80007bbb05d8 x18: 0000000000000400
[ 0.522376] x17: 0000000000000000 x16: 0000000000000000
[ 0.522727] x15: 0000000000000400 x14: 0000000000000400
[ 0.523068] x13: 0000000000000001 x12: 0000000000000001
[ 0.523421] x11: 0000000000000000 x10: 0000000000000970
[ 0.523749] x9 : ffff0000098c3a60 x8 : ffff80007bbab190
[ 0.524017] x7 : ffff80007bbaa880 x6 : 0000000000000c88
[ 0.524305] x5 : ffff0000093d96c8 x4 : 61c8864680b583eb
[ 0.524567] x3 : ffff0000093d6180 x2 : ffffffffffffffff
[ 0.524872] x1 : 0000800074c12000 x0 : 0000800074c12000
[ 0.525207] Process kdevtmpfs (pid: 13, stack limit = 0x(____ptrval____))
[ 0.525529] Call trace:
[ 0.525806] __destroy_inode+0x94/0x2a8
[ 0.526108] destroy_inode+0x34/0x88
[ 0.526370] evict+0x144/0x1c8
[ 0.526636] iput+0x184/0x230
[ 0.526871] dentry_unlink_inode+0x118/0x130
[ 0.527152] d_delete+0xd8/0xe0
[ 0.527420] vfs_unlink+0x240/0x270
[ 0.527665] handle_remove+0x1d8/0x330
[ 0.527875] devtmpfsd+0x138/0x1c8
[ 0.528085] kthread+0x14c/0x158
[ 0.528291] ret_from_fork+0x10/0x18
[ 0.528720] Code: 92800002 aa1403e0 d538d081 8b010000 (c85f7c04)
[ 0.529367] ---[ end trace 5a3dee47727f877c ]---
Rework to set suppress_bind_attrs flag to avoid removing the device when
CONFIG_DEBUG_TEST_DRIVER_REMOVE=y. This applies for pic32_uart and
xilinx_uartps as well.
Co-developed-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
drivers/tty/serial/amba-pl011.c | 2 ++
drivers/tty/serial/pic32_uart.c | 1 +
drivers/tty/serial/xilinx_uartps.c | 1 +
3 files changed, 4 insertions(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index ebd33c0232e6..89ade213a1a9 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2780,6 +2780,7 @@ static struct platform_driver arm_sbsa_uart_platform_driver = {
.name = "sbsa-uart",
.of_match_table = of_match_ptr(sbsa_uart_of_match),
.acpi_match_table = ACPI_PTR(sbsa_uart_acpi_match),
+ .suppress_bind_attrs = IS_BUILTIN(CONFIG_SERIAL_AMBA_PL011),
},
};
@@ -2808,6 +2809,7 @@ static struct amba_driver pl011_driver = {
.drv = {
.name = "uart-pl011",
.pm = &pl011_dev_pm_ops,
+ .suppress_bind_attrs = IS_BUILTIN(CONFIG_SERIAL_AMBA_PL011),
},
.id_table = pl011_ids,
.probe = pl011_probe,
diff --git a/drivers/tty/serial/pic32_uart.c b/drivers/tty/serial/pic32_uart.c
index fd80d999308d..0bdf1687983f 100644
--- a/drivers/tty/serial/pic32_uart.c
+++ b/drivers/tty/serial/pic32_uart.c
@@ -919,6 +919,7 @@ static struct platform_driver pic32_uart_platform_driver = {
.driver = {
.name = PIC32_DEV_NAME,
.of_match_table = of_match_ptr(pic32_serial_dt_ids),
+ .suppress_bind_attrs = IS_BUILTIN(CONFIG_SERIAL_PIC32),
},
};
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 57c66d2c3471..379242b96790 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1719,6 +1719,7 @@ static struct platform_driver cdns_uart_platform_driver = {
.name = CDNS_UART_NAME,
.of_match_table = cdns_uart_of_match,
.pm = &cdns_uart_dev_pm_ops,
+ .suppress_bind_attrs = IS_BUILTIN(CONFIG_SERIAL_XILINX_PS_UART),
},
};
--
2.19.1
^ permalink raw reply related
* [PATCH v2 2/2] writeback: don't decrement wb->refcnt if !wb->bdi
From: Anders Roxell @ 2018-10-30 11:35 UTC (permalink / raw)
To: linux, gregkh, akpm
Cc: linux-serial, linux-kernel, linux-arm-kernel, linux-mm, tj,
Anders Roxell, Arnd Bergmann
In-Reply-To: <20181030113545.30999-1-anders.roxell@linaro.org>
This happened while running in qemu-system-aarch64, the AMBA PL011 UART
driver when enabling CONFIG_DEBUG_TEST_DRIVER_REMOVE.
arch_initcall(pl011_init) came before subsys_initcall(default_bdi_init),
devtmpfs' handle_remove() crashes because the reference count is a NULL
pointer only because wb->bdi hasn't been initialized yet.
Rework so that wb_put have an extra check if wb->bdi before decrement
wb->refcnt and also add a WARN_ON_ONCE to get a warning if it happens again
in other drivers.
Fixes: 52ebea749aae ("writeback: make backing_dev_info host cgroup-specific bdi_writebacks")
Co-developed-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
include/linux/backing-dev-defs.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 9a6bc0951cfa..c31157135598 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -258,6 +258,14 @@ static inline void wb_get(struct bdi_writeback *wb)
*/
static inline void wb_put(struct bdi_writeback *wb)
{
+ if (WARN_ON_ONCE(!wb->bdi)) {
+ /*
+ * A driver bug might cause a file to be removed before bdi was
+ * initialized.
+ */
+ return;
+ }
+
if (wb != &wb->bdi->wb)
percpu_ref_put(&wb->refcnt);
}
--
2.19.1
^ permalink raw reply related
* Re: [PATCH 0/7] serial: Finish kgdb on qcom_geni; fix many lockdep splats w/ kgdb
From: Daniel Thompson @ 2018-10-30 11:56 UTC (permalink / raw)
To: Douglas Anderson
Cc: Jason Wessel, tglx, mingo, gregkh, linux-arm-msm, kgdb-bugreport,
nm, linux-mips, dalias, catalin.marinas, vigneshr, linux-aspeed,
linux-sh, peterz, will.deacon, mhocko, paulus, hpa, sparclinux,
marex, sfr, ysato, linux-hexagon, x86, linux, pombredanne, tony,
mingo, joel, linux-serial, rolf.evers.fischer, jhogan, asierra,
linux-snps-arc
In-Reply-To: <20181029180707.207546-1-dianders@chromium.org>
On Mon, Oct 29, 2018 at 11:07:00AM -0700, Douglas Anderson wrote:
> Looking back, this is pretty much two series squashed that could be
> treated indepdently. The first is a serial series and the second is a
> kgdb series.
Indeed.
I couldn't work out the link between the first 5 patches and the last 2
until I read this...
Is anything in the 01->05 patch set even related to kgdb? From the stack
traces it looks to me like the lock dep warning would trigger for any
sysrq. I think separating into two threads for v2 would be sensible.
Daniel.
>
> For all serial patches I'd expect them to go through the tty tree once
> they've been reviewed.
>
> If folks are OK w/ the 'smp' patch it probably should go in some core
> kernel tree. The kgdb patch won't work without it, though, so to land
> that we'd need coordination between the folks landing that and the
> folks landing the 'smp' patch.
>
>
> Douglas Anderson (7):
> serial: qcom_geni_serial: Finish supporting sysrq
> serial: core: Allow processing sysrq at port unlock time
> serial: qcom_geni_serial: Process sysrq at port unlock time
> serial: core: Include console.h from serial_core.h
> serial: 8250: Process sysrq at port unlock time
> smp: Don't yell about IRQs disabled in kgdb_roundup_cpus()
> kgdb: Remove irq flags and local_irq_enable/disable from roundup
>
> arch/arc/kernel/kgdb.c | 4 +--
> arch/arm/kernel/kgdb.c | 4 +--
> arch/arm64/kernel/kgdb.c | 4 +--
> arch/hexagon/kernel/kgdb.c | 11 ++----
> arch/mips/kernel/kgdb.c | 4 +--
> arch/powerpc/kernel/kgdb.c | 2 +-
> arch/sh/kernel/kgdb.c | 4 +--
> arch/sparc/kernel/smp_64.c | 2 +-
> arch/x86/kernel/kgdb.c | 9 ++---
> drivers/tty/serial/8250/8250_aspeed_vuart.c | 6 +++-
> drivers/tty/serial/8250/8250_fsl.c | 6 +++-
> drivers/tty/serial/8250/8250_omap.c | 6 +++-
> drivers/tty/serial/8250/8250_port.c | 8 ++---
> drivers/tty/serial/qcom_geni_serial.c | 10 ++++--
> include/linux/kgdb.h | 9 ++---
> include/linux/serial_core.h | 38 ++++++++++++++++++++-
> kernel/debug/debug_core.c | 2 +-
> kernel/smp.c | 4 ++-
> 18 files changed, 80 insertions(+), 53 deletions(-)
>
> --
> 2.19.1.568.g152ad8e336-goog
>
^ permalink raw reply
* Re: [PATCH 0/7] serial: Finish kgdb on qcom_geni; fix many lockdep splats w/ kgdb
From: Russell King - ARM Linux @ 2018-10-30 12:31 UTC (permalink / raw)
To: Daniel Thompson
Cc: Douglas Anderson, Jason Wessel, tglx, mingo, gregkh,
linux-arm-msm, kgdb-bugreport, nm, linux-mips, dalias,
catalin.marinas, vigneshr, linux-aspeed, linux-sh, peterz,
will.deacon, mhocko, paulus, hpa, sparclinux, marex, sfr, ysato,
linux-hexagon, x86, pombredanne, tony, mingo, joel, linux-serial,
rolf.evers.fischer, jhogan, asierra
In-Reply-To: <20181030115628.eqtyqdugkpkxvyr2@holly.lan>
On Tue, Oct 30, 2018 at 11:56:28AM +0000, Daniel Thompson wrote:
> On Mon, Oct 29, 2018 at 11:07:00AM -0700, Douglas Anderson wrote:
> > Looking back, this is pretty much two series squashed that could be
> > treated indepdently. The first is a serial series and the second is a
> > kgdb series.
>
> Indeed.
>
> I couldn't work out the link between the first 5 patches and the last 2
> until I read this...
>
> Is anything in the 01->05 patch set even related to kgdb? From the stack
> traces it looks to me like the lock dep warning would trigger for any
> sysrq. I think separating into two threads for v2 would be sensible.
I'm concerned about calling smp_call_function() from IRQ context with
IRQs disabled - that will block the ability of the _calling_ CPU to
process IPIs from other CPUs in the system. If we have other CPUs
waiting on their IPIs to complete on _this_ CPU, we could end up
deadlocking while trying to grab the CSD lock.
This is the intention of warnings in smp_call_function*() - to catch
cases where deadlocks _can_ occur, but do not reliably show up.
The exceptions to the warning (disregarding oops_in_progress) are
chosen to allow IRQs-disabled calls when we're sure that the rest of
the system isn't going to be sending the calling CPU an IPI (eg,
because the CPU isn't marked online, and we only send IPIs to online
CPUs, or if we're still early in the kernel boot and hence have no
other CPUs running.) The exception is oops_in_progress, which can
occur at any time - even with the current CPU already holding some
CSD locks (eg, oops while trying to send an IPI.)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH 0/7] serial: Finish kgdb on qcom_geni; fix many lockdep splats w/ kgdb
From: Andy Shevchenko @ 2018-10-30 12:36 UTC (permalink / raw)
To: Douglas Anderson
Cc: Jason Wessel, Daniel Thompson, tglx, mingo, gregkh, linux-arm-msm,
kgdb-bugreport, nm, linux-mips, dalias, catalin.marinas, vigneshr,
linux-aspeed, linux-sh, peterz, will.deacon, mhocko, paulus, hpa,
sparclinux, marex, sfr, ysato, linux-hexagon, x86, linux,
pombredanne, tony, mingo, joel, linux-serial, rolf.evers.fischer,
jhogan
In-Reply-To: <20181029180707.207546-1-dianders@chromium.org>
On Mon, Oct 29, 2018 at 11:07:00AM -0700, Douglas Anderson wrote:
> I started out this series trying to make sysrq work over the serial
> console on qcom_geni_serial, then fell into a rat's nest.
>
> To solve the deadlock I faced when enabling sysrq I tried to borrow
> code from '8250_port.c' which avoided grabbing the port lock in
> console_write(). ...but since these days I try to run with lockdep on
> all the time, I found it caused an annoying lockdep splat (which I
> also reproduced on my rk3399 board). ...so I instead changed my
> qcom_geni_serial solution to borrow code from 'msm_serial.c'
>
> I wasn't super happy with the solution in 'msm_serial.c' though. I
> don't like releasing the spinlock there. Not only is it ugly but it
> means we are unlocking / re-locking _all the time_ even though sysrq
> characters are rare. ...so I came up with what I think is a better
> solution and then implemented it for qcom_geni_serial.
>
> Since I had a good way to test 8250-based UARTs, I also fixed that
> driver to use my new method. When doing so, I ran into a missing
> msm_serial.c at all, so I didn't switch that (or all other serial
> drivers for that matter) to the new method.
>
> After fixing all the above issues, I found the next lockdep splat in
> kgdb and I think I've worked around it in a good-enough way, but I'm
> much less confident about this. Hopefully folks can take a look at
> it.
>
> In general, patches earlier in this series should be "less
> controversial" and hopefully can land even if people don't like
> patches later in the series. ;-)
>
> Looking back, this is pretty much two series squashed that could be
> treated indepdently. The first is a serial series and the second is a
> kgdb series.
>
> For all serial patches I'd expect them to go through the tty tree once
> they've been reviewed.
>
> If folks are OK w/ the 'smp' patch it probably should go in some core
> kernel tree. The kgdb patch won't work without it, though, so to land
> that we'd need coordination between the folks landing that and the
> folks landing the 'smp' patch.
I have got only 0/7 and 5/7, everything okay with your mail client and other tools?
>
>
> Douglas Anderson (7):
> serial: qcom_geni_serial: Finish supporting sysrq
> serial: core: Allow processing sysrq at port unlock time
> serial: qcom_geni_serial: Process sysrq at port unlock time
> serial: core: Include console.h from serial_core.h
> serial: 8250: Process sysrq at port unlock time
> smp: Don't yell about IRQs disabled in kgdb_roundup_cpus()
> kgdb: Remove irq flags and local_irq_enable/disable from roundup
>
> arch/arc/kernel/kgdb.c | 4 +--
> arch/arm/kernel/kgdb.c | 4 +--
> arch/arm64/kernel/kgdb.c | 4 +--
> arch/hexagon/kernel/kgdb.c | 11 ++----
> arch/mips/kernel/kgdb.c | 4 +--
> arch/powerpc/kernel/kgdb.c | 2 +-
> arch/sh/kernel/kgdb.c | 4 +--
> arch/sparc/kernel/smp_64.c | 2 +-
> arch/x86/kernel/kgdb.c | 9 ++---
> drivers/tty/serial/8250/8250_aspeed_vuart.c | 6 +++-
> drivers/tty/serial/8250/8250_fsl.c | 6 +++-
> drivers/tty/serial/8250/8250_omap.c | 6 +++-
> drivers/tty/serial/8250/8250_port.c | 8 ++---
> drivers/tty/serial/qcom_geni_serial.c | 10 ++++--
> include/linux/kgdb.h | 9 ++---
> include/linux/serial_core.h | 38 ++++++++++++++++++++-
> kernel/debug/debug_core.c | 2 +-
> kernel/smp.c | 4 ++-
> 18 files changed, 80 insertions(+), 53 deletions(-)
>
> --
> 2.19.1.568.g152ad8e336-goog
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 4/9] mailbox: tegra-hsp: Add support for shared mailboxes
From: Pekka Pessi @ 2018-10-30 16:15 UTC (permalink / raw)
To: Thierry Reding
Cc: Jassi Brar, Greg Kroah-Hartman, Jiri Slaby, Mikko Perttunen,
Jon Hunter, Timo Alho, Mika Liljeberg, linux-tegra, linux-serial,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20181029131614.GA14616@ulmo>
> I guess it being an example doesn't make it strictly a recommendation, but
> I wonder if we should avoid giving examples that use mappings which we
> discourage.
Well, yes, that is example for top0 – but I agree, giving examples that
do not make much sense is not very productive.
> So currently
> we list all available interrupts in DT and then we could pick just the
> first. But what if somebody else already picked the first and "owns" it
We have rather static hardware allocation in device trees, if an another
VM instance or another aux cpu owns interrupt, we just remove it from
the DTS or DTSI. So the native Linux would see all the possible
interrupts (that are not used by SPE or RCE, for instance), but a
virtualized one would see only one.
> For mailboxes that the CCPLEX writes to, we can reconfigure them as
> producers by disabling the FULL interrupt and enabling the EMPTY
> interrupt instead. We can do that the first time somebody calls the
> mbox_send_message() on the mailbox.
The problem here is "disabling the FULL interrupt". The main problem are
the mailbox-specific IE registers, they are not really shared, but the
consumer owns the full_ie and producer empty_ie. We can not enable or
disable interrupts without interfering with the interrupt settings of
the remote end, when the interrupt is enabled, we either modify
full_ie/empty_ie register or run with a risk that it has incorrect value.
Can we just leave the mbox channel without interrupts? How the consumer
indicates that it is interested in receiving messages? mbox_peek_data()?
--Pekka
On 10/29/2018 03:16 PM, Thierry Reding wrote:
> On Mon, Oct 29, 2018 at 02:25:42PM +0200, Pekka Pessi wrote:
>> Hi Thierry,
>>
>> From the 0/9:
>>> Are you aware of any others that we need to take into account?
>> We would like to use upstream driver for RCE (and probably AON and SCE)
>> mailbox handling, too. Eventually.
>>> This is a bit
>>> of a problem because the mailbox driver doesn't really know anything
>>> about the direction when it starts up, so how would it make the decision
>>> about how to program the registers?
>> I'm afraid that information must be stored in the device tree.
>>> What's somewhat surprising is that you're saying that both FULL and
>>> EMPTY interrupts should be handled by the same shared interrupt. That's
>>> the opposite of what the recommended programming sequence is that the
>>> TRM specifies.
>> Which TRM you mean? Something here?
>>
>> https://p4viewer.nvidia.com/get///hw/ar/doc/Tech_Pubs/Xavier/TRM/
>>
>> I browsed through he Xavier_TRM_DP09253001v1.pdf HSP section and I could not
>> find any recommendations? But see below.
> I'm referring to Xavier_TRM_Public.pdf in that location. If you look at
> page 4422, "Shared Interrupts Configuration", the example has aggregated
> EMPTY and FULL interrupts on shared interrupts 0 and 1, respectively. I
> guess it being an example doesn't make it strictly a recommendation, but
> I wonder if we should avoid giving examples that use mappings which we
> discourage.
>
>>> Why is it better to handle both FULL and EMPTY interrupts
>>> with the same shared interrupt?
>> Only top0 has 8 interrupts, rest of the HSP blocks have only 4 (or 5 in case
>> of top1) interrupts per HSP available through LIC. Hogging two of them means
>> that only two VMs can access a HSP.
> Virtualization isn't something that we're very concerned about upstream,
> but I'll take that under consideration.
>
>>> Would it be safe to clear all of the IE registers to 0 on driver probe?
>> Nope, the driver should clear only the IE register for the shared interrupt
>> that the driver uses. Other IEs are used by other entities.
> Right, that makes sense. But within that IE register it can consider all
> bits fair game, right? One thing I wonder, though, is whether there
> should be some external mechanism to set the shared interrupt to use. If
> we go purely by convention we'll eventually get this wrong. So currently
> we list all available interrupts in DT and then we could pick just the
> first. But what if somebody else already picked the first and "owns" it?
>
> I'm not sure we have any practical mechanism to rewrite the DTB, but
> perhaps something to keep in mind if ever we need to support other
> entities down the road.
>
>>> If they are indeed separate
>>> for each processor, it should be fairly easy to keep track of the
>>> mailboxes used by the kernel and process only those.
>> Yes, that is what we should do. Again the directionality should be specified
>> in DT.
> Currently we encode the shared mailboxes in DT like this:
>
> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM 0>,
> <&hsp_aon TEGRA_HSP_MBOX_TYPE_SM 1>;
> mbox-names = "rx", "tx";
>
> I suppose we could change that to something like:
>
> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM (0 << 31 | 0)>,
> <&hsp_aon TEGRA_HSP_MBOX_TYPE_SM (1 << 31 | 0)>;
>
> Where the MSB of the mailbox index would indicate the direction. We
> could maybe add some eye-candy to make it easier to read:
>
> mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM TEGRA_HSP_MBOX_SM_RX(0)>,
> <&hsp_aon TEGRA_HSP_MBOX_TYPE_SM TEGRA_HSP_MBOX_SM_TX(1)>;
>
> That said, I wonder if perhaps it is safe to treat all mailboxes as
> consumers by default and omit and direction in DT. If a mailbox is used
> as consumer, then the CCPLEX is only interested in FULL interrupts, so
> we enable those. The entity that shares the mailbox will write data to
> it and is only interested in the EMPTY interrupt, which we don't touch
> from the CCPLEX.
>
> For mailboxes that the CCPLEX writes to, we can reconfigure them as
> producers by disabling the FULL interrupt and enabling the EMPTY
> interrupt instead. We can do that the first time somebody calls the
> mbox_send_message() on the mailbox.
>
> Do you see any problems with that approach?
>
>>> I'm not entirely clear on what the advantages are of using the per-
>>> mailbox registers, or how they are supposed to be used. The existing
>>> documentation doesn't really explain how these are supposed to be used
>>> either, so I was mostly just going by trial and error.
>> On virtualized configs, multiple VMs can use same HSP block but each VM must
>> use its own interrupt. Because all the IE registers are on the same 64 KB
>> page, the writes to the page are trapped by hypervisor, which means that
>> enabling or disabling an interrupt via the shared IE register is pretty
>> heavy operation. The per-mailbox IE registers are on a page accessed freely
>> by the IE, no need to trap.
> Do the per-mailbox IE registers act as a second level enable, then? The
> HSP driver would still have to set the IE register to aggregate FULL and
> EMPTY interrupts as needed, but could then use the per-mailbox IE
> registers to actually enable and disable (that is, unmask and mask) the
> interrupts?
>
>> If a VM uses only one empty mailbox interrupt, using a dedicated "shared"
>> interrupt and disabling that in GIC could be a lighter operation, we used to
>> do that in Parker.
> Okay, that seems like it would somewhat complicate things and we don't
> really support VM uses yet, so I think it best to leave that out for
> now.
>
> Thierry
>
>> On 10/29/2018 12:39 PM, Thierry Reding wrote:
>>> On Mon, Oct 29, 2018 at 12:04:22PM +0200, Pekka Pessi wrote:
>>>> Hi Thierry,
>>>>
>>>> There is typically one entity (aux cpu or a VM running on CCPLEX) owning the
>>>> "empty" or producer side of mailbox (iow, waking up on empty) and another
>>>> entity owning the "full" or consumer side of mailbox (waking up on full). An
>>>> entity should not muck with the interrupts used by the opposite side.
>>> Okay, that explains some of my observations. I was initially trying to
>>> program interrupt enables for both FULL and EMPTY interrupts for all
>>> mailboxes, but then I'd usually get timeouts because the consumer wasn't
>>> responding (i.e. the SPE wasn't getting FULL interrupts for the CCPLEX's
>>> TX mailbox).
>>>
>>> If I understand correctly, you're saying that the CPU should only be
>>> using the EMPTY interrupts for it's TX mailbox (while leaving the FULL
>>> interrupts completely untouched) and only the FULL interrupt for it's
>>> RX mailbox (while leaving the EMPTY interrupts untouched). This is a bit
>>> of a problem because the mailbox driver doesn't really know anything
>>> about the direction when it starts up, so how would it make the decision
>>> about how to program the registers?
>>>
>>>> One entity typically owns one shared interrupt only. For the
>>>> BPMP/SCE/RCE/SPE HSP blocks the shared interrupt 0 is owned by the auxiliary
>>>> processor itself, the shared interrupts 1..4 are connected to LIC and are
>>>> available to other entities. The convention is to go through the interrupts
>>>> 0..4 and then using the first available shared interrupt for both full and
>>>> empty.
>>> That partially matches another of my observations. It seems like we
>>> can't use the shared interrupt 0 at all on at least the AON HSP. That's
>>> fine because that HSP instance contains the TX mailbox for TCU and by
>>> the current convention in the HSP driver, shared interrupt 0 would be
>>> aggregating the FULL interrupts, which according to the above we don't
>>> need for TX mailboxes.
>>>
>>> What's somewhat surprising is that you're saying that both FULL and
>>> EMPTY interrupts should be handled by the same shared interrupt. That's
>>> the opposite of what the recommended programming sequence is that the
>>> TRM specifies. Why is it better to handle both FULL and EMPTY interrupts
>>> with the same shared interrupt?
>>>
>>>> The interrupt functions should use a mask for mailboxes owned by kernel (in
>>>> essence what the IE register should be for the HSP shared interrupt owned by
>>>> the kernel) and serve only those mailboxes owned by kernel. Note that there
>>>> is no reset for HSP in Xavier, and the IE register contents may be stale.
>>> Would it be safe to clear all of the IE registers to 0 on driver probe?
>>> I seem to remember trying to do that and getting similar behaviour to
>>> what I describe above, namely that interrupts on the SPE weren't working
>>> anymore. I concluded that the IE register must be shared between the
>>> various processors, even though that's somewhat suprising given that
>>> there is no way to synchronize accesses to those registers, so their
>>> programming would be somewhat up to chance.
>>>
>>> Do you know any more about these registers? If they are indeed separate
>>> for each processor, it should be fairly easy to keep track of the
>>> mailboxes used by the kernel and process only those. Again I don't know
>>> how exactly to distinguish between TX and RX mailboxes because they all
>>> start out the same and only their use defines which direction they go.
>>> Currently this works because we program them as consumers by default.
>>> That means we enable the FULL interrupts but keep EMPTY interrupts
>>> disabled until a message in transmitted on the mailbox, at which point
>>> we enable the EMPTY interrupt. I suppose at that point we should also
>>> disable the FULL interrupt, given the above discussion.
>>>
>>>> And lastly, if we want to support only Xavier and later, perhaps we should
>>>> be more clear in the bindings? There are no mailbox-specific interrupt
>>>> enable registers available on Parker and your design relies on them.
>>> That was certainly not the intention. I thought I had seen the per-
>>> mailbox interrupt enable registers also in Tegra186 documentation, but
>>> after double-checking they're indeed not there. I don't think the driver
>>> currently "relies" on them because it uses them in addition to the
>>> HSP_IE registers. I suppose that accessing them might cause aborts on
>>> Tegra186 if they don't exist, though.
>>>
>>> I'm not entirely clear on what the advantages are of using the per-
>>> mailbox registers, or how they are supposed to be used. The existing
>>> documentation doesn't really explain how these are supposed to be used
>>> either, so I was mostly just going by trial and error.
>>>
>>> Do you know anything more on how to use these registers? I can easily
>>> make them Tegra194 specific in the code, but if they're aren't any clear
>>> advantages, it might just be easier to stick with HSP_IE programming
>>> only.
>>>
>>> Thierry
^ permalink raw reply
* [PATCH v2 0/5] serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250
From: Douglas Anderson @ 2018-10-30 22:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: dan.carpenter, vigneshr, linux-aspeed, andrew, linux-arm-msm,
Douglas Anderson, tony, joel, linux-serial, andriy.shevchenko,
linux-arm-kernel, jk
I started out this series trying to make sysrq work over the serial
console on qcom_geni_serial, then fell into a rat's nest.
To solve the deadlock I faced when enabling sysrq I tried to borrow
code from '8250_port.c' which avoided grabbing the port lock in
console_write(). ...but since these days I try to run with lockdep on
all the time, I found it caused an annoying lockdep splat (which I
also reproduced on my rk3399 board). ...so I instead changed my
qcom_geni_serial solution to borrow code from 'msm_serial.c'
I wasn't super happy with the solution in 'msm_serial.c' though. I
don't like releasing the spinlock there. Not only is it ugly but it
means we are unlocking / re-locking _all the time_ even though sysrq
characters are rare. ...so I came up with what I think is a better
solution and then implemented it for qcom_geni_serial.
Since I had a good way to test 8250-based UARTs, I also fixed that
driver to use my new method. When doing so, I ran into a missing
include in serial_core.h. NOTE: I didn't have a way to test
msm_serial.c at all, so I didn't switch that (or all other serial
drivers for that matter) to the new method.
NOTE: from a serial point of view v2 is the same as v1 but I've
removed the extra kgdb-related patches and made it obvious that this
is really for all sysrq, not just kgdb. I've also generally tried to
curate the CCs more properly.
Douglas Anderson (5):
serial: qcom_geni_serial: Finish supporting sysrq
serial: core: Allow processing sysrq at port unlock time
serial: qcom_geni_serial: Process sysrq at port unlock time
serial: core: Include console.h from serial_core.h
serial: 8250: Process sysrq at port unlock time
drivers/tty/serial/8250/8250_aspeed_vuart.c | 6 +++-
drivers/tty/serial/8250/8250_fsl.c | 6 +++-
drivers/tty/serial/8250/8250_omap.c | 6 +++-
drivers/tty/serial/8250/8250_port.c | 8 ++---
drivers/tty/serial/qcom_geni_serial.c | 10 ++++--
include/linux/serial_core.h | 38 ++++++++++++++++++++-
6 files changed, 63 insertions(+), 11 deletions(-)
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply
* [PATCH v2 1/5] serial: qcom_geni_serial: Finish supporting sysrq
From: Douglas Anderson @ 2018-10-30 22:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: dan.carpenter, vigneshr, linux-aspeed, andrew, linux-arm-msm,
Douglas Anderson, tony, joel, linux-serial, andriy.shevchenko,
linux-arm-kernel, jk
In-Reply-To: <20181030221107.79758-1-dianders@chromium.org>
The geni serial driver already had some sysrq code in it, but since
SUPPORT_SYSRQ wasn't defined the code didn't do anything useful.
Let's make it useful by adding that define using the same formula
found in other serial drivers.
In order to prevent deadlock, we'll take a page from the
'msm_serial.c' where the spinlock is released around
uart_handle_sysrq_char(). This seemed better than copying from
'8250_port.c' where we skip locking in the console_write function
since the '8250_port.c' method can cause lockdep warnings when
dropping into kgdb.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/tty/serial/qcom_geni_serial.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 1515074e18fb..b83e3554bced 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1,6 +1,10 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
+#if defined(CONFIG_SERIAL_QCOM_GENI_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+# define SUPPORT_SYSRQ
+#endif
+
#include <linux/clk.h>
#include <linux/console.h>
#include <linux/io.h>
@@ -495,7 +499,10 @@ static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
continue;
}
+ spin_unlock(&uport->lock);
sysrq = uart_handle_sysrq_char(uport, buf[c]);
+ spin_lock(&uport->lock);
+
if (!sysrq)
tty_insert_flip_char(tport, buf[c], TTY_NORMAL);
}
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* [PATCH v2 2/5] serial: core: Allow processing sysrq at port unlock time
From: Douglas Anderson @ 2018-10-30 22:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: dan.carpenter, vigneshr, linux-aspeed, andrew, linux-arm-msm,
Douglas Anderson, tony, joel, linux-serial, andriy.shevchenko,
linux-arm-kernel, jk
In-Reply-To: <20181030221107.79758-1-dianders@chromium.org>
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 406edae44ca3..3460b15a2607 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -173,6 +173,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 */
@@ -482,8 +483,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);
+}
#endif
/*
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* [PATCH v2 3/5] serial: qcom_geni_serial: Process sysrq at port unlock time
From: Douglas Anderson @ 2018-10-30 22:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: dan.carpenter, vigneshr, linux-aspeed, andrew, linux-arm-msm,
Douglas Anderson, tony, joel, linux-serial, andriy.shevchenko,
linux-arm-kernel, jk
In-Reply-To: <20181030221107.79758-1-dianders@chromium.org>
Let's take advantage of the new ("serial: core: Allow processing sysrq
at port unlock time") to handle sysrqs more cleanly.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/tty/serial/qcom_geni_serial.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index b83e3554bced..29ddc1fc65f8 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -499,9 +499,7 @@ static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
continue;
}
- spin_unlock(&uport->lock);
- sysrq = uart_handle_sysrq_char(uport, buf[c]);
- spin_lock(&uport->lock);
+ sysrq = uart_prepare_sysrq_char(uport, buf[c]);
if (!sysrq)
tty_insert_flip_char(tport, buf[c], TTY_NORMAL);
@@ -811,7 +809,8 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
qcom_geni_serial_handle_rx(uport, drop_rx);
out_unlock:
- spin_unlock_irqrestore(&uport->lock, flags);
+ uart_unlock_and_check_sysrq(uport, flags);
+
return IRQ_HANDLED;
}
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* [PATCH v2 4/5] serial: core: Include console.h from serial_core.h
From: Douglas Anderson @ 2018-10-30 22:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: dan.carpenter, vigneshr, linux-aspeed, andrew, linux-arm-msm,
Douglas Anderson, tony, joel, linux-serial, andriy.shevchenko,
linux-arm-kernel, jk
In-Reply-To: <20181030221107.79758-1-dianders@chromium.org>
In the static inline function uart_handle_break() in serial_core.h we
dereference port->cons. That gives an error unless console.h is also
included.
This error hasn't shown up till now because everyone who has defined
SUPPORT_SYSRQ has also included console.h, but it's a bit ugly to make
this requirement. Let's make the include explicit.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
include/linux/serial_core.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 3460b15a2607..ff9d0ee32f11 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -22,6 +22,7 @@
#include <linux/bitops.h>
#include <linux/compiler.h>
+#include <linux/console.h>
#include <linux/interrupt.h>
#include <linux/circ_buf.h>
#include <linux/spinlock.h>
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* [PATCH v2 5/5] serial: 8250: Process sysrq at port unlock time
From: Douglas Anderson @ 2018-10-30 22:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: dan.carpenter, vigneshr, linux-aspeed, andrew, linux-arm-msm,
Douglas Anderson, tony, joel, linux-serial, andriy.shevchenko,
linux-arm-kernel, jk
In-Reply-To: <20181030221107.79758-1-dianders@chromium.org>
Let's take advantage of the new ("serial: core: Allow processing sysrq
at port unlock time") to handle sysrqs more cleanly.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I don't have a great way to test all 8250 variants, but I've at least
tested rk3288 / rk3399 and they seem to work. Hopefully I got the
aspeed_vuart / fsl / omap variants right (I only compile tested
those).
drivers/tty/serial/8250/8250_aspeed_vuart.c | 6 +++++-
drivers/tty/serial/8250/8250_fsl.c | 6 +++++-
drivers/tty/serial/8250/8250_omap.c | 6 +++++-
drivers/tty/serial/8250/8250_port.c | 8 +++-----
4 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 435bec40dee6..0438d9a905ce 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -5,6 +5,10 @@
* Copyright (C) 2016 Jeremy Kerr <jk@ozlabs.org>, IBM Corp.
* Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>, IBM Corp.
*/
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
#include <linux/device.h>
#include <linux/module.h>
#include <linux/of_address.h>
@@ -293,7 +297,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
if (lsr & UART_LSR_THRE)
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_unlock_and_check_sysrq(port, flags);
return 1;
}
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 6640a4c7ddd1..ff3dcaea5d93 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -1,4 +1,8 @@
// SPDX-License-Identifier: GPL-2.0
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
#include <linux/serial_reg.h>
#include <linux/serial_8250.h>
@@ -54,7 +58,7 @@ int fsl8250_handle_irq(struct uart_port *port)
serial8250_tx_chars(up);
up->lsr_saved_flags = orig_lsr;
- spin_unlock_irqrestore(&up->port.lock, flags);
+ uart_unlock_and_check_sysrq(&up->port, flags);
return 1;
}
EXPORT_SYMBOL_GPL(fsl8250_handle_irq);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index a019286f8bb6..ad7ba7d0f28d 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -8,6 +8,10 @@
*
*/
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
#include <linux/device.h>
#include <linux/io.h>
#include <linux/module.h>
@@ -1085,7 +1089,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
}
}
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_unlock_and_check_sysrq(port, flags);
serial8250_rpm_put(up);
return 1;
}
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3f779d25ec0c..d2f3310abe54 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1736,7 +1736,7 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
else if (lsr & UART_LSR_FE)
flag = TTY_FRAME;
}
- if (uart_handle_sysrq_char(port, ch))
+ if (uart_prepare_sysrq_char(port, ch))
return;
uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);
@@ -1878,7 +1878,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE))
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_unlock_and_check_sysrq(port, flags);
return 1;
}
EXPORT_SYMBOL_GPL(serial8250_handle_irq);
@@ -3239,9 +3239,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
serial8250_rpm_get(up);
- if (port->sysrq)
- locked = 0;
- else if (oops_in_progress)
+ if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
else
spin_lock_irqsave(&port->lock, flags);
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* Re: [PATCH 0/7] serial: Finish kgdb on qcom_geni; fix many lockdep splats w/ kgdb
From: Doug Anderson @ 2018-10-30 22:20 UTC (permalink / raw)
To: Daniel Thompson
Cc: Nishanth Menon, linux-mips, dalias, Benjamin Herrenschmidt,
vigneshr, linux-aspeed, linux-sh, Peter Zijlstra, Catalin Marinas,
Will Deacon, mhocko, paulus, mpe, H. Peter Anvin, sparclinux,
Jiri Slaby, Ingo Molnar, marex, Stephen Rothwell, ysato,
linux-hexagon, x86, Russell King - ARM Linux, Tony Lindgren,
Ingo Molnar, joel, linux-serial, kgdb-bugreport, jhogan,
rolf.evers.fischer, asierra
In-Reply-To: <20181030115628.eqtyqdugkpkxvyr2@holly.lan>
Hi,
On Tue, Oct 30, 2018 at 4:56 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Oct 29, 2018 at 11:07:00AM -0700, Douglas Anderson wrote:
> > Looking back, this is pretty much two series squashed that could be
> > treated indepdently. The first is a serial series and the second is a
> > kgdb series.
>
> Indeed.
>
> I couldn't work out the link between the first 5 patches and the last 2
> until I read this...
>
> Is anything in the 01->05 patch set even related to kgdb? From the stack
> traces it looks to me like the lock dep warning would trigger for any
> sysrq. I think separating into two threads for v2 would be sensible.
Yes, sorry about the mess. Splitting this into two series makes the
most sense. Then I can focus more on trying to get the CCs right and
people can just get the patches that matter to them.
OK, I've sent v2 of both series out now. Please yell if you can't
find them for whatever reason.
-Doug
-Doug
^ permalink raw reply
* Re: [PATCHv3] panic: avoid deadlocks in re-entrant console drivers
From: Petr Mladek @ 2018-10-31 12:27 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-kernel, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181025101036.6823-1-sergey.senozhatsky@gmail.com>
On Thu 2018-10-25 19:10:36, Sergey Senozhatsky wrote:
> >From printk()/serial console point of view panic() is special, because
> it may force CPU to re-enter printk() or/and serial console driver.
> Therefore, some of serial consoles drivers are re-entrant. E.g. 8250:
>
> serial8250_console_write()
> {
> if (port->sysrq)
> locked = 0;
> else if (oops_in_progress)
> locked = spin_trylock_irqsave(&port->lock, flags);
> else
> spin_lock_irqsave(&port->lock, flags);
> ...
> }
>
> panic() does set oops_in_progress via bust_spinlocks(1), so in theory
> we should be able to re-enter serial console driver from panic():
>
> CPU0
> <NMI>
> uart_console_write()
> serial8250_console_write() // if (oops_in_progress)
> // spin_trylock_irqsave()
> call_console_drivers()
> console_unlock()
> console_flush_on_panic()
> bust_spinlocks(1) // oops_in_progress++
> panic()
> <NMI/>
> spin_lock_irqsave(&port->lock, flags) // spin_lock_irqsave()
> serial8250_console_write()
> call_console_drivers()
> console_unlock()
> printk()
> ...
>
> However, this does not happen and we deadlock in serial console on
> port->lock spinlock. And the problem is that console_flush_on_panic()
> called after bust_spinlocks(0):
>
> void panic(const char *fmt, ...)
> {
> bust_spinlocks(1);
> ...
> bust_spinlocks(0);
> console_flush_on_panic();
> ...
> }
>
> bust_spinlocks(0) decrements oops_in_progress, so oops_in_progress
> can go back to zero. Thus even re-entrant console drivers will simply
> spin on port->lock spinlock. Given that port->lock may already be
> locked either by a stopped CPU, or by the very same CPU we execute
> panic() on (for instance, NMI panic() on printing CPU) the system
> deadlocks and does not reboot.
>
> Fix this by removing bust_spinlocks(0), so oops_in_progress is always
> set in panic() now and, thus, re-entrant console drivers will trylock
> the port->lock instead of spinning on it forever, when we call them
> from console_flush_on_panic().
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
The patch makes sense to me. The locks should stay busted also for
console_flush_on_panic().
With the added #include <linux/vt_kern.h>:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCHv3] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-11-01 1:48 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, linux-kernel, Steven Rostedt, Daniel Wang,
Peter Zijlstra, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20181031122736.epbxu424l3xagge5@pathway.suse.cz>
On (10/31/18 13:27), Petr Mladek wrote:
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>
> The patch makes sense to me. The locks should stay busted also for
> console_flush_on_panic().
>
> With the added #include <linux/vt_kern.h>:
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
Thanks!
Since there are no objections - how shall we route it? Via printk tree?
I'd also prefer this patch to be in -stable, it fixes a real issue after
all.
-ss
^ permalink raw reply
* [PATCH v1 0/3] clk/serial tegra: uart related fixes
From: Marcel Ziswiler @ 2018-11-01 1:52 UTC (permalink / raw)
To: linux-tegra, linux-kernel
Cc: Marcel Ziswiler, Thierry Reding, linux-serial, Jiri Slaby,
Stephen Boyd, Greg Kroah-Hartman, Prashant Gaikwad,
Peter De Schrijver, Laxman Dewangan, Jonathan Hunter,
Michael Turquette, linux-clk
This series features some UART related clock issue fix and clean-up.
Marcel Ziswiler (3):
clk: tegra: get rid of duplicate defines
clk: tegra: ignore unused vfir clock shared with uartb
serial: tegra: fix some spelling mistakes
drivers/clk/tegra/clk-tegra-periph.c | 5 +----
drivers/tty/serial/serial-tegra.c | 10 +++++-----
2 files changed, 6 insertions(+), 9 deletions(-)
--
2.14.5
^ permalink raw reply
* [PATCH v1 3/3] serial: tegra: fix some spelling mistakes
From: Marcel Ziswiler @ 2018-11-01 1:52 UTC (permalink / raw)
To: linux-tegra, linux-kernel
Cc: Marcel Ziswiler, Thierry Reding, Jiri Slaby, Jonathan Hunter,
linux-serial, Laxman Dewangan, Greg Kroah-Hartman
In-Reply-To: <20181101015230.27310-1-marcel@ziswiler.com>
From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Fix a few spelling mistakes I stumbled upon while debugging a customers
UART issues.
Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---
drivers/tty/serial/serial-tegra.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index af2a29cfbbe9..d5269aaaf9b2 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -746,7 +746,7 @@ static void tegra_uart_stop_rx(struct uart_port *u)
if (!tup->rx_in_progress)
return;
- tegra_uart_wait_sym_time(tup, 1); /* wait a character interval */
+ tegra_uart_wait_sym_time(tup, 1); /* wait one character interval */
ier = tup->ier_shadow;
ier &= ~(UART_IER_RDI | UART_IER_RLSI | UART_IER_RTOIE |
@@ -887,7 +887,7 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
*
* EORD is different interrupt than RX_TIMEOUT - RX_TIMEOUT occurs when
* the DATA is sitting in the FIFO and couldn't be transferred to the
- * DMA as the DMA size alignment(4 bytes) is not met. EORD will be
+ * DMA as the DMA size alignment (4 bytes) is not met. EORD will be
* triggered when there is a pause of the incomming data stream for 4
* characters long.
*
@@ -1079,7 +1079,7 @@ static void tegra_uart_set_termios(struct uart_port *u,
if (tup->rts_active)
set_rts(tup, false);
- /* Clear all interrupts as configuration is going to be change */
+ /* Clear all interrupts as configuration is going to be changed */
tegra_uart_write(tup, tup->ier_shadow | UART_IER_RDI, UART_IER);
tegra_uart_read(tup, UART_IER);
tegra_uart_write(tup, 0, UART_IER);
@@ -1165,10 +1165,10 @@ static void tegra_uart_set_termios(struct uart_port *u,
/* update the port timeout based on new settings */
uart_update_timeout(u, termios->c_cflag, baud);
- /* Make sure all write has completed */
+ /* Make sure all writes have completed */
tegra_uart_read(tup, UART_IER);
- /* Reenable interrupt */
+ /* Re-enable interrupt */
tegra_uart_write(tup, tup->ier_shadow, UART_IER);
tegra_uart_read(tup, UART_IER);
--
2.14.5
^ permalink raw reply related
* Re: [PATCHv3] panic: avoid deadlocks in re-entrant console drivers
From: Petr Mladek @ 2018-11-01 8:08 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-kernel, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181101014821.GB502@jagdpanzerIV>
On Thu 2018-11-01 10:48:21, Sergey Senozhatsky wrote:
> On (10/31/18 13:27), Petr Mladek wrote:
> > >
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >
> > The patch makes sense to me. The locks should stay busted also for
> > console_flush_on_panic().
> >
> > With the added #include <linux/vt_kern.h>:
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
>
> Thanks!
>
> Since there are no objections - how shall we route it? Via printk tree?
Good question. OK, I am going to put it into printk.git unless I hear
complains withing next couple of days.
> I'd also prefer this patch to be in -stable, it fixes a real issue after
> all.
It makes sense.
Best Regards,
Petr
^ permalink raw reply
* [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250
From: Florian Fainelli @ 2018-11-01 18:26 UTC (permalink / raw)
To: linux-kernel
Cc: arndb, Florian Fainelli, Greg Kroah-Hartman, Jiri Slaby,
Nishanth Menon, Hendrik Brueckner, Christian Borntraeger,
Michael Moese, Lokesh Vutla, open list:SERIAL DRIVERS
It is way too easy to miss enabling SERIAL_OF_PLATFORM which would
result in the inability for the kernel to have a valid console device,
which can be seen with:
Warning: unable to open an initial console.
and then:
Run /init as init process
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100
Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there
really is no drawback to defaulting this config to the value of
SERIAL_8250.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/tty/serial/8250/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 15c2c5463835..d7737dca0e48 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -484,6 +484,7 @@ config SERIAL_8250_PXA
config SERIAL_OF_PLATFORM
tristate "Devicetree based probing for 8250 ports"
depends on SERIAL_8250 && OF
+ default SERIAL_8250
help
This option is used for all 8250 compatible serial ports that
are probed through devicetree, including Open Firmware based
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/7] serial: qcom_geni_serial: Finish supporting sysrq
From: Stephen Boyd @ 2018-11-02 16:47 UTC (permalink / raw)
To: Daniel Thompson, Jason Wessel, gregkh, mingo, tglx
Cc: linux-arm-msm, kgdb-bugreport, Douglas Anderson, linux-kernel,
linux-serial, jslaby
In-Reply-To: <20181029180707.207546-2-dianders@chromium.org>
Quoting Douglas Anderson (2018-10-29 11:07:01)
> The geni serial driver already had some sysrq code in it, but since
> SUPPORT_SYSRQ wasn't defined the code didn't do anything useful.
> Let's make it useful by adding that define using the same formula
> found in other serial drivers.
>
> In order to prevent deadlock, we'll take a page from the
> 'msm_serial.c' where the spinlock is released around
> uart_handle_sysrq_char(). This seemed better than copying from
> '8250_port.c' where we skip locking in the console_write function
> since the '8250_port.c' method can cause lockdep warnings when
> dropping into kgdb.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Given that it's the same as msm_serial.c, and I wrote the msm_serial.c
"hack" then:
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox