On Thu, Sep 30, 2021 at 10:45:31AM -0300, Fabio Estevam wrote:
> Hi Johan,
>
> On 30/09/2021 04:54, Johan Hovold wrote:
>
> > 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).
> 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.
> @@ -1977,9 +1979,7 @@ imx_uart_console_write(struct console *co, const
> char *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
