[PATCH v9 0/4] Serial: 8250: Fix PSLVERR related issues

[PATCH v9 0/4] Serial: 8250: Fix PSLVERR related issues

[Yunhui Cui]

Cause of PSLVERR:
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.

Patch[1]: Fixes a panic caused by PSLVERR due to concurrent UART access.
Patch[2]: Fixes a panic caused by PSLVERR during RX_TIMEOUT conditions.
Patch[3] & Patch[4]: Improvements to minimize the occurrence of PSLVERR.

v1 -> v2:
Added UART_LSR_DR check in shutdown() to avoid PSLVERR issues.
Added Fixes: tag to reference upstream issues.
v2 -> v3:
Added lock protection in more functions (e.g., autoconfig_irq()) to
ensure atomicity.
Used lockdep_assert_held_once to detect potential deadlock risks early.
v3 -> v4:
Introduced serial8250_discard_data() to unify data read logic and avoid
code duplication.
Addressed PSLVERR caused by RX_TIMEOUT.
Split complex fixes into multiple patches (1/4 to 4/4).
v4 -> v5:
Removed reads from UART_FCR, using up->fcr to determine FIFO enable status.
Removed return value from serial8250_discard_data().
v5 -> v6:
Based on latest linux-next code: Resolved redundant dont_test_tx_en code.
Updated comments and git commit descriptions.
v6 -> v7:
Reverted PSLVERR-related changes in serial8250_get_poll_char().
v7 -> v8:
Added Cc: stable@vger.kernel.org to patch[1] and patch[4].
v8 -> v9:
Reordered the patches: bugfixes as 1-2, improvements as 3-4.

Yunhui Cui (4):
  serial: 8250: fix panic due to PSLVERR
  serial: 8250_dw: fix PSLVERR on RX_TIMEOUT
  serial: 8250: avoid potential PSLVERR issue
  serial: 8250_dw: assert port->lock is held in dw8250_force_idle()

 drivers/tty/serial/8250/8250.h      | 13 +++++++++++++
 drivers/tty/serial/8250/8250_dw.c   | 14 +++++++++++++-
 drivers/tty/serial/8250/8250_port.c | 29 +++++++++++++++++------------
 3 files changed, 43 insertions(+), 13 deletions(-)

-- 
2.39.5

[PATCH v9 1/4] 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 serial_port_in(port, UART_RX).
This satisfies the PSLVERR trigger condition.

When 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) in dw8250_check_lcr(), causing it to enter
dw8250_force_idle().

Put serial_port_out(port, UART_LCR, UART_LCR_WLEN8) under the port->lock
to fix this issue.

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>
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/8250/8250_port.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 6d7b8c4667c9c..07fe818dffa34 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2376,9 +2376,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;
-- 
2.39.5

[PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX_TIMEOUT

[Yunhui Cui]

The DW UART may trigger the RX_TIMEOUT interrupt without data
present and remain stuck in this state indefinitely. The
dw8250_handle_irq() function detects this condition by checking
if the UART_LSR_DR bit is not set when RX_TIMEOUT occurs. When
detected, it performs a "dummy read" to recover the DW UART from
this state.

When the PSLVERR_RESP_EN parameter is set to 1, reading the UART_RX
while the FIFO is enabled and UART_LSR_DR is not set will generate a
PSLVERR error, which may lead to a system panic. There are two methods
to prevent PSLVERR: one is to check if UART_LSR_DR is set before reading
UART_RX when the FIFO is enabled, and the other is to read UART_RX when
the FIFO is disabled.

Given these two scenarios, the FIFO must be disabled before the
"dummy read" operation and re-enabled afterward to maintain normal
UART functionality.

Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/8250/8250_dw.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1902f29444a1c..082b7fcf251db 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -297,9 +297,17 @@ static int dw8250_handle_irq(struct uart_port *p)
 		uart_port_lock_irqsave(p, &flags);
 		status = serial_lsr_in(up);
 
-		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
+		if (!(status & (UART_LSR_DR | UART_LSR_BI))) {
+			/* To avoid PSLVERR, disable the FIFO first. */
+			if (up->fcr & UART_FCR_ENABLE_FIFO)
+				serial_out(up, UART_FCR, 0);
+
 			serial_port_in(p, UART_RX);
 
+			if (up->fcr & UART_FCR_ENABLE_FIFO)
+				serial_out(up, UART_FCR, up->fcr);
+		}
+
 		uart_port_unlock_irqrestore(p, flags);
 	}
 
