Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Johan Hovold <johan@kernel.org>]

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