Re: [PATCH v3] serial: 8250: fix panic due to PSLVERR

[Andy Shevchenko]

On Mon, Apr 14, 2025 at 11:14:50AM +0800, Yunhui Cui wrote:

You forgot to rebase against latest tty-next or, if there is something
in the latter (but I don't see right now), even tty-testing.

> When the PSLVERR_RESP_EN parameter is set to 1, the device generates
> an error response if an attempt is made to read an empty RBR (Receive
> Buffer Register) while the FIFO is enabled.
> 
> In serial8250_do_startup(), calling serial_port_out(port, UART_LCR,
> UART_LCR_WLEN8) triggers dw8250_check_lcr(), which invokes
> dw8250_force_idle() and serial8250_clear_and_reinit_fifos(). The latter
> function enables the FIFO via serial_out(p, UART_FCR, p->fcr).
> Execution proceeds to the dont_test_tx_en label:
> ...
> serial_port_in(port, UART_RX);
> This satisfies the PSLVERR trigger condition.
> 
> Because another CPU(e.g., using printk()) is accessing the UART (UART
> is busy), the current CPU fails the check (value & ~UART_LCR_SPAR) ==
> (lcr & ~UART_LCR_SPAR), causing it to enter dw8250_force_idle().
> 
> To fix this, all calls to serial_out(UART_LCR) and serial_in(UART_RX)
> should be executed under port->lock. Additionally, checking the readiness
> via UART_LSR should also be done under port->lock.
> 
> Panic backtrace:
> [    0.442336] Oops - unknown exception [#1]
> [    0.442343] epc : dw8250_serial_in32+0x1e/0x4a
> [    0.442351]  ra : serial8250_do_startup+0x2c8/0x88e
> ...
> [    0.442416] console_on_rootfs+0x26/0x70

This patch seems need split to three. See below.

...

First of all, while everything looks better now, there is a chance in the
future to miss the same issue again. In order to avoid that I suggest to
introduce a new helper where you made this check _and_ add a comment why.

(Note that currently you have a mixture of serial_in()/serial_port_in() in
 some cases.)

static inline unsigned int serial8250_discard_data(struct uart_8250_port *up)
{
	u16 lsr;

	lsr = serial_in(up, UART_LSR);
	if (lsr & UART_LSR_DR)
		return serial_in(up, UART_RX);

	return 0;
}

And this can be one patch (patch 2).

...

> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c

Changes here deserve the separate patch (patch 1).

...

> +	/*
> +	 * Serial_in(p, UART_RX) should be under port->lock, but we can't add

serial_in()

> +	 * it to avoid AA deadlock as we're unsure if serial_out*(...UART_LCR)
> +	 * is under port->lock.
> +	 */
> +	lockdep_assert_held_once(&p->lock);

...

> +	uart_port_lock_irqsave(port, &flags);

And one patch (patch 3) about locking.

-- 
With Best Regards,
Andy Shevchenko