-- 
2.39.5

[PATCH v9 3/4] serial: 8250: avoid potential PSLVERR issue

[Yunhui Cui]

When the PSLVERR_RESP_EN parameter is set to 1, reading UART_RX while
the FIFO is enabled and UART_LSR_DR is not set will generate a PSLVERR
error.

Failure to check the UART_LSR_DR before reading UART_RX, or the non-
atomic nature of clearing the FIFO and reading UART_RX, poses
potential risks that could lead to PSLVERR.

PSLVERR is addressed through two methods. One is to introduce
serial8250_discard_data() to check whether UART_LSR_DR is set before
reading UART_RX, thus solving the PSLVERR issue when the FIFO is
enabled. The other is to place FIFO clearing and reading of UART_RX
under port->lock.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 drivers/tty/serial/8250/8250.h      | 13 +++++++++++++
 drivers/tty/serial/8250/8250_port.c | 26 +++++++++++++++-----------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 18530c31a5981..b3fb8a550db35 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -162,6 +162,19 @@ static inline u16 serial_lsr_in(struct uart_8250_port *up)
 	return lsr;
 }
 
+/*
+ * To avoid PSLVERR, check UART_LSR_DR in UART_LSR before
+ * reading UART_RX.
+ */
+static inline void serial8250_discard_data(struct uart_8250_port *up)
+{
+	u16 lsr;
+
+	lsr = serial_in(up, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		serial_in(up, UART_RX);
+}
+
 /*
  * For the 16C950
  */
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 07fe818dffa34..0560df9b064f9 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -764,8 +764,6 @@ static void disable_rsa(struct uart_8250_port *up)
 
 	if (up->port.type == PORT_RSA &&
 	    up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16) {
-		uart_port_lock_irq(&up->port);
-
 		mode = serial_in(up, UART_RSA_MSR);
 		result = !(mode & UART_RSA_MSR_FIFO);
 
@@ -777,7 +775,6 @@ static void disable_rsa(struct uart_8250_port *up)
 
 		if (result)
 			up->port.uartclk = SERIAL_RSA_BAUD_BASE_LO * 16;
-		uart_port_unlock_irq(&up->port);
 	}
 }
 #endif /* CONFIG_SERIAL_8250_RSA */
@@ -1353,9 +1350,8 @@ 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);
+	serial8250_discard_data(up);
 	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);
@@ -2260,13 +2256,20 @@ 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.
+	 * Read UART_RX to clear interrupts (e.g., Character Timeout).
+	 * To prevent PSLVERR, we can either disable the FIFO before reading
+	 * UART_RX or read UART_RX only when UART_LSR_DR is set while the FIFO
+	 * remains enabled. If using the latter approach to avoid PSLVERR, it
+	 * creates a contradiction with the interrupt-clearing (see the
+	 * rx_timeout handling in dw8250_handle_irq()).
 	 */
 	serial_port_in(port, UART_LSR);
 	serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 
@@ -2423,15 +2426,13 @@ int serial8250_do_startup(struct uart_port *port)
 		}
 	}
 
-	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);
+	serial8250_discard_data(up);
+	uart_port_unlock_irqrestore(port, flags);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 	up->lsr_saved_flags = 0;
@@ -2513,7 +2514,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
@@ -2532,8 +2532,12 @@ void serial8250_do_shutdown(struct uart_port *port)
 	/*
 	 * Read data port to reset things, and then unlink from
 	 * the IRQ chain.
+	 *
+	 * Since reading UART_RX clears interrupts, doing so with
+	 * FIFO disabled won't trigger PSLVERR.
 	 */
 	serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 	serial8250_rpm_put(up);
 
 	up->ops->release_irq(up);
-- 
2.39.5

[PATCH v9 4/4] serial: 8250_dw: assert port->lock is held in dw8250_force_idle()

[Yunhui Cui]

