Re: [PATCH v2] serial: imx: Fix sysrq deadlock
From: Johan Hovold <johan@kernel.org>
Date: 2021-10-01 07:52:51
On Thu, Sep 30, 2021 at 10:45:31AM -0300, Fabio Estevam wrote:
quoted hunk ↗ jump to hunk
Hi Johan, On 30/09/2021 04:54, Johan Hovold wrote:quoted
This is just so broken; you can't just drop the lock. And you clearly haven't even tried to understand how uart_unlock_and_check_sysrq() works. Please take a closer look at the commit you're trying to mimic.Thanks for the feedback. I have changed it to:diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index 8b121cd869e9..b7cda50602d5 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c@@ -803,7 +803,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void*dev_id) continue; } - if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx)) + if (uart_prepare_sysrq_char(&sport->port, rx))
Why did you drop the cast? If there's anything in the high bits you'd see the help text printed as you report below (even if it seems unlikely).
quoted hunk ↗ jump to hunk
continue; if (unlikely(rx & URXD_ERR)) {@@ -844,6 +844,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void*dev_id) } out: + uart_unlock_and_check_sysrq(&sport->port); tty_flip_buffer_push(port); return IRQ_HANDLED;@@ -959,6 +960,7 @@ static irqreturn_t imx_uart_int(int irq, void*dev_id) imx_uart_writel(sport, USR1_AGTIM, USR1); __imx_uart_rxint(irq, dev_id); + spin_lock(&sport->port.lock); ret = IRQ_HANDLED; }
It's a step in the right direction, but you need to restructure the code so that you don't need to drop and reacquire the lock.
quoted hunk ↗ jump to hunk
@@ -1977,9 +1979,7 @@ imx_uart_console_write(struct console *co, constchar *s, unsigned int count) unsigned int ucr1; int locked = 1; - if (sport->port.sysrq) - locked = 0; - else if (oops_in_progress) + if (oops_in_progress) locked = spin_trylock_irqsave(&sport->port.lock, flags); else spin_lock_irqsave(&sport->port.lock, flags);
And you need to fix the commit summary and commit message since you're actually fixing any deadlock. You're just suppressing a false positive lockdep warning due to the above sysrq hack.
This makes the deadlock not happen after running: echo t > /proc/sysrq-trigger , but entering <break> + t via the console does not work anymore. It returns the sysrq help instead: sysrq: HELP : loglevel(0-9) reboot(b) crash(c) show-all-locks(d) terminate-all-tasks(e) memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j) sak(k) show-backtrace-all-active-cpu s(l) show-memory-usage(m) nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q) unraw(r) sync(s) show-task-states(t) unmount(u) show-blocked-tasks(w) dump-ftrace-buffer(z)
So either you're just pushing garbage to the sysrq handler due to the dropped cast above or you may, for example, have a NUL char in the receiver due to the break that you don't discard. I'd start with logging the key that gets passed to the sysrq handler. Johan