Re: [PATCH] serial: 8250: restore FIFO in serial8250_console_restore()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] serial: 8250: open-coded FCR restore in serial8250_console_restore()
--

commit 1d1d530952ba054a9213852856484185009eeee9
Author: fuzheng.meng <fuzheng.meng@semidrive.com>

serial: 8250: restore FIFO in serial8250_console_restore()

This commit factors out serial8250_console_use_fifo() and uses it in
serial8250_console_restore() to reprogram the FIFO control register after the
UART loses power during system suspend with console_suspend disabled.

> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 36b2b0a2c05cd..ee6b02cff0156 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c

[ ... ]

> @@ -3345,6 +3376,19 @@ static void serial8250_console_restore(struct uart_8250_port *up)
>  	serial8250_set_divisor(port, baud, quot, frac);
>  	serial_port_out(port, UART_LCR, up->lcr);
>  	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
> +
> +	/*
> +	 * System sleep may reset the FCR, leaving the FIFO disabled,
> +	 * while the cached up->fcr still tells console_write() to push
> +	 * tx_loadsz bytes into the FIFO at once; with the hardware FIFO
> +	 * off, the THR holds only one byte and earlier ones are
> +	 * overwritten, which garbles the console output after resume.
> +	 * Restore the FCR when the FIFO write path is going to be used.
> +	 */
> +	if (serial8250_console_use_fifo(up)) {
> +		serial_port_out(port, UART_FCR, UART_FCR_ENABLE_FIFO);
> +		serial_port_out(port, UART_FCR, up->fcr);
> +	}
>  }

[Severity: Medium]
Could serial8250_console_restore() call the existing serial8250_set_fcr()
helper (after dropping its unused termios parameter) instead of open-coding
the lcr and fcr writes here?

drivers/tty/serial/8250/8250_port.c:serial8250_set_fcr() {
	struct uart_8250_port *up = up_to_u8250p(port);
	bool is_16750 = port->type == PORT_16750;

	if (is_16750)
		serial_port_out(port, UART_FCR, up->fcr);

	/*
	 * LCR DLAB must be reset to enable 64-byte FIFO mode. If the FCR is written without DLAB
	 * set, this mode will be disabled.
	 */
	serial_port_out(port, UART_LCR, up->lcr);

	if (is_16750)
		return;

	/* emulated UARTs (Lucent Venus 167x) need two steps */
	if (up->fcr & UART_FCR_ENABLE_FIFO)
		serial_port_out(port, UART_FCR, UART_FCR_ENABLE_FIFO);

	serial_port_out(port, UART_FCR, up->fcr);
}

In serial8250_console_restore(), serial8250_set_divisor() sets the divisor
latch access bit, and writing up->lcr to the line control register before
programming the fifo control register clears that bit. On 16750 ports,
writing up->fcr after clearing the divisor latch access bit disables 64-byte
fifo mode and leaves a 16-byte hardware fifo while up->tx_loadsz remains 64,
which can overflow the fifo in __serial8250_console_fifo_write().

Also, if serial8250_console_use_fifo(up) returns false (for instance when
uart_console_hwflow_active(port) is true, when auto-flow control is enabled in
up->mcr, when up->tx_loadsz is 1, or when the tty port is not initialized),
doesn't skipping the fcr write leave up->fcr unprogrammed after a power reset?

Finally, the commit message mentions that writing the enable bit followed by
the cached up->fcr matches dw8250_idle_exit(), but dw8250_idle_exit() in
drivers/tty/serial/8250/8250_dw.c only performs a single write:

	if (up->capabilities & UART_CAP_FIFO)
		serial_port_out(p, UART_FCR, up->fcr);

Using serial8250_set_fcr() directly after serial8250_set_divisor() would
handle both the 16750 ordering requirement and the two-step write without
duplicating the sequence.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102093609.2400381-1-fuzheng.meng@semidrive.com?part=1