Reading UART_RX and checking whether UART_LSR_DR is set should be
atomic. Ensure the caller of dw8250_force_idle() holds port->lock.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 drivers/tty/serial/8250/8250_dw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 082b7fcf251db..686f9117a3339 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>
@@ -117,6 +118,9 @@ static void dw8250_force_idle(struct uart_port *p)
 	struct uart_8250_port *up = up_to_u8250p(p);
 	unsigned int lsr;
 
+	/* Reading UART_LSR and UART_RX should be atomic. */
+	lockdep_assert_held_once(&p->lock);
+
 	/*
 	 * The following call currently performs serial_out()
 	 * against the FCR register. Because it differs to LCR
-- 
2.39.5

Re: [PATCH v9 1/4] serial: 8250: fix panic due to PSLVERR

[John Ogness]

On 2025-06-10, Yunhui Cui <cuiyunhui@bytedance.com> 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 serial_port_in(port, UART_RX).
> This satisfies the PSLVERR trigger condition.
>
> When 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) in dw8250_check_lcr(), causing it to enter
> dw8250_force_idle().
>
> Put serial_port_out(port, UART_LCR, UART_LCR_WLEN8) under the port->lock
> to fix this issue.
>
> 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>
> Cc: stable@vger.kernel.org

Reviewed-by: John Ogness <john.ogness@linutronix.de>

Re: [PATCH v9 3/4] serial: 8250: avoid potential PSLVERR issue

[John Ogness]

On 2025-06-10, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> When the PSLVERR_RESP_EN parameter is set to 1, reading UART_RX while
> the FIFO is enabled and UART_LSR_DR is not set will generate a PSLVERR
> error.
>
> Failure to check the UART_LSR_DR before reading UART_RX, or the non-
> atomic nature of clearing the FIFO and reading UART_RX, poses
> potential risks that could lead to PSLVERR.
>
> PSLVERR is addressed through two methods. One is to introduce
> serial8250_discard_data() to check whether UART_LSR_DR is set before
> reading UART_RX, thus solving the PSLVERR issue when the FIFO is
> enabled. The other is to place FIFO clearing and reading of UART_RX
> under port->lock.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>

Reviewed-by: John Ogness <john.ogness@linutronix.de>

Re: [PATCH v9 4/4] serial: 8250_dw: assert port->lock is held in dw8250_force_idle()

[John Ogness]

On 2025-06-10, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> Reading UART_RX and checking whether UART_LSR_DR is set should be
> atomic. Ensure the caller of dw8250_force_idle() holds port->lock.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 082b7fcf251db..686f9117a3339 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>
> @@ -117,6 +118,9 @@ static void dw8250_force_idle(struct uart_port *p)
>  	struct uart_8250_port *up = up_to_u8250p(p);
>  	unsigned int lsr;
>  
> +	/* Reading UART_LSR and UART_RX should be atomic. */
> +	lockdep_assert_held_once(&p->lock);
> +

It may be possible that during panic the port lock might not be held for
console printing:

serial8250_console_write()
  oops_in_progress and failed trylock
  serial8250_console_restore()
    serial_port_out(..., UART_LCR, ...)
      dw8250_serial_out*()
        dw8250_check_lcr()
          dw8250_force_idle()

A similar incident was discussed before [0]. In that case the result was
that the lockdep assertion was removed.

John Ogness

[0] https://lore.kernel.org/r/20230811064340.13400-1-jirislaby@kernel.org

Re: [External] Re: [PATCH v9 4/4] serial: 8250_dw: assert port->lock is held in dw8250_force_idle()

[yunhui cui]

Hi John,

On Fri, Jun 20, 2025 at 8:14 PM John Ogness <john.ogness@linutronix.de> wrote:
>
> On 2025-06-10, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > Reading UART_RX and checking whether UART_LSR_DR is set should be
> > atomic. Ensure the caller of dw8250_force_idle() holds port->lock.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >  drivers/tty/serial/8250/8250_dw.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index 082b7fcf251db..686f9117a3339 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>
> > @@ -117,6 +118,9 @@ static void dw8250_force_idle(struct uart_port *p)
> >       struct uart_8250_port *up = up_to_u8250p(p);
> >       unsigned int lsr;
> >
> > +     /* Reading UART_LSR and UART_RX should be atomic. */
> > +     lockdep_assert_held_once(&p->lock);
> > +
>
> It may be possible that during panic the port lock might not be held for
> console printing:
>
> serial8250_console_write()
>   oops_in_progress and failed trylock
>   serial8250_console_restore()
>     serial_port_out(..., UART_LCR, ...)
>       dw8250_serial_out*()
>         dw8250_check_lcr()
>           dw8250_force_idle()
>
> A similar incident was discussed before [0]. In that case the result was
> that the lockdep assertion was removed.
>
> John Ogness
>
> [0] https://lore.kernel.org/r/20230811064340.13400-1-jirislaby@kernel.org


