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

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

[fuzheng.meng]

When console_suspend is disabled, the serial console stays enabled
over a system suspend, and serial8250_suspend_port() writes a canary
value to the SCR register (if the port has one) to detect whether the
UART lost power during the sleep cycle.

If the hardware did lose power, all registers are reset, and the
first console write after resume detects the canary mismatch and
calls serial8250_console_restore(), which reprograms divisor, LCR and
MCR, but not the FCR.

serial8250_console_write() decides whether to take the FIFO write
path from the cached up->fcr, which still carries
UART_FCR_ENABLE_FIFO. With the hardware FIFO left disabled by the
reset, the transmit holding register holds only one byte, while
serial8250_console_fifo_write() pushes up to tx_loadsz bytes into the
port after waiting for THRE only once. The burst overwrites itself
and the console output right after resume is garbled.

Factor the FIFO write path condition out into a new
serial8250_console_use_fifo() helper and use it from both
serial8250_console_write() and serial8250_console_restore(). Restore
the FIFO in serial8250_console_restore() when the FIFO write path is
going to be used: first write UART_FCR_ENABLE_FIFO, then the cached
fcr, as dw8250_idle_exit() does after its registers are reset.

Tested on a DesignWare 8250 based platform with console_suspend=0
over a suspend/resume cycle.

Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
Cc: stable@vger.kernel.org
Signed-off-by: fuzheng.meng <fuzheng.meng@semidrive.com>
---
 drivers/tty/serial/8250/8250_port.c | 64 ++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 36b2b0a2c05c..ee6b02cff015 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3321,6 +3321,37 @@ static void serial8250_console_wait_putchar(struct uart_port *port, unsigned cha
 	serial8250_console_putchar(port, ch);
 }
 
+/*
+ * Console output can go through the TX FIFO in one tx_loadsz chunk only
+ * if the port has a FIFO and the software state expects it enabled. The
+ * same conditions tell console_restore() whether the FIFO has to be
+ * brought back after a system sleep reset.
+ */
+static bool serial8250_console_use_fifo(struct uart_8250_port *up)
+{
+	struct uart_port *port = &up->port;
+
+	return (up->capabilities & UART_CAP_FIFO) &&
+		/*
+		 * BCM283x requires to check the fifo
+		 * after each byte.
+		 */
+		!(up->capabilities & UART_CAP_MINI) &&
+		/*
+		 * tx_loadsz contains the transmit fifo size
+		 */
+		up->tx_loadsz > 1 &&
+		(up->fcr & UART_FCR_ENABLE_FIFO) &&
+		port->state &&
+		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
+		/*
+		 * After we put a data in the fifo, the controller will send
+		 * it regardless of the CTS state. Therefore, only use fifo
+		 * if we don't use control flow.
+		 */
+		!uart_console_hwflow_active(port);
+}
+
 /*
  *	Restore serial console when h/w power-off detected
  */
@@ -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);
+	}
 }
 
 /*
@@ -3495,25 +3539,7 @@ void serial8250_console_write(struct uart_8250_port *up,
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
-	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
-		/*
-		 * BCM283x requires to check the fifo
-		 * after each byte.
-		 */
-		!(up->capabilities & UART_CAP_MINI) &&
-		/*
-		 * tx_loadsz contains the transmit fifo size
-		 */
-		up->tx_loadsz > 1 &&
-		(up->fcr & UART_FCR_ENABLE_FIFO) &&
-		port->state &&
-		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
-		/*
-		 * After we put a data in the fifo, the controller will send
-		 * it regardless of the CTS state. Therefore, only use fifo
-		 * if we don't use control flow.
-		 */
-		!uart_console_hwflow_active(&up->port);
+	use_fifo = serial8250_console_use_fifo(up);
 
 	nbcon_exit_unsafe(wctxt);
 
-- 
2.43.0
注意：本电子邮件及其任何附件均归北京芯驰半导体科技股份有限公司及其各分子公司（"芯驰科技"）所有。本电子邮件及其附件仅供指定的收件人或收件实体使用，并可能包含芯驰科技的机密信息。禁止任何未经授权的使用、复制、传播或其他因依赖本电子邮件而采取或不采取的行为。如果您不是此电子邮件的预期收件人，请立即通过回复邮件通知发件人，并从您的系统中删除此邮件及任何附件。非常感谢。
Attention: This email and any attachment hereto belong to Beijing Semidrive Technology Corporation and each of its affiliates and subsidiaries ("SemiDrive Group"). This email and its attachments are intended solely for the use of the individual or entity to which they are addressed and may contain confidential information of SemiDrive Group. Any unauthorized use, copying, dissemination or other action taken or omitted in reliance upon this email is strictly prohibited. If you are not the intended recipient, please notify the sender immediately by reply email and delete this message and any attachments from your system. Thank you.

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

