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

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

[Yunhui Cui]

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

Fixes: c49436b657d0 ("serial: 8250_dw: Improve unwritable LCR workaround")
Link: https://lore.kernel.org/all/84cydt5peu.fsf@jogness.linutronix.de/T/
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 drivers/tty/serial/8250/8250_dw.c   |  8 +++++
 drivers/tty/serial/8250/8250_port.c | 46 ++++++++++++++++++-----------
 2 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index af24ec25d976..e97200ff30e3 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
+#include <linux/lockdep.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
@@ -112,6 +113,13 @@ static void dw8250_force_idle(struct uart_port *p)
 	struct uart_8250_port *up = up_to_u8250p(p);
 	unsigned int lsr;
 
+	/*
+	 * Serial_in(p, UART_RX) should be under port->lock, but we can't add
+	 * it to avoid AA deadlock as we're unsure if serial_out*(...UART_LCR)
+	 * is under port->lock.
+	 */
+	lockdep_assert_held_once(&p->lock);
+
 	serial8250_clear_and_reinit_fifos(up);
 
 	/*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3f256e96c722..21bbd18195f5 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1328,6 +1328,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	unsigned int ICP = 0;
 	unsigned long irqs;
 	int irq;
+	u16 lsr;
 
 	if (port->flags & UPF_FOURPORT) {
 		ICP = (port->iobase & 0xfe0) | 0x1f;
@@ -1357,9 +1358,10 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	/* Synchronize UART_IER access against the console. */
 	uart_port_lock_irq(port);
 	serial_out(up, UART_IER, UART_IER_ALL_INTR);