If so, we can drop this patch.

Thanks,
Yunhui

Re: [PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX_TIMEOUT

[yunhui cui]

Hi John,


On Tue, Jun 10, 2025 at 5:22 PM Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>
> The DW UART may trigger the RX_TIMEOUT interrupt without data
> present and remain stuck in this state indefinitely. The
> dw8250_handle_irq() function detects this condition by checking
> if the UART_LSR_DR bit is not set when RX_TIMEOUT occurs. When
> detected, it performs a "dummy read" to recover the DW UART from
> this state.
>
> When the PSLVERR_RESP_EN parameter is set to 1, reading the UART_RX
> while the FIFO is enabled and UART_LSR_DR is not set will generate a
> PSLVERR error, which may lead to a system panic. There are two methods
> to prevent PSLVERR: one is to check if UART_LSR_DR is set before reading
> UART_RX when the FIFO is enabled, and the other is to read UART_RX when
> the FIFO is disabled.
>
> Given these two scenarios, the FIFO must be disabled before the
> "dummy read" operation and re-enabled afterward to maintain normal
> UART functionality.
>
> Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/tty/serial/8250/8250_dw.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 1902f29444a1c..082b7fcf251db 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -297,9 +297,17 @@ static int dw8250_handle_irq(struct uart_port *p)
>                 uart_port_lock_irqsave(p, &flags);
>                 status = serial_lsr_in(up);
>
> -               if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> +               if (!(status & (UART_LSR_DR | UART_LSR_BI))) {
> +                       /* To avoid PSLVERR, disable the FIFO first. */
> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
> +                               serial_out(up, UART_FCR, 0);
> +
>                         serial_port_in(p, UART_RX);
>
> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
> +                               serial_out(up, UART_FCR, up->fcr);
> +               }
> +
>                 uart_port_unlock_irqrestore(p, flags);
>         }
>
> --
> 2.39.5
>

Any comments on this patch?

Thanks,
Yunhui

Re: [PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX_TIMEOUT

[John Ogness]

Hi Yunhui,

On 2025-06-23, yunhui cui <cuiyunhui@bytedance.com> wrote:
>> The DW UART may trigger the RX_TIMEOUT interrupt without data
>> present and remain stuck in this state indefinitely. The
>> dw8250_handle_irq() function detects this condition by checking
>> if the UART_LSR_DR bit is not set when RX_TIMEOUT occurs. When
>> detected, it performs a "dummy read" to recover the DW UART from
>> this state.
>>
>> When the PSLVERR_RESP_EN parameter is set to 1, reading the UART_RX
>> while the FIFO is enabled and UART_LSR_DR is not set will generate a
>> PSLVERR error, which may lead to a system panic. There are two methods
>> to prevent PSLVERR: one is to check if UART_LSR_DR is set before reading
>> UART_RX when the FIFO is enabled, and the other is to read UART_RX when
>> the FIFO is disabled.
>>
>> Given these two scenarios, the FIFO must be disabled before the
>> "dummy read" operation and re-enabled afterward to maintain normal
>> UART functionality.
>>
>> Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/tty/serial/8250/8250_dw.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index 1902f29444a1c..082b7fcf251db 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -297,9 +297,17 @@ static int dw8250_handle_irq(struct uart_port *p)
>>                 uart_port_lock_irqsave(p, &flags);
>>                 status = serial_lsr_in(up);
>>
>> -               if (!(status & (UART_LSR_DR | UART_LSR_BI)))
>> +               if (!(status & (UART_LSR_DR | UART_LSR_BI))) {
>> +                       /* To avoid PSLVERR, disable the FIFO first. */
>> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
>> +                               serial_out(up, UART_FCR, 0);
>> +
>>                         serial_port_in(p, UART_RX);
>>
>> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
>> +                               serial_out(up, UART_FCR, up->fcr);
>> +               }
>> +
>>                 uart_port_unlock_irqrestore(p, flags);
>>         }
>>
>> --
>> 2.39.5
>
> Any comments on this patch?