[sashiko-bot]

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

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

[Greg KH]

On Mon, Nov 02, 2026 at 05:36:09PM +0800, fuzheng.meng wrote:
> When console_suspend is disabled, the serial console stays enabled
> over a system suspend, and serial8250_suspend_port() writes a canary
> value to the SCR register (if the port has one) to detect whether the
> UART lost power during the sleep cycle.
> 
> If the hardware did lose power, all registers are reset, and the
> first console write after resume detects the canary mismatch and
> calls serial8250_console_restore(), which reprograms divisor, LCR and
> MCR, but not the FCR.
> 
> serial8250_console_write() decides whether to take the FIFO write
> path from the cached up->fcr, which still carries
> UART_FCR_ENABLE_FIFO. With the hardware FIFO left disabled by the
> reset, the transmit holding register holds only one byte, while
> serial8250_console_fifo_write() pushes up to tx_loadsz bytes into the
> port after waiting for THRE only once. The burst overwrites itself
> and the console output right after resume is garbled.
> 
> Factor the FIFO write path condition out into a new
> serial8250_console_use_fifo() helper and use it from both
> serial8250_console_write() and serial8250_console_restore(). Restore
> the FIFO in serial8250_console_restore() when the FIFO write path is
> going to be used: first write UART_FCR_ENABLE_FIFO, then the cached
> fcr, as dw8250_idle_exit() does after its registers are reset.
> 
> Tested on a DesignWare 8250 based platform with console_suspend=0
> over a suspend/resume cycle.
> 
> Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: fuzheng.meng <fuzheng.meng@semidrive.com>

Real name please, not an email alias.

thanks,

greg k-h

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

[Fuzheng Meng]

When console_suspend is disabled, the serial console stays enabled
over a system suspend, and serial8250_suspend_port() writes a canary
value to the SCR register (if the port has one) to detect whether the
UART lost power during the sleep cycle.

If the hardware did lose power, all registers are reset, and the
first console write after resume detects the canary mismatch and
calls serial8250_console_restore(), which reprograms divisor, LCR and
MCR, but not the FCR.

serial8250_console_write() decides whether to take the FIFO write
path from the cached up->fcr, which still carries
UART_FCR_ENABLE_FIFO. With the hardware FIFO left disabled by the
reset, the transmit holding register holds only one byte, while
serial8250_console_fifo_write() pushes up to tx_loadsz bytes into the
port after waiting for THRE only once. The burst overwrites itself
and the console output right after resume is garbled.

Factor the FIFO write path condition out into a new
serial8250_console_use_fifo() helper and use it from both
serial8250_console_write() and serial8250_console_restore(). Restore
the FIFO in serial8250_console_restore() when the FIFO write path is
going to be used: first write UART_FCR_ENABLE_FIFO, then the cached
fcr, as dw8250_idle_exit() does after its registers are reset.

Tested on a DesignWare 8250 based platform with console_suspend=0
over a suspend/resume cycle.

Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
Cc: stable@vger.kernel.org
Signed-off-by: Fuzheng Meng <fuzheng.meng@semidrive.com>
---
Changes in v2:
- Use my real name (Fuzheng Meng) in the From: header and Signed-off-by
  tag instead of the account alias, as requested in the review of v1.
  No code changes.
 drivers/tty/serial/8250/8250_port.c | 64 ++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 36b2b0a2c05c..ee6b02cff015 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3321,6 +3321,37 @@ static void serial8250_console_wait_putchar(struct uart_port *port, unsigned cha
 	serial8250_console_putchar(port, ch);
 }
 
+/*
+ * Console output can go through the TX FIFO in one tx_loadsz chunk only
+ * if the port has a FIFO and the software state expects it enabled. The
+ * same conditions tell console_restore() whether the FIFO has to be
+ * brought back after a system sleep reset.
+ */
+static bool serial8250_console_use_fifo(struct uart_8250_port *up)
+{
+	struct uart_port *port = &up->port;
+
+	return (up->capabilities & UART_CAP_FIFO) &&
+		/*
+		 * BCM283x requires to check the fifo
+		 * after each byte.
+		 */
+		!(up->capabilities & UART_CAP_MINI) &&
+		/*
+		 * tx_loadsz contains the transmit fifo size
+		 */
+		up->tx_loadsz > 1 &&
+		(up->fcr & UART_FCR_ENABLE_FIFO) &&
+		port->state &&
+		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
+		/*
+		 * After we put a data in the fifo, the controller will send
+		 * it regardless of the CTS state. Therefore, only use fifo
+		 * if we don't use control flow.
+		 */
+		!uart_console_hwflow_active(port);
+}
+
 /*
  *	Restore serial console when h/w power-off detected
  */