+	lsr = serial_in(up, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
 	uart_port_unlock_irq(port);
-	serial_in(up, UART_LSR);
-	serial_in(up, UART_RX);
 	serial_in(up, UART_IIR);
 	serial_in(up, UART_MSR);
 	serial_out(up, UART_TX, 0xFF);
@@ -2137,19 +2139,16 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
 static int serial8250_get_poll_char(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
-	int status;
+	int status = NO_POLL_CHAR;
 	u16 lsr;
 
 	serial8250_rpm_get(up);
 
+	uart_port_lock_irqsave(port, &flags);
 	lsr = serial_port_in(port, UART_LSR);
-
-	if (!(lsr & UART_LSR_DR)) {
-		status = NO_POLL_CHAR;
-		goto out;
-	}
-
-	status = serial_port_in(port, UART_RX);
+	if ((lsr & UART_LSR_DR))
+		status = serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 out:
 	serial8250_rpm_put(up);
 	return status;
@@ -2264,13 +2263,16 @@ int serial8250_do_startup(struct uart_port *port)
 	 * Clear the FIFO buffers and disable them.
 	 * (they will be reenabled in set_termios())
 	 */
+	uart_port_lock_irqsave(port, &flags);
 	serial8250_clear_fifos(up);
 
 	/*
 	 * Clear the interrupt registers.
 	 */
-	serial_port_in(port, UART_LSR);
-	serial_port_in(port, UART_RX);
+	lsr = serial_port_in(port, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 
@@ -2380,9 +2382,10 @@ int serial8250_do_startup(struct uart_port *port)
 	/*
 	 * Now, initialize the UART
 	 */
-	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
 
 	uart_port_lock_irqsave(port, &flags);
+	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
+
 	if (up->port.flags & UPF_FOURPORT) {
 		if (!up->port.irq)
 			up->port.mctrl |= TIOCM_OUT1;
@@ -2428,15 +2431,16 @@ int serial8250_do_startup(struct uart_port *port)
 	}
 
 dont_test_tx_en:
-	uart_port_unlock_irqrestore(port, flags);
 
 	/*
 	 * Clear the interrupt registers again for luck, and clear the
 	 * saved flags to avoid getting false values from polling
 	 * routines or the previous session.
 	 */
-	serial_port_in(port, UART_LSR);
-	serial_port_in(port, UART_RX);
+	lsr = serial_port_in(port, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 	up->lsr_saved_flags = 0;
@@ -2492,6 +2496,7 @@ void serial8250_do_shutdown(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned long flags;
+	u16 lsr;
 
 	serial8250_rpm_get(up);
 	/*
@@ -2518,7 +2523,6 @@ void serial8250_do_shutdown(struct uart_port *port)
 		port->mctrl &= ~TIOCM_OUT2;
 
 	serial8250_set_mctrl(port, port->mctrl);
-	uart_port_unlock_irqrestore(port, flags);
 
 	/*
 	 * Disable break condition and FIFOs
@@ -2526,6 +2530,7 @@ void serial8250_do_shutdown(struct uart_port *port)
 	serial_port_out(port, UART_LCR,
 			serial_port_in(port, UART_LCR) & ~UART_LCR_SBC);
 	serial8250_clear_fifos(up);
+	uart_port_unlock_irqrestore(port, flags);
 
 #ifdef CONFIG_SERIAL_8250_RSA
 	/*
@@ -2538,7 +2543,12 @@ 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);
+	uart_port_lock_irqsave(port, &flags);
+	lsr = serial_port_in(port, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
+
 	serial8250_rpm_put(up);
 
 	up->ops->release_irq(up);
-- 
2.39.2

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

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8227 bytes --]

On Mon, 14 Apr 2025, Yunhui Cui wrote:

> 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
> 
> Fixes: c49436b657d0 ("serial: 8250_dw: Improve unwritable LCR workaround")
> Link: https://lore.kernel.org/all/84cydt5peu.fsf@jogness.linutronix.de/T/
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>

As Andy mentioned, this change looks it would benefit from splitting to 
multiple parts.

However, this change brings back some memories from a few years back.
Back then, there was a reporter who had issues issues related to 
dw8250_force_idle() or writing some of the registers (IIRC). I ended up 
looking into finding a better solution to the write-while-BUSY problem 
which entirely replaced dw8250_force_idle() that is quite hacky and seems 
unreliable on fundamendal level.

Sadly, once I had posted a patch for testing, the reporter went dead 
silent so the patch was left rotting as I had no time to try to reproduce.

Perhaps the patch I created back then would be useful for addressing this 
problem you're facing (the patch is attached). I've rebased the patch on 
top of the tty-next now (but I did no testing beyond compiling). There are 
a few further thoughts / missing bits mentioned in the comments within the 
patch itself (I did not try to updated them now, so the comments may have 
rotten too).

> ---
>  drivers/tty/serial/8250/8250_dw.c   |  8 +++++
>  drivers/tty/serial/8250/8250_port.c | 46 ++++++++++++++++++-----------
>  2 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index af24ec25d976..e97200ff30e3 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -13,6 +13,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
> +#include <linux/lockdep.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/notifier.h>
> @@ -112,6 +113,13 @@ static void dw8250_force_idle(struct uart_port *p)
>  	struct uart_8250_port *up = up_to_u8250p(p);
>  	unsigned int lsr;
>  
> +	/*
> +	 * Serial_in(p, UART_RX) should be under port->lock, but we can't add
> +	 * it to avoid AA deadlock as we're unsure if serial_out*(...UART_LCR)
> +	 * is under port->lock.
> +	 */
> +	lockdep_assert_held_once(&p->lock);
> +
>  	serial8250_clear_and_reinit_fifos(up);
>  
>  	/*
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 3f256e96c722..21bbd18195f5 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1328,6 +1328,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  	unsigned int ICP = 0;
>  	unsigned long irqs;
>  	int irq;
> +	u16 lsr;
>  
>  	if (port->flags & UPF_FOURPORT) {
>  		ICP = (port->iobase & 0xfe0) | 0x1f;
> @@ -1357,9 +1358,10 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  	/* Synchronize UART_IER access against the console. */
>  	uart_port_lock_irq(port);
>  	serial_out(up, UART_IER, UART_IER_ALL_INTR);
> +	lsr = serial_in(up, UART_LSR);
> +	if (lsr & UART_LSR_DR)
> +		serial_port_in(port, UART_RX);
>  	uart_port_unlock_irq(port);
> -	serial_in(up, UART_LSR);
> -	serial_in(up, UART_RX);
>  	serial_in(up, UART_IIR);
>  	serial_in(up, UART_MSR);
>  	serial_out(up, UART_TX, 0xFF);
> @@ -2137,19 +2139,16 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>  static int serial8250_get_poll_char(struct uart_port *port)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
> -	int status;
> +	int status = NO_POLL_CHAR;
>  	u16 lsr;
>  
>  	serial8250_rpm_get(up);
>  
> +	uart_port_lock_irqsave(port, &flags);
>  	lsr = serial_port_in(port, UART_LSR);
> -
> -	if (!(lsr & UART_LSR_DR)) {
> -		status = NO_POLL_CHAR;
> -		goto out;
> -	}
> -
> -	status = serial_port_in(port, UART_RX);
> +	if ((lsr & UART_LSR_DR))
> +		status = serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);
>  out:
>  	serial8250_rpm_put(up);
>  	return status;
> @@ -2264,13 +2263,16 @@ int serial8250_do_startup(struct uart_port *port)
>  	 * Clear the FIFO buffers and disable them.
>  	 * (they will be reenabled in set_termios())
>  	 */
> +	uart_port_lock_irqsave(port, &flags);
>  	serial8250_clear_fifos(up);
>  
>  	/*
>  	 * Clear the interrupt registers.
>  	 */
> -	serial_port_in(port, UART_LSR);
> -	serial_port_in(port, UART_RX);
> +	lsr = serial_port_in(port, UART_LSR);
> +	if (lsr & UART_LSR_DR)
> +		serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);
>  	serial_port_in(port, UART_IIR);
>  	serial_port_in(port, UART_MSR);
>  
> @@ -2380,9 +2382,10 @@ int serial8250_do_startup(struct uart_port *port)
>  	/*
>  	 * Now, initialize the UART
>  	 */
> -	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
>  
>  	uart_port_lock_irqsave(port, &flags);
> +	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
> +
>  	if (up->port.flags & UPF_FOURPORT) {
>  		if (!up->port.irq)
>  			up->port.mctrl |= TIOCM_OUT1;
> @@ -2428,15 +2431,16 @@ int serial8250_do_startup(struct uart_port *port)
>  	}
>  
>  dont_test_tx_en:

I don't see this in the tty-next branch?

~/linux/tty-next$ git grep dont_test_tx_en | cat -
~/linux/tty-next$ 

-- 
 i.

> -	uart_port_unlock_irqrestore(port, flags);
>  
>  	/*
>  	 * Clear the interrupt registers again for luck, and clear the
>  	 * saved flags to avoid getting false values from polling
>  	 * routines or the previous session.
>  	 */
> -	serial_port_in(port, UART_LSR);
> -	serial_port_in(port, UART_RX);
> +	lsr = serial_port_in(port, UART_LSR);
> +	if (lsr & UART_LSR_DR)
> +		serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);
>  	serial_port_in(port, UART_IIR);
>  	serial_port_in(port, UART_MSR);
>  	up->lsr_saved_flags = 0;
> @@ -2492,6 +2496,7 @@ void serial8250_do_shutdown(struct uart_port *port)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
>  	unsigned long flags;
> +	u16 lsr;
>  
>  	serial8250_rpm_get(up);
>  	/*
> @@ -2518,7 +2523,6 @@ void serial8250_do_shutdown(struct uart_port *port)
>  		port->mctrl &= ~TIOCM_OUT2;
>  
>  	serial8250_set_mctrl(port, port->mctrl);
> -	uart_port_unlock_irqrestore(port, flags);
>  
>  	/*
>  	 * Disable break condition and FIFOs
> @@ -2526,6 +2530,7 @@ void serial8250_do_shutdown(struct uart_port *port)
>  	serial_port_out(port, UART_LCR,
>  			serial_port_in(port, UART_LCR) & ~UART_LCR_SBC);
>  	serial8250_clear_fifos(up);
> +	uart_port_unlock_irqrestore(port, flags);
>  
>  #ifdef CONFIG_SERIAL_8250_RSA
>  	/*
> @@ -2538,7 +2543,12 @@ 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);
> +	uart_port_lock_irqsave(port, &flags);
> +	lsr = serial_port_in(port, UART_LSR);
> +	if (lsr & UART_LSR_DR)
> +		serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);
> +
>  	serial8250_rpm_put(up);
>  
>  	up->ops->release_irq(up);
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; NAME=0001-serial-8250_dw-Ensure-BUSY-is-deasserted.patch, Size: 7446 bytes --]

From 10237640b15ada313c3ac3021d7cc9aeb774d5c6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Mon, 14 Apr 2025 14:23:36 +0300
Subject: [PATCH 1/1] serial: 8250_dw: Ensure BUSY is deasserted
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
Existance of BUSY depends on uart_16550_compatible, if UART HW is
configured with 16550 compatible those registers can always be
written.

There currently is dw8250_force_idle() which attempts to archive
non-BUSY state by disabling FIFO, however, the solution is unreliable
when Rx keeps getting more and more characters.

Create a sequence of operations that ensures UART cannot keep BUSY
asserted indefinitely. The new sequence relies on enabling loopback
mode temporarily to prevent incoming Rx characters keeping UART BUSY.

Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
writes.

This issue was reported by qianfan Zhao who put lots of debugging
effort into understanding the solution space.

Reported-by: qianfan Zhao <qianfanguijin@163.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 149 +++++++++++++++++++++---------
 1 file changed, 103 insertions(+), 46 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1902f29444a1..8a9dffc85fe3 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -42,6 +42,8 @@
 /* DesignWare specific register fields */
 #define DW_UART_MCR_SIRE		BIT(6)
 
+#define DW_UART_USR_BUSY		BIT(0)
+
 /* Renesas specific register fields */
 #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
 #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
@@ -77,6 +79,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		in_idle:1;
 };
 
 static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
@@ -108,36 +111,89 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
 }
 
 /*
- * This function is being called as part of the uart_port::serial_out()
- * routine. Hence, it must not call serial_port_out() or serial_out()
- * against the modified registers here, i.e. LCR.
+ * Ensure BUSY is not asserted. If DW UART is configured with
+ * !uart_16550_compatible, the writes to LCR, DLL, and DLH fail while
+ * BUSY is asserted.
+ *
+ * Context: port's lock must be held
  */