I do not know enough about the hardware. Is a dummy read really the only
way to exit the RX_TIMEOUT state?

What if there are bytes in the TX-FIFO. Are they in danger of being
cleared?

From [0] I see:

"Writing a "0" to bit 0 will disable the FIFOs, in essence turning the
 UART into 8250 compatibility mode. In effect this also renders the rest
 of the settings in this register to become useless. If you write a "0"
 here it will also stop the FIFOs from sending or receiving data, so any
 data that is sent through the serial data port may be scrambled after
 this setting has been changed. It would be recommended to disable FIFOs
 only if you are trying to reset the serial communication protocol and
 clearing any working buffers you may have in your application
 software. Some documentation suggests that setting this bit to "0" also
 clears the FIFO buffers, but I would recommend explicit buffer clearing
 instead using bits 1 and 2."

Have you performed tests where you fill the TX-FIFO and then
disable/enable the FIFO to see if the TX-bytes survive?

John Ogness

[0] https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming

Re: [External] Re: [PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX_TIMEOUT

[yunhui cui]

Hi John,

On Mon, Jun 23, 2025 at 4:32 PM John Ogness <john.ogness@linutronix.de> wrote:
>
> Hi Yunhui,
>
> On 2025-06-23, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >> The DW UART may trigger the RX_TIMEOUT interrupt without data
> >> present and remain stuck in this state indefinitely. The
> >> dw8250_handle_irq() function detects this condition by checking
> >> if the UART_LSR_DR bit is not set when RX_TIMEOUT occurs. When
> >> detected, it performs a "dummy read" to recover the DW UART from
> >> this state.
> >>
> >> When the PSLVERR_RESP_EN parameter is set to 1, reading the UART_RX
> >> while the FIFO is enabled and UART_LSR_DR is not set will generate a
> >> PSLVERR error, which may lead to a system panic. There are two methods
> >> to prevent PSLVERR: one is to check if UART_LSR_DR is set before reading
> >> UART_RX when the FIFO is enabled, and the other is to read UART_RX when
> >> the FIFO is disabled.
> >>
> >> Given these two scenarios, the FIFO must be disabled before the
> >> "dummy read" operation and re-enabled afterward to maintain normal
> >> UART functionality.
> >>
> >> Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
> >> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>  drivers/tty/serial/8250/8250_dw.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> >> index 1902f29444a1c..082b7fcf251db 100644
> >> --- a/drivers/tty/serial/8250/8250_dw.c
> >> +++ b/drivers/tty/serial/8250/8250_dw.c
> >> @@ -297,9 +297,17 @@ static int dw8250_handle_irq(struct uart_port *p)
> >>                 uart_port_lock_irqsave(p, &flags);
> >>                 status = serial_lsr_in(up);
> >>
> >> -               if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> >> +               if (!(status & (UART_LSR_DR | UART_LSR_BI))) {
> >> +                       /* To avoid PSLVERR, disable the FIFO first. */
> >> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
> >> +                               serial_out(up, UART_FCR, 0);
> >> +
> >>                         serial_port_in(p, UART_RX);
> >>
> >> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
> >> +                               serial_out(up, UART_FCR, up->fcr);
> >> +               }
> >> +
> >>                 uart_port_unlock_irqrestore(p, flags);
> >>         }
> >>
> >> --
> >> 2.39.5
> >
> > Any comments on this patch?
>
> I do not know enough about the hardware. Is a dummy read really the only
> way to exit the RX_TIMEOUT state?
>
> What if there are bytes in the TX-FIFO. Are they in danger of being
> cleared?
>
> From [0] I see:
>
> "Writing a "0" to bit 0 will disable the FIFOs, in essence turning the
>  UART into 8250 compatibility mode. In effect this also renders the rest
>  of the settings in this register to become useless. If you write a "0"
>  here it will also stop the FIFOs from sending or receiving data, so any
>  data that is sent through the serial data port may be scrambled after
>  this setting has been changed. It would be recommended to disable FIFOs
>  only if you are trying to reset the serial communication protocol and
>  clearing any working buffers you may have in your application
>  software. Some documentation suggests that setting this bit to "0" also
>  clears the FIFO buffers, but I would recommend explicit buffer clearing
>  instead using bits 1 and 2."
>
> Have you performed tests where you fill the TX-FIFO and then
> disable/enable the FIFO to see if the TX-bytes survive?