@@ -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);
+	}
 }
 
 /*
@@ -3495,25 +3539,7 @@ void serial8250_console_write(struct uart_8250_port *up,
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
-	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
-		/*
-		 * BCM283x requires to check the fifo
-		 * after each byte.
-		 */
-		!(up->capabilities & UART_CAP_MINI) &&
-		/*
-		 * tx_loadsz contains the transmit fifo size
-		 */
-		up->tx_loadsz > 1 &&
-		(up->fcr & UART_FCR_ENABLE_FIFO) &&
-		port->state &&
-		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
-		/*
-		 * After we put a data in the fifo, the controller will send
-		 * it regardless of the CTS state. Therefore, only use fifo
-		 * if we don't use control flow.
-		 */
-		!uart_console_hwflow_active(&up->port);
+	use_fifo = serial8250_console_use_fifo(up);
 
 	nbcon_exit_unsafe(wctxt);
 
-- 
2.43.0
注意：本电子邮件及其任何附件均归北京芯驰半导体科技股份有限公司及其各分子公司（"芯驰科技"）所有。本电子邮件及其附件仅供指定的收件人或收件实体使用，并可能包含芯驰科技的机密信息。禁止任何未经授权的使用、复制、传播或其他因依赖本电子邮件而采取或不采取的行为。如果您不是此电子邮件的预期收件人，请立即通过回复邮件通知发件人，并从您的系统中删除此邮件及任何附件。非常感谢。
Attention: This email and any attachment hereto belong to Beijing Semidrive Technology Corporation and each of its affiliates and subsidiaries ("SemiDrive Group"). This email and its attachments are intended solely for the use of the individual or entity to which they are addressed and may contain confidential information of SemiDrive Group. Any unauthorized use, copying, dissemination or other action taken or omitted in reliance upon this email is strictly prohibited. If you are not the intended recipient, please notify the sender immediately by reply email and delete this message and any attachments from your system. Thank you.

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

[sashiko-bot]

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 6d2b7c890ac68c261011475644811d6fe155a916
Author: Fuzheng Meng <fuzheng.meng@semidrive.com>

serial: 8250: restore FIFO in serial8250_console_restore()

This commit factors the FIFO write condition into a
serial8250_console_use_fifo() helper and uses it in
serial8250_console_restore() to reprogram UART_FCR when the UART loses power
during system sleep 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]
As reported by Sashiko on v1 [1], should serial8250_console_restore() call the
existing serial8250_set_fcr() helper instead of writing UART_LCR and
open-coding the UART_FCR restore conditionally on
serial8250_console_use_fifo()?

In drivers/tty/serial/8250/8250_port.c, serial8250_set_divisor() leaves
UART_LCR_DLAB set in UART_LCR, and serial8250_set_fcr() handles both the
PORT_16750 ordering requirement and the two-step FCR write:

static void serial8250_set_fcr(struct uart_port *port, struct ktermios *termios)
{
	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);
}

By writing up->lcr to UART_LCR before writing UART_FCR in
serial8250_console_restore(), UART_LCR_DLAB is already cleared when UART_FCR
is written. On TI 16750 (PORT_16750) UARTs, writing UART_FCR with DLAB
cleared disables 64-byte FIFO mode (UART_FCR7_64BYTE), leaving the hardware TX
FIFO in 16-byte mode while up->tx_loadsz remains 64 and
__serial8250_console_fifo_write() bursts up to 64 bytes per UART_LSR_THRE
wait, overflowing the 16-byte FIFO.

In addition, gating the UART_FCR write on serial8250_console_use_fifo(up)
skips restoring up->fcr after power loss when console burst-FIFO writes are
not used (for example when uart_console_hwflow_active(port) is true even
though UART_MCR_AFE is restored via up->mcr, when UART_CAP_MINI is set or
tx_loadsz is 1, or when TTY_PORT_INITIALIZED is not set such as on
8250_ingenic where ingenic_uart_serial_out() sets UART_FCR_UME on UART_FCR
writes), while serial8250_console_write() still clears up->canary = 0. If
device_may_wakeup() is enabled on the tty device, uart_resume_port() returns
early without calling set_termios(), leaving the hardware FIFO unprogrammed
for normal interrupt-driven TTY transmission in serial8250_tx_chars() as
well.

Also, the commit message mentions that writing UART_FCR_ENABLE_FIFO followed
by up->fcr matches dw8250_idle_exit(), whereas 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);

and it is serial8250_set_fcr() that performs the two-step write. Would calling
serial8250_set_fcr(port, NULL) right after serial8250_set_divisor() in
serial8250_console_restore() avoid duplicating this logic and restore both
UART_LCR and UART_FCR across all port types?

[1] https://lore.kernel.org/r/20261102093609.2400381-1-fuzheng.meng@semidrive.com

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