-static void dw8250_force_idle(struct uart_port *p)
+static int dw8250_idle_enter(struct uart_port *p)
 {
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	struct uart_8250_port *up = up_to_u8250p(p);
-	unsigned int lsr;
+	u32 lsr;
 
-	/*
-	 * The following call currently performs serial_out()
-	 * against the FCR register. Because it differs to LCR
-	 * there will be no infinite loop, but if it ever gets
-	 * modified, we might need a new custom version of it
-	 * that avoids infinite recursion.
-	 */
-	serial8250_clear_and_reinit_fifos(up);
+	if (d->uart_16550_compatible)
+		return 0;
 
-	/*
-	 * With PSLVERR_RESP_EN parameter set to 1, the device generates an
-	 * error response when an attempt to read an empty RBR with FIFO
-	 * enabled.
-	 */
-	if (up->fcr & UART_FCR_ENABLE_FIFO) {
-		lsr = serial_port_in(p, UART_LSR);
-		if (!(lsr & UART_LSR_DR))
-			return;
+	d->in_idle = 1;
+
+	/* Prevent triggering interrupt from RBR filling */
+	p->serial_out(p, UART_IER, 0);
+
+	serial8250_rx_dma_flush(up);
+	// What about Tx DMA? Should probably pause that too and resume
+	// afterwards.
+
+	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
+	if (up->capabilities & UART_CAP_FIFO)
+		p->serial_out(p, UART_FCR, 0);
+
+	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
+		ndelay(p->frame_time);
+
+	lsr = serial_lsr_in(up);
+	if (lsr & UART_LSR_DR) {
+		p->serial_in(p, UART_RX);
+		up->lsr_saved_flags = 0;
 	}
 
-	serial_port_in(p, UART_RX);
+	/* Now guaranteed to have BUSY deasserted? Just sanity check */
+	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
+		return -EBUSY;
+
+	return 0;
+}
+
+static void dw8250_idle_exit(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	struct uart_8250_port *up = up_to_u8250p(p);
+
+	if (d->uart_16550_compatible)
+		return;
+
+	if (up->capabilities & UART_CAP_FIFO)
+		p->serial_out(p, UART_FCR, up->fcr);
+	p->serial_out(p, UART_MCR, up->mcr);
+	p->serial_out(p, UART_IER, up->ier);
+
+	// Maybe move the DMA Rx restart check in dma_rx_complete() to own
+	// function (serial8250_rx_dma_restart()) and call it from here.
+	// DMA Tx resume
+
+	d->in_idle = 0;
+}
+
+static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
+			       unsigned int quot, unsigned int quot_frac)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+	int ret;
+
+	ret = dw8250_idle_enter(p);
+	if (ret < 0)
+		goto idle_failed;
+
+	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
+	if (!(p->serial_in(p, UART_LCR) & UART_LCR_DLAB))
+		goto idle_failed;
+
+	serial_dl_write(up, quot);
+	p->serial_out(p, UART_LCR, up->lcr);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 /*
@@ -148,37 +204,37 @@ static void dw8250_force_idle(struct uart_port *p)
 static void dw8250_check_lcr(struct uart_port *p, int offset, int value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
-	void __iomem *addr = p->membase + (offset << p->regshift);
-	int tries = 1000;
+	unsigned int lcr = p->serial_in(p, UART_LCR);
+	int ret;
 
 	if (offset != UART_LCR || d->uart_16550_compatible)
 		return;
 
 	/* Make sure LCR write wasn't ignored */