Sorry, I haven't conducted relevant tests. The reason I made this
modification is that it clearly contradicts the logic of avoiding
PSLVERR. Disabling the FIFO can at least prevent the Panic() caused by
PSVERR.

>
> John Ogness
>
> [0] https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming

Thanks,
Yunhui

Re: [External] Re: [PATCH v9 1/4] serial: 8250: fix panic due to PSLVERR

[yunhui cui]

Hi John,

On Fri, Jun 20, 2025 at 7:20 PM John Ogness <john.ogness@linutronix.de> wrote:
>
> On 2025-06-10, Yunhui Cui <cuiyunhui@bytedance.com> 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 serial_port_in(port, UART_RX).
> > This satisfies the PSLVERR trigger condition.
> >
> > When 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) in dw8250_check_lcr(), causing it to enter
> > dw8250_force_idle().
> >
> > Put serial_port_out(port, UART_LCR, UART_LCR_WLEN8) under the port->lock
> > to fix this issue.
> >
> > 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>
> > Cc: stable@vger.kernel.org
>
> Reviewed-by: John Ogness <john.ogness@linutronix.de>

In this patchset, patch[4] has been dropped, and patch[2] may still
need discussion. Could you please pick patch[1] and patch[3] first?

Thanks,
Yunhui

Re: [External] Re: [PATCH v9 1/4] serial: 8250: fix panic due to PSLVERR

[Greg KH]

On Thu, Jul 17, 2025 at 08:19:48PM +0800, yunhui cui wrote:
> Hi John,
> 
> On Fri, Jun 20, 2025 at 7:20 PM John Ogness <john.ogness@linutronix.de> wrote:
> >
> > On 2025-06-10, Yunhui Cui <cuiyunhui@bytedance.com> 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 serial_port_in(port, UART_RX).
> > > This satisfies the PSLVERR trigger condition.
> > >
> > > When 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) in dw8250_check_lcr(), causing it to enter
> > > dw8250_force_idle().
> > >
> > > Put serial_port_out(port, UART_LCR, UART_LCR_WLEN8) under the port->lock
> > > to fix this issue.
> > >
> > > 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>
> > > Cc: stable@vger.kernel.org
> >
> > Reviewed-by: John Ogness <john.ogness@linutronix.de>
> 
> In this patchset, patch[4] has been dropped, and patch[2] may still
> need discussion. Could you please pick patch[1] and patch[3] first?

Please resend just the patches you want applied, picking out of a series
is hard with our current tools, and confusing.

thanks,

greg k-h

