Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tty: serial: 8250: lockless data race on lsr_saved_flags in serial8250_do_shutdown()
--

commit 174d9274c3df70fd0549f2a70fbdf6e13b92b8d2
Author: Moteen Shah <m-shah@ti.com>

serial: 8250_dw: Add capability to skip empty FIFO read

This commit introduces a new UART capability to guard against empty FIFO
reads during startup and shutdown. This prevents data aborts on the TDA54
IP block where reading an empty FIFO is fatal.

> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 38fa45e74a37a..f533310145849 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
[ ... ]
> @@ -2455,7 +2460,8 @@ void serial8250_do_shutdown(struct uart_port *port)
>  	 * Read data port to reset things, and then unlink from
>  	 * the IRQ chain.
>  	 */
> -	serial_port_in(port, UART_RX);
> +	if (!(up->capabilities & UART_CAP_RXFIFO_EMPTY_READ) || (serial_lsr_in(up) & UART_LSR_DR))
> +		serial_port_in(port, UART_RX);

[Severity: Medium]
Does calling serial_lsr_in() here introduce a data race on up->lsr_saved_flags?

In serial8250_do_shutdown(), the uart_port_lock is explicitly released
before reaching this point. Calling serial_lsr_in() locklessly causes a
read-modify-write operation on the shared lsr_saved_flags state:

drivers/tty/serial/8250/8250.h:serial_lsr_in() {
	u16 lsr = up->lsr_saved_flags;

	lsr |= serial_in(up, UART_LSR);
	up->lsr_saved_flags = lsr & up->lsr_save_mask;

	return lsr;
}

If a console write occurs concurrently (e.g. an asynchronous printk() from
another CPU), serial8250_console_write() will also invoke serial_lsr_in()
via wait_for_xmitr() while holding the lock. Can this race corrupt the
lsr_saved_flags state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916085520.2259420-1-m-shah@ti.com?part=2