-	while (tries--) {
-		unsigned int lcr = serial_port_in(p, offset);
-
-		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
-			return;
+	if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
+		return;
 
-		dw8250_force_idle(p);
+	if (d->in_idle) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		return;
+	}
 
-#ifdef CONFIG_64BIT
-		if (p->type == PORT_OCTEON)
-			__raw_writeq(value & 0xff, addr);
-		else
-#endif
-		if (p->iotype == UPIO_MEM32)
-			writel(value, addr);
-		else if (p->iotype == UPIO_MEM32BE)
-			iowrite32be(value, addr);
-		else
-			writeb(value, addr);
+	ret = dw8250_idle_enter(p);
+	if (ret < 0) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		goto idle_failed;
 	}
-	/*
-	 * FIXME: this deadlocks if port->lock is already held
-	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
-	 */
+
+	p->serial_out(p, UART_LCR, value);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 /* Returns once the transmitter is empty or we run out of retries */
@@ -547,6 +603,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->dev		= dev;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->set_divisor	= dw8250_set_divisor;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)

base-commit: 043806bc9dbc6597dd15e6ca9220ae2746425f2f
-- 
2.39.5

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7328 bytes --]

On Mon, 14 Apr 2025, Ilpo Järvinen wrote:

> On Mon, 14 Apr 2025, Yunhui Cui wrote:
> 
> > 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
> > 
> > Fixes: c49436b657d0 ("serial: 8250_dw: Improve unwritable LCR workaround")
> > Link: https://lore.kernel.org/all/84cydt5peu.fsf@jogness.linutronix.de/T/
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> 
> As Andy mentioned, this change looks it would benefit from splitting to 
> multiple parts.
> 
> However, this change brings back some memories from a few years back.
> Back then, there was a reporter who had issues issues related to 
> dw8250_force_idle() or writing some of the registers (IIRC). I ended up 
> looking into finding a better solution to the write-while-BUSY problem 
> which entirely replaced dw8250_force_idle() that is quite hacky and seems 
> unreliable on fundamendal level.
> 
> Sadly, once I had posted a patch for testing, the reporter went dead 
> silent so the patch was left rotting as I had no time to try to reproduce.
> 
> Perhaps the patch I created back then would be useful for addressing this 
> problem you're facing (the patch is attached). I've rebased the patch on 
> top of the tty-next now (but I did no testing beyond compiling). There are 
> a few further thoughts / missing bits mentioned in the comments within the 
> patch itself (I did not try to updated them now, so the comments may have 
> rotten too).