Re: [External] Re: [PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX_TIMEOUT

[John Ogness]

Added Douglas Anderson, author of commit 424d79183af0 ("serial: 8250_dw:
Avoid "too much work" from bogus rx timeout interrupt").

On 2025-07-11, yunhui cui <cuiyunhui@bytedance.com> wrote:
>> On 2025-06-23, yunhui cui <cuiyunhui@bytedance.com> wrote:
>> >> The DW UART may trigger the RX_TIMEOUT interrupt without data
>> >> present and remain stuck in this state indefinitely. The
>> >> dw8250_handle_irq() function detects this condition by checking
>> >> if the UART_LSR_DR bit is not set when RX_TIMEOUT occurs. When
>> >> detected, it performs a "dummy read" to recover the DW UART from
>> >> this state.
>> >>
>> >> When the PSLVERR_RESP_EN parameter is set to 1, reading the UART_RX
>> >> while the FIFO is enabled and UART_LSR_DR is not set will generate a
>> >> PSLVERR error, which may lead to a system panic. There are two methods
>> >> to prevent PSLVERR: one is to check if UART_LSR_DR is set before reading
>> >> UART_RX when the FIFO is enabled, and the other is to read UART_RX when
>> >> the FIFO is disabled.
>> >>
>> >> Given these two scenarios, the FIFO must be disabled before the
>> >> "dummy read" operation and re-enabled afterward to maintain normal
>> >> UART functionality.
>> >>
>> >> Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
>> >> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>> >> Cc: stable@vger.kernel.org
>> >> ---
>> >>  drivers/tty/serial/8250/8250_dw.c | 10 +++++++++-
>> >>  1 file changed, 9 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> >> index 1902f29444a1c..082b7fcf251db 100644
>> >> --- a/drivers/tty/serial/8250/8250_dw.c
>> >> +++ b/drivers/tty/serial/8250/8250_dw.c
>> >> @@ -297,9 +297,17 @@ static int dw8250_handle_irq(struct uart_port *p)
>> >>                 uart_port_lock_irqsave(p, &flags);
>> >>                 status = serial_lsr_in(up);
>> >>
>> >> -               if (!(status & (UART_LSR_DR | UART_LSR_BI)))
>> >> +               if (!(status & (UART_LSR_DR | UART_LSR_BI))) {
>> >> +                       /* To avoid PSLVERR, disable the FIFO first. */
>> >> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
>> >> +                               serial_out(up, UART_FCR, 0);
>> >> +
>> >>                         serial_port_in(p, UART_RX);
>> >>
>> >> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
>> >> +                               serial_out(up, UART_FCR, up->fcr);
>> >> +               }
>> >> +
>> >>                 uart_port_unlock_irqrestore(p, flags);
>> >>         }
>>
>> I do not know enough about the hardware. Is a dummy read really the only
>> way to exit the RX_TIMEOUT state?
>>
>> What if there are bytes in the TX-FIFO. Are they in danger of being
>> cleared?
>>
>> From [0] I see:
>>
>> "Writing a "0" to bit 0 will disable the FIFOs, in essence turning the
>>  UART into 8250 compatibility mode. In effect this also renders the rest
>>  of the settings in this register to become useless. If you write a "0"
>>  here it will also stop the FIFOs from sending or receiving data, so any
>>  data that is sent through the serial data port may be scrambled after
>>  this setting has been changed. It would be recommended to disable FIFOs
>>  only if you are trying to reset the serial communication protocol and
>>  clearing any working buffers you may have in your application
>>  software. Some documentation suggests that setting this bit to "0" also
>>  clears the FIFO buffers, but I would recommend explicit buffer clearing
>>  instead using bits 1 and 2."
>>
>> Have you performed tests where you fill the TX-FIFO and then
>> disable/enable the FIFO to see if the TX-bytes survive?
>
> Sorry, I haven't conducted relevant tests. The reason I made this
> modification is that it clearly contradicts the logic of avoiding
> PSLVERR. Disabling the FIFO can at least prevent the Panic() caused by
> PSVERR.

I am just wondering if there is some other way to avoid this. But since
we are talking about a hardware quirk and it is only related to
suspend/resume, maybe it is acceptable to risk data corruption in this
case. (?)

I am hoping Douglas can chime in.

John Ogness

>> [0] https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming

Re: [External] Re: [PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX_TIMEOUT

[Doug Anderson]

Hi,

On Thu, Jul 17, 2025 at 7:14 AM John Ogness <john.ogness@linutronix.de> wrote:
>
> Added Douglas Anderson, author of commit 424d79183af0 ("serial: 8250_dw:
> Avoid "too much work" from bogus rx timeout interrupt").
>
> On 2025-07-11, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >> On 2025-06-23, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >> >> The DW UART may trigger the RX_TIMEOUT interrupt without data
> >> >> present and remain stuck in this state indefinitely. The
> >> >> dw8250_handle_irq() function detects this condition by checking
> >> >> if the UART_LSR_DR bit is not set when RX_TIMEOUT occurs. When
> >> >> detected, it performs a "dummy read" to recover the DW UART from
> >> >> this state.
> >> >>
> >> >> When the PSLVERR_RESP_EN parameter is set to 1, reading the UART_RX
> >> >> while the FIFO is enabled and UART_LSR_DR is not set will generate a
> >> >> PSLVERR error, which may lead to a system panic. There are two methods
> >> >> to prevent PSLVERR: one is to check if UART_LSR_DR is set before reading
> >> >> UART_RX when the FIFO is enabled, and the other is to read UART_RX when
> >> >> the FIFO is disabled.
> >> >>
> >> >> Given these two scenarios, the FIFO must be disabled before the
> >> >> "dummy read" operation and re-enabled afterward to maintain normal
> >> >> UART functionality.
> >> >>
> >> >> Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
> >> >> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >> >> Cc: stable@vger.kernel.org
> >> >> ---
> >> >>  drivers/tty/serial/8250/8250_dw.c | 10 +++++++++-
> >> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> >> >> index 1902f29444a1c..082b7fcf251db 100644
> >> >> --- a/drivers/tty/serial/8250/8250_dw.c
> >> >> +++ b/drivers/tty/serial/8250/8250_dw.c
> >> >> @@ -297,9 +297,17 @@ static int dw8250_handle_irq(struct uart_port *p)
> >> >>                 uart_port_lock_irqsave(p, &flags);
> >> >>                 status = serial_lsr_in(up);
> >> >>
> >> >> -               if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> >> >> +               if (!(status & (UART_LSR_DR | UART_LSR_BI))) {
> >> >> +                       /* To avoid PSLVERR, disable the FIFO first. */
> >> >> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
> >> >> +                               serial_out(up, UART_FCR, 0);
> >> >> +
> >> >>                         serial_port_in(p, UART_RX);
> >> >>
> >> >> +                       if (up->fcr & UART_FCR_ENABLE_FIFO)
> >> >> +                               serial_out(up, UART_FCR, up->fcr);
> >> >> +               }
> >> >> +
> >> >>                 uart_port_unlock_irqrestore(p, flags);
> >> >>         }
> >>
> >> I do not know enough about the hardware. Is a dummy read really the only
> >> way to exit the RX_TIMEOUT state?
> >>
> >> What if there are bytes in the TX-FIFO. Are they in danger of being
> >> cleared?
> >>
> >> From [0] I see:
> >>
> >> "Writing a "0" to bit 0 will disable the FIFOs, in essence turning the
> >>  UART into 8250 compatibility mode. In effect this also renders the rest
> >>  of the settings in this register to become useless. If you write a "0"
> >>  here it will also stop the FIFOs from sending or receiving data, so any
> >>  data that is sent through the serial data port may be scrambled after
> >>  this setting has been changed. It would be recommended to disable FIFOs
> >>  only if you are trying to reset the serial communication protocol and
> >>  clearing any working buffers you may have in your application
> >>  software. Some documentation suggests that setting this bit to "0" also
> >>  clears the FIFO buffers, but I would recommend explicit buffer clearing
> >>  instead using bits 1 and 2."
> >>
> >> Have you performed tests where you fill the TX-FIFO and then
> >> disable/enable the FIFO to see if the TX-bytes survive?
> >
> > Sorry, I haven't conducted relevant tests. The reason I made this
> > modification is that it clearly contradicts the logic of avoiding
> > PSLVERR. Disabling the FIFO can at least prevent the Panic() caused by
> > PSVERR.
>
> I am just wondering if there is some other way to avoid this. But since
> we are talking about a hardware quirk and it is only related to
> suspend/resume, maybe it is acceptable to risk data corruption in this
> case. (?)
>
> I am hoping Douglas can chime in.
>
> John Ogness
>
> >> [0] https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming

I'm not sure I have too much to add here. :( I did the investigation
and wrote the original patch over 8 years ago and I no longer have
access to the hardware where the problem first reproduced. I vaguely
remember the problem, but only because I re-read the commit message I
wrote 8 years ago. :-P

I will say that for the hardware I was working with, it wouldn't have
been the end of the world if there was a tiny bit of UART corruption
around suspend / resume. Of course, nothing about the workaround
specifically checks for suspend/resume, that was just how we were
reproducing problems. If there is ever any other way to get a "RX
timeout with no data" then we'd also potentially cause corruption with
the new patch. Still better than an interrupt storm or a panic,
though...

Not to say that I'm NAKing anything (since I'm a bit of a bystander in
this case), but I wonder if there's anything you could do better?
Ideas, maybe?

1. Could the PSLVERR be ignored in this case?

2. Could we temporarily disable generation of the PSLVERR for this read?

3. Could we detect when PSLVERR_RESP_EN=1 and only do the FIFO
disable/enable dance in that case?

-Doug