After some further thinking. I realized you're interested in the opposite 
case (empty rx), whereas my patch focused on addressing (rx getting 
refilled constantly) so it doesn't seem that useful for your case. The 
patch shows though the direction I'd like to head with idle forcing 
approach.

--
 i.

> > ---
> >  drivers/tty/serial/8250/8250_dw.c   |  8 +++++
> >  drivers/tty/serial/8250/8250_port.c | 46 ++++++++++++++++++-----------
> >  2 files changed, 36 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index af24ec25d976..e97200ff30e3 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> > +#include <linux/lockdep.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/notifier.h>
> > @@ -112,6 +113,13 @@ static void dw8250_force_idle(struct uart_port *p)
> >  	struct uart_8250_port *up = up_to_u8250p(p);
> >  	unsigned int lsr;
> >  
> > +	/*
> > +	 * Serial_in(p, UART_RX) should be under port->lock, but we can't add
> > +	 * it to avoid AA deadlock as we're unsure if serial_out*(...UART_LCR)
> > +	 * is under port->lock.
> > +	 */
> > +	lockdep_assert_held_once(&p->lock);
> > +
> >  	serial8250_clear_and_reinit_fifos(up);
> >  
> >  	/*
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 3f256e96c722..21bbd18195f5 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -1328,6 +1328,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
> >  	unsigned int ICP = 0;
> >  	unsigned long irqs;
> >  	int irq;
> > +	u16 lsr;
> >  
> >  	if (port->flags & UPF_FOURPORT) {
> >  		ICP = (port->iobase & 0xfe0) | 0x1f;
> > @@ -1357,9 +1358,10 @@ static void autoconfig_irq(struct uart_8250_port *up)
> >  	/* Synchronize UART_IER access against the console. */
> >  	uart_port_lock_irq(port);
> >  	serial_out(up, UART_IER, UART_IER_ALL_INTR);
> > +	lsr = serial_in(up, UART_LSR);
> > +	if (lsr & UART_LSR_DR)
> > +		serial_port_in(port, UART_RX);
> >  	uart_port_unlock_irq(port);
> > -	serial_in(up, UART_LSR);
> > -	serial_in(up, UART_RX);
> >  	serial_in(up, UART_IIR);
> >  	serial_in(up, UART_MSR);
> >  	serial_out(up, UART_TX, 0xFF);
> > @@ -2137,19 +2139,16 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> >  static int serial8250_get_poll_char(struct uart_port *port)
> >  {
> >  	struct uart_8250_port *up = up_to_u8250p(port);
> > -	int status;
> > +	int status = NO_POLL_CHAR;
> >  	u16 lsr;
> >  
> >  	serial8250_rpm_get(up);
> >  
> > +	uart_port_lock_irqsave(port, &flags);
> >  	lsr = serial_port_in(port, UART_LSR);
> > -
> > -	if (!(lsr & UART_LSR_DR)) {
> > -		status = NO_POLL_CHAR;
> > -		goto out;
> > -	}
> > -
> > -	status = serial_port_in(port, UART_RX);
> > +	if ((lsr & UART_LSR_DR))
> > +		status = serial_port_in(port, UART_RX);
> > +	uart_port_unlock_irqrestore(port, flags);
> >  out:
> >  	serial8250_rpm_put(up);
> >  	return status;
> > @@ -2264,13 +2263,16 @@ int serial8250_do_startup(struct uart_port *port)
> >  	 * Clear the FIFO buffers and disable them.
> >  	 * (they will be reenabled in set_termios())
> >  	 */
> > +	uart_port_lock_irqsave(port, &flags);
> >  	serial8250_clear_fifos(up);
> >  
> >  	/*
> >  	 * Clear the interrupt registers.
> >  	 */
> > -	serial_port_in(port, UART_LSR);
> > -	serial_port_in(port, UART_RX);
> > +	lsr = serial_port_in(port, UART_LSR);
> > +	if (lsr & UART_LSR_DR)
> > +		serial_port_in(port, UART_RX);
> > +	uart_port_unlock_irqrestore(port, flags);
> >  	serial_port_in(port, UART_IIR);
> >  	serial_port_in(port, UART_MSR);
> >  
> > @@ -2380,9 +2382,10 @@ int serial8250_do_startup(struct uart_port *port)
> >  	/*
> >  	 * Now, initialize the UART
> >  	 */
> > -	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
> >  
> >  	uart_port_lock_irqsave(port, &flags);
> > +	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
> > +
> >  	if (up->port.flags & UPF_FOURPORT) {
> >  		if (!up->port.irq)
> >  			up->port.mctrl |= TIOCM_OUT1;
> > @@ -2428,15 +2431,16 @@ int serial8250_do_startup(struct uart_port *port)
> >  	}
> >  
> >  dont_test_tx_en:
> 
> I don't see this in the tty-next branch?
> 
> ~/linux/tty-next$ git grep dont_test_tx_en | cat -
> ~/linux/tty-next$ 
> 
> 

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

[yunhui cui]

Hi Andy,

On Mon, Apr 14, 2025 at 4:47 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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).

Splitting into a patchset is fine. What does "patch 1" refer to here?

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

Okay.

>
> > +      * 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.

Okay.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thanks,
Yunhui

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

[Andy Shevchenko]

On Mon, Apr 21, 2025 at 10:55:05AM +0800, yunhui cui wrote:
> On Mon, Apr 14, 2025 at 4:47 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Apr 14, 2025 at 11:14:50AM +0800, Yunhui Cui wrote:

...

> > This patch seems need split to three. See below.

...

> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> >
> > Changes here deserve the separate patch (patch 1).
> 
> Splitting into a patchset is fine. What does "patch 1" refer to here?

A new series out of three patches, the above change looks good to be placed in
patch 1/3 in that _new_ series. Same for patch 2/3 and 3/3 below.

...

> > > +     /*
> > > +      * Serial_in(p, UART_RX) should be under port->lock, but we can't add
> >
> > serial_in()
> 
> Okay.
> 
> > > +      * 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.
> 
> Okay.

-- 
With Best Regards,
Andy Shevchenko