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

[PATCH v5 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 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().

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>
---
 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 3f256e96c722..a913135d5217 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2380,9 +2380,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.2

[PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

[Yunhui Cui]

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.

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

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b861585ca02a..6f97ff3a197d 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 a913135d5217..1666b965f6a0 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1357,9 +1357,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);
@@ -2137,25 +2136,22 @@ 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;
+	unsigned long flags;
 
 	serial8250_rpm_get(up);
 
+	uart_port_lock_irqsave(port, &flags);
 	lsr = serial_port_in(port, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		status = serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 
-	if (!(lsr & UART_LSR_DR)) {
-		status = NO_POLL_CHAR;
-		goto out;
-	}
-
-	status = serial_port_in(port, UART_RX);
-out:
 	serial8250_rpm_put(up);
 	return status;
 }
 
-
 static void serial8250_put_poll_char(struct uart_port *port,
 			 unsigned char c)
 {
@@ -2264,13 +2260,17 @@ 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).
+	 * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
+	 * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
 	 */
 	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);
 
@@ -2429,15 +2429,14 @@ 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);
+	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;
@@ -2519,7 +2518,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
@@ -2527,6 +2525,14 @@ 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);
+	/*
+	 * 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);
 
 #ifdef CONFIG_SERIAL_8250_RSA
 	/*
@@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
 	disable_rsa(up);
 #endif
 
-	/*
-	 * Read data port to reset things, and then unlink from
-	 * the IRQ chain.
-	 */
-	serial_port_in(port, UART_RX);
 	serial8250_rpm_put(up);
 
 	up->ops->release_irq(up);
-- 
2.39.2

[PATCH v5 3/4] serial: 8250_dw: warning on entering dw8250_force_idle unlocked

[Yunhui Cui]

Read UART_RX and check UART_LSR_DR in critical section. Unsure if
caller of dw8250_force_idle() holds port->lock. Don't acquire it
directly to avoid deadlock. Use lockdep_assert_held_once for warning.

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

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index af24ec25d976..f41c4a9ed58b 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;
 
+	/*
+	 * The 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);
 
 	/*
-- 
2.39.2

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

[Yunhui Cui]

In the case of RX_TIMEOUT, to avoid PSLVERR, disable the FIFO
before reading UART_RX when UART_LSR_DR is not set.

Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 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 f41c4a9ed58b..ffa8cb10b39c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -288,9 +288,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);
+
 			(void) p->serial_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.2

Re: [PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

[Ilpo Järvinen]

On Tue, 6 May 2025, Yunhui Cui wrote:

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

Don't expect reader to know the condition how PSLVERR is triggered. I know 
it's worded out in the other patch but also explain it here.

You're only explaining problem and missing what this patch does to solve 
the problem.

> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  drivers/tty/serial/8250/8250.h      | 13 +++++++++
>  drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
>  2 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index b861585ca02a..6f97ff3a197d 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 a913135d5217..1666b965f6a0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1357,9 +1357,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);
> @@ -2137,25 +2136,22 @@ 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;
> +	unsigned long flags;
>  
>  	serial8250_rpm_get(up);
>  
> +	uart_port_lock_irqsave(port, &flags);
>  	lsr = serial_port_in(port, UART_LSR);
> +	if (lsr & UART_LSR_DR)
> +		status = serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);
>  
> -	if (!(lsr & UART_LSR_DR)) {
> -		status = NO_POLL_CHAR;
> -		goto out;
> -	}
> -
> -	status = serial_port_in(port, UART_RX);
> -out:
>  	serial8250_rpm_put(up);
>  	return status;

Not a problem that originates from you, but IMO calling this variable 
"status" is quite misleading when it is the character (or NO_POLL_CHAR 
is no character is present).

>  }
>  
> -
>  static void serial8250_put_poll_char(struct uart_port *port,
>  			 unsigned char c)
>  {
> @@ -2264,13 +2260,17 @@ 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).
> +	 * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> +	 * FIFO disabled, read UART_RX without LSR check, no PSLVERR.

I don't understand what the last two lines mean and I don't see the 
connection to the code that is below the comment either, could you try to 
rephrase the comment.

>  	 */
>  	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);
>  
> @@ -2429,15 +2429,14 @@ 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);
> +	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;
> @@ -2519,7 +2518,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
> @@ -2527,6 +2525,14 @@ 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);
> +	/*
> +	 * 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);
>  
>  #ifdef CONFIG_SERIAL_8250_RSA
>  	/*
> @@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
>  	disable_rsa(up);
>  #endif
>  
> -	/*
> -	 * Read data port to reset things, and then unlink from
> -	 * the IRQ chain.
> -	 */
> -	serial_port_in(port, UART_RX);
>  	serial8250_rpm_put(up);
>  
>  	up->ops->release_irq(up);
> 

-- 
 i.

Re: [PATCH v5 3/4] serial: 8250_dw: warning on entering dw8250_force_idle unlocked

[Ilpo Järvinen]

On Tue, 6 May 2025, Yunhui Cui wrote:

> Read UART_RX and check UART_LSR_DR in critical section. Unsure if

Unsure if -> Ensure the

> caller of dw8250_force_idle() holds port->lock. Don't acquire it
> directly to avoid deadlock. Use lockdep_assert_held_once for warning.

Add (), although the last two sentences don't seem that useful, IMO.

> 
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index af24ec25d976..f41c4a9ed58b 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;
>  
> +	/*
> +	 * The 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.

I'm left to wonder who/what "we" is here? Could you change it something 
more precise.

> +	 */
> +	lockdep_assert_held_once(&p->lock);
> +
>  	serial8250_clear_and_reinit_fifos(up);
>  
>  	/*
> 

-- 
 i.

Re: [PATCH v5 3/4] serial: 8250_dw: warning on entering dw8250_force_idle unlocked

[Ilpo Järvinen]

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

Also, you should also improve the shortlog (in Subject) to something less 
vague, e.g.:

serial: 8250_dw: assert port->lock is held in dw8250_force_idle()

On Tue, 6 May 2025, Ilpo Järvinen wrote:
> On Tue, 6 May 2025, Yunhui Cui wrote:
> 
> > Read UART_RX and check UART_LSR_DR in critical section. Unsure if
> 
> Unsure if -> Ensure the
> 
> > caller of dw8250_force_idle() holds port->lock. Don't acquire it
> > directly to avoid deadlock. Use lockdep_assert_held_once for warning.
> 
> Add (), although the last two sentences don't seem that useful, IMO.
> 
> > 
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >  drivers/tty/serial/8250/8250_dw.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index af24ec25d976..f41c4a9ed58b 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;
> >  
> > +	/*
> > +	 * The 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.
> 
> I'm left to wonder who/what "we" is here? Could you change it something 
> more precise.
> 
> > +	 */
> > +	lockdep_assert_held_once(&p->lock);
> > +
> >  	serial8250_clear_and_reinit_fifos(up);
> >  
> >  	/*
> > 
> 
> 

-- 
 i.

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

[Ilpo Järvinen]

I think shortlog should include mention to "dummy read" to make it a 
bit more specific.

On Tue, 6 May 2025, Yunhui Cui wrote:

> In the case of RX_TIMEOUT, to avoid PSLVERR, disable the FIFO

As with patch 2, please don't assume it is know to the reader how PSLVERR 
is triggered.

> before reading UART_RX when UART_LSR_DR is not set.

IMO, it would be better to explain the problem better first, something 
along these lines:

DW UART can fire RX_TIMEOUT interrupt without data and remain in that 
state forever. dw8250_handle_irq() detects this condition by checking if 
UART_LSR_DR is not asserted when RX_TIMEOUT occurred, and if detected, 
performs a dummy read to kick DW UART out of this state.

Performing dummy read from UART_RX is problematic because with ... it lead 
to ...

And only then explain the solution (disable FIFO for while performing of 
the dummy read).

> 
> Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  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 f41c4a9ed58b..ffa8cb10b39c 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -288,9 +288,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);
> +
>  			(void) p->serial_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.

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

[Ilpo Järvinen]

On Tue, 6 May 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:

Where is 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

Because -> When ? 

missing space before (

> is busy), the current CPU fails the check (value & ~UART_LCR_SPAR) ==
> (lcr & ~UART_LCR_SPAR)

Add:

... 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>
> ---
>  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 3f256e96c722..a913135d5217 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2380,9 +2380,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;
> 

-- 
 i.

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

[yunhui cui]

 Hi Ilpo,

On Tue, May 6, 2025 at 8:25 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> I think shortlog should include mention to "dummy read" to make it a
> bit more specific.
>
> On Tue, 6 May 2025, Yunhui Cui wrote:
>
> > In the case of RX_TIMEOUT, to avoid PSLVERR, disable the FIFO
>
> As with patch 2, please don't assume it is know to the reader how PSLVERR
> is triggered.
>
> > before reading UART_RX when UART_LSR_DR is not set.
>
> IMO, it would be better to explain the problem better first, something
> along these lines:
>
> DW UART can fire RX_TIMEOUT interrupt without data and remain in that
> state forever. dw8250_handle_irq() detects this condition by checking if
> UART_LSR_DR is not asserted when RX_TIMEOUT occurred, and if detected,
> performs a dummy read to kick DW UART out of this state.
>
> Performing dummy read from UART_RX is problematic because with ... it lead
> to ...
>
> And only then explain the solution (disable FIFO for while performing of
> the dummy read).

Okay, thank you. It will be updated in the next version.

>
> >
> > Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >  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 f41c4a9ed58b..ffa8cb10b39c 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -288,9 +288,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);
> > +
> >                       (void) p->serial_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.
>

Thanks,
Yunhui

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

[yunhui cui]

Hi Ilpo,

On Tue, May 6, 2025 at 8:35 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 6 May 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:
>
> Where is dont_test_tx_en label?

Oh, I'll rebase the latest code of linux-next.

>
> > ...
> > serial_port_in(port, UART_RX);
> > This satisfies the PSLVERR trigger condition.
> >
> > Because another CPU(e.g., using printk()) is accessing the UART (UART
>
> Because -> When ?
>
> missing space before (
>
> > is busy), the current CPU fails the check (value & ~UART_LCR_SPAR) ==
> > (lcr & ~UART_LCR_SPAR)
>
> Add:
>
> ... in dw8250_check_lcr()

Okay, it will be updated in the next version.

>
> >, 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>
> > ---
> >  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 3f256e96c722..a913135d5217 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -2380,9 +2380,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;
> >
>
> --
>  i.
>

Thanks,
Yunhui

Re: [External] Re: [PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

[yunhui cui]

Hi Ilpo,

On Tue, May 6, 2025 at 8:00 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 6 May 2025, Yunhui Cui wrote:
>
> > 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.
>
> Don't expect reader to know the condition how PSLVERR is triggered. I know
> it's worded out in the other patch but also explain it here.
>
> You're only explaining problem and missing what this patch does to solve
> the problem.
>
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >  drivers/tty/serial/8250/8250.h      | 13 +++++++++
> >  drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
> >  2 files changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > index b861585ca02a..6f97ff3a197d 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 a913135d5217..1666b965f6a0 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -1357,9 +1357,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);
> > @@ -2137,25 +2136,22 @@ 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;
> > +     unsigned long flags;
> >
> >       serial8250_rpm_get(up);
> >
> > +     uart_port_lock_irqsave(port, &flags);
> >       lsr = serial_port_in(port, UART_LSR);
> > +     if (lsr & UART_LSR_DR)
> > +             status = serial_port_in(port, UART_RX);
> > +     uart_port_unlock_irqrestore(port, flags);
> >
> > -     if (!(lsr & UART_LSR_DR)) {
> > -             status = NO_POLL_CHAR;
> > -             goto out;
> > -     }
> > -
> > -     status = serial_port_in(port, UART_RX);
> > -out:
> >       serial8250_rpm_put(up);
> >       return status;
>
> Not a problem that originates from you, but IMO calling this variable
> "status" is quite misleading when it is the character (or NO_POLL_CHAR
> is no character is present).
>
> >  }
> >
> > -
> >  static void serial8250_put_poll_char(struct uart_port *port,
> >                        unsigned char c)
> >  {
> > @@ -2264,13 +2260,17 @@ 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).
> > +      * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> > +      * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
>
> I don't understand what the last two lines mean and I don't see the
> connection to the code that is below the comment either, could you try to
> rephrase the comment.

The original intention was to check UART_LSR_DR first when reading
UART_RX. However, the purpose of serial_port_in(port, UART_RX) here is
to clear the interrupt, such as the interrupt caused by RX_TIMEOUT.

The logic for clearing the interrupt in the interrupt handling
function of RX_TIMEOUT is !UART_LSR_DR. And to avoid PSLVERR, we need
to check UART_LSR_DR first. To meet the requirements of both, the FIFO
needs to be disabled.

Therefore, we should put serial8250_clear_fifos() and the execution of
serial_port_in(port, UART_RX) without checking UART_LSR_DR under the
port->lock.

>
> >        */
> >       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);
> >
> > @@ -2429,15 +2429,14 @@ 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);
> > +     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;
> > @@ -2519,7 +2518,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
> > @@ -2527,6 +2525,14 @@ 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);
> > +     /*
> > +      * 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);
> >
> >  #ifdef CONFIG_SERIAL_8250_RSA
> >       /*
> > @@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
> >       disable_rsa(up);
> >  #endif
> >
> > -     /*
> > -      * Read data port to reset things, and then unlink from
> > -      * the IRQ chain.
> > -      */
> > -     serial_port_in(port, UART_RX);
> >       serial8250_rpm_put(up);
> >
> >       up->ops->release_irq(up);
> >
>
> --
>  i.
>

Thanks,
Yunhui

Re: [External] Re: [PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

[Ilpo Järvinen]

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

On Mon, 12 May 2025, yunhui cui wrote:

> Hi Ilpo,
> 
> On Tue, May 6, 2025 at 8:00 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 6 May 2025, Yunhui Cui wrote:
> >
> > > 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.
> >
> > Don't expect reader to know the condition how PSLVERR is triggered. I know
> > it's worded out in the other patch but also explain it here.
> >
> > You're only explaining problem and missing what this patch does to solve
> > the problem.
> >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > ---
> > >  drivers/tty/serial/8250/8250.h      | 13 +++++++++
> > >  drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
> > >  2 files changed, 35 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > > index b861585ca02a..6f97ff3a197d 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 a913135d5217..1666b965f6a0 100644
> > > --- a/drivers/tty/serial/8250/8250_port.c
> > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > @@ -1357,9 +1357,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);
> > > @@ -2137,25 +2136,22 @@ 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;
> > > +     unsigned long flags;
> > >
> > >       serial8250_rpm_get(up);
> > >
> > > +     uart_port_lock_irqsave(port, &flags);
> > >       lsr = serial_port_in(port, UART_LSR);
> > > +     if (lsr & UART_LSR_DR)
> > > +             status = serial_port_in(port, UART_RX);
> > > +     uart_port_unlock_irqrestore(port, flags);
> > >
> > > -     if (!(lsr & UART_LSR_DR)) {
> > > -             status = NO_POLL_CHAR;
> > > -             goto out;
> > > -     }
> > > -
> > > -     status = serial_port_in(port, UART_RX);
> > > -out:
> > >       serial8250_rpm_put(up);
> > >       return status;
> >
> > Not a problem that originates from you, but IMO calling this variable
> > "status" is quite misleading when it is the character (or NO_POLL_CHAR
> > is no character is present).
> >
> > >  }
> > >
> > > -
> > >  static void serial8250_put_poll_char(struct uart_port *port,
> > >                        unsigned char c)
> > >  {
> > > @@ -2264,13 +2260,17 @@ 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).
> > > +      * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> > > +      * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
> >
> > I don't understand what the last two lines mean and I don't see the
> > connection to the code that is below the comment either, could you try to
> > rephrase the comment.
> 
> The original intention was to check UART_LSR_DR first when reading
> UART_RX. However, the purpose of serial_port_in(port, UART_RX) here is
> to clear the interrupt, such as the interrupt caused by RX_TIMEOUT.

I understood the first sentence in the comment but the rest of it is very 
cryptic and has many grammar issues too. Also, the extent of passive voice 
there makes it hard to know who does what (UART / kernel).

> The logic for clearing the interrupt in the interrupt handling
> function of RX_TIMEOUT is !UART_LSR_DR. And to avoid PSLVERR, we need
> to check UART_LSR_DR first. To meet the requirements of both, the FIFO
> needs to be disabled.

The grammar is so broken, it failed to convey that message.

> Therefore, we should put serial8250_clear_fifos() and the execution of
> serial_port_in(port, UART_RX) without checking UART_LSR_DR under the
> port->lock.
> 
> >
> > >        */
> > >       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);
> > >
> > > @@ -2429,15 +2429,14 @@ 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);
> > > +     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;
> > > @@ -2519,7 +2518,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
> > > @@ -2527,6 +2525,14 @@ 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);
> > > +     /*
> > > +      * 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);
> > >
> > >  #ifdef CONFIG_SERIAL_8250_RSA
> > >       /*
> > > @@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
> > >       disable_rsa(up);
> > >  #endif
> > >
> > > -     /*
> > > -      * Read data port to reset things, and then unlink from
> > > -      * the IRQ chain.
> > > -      */
> > > -     serial_port_in(port, UART_RX);
> > >       serial8250_rpm_put(up);
> > >
> > >       up->ops->release_irq(up);
> > >
> >
> > --
> >  i.
> >
> 
> Thanks,
> Yunhui
> 

-- 
 i.

Re: [External] Re: [PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

[yunhui cui]

Hi Ilpo,

On Mon, May 12, 2025 at 6:27 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Mon, 12 May 2025, yunhui cui wrote:
>
> > Hi Ilpo,
> >
> > On Tue, May 6, 2025 at 8:00 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Tue, 6 May 2025, Yunhui Cui wrote:
> > >
> > > > 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.
> > >
> > > Don't expect reader to know the condition how PSLVERR is triggered. I know
> > > it's worded out in the other patch but also explain it here.
> > >
> > > You're only explaining problem and missing what this patch does to solve
> > > the problem.
> > >
> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > ---
> > > >  drivers/tty/serial/8250/8250.h      | 13 +++++++++
> > > >  drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
> > > >  2 files changed, 35 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > > > index b861585ca02a..6f97ff3a197d 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 a913135d5217..1666b965f6a0 100644
> > > > --- a/drivers/tty/serial/8250/8250_port.c
> > > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > > @@ -1357,9 +1357,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);
> > > > @@ -2137,25 +2136,22 @@ 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;
> > > > +     unsigned long flags;
> > > >
> > > >       serial8250_rpm_get(up);
> > > >
> > > > +     uart_port_lock_irqsave(port, &flags);
> > > >       lsr = serial_port_in(port, UART_LSR);
> > > > +     if (lsr & UART_LSR_DR)
> > > > +             status = serial_port_in(port, UART_RX);
> > > > +     uart_port_unlock_irqrestore(port, flags);
> > > >
> > > > -     if (!(lsr & UART_LSR_DR)) {
> > > > -             status = NO_POLL_CHAR;
> > > > -             goto out;
> > > > -     }
> > > > -
> > > > -     status = serial_port_in(port, UART_RX);
> > > > -out:
> > > >       serial8250_rpm_put(up);
> > > >       return status;
> > >
> > > Not a problem that originates from you, but IMO calling this variable
> > > "status" is quite misleading when it is the character (or NO_POLL_CHAR
> > > is no character is present).
> > >
> > > >  }
> > > >
> > > > -
> > > >  static void serial8250_put_poll_char(struct uart_port *port,
> > > >                        unsigned char c)
> > > >  {
> > > > @@ -2264,13 +2260,17 @@ 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).
> > > > +      * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> > > > +      * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
> > >
> > > I don't understand what the last two lines mean and I don't see the
> > > connection to the code that is below the comment either, could you try to
> > > rephrase the comment.
> >
> > The original intention was to check UART_LSR_DR first when reading
> > UART_RX. However, the purpose of serial_port_in(port, UART_RX) here is
> > to clear the interrupt, such as the interrupt caused by RX_TIMEOUT.
>
> I understood the first sentence in the comment but the rest of it is very
> cryptic and has many grammar issues too. Also, the extent of passive voice
> there makes it hard to know who does what (UART / kernel).
>
> > The logic for clearing the interrupt in the interrupt handling
> > function of RX_TIMEOUT is !UART_LSR_DR. And to avoid PSLVERR, we need
> > to check UART_LSR_DR first. To meet the requirements of both, the FIFO
> > needs to be disabled.
>
> The grammar is so broken, it failed to convey that message.

The purpose of serial_port_in(port, UART_RX) is to clear interrupts
such as rx_timeout. In dw8250_handle_irq(), serial_port_in(p, UART_RX)
is called when the LSR does not have the UART_LSR_DR bit set.

To avoid PSLVERR when the FIFO is enabled, serial_in(up, UART_RX)
should be called only when the LSR has the UART_LSR_DR bit set.

These two logics are clearly contradictory. Therefore, both
serial8250_clear_fifos() and serial_port_in(port, UART_RX) are placed
under the protection of port->lock.

If you believe this is not a potential issue, that's fine. I can
remove this patch in the next patchset version.

>
> > Therefore, we should put serial8250_clear_fifos() and the execution of
> > serial_port_in(port, UART_RX) without checking UART_LSR_DR under the
> > port->lock.
> >
> > >
> > > >        */
> > > >       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);
> > > >
> > > > @@ -2429,15 +2429,14 @@ 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);
> > > > +     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;
> > > > @@ -2519,7 +2518,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
> > > > @@ -2527,6 +2525,14 @@ 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);
> > > > +     /*
> > > > +      * 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);
> > > >
> > > >  #ifdef CONFIG_SERIAL_8250_RSA
> > > >       /*
> > > > @@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
> > > >       disable_rsa(up);
> > > >  #endif
> > > >
> > > > -     /*
> > > > -      * Read data port to reset things, and then unlink from
> > > > -      * the IRQ chain.
> > > > -      */
> > > > -     serial_port_in(port, UART_RX);
> > > >       serial8250_rpm_put(up);
> > > >
> > > >       up->ops->release_irq(up);
> > > >
> > >
> > > --
> > >  i.
> > >
> >
> > Thanks,
> > Yunhui
> >
>
> --
>  i.

Thanks,
Yunhui

Re: [External] Re: [PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

[Ilpo Järvinen]

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

On Mon, 12 May 2025, yunhui cui wrote:

> Hi Ilpo,
> 
> On Mon, May 12, 2025 at 6:27 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Mon, 12 May 2025, yunhui cui wrote:
> >
> > > Hi Ilpo,
> > >
> > > On Tue, May 6, 2025 at 8:00 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > >
> > > > On Tue, 6 May 2025, Yunhui Cui wrote:
> > > >
> > > > > 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.
> > > >
> > > > Don't expect reader to know the condition how PSLVERR is triggered. I know
> > > > it's worded out in the other patch but also explain it here.
> > > >
> > > > You're only explaining problem and missing what this patch does to solve
> > > > the problem.
> > > >
> > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > ---
> > > > >  drivers/tty/serial/8250/8250.h      | 13 +++++++++
> > > > >  drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
> > > > >  2 files changed, 35 insertions(+), 21 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > > > > index b861585ca02a..6f97ff3a197d 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 a913135d5217..1666b965f6a0 100644
> > > > > --- a/drivers/tty/serial/8250/8250_port.c
> > > > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > > > @@ -1357,9 +1357,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);
> > > > > @@ -2137,25 +2136,22 @@ 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;
> > > > > +     unsigned long flags;
> > > > >
> > > > >       serial8250_rpm_get(up);
> > > > >
> > > > > +     uart_port_lock_irqsave(port, &flags);
> > > > >       lsr = serial_port_in(port, UART_LSR);
> > > > > +     if (lsr & UART_LSR_DR)
> > > > > +             status = serial_port_in(port, UART_RX);
> > > > > +     uart_port_unlock_irqrestore(port, flags);
> > > > >
> > > > > -     if (!(lsr & UART_LSR_DR)) {
> > > > > -             status = NO_POLL_CHAR;
> > > > > -             goto out;
> > > > > -     }
> > > > > -
> > > > > -     status = serial_port_in(port, UART_RX);
> > > > > -out:
> > > > >       serial8250_rpm_put(up);
> > > > >       return status;
> > > >
> > > > Not a problem that originates from you, but IMO calling this variable
> > > > "status" is quite misleading when it is the character (or NO_POLL_CHAR
> > > > is no character is present).
> > > >
> > > > >  }
> > > > >
> > > > > -
> > > > >  static void serial8250_put_poll_char(struct uart_port *port,
> > > > >                        unsigned char c)
> > > > >  {
> > > > > @@ -2264,13 +2260,17 @@ 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).
> > > > > +      * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> > > > > +      * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
> > > >
> > > > I don't understand what the last two lines mean and I don't see the
> > > > connection to the code that is below the comment either, could you try to
> > > > rephrase the comment.
> > >
> > > The original intention was to check UART_LSR_DR first when reading
> > > UART_RX. However, the purpose of serial_port_in(port, UART_RX) here is
> > > to clear the interrupt, such as the interrupt caused by RX_TIMEOUT.
> >
> > I understood the first sentence in the comment but the rest of it is very
> > cryptic and has many grammar issues too. Also, the extent of passive voice
> > there makes it hard to know who does what (UART / kernel).
> >
> > > The logic for clearing the interrupt in the interrupt handling
> > > function of RX_TIMEOUT is !UART_LSR_DR. And to avoid PSLVERR, we need
> > > to check UART_LSR_DR first. To meet the requirements of both, the FIFO
> > > needs to be disabled.
> >
> > The grammar is so broken, it failed to convey that message.
> 
> The purpose of serial_port_in(port, UART_RX) is to clear interrupts
> such as rx_timeout. In dw8250_handle_irq(), serial_port_in(p, UART_RX)
> is called when the LSR does not have the UART_LSR_DR bit set.
> 
> To avoid PSLVERR when the FIFO is enabled, serial_in(up, UART_RX)
> should be called only when the LSR has the UART_LSR_DR bit set.
> 
> These two logics are clearly contradictory. Therefore, both
> serial8250_clear_fifos() and serial_port_in(port, UART_RX) are placed
> under the protection of port->lock.
> 
> If you believe this is not a potential issue, that's fine. I can
> remove this patch in the next patchset version.

No, my goal is not to get this removed from the patch series.

I meant that the comment wording needs to be fixed for the next version 
such that it is understandable for those that are not deeply familiar with 
what is related to PSLVERR. Currently even I struggle to follow what's 
written into that comment (unless I read heavily between lines and base 
guesses on the extra knowledge I've about how this entire patchset relates 
to PSLVERR).

> > > Therefore, we should put serial8250_clear_fifos() and the execution of
> > > serial_port_in(port, UART_RX) without checking UART_LSR_DR under the
> > > port->lock.
> > >
> > > >
> > > > >        */
> > > > >       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);
> > > > >
> > > > > @@ -2429,15 +2429,14 @@ 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);
> > > > > +     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;
> > > > > @@ -2519,7 +2518,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
> > > > > @@ -2527,6 +2525,14 @@ 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);
> > > > > +     /*
> > > > > +      * 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);
> > > > >
> > > > >  #ifdef CONFIG_SERIAL_8250_RSA
> > > > >       /*
> > > > > @@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
> > > > >       disable_rsa(up);
> > > > >  #endif
> > > > >
> > > > > -     /*
> > > > > -      * Read data port to reset things, and then unlink from
> > > > > -      * the IRQ chain.
> > > > > -      */
> > > > > -     serial_port_in(port, UART_RX);
> > > > >       serial8250_rpm_put(up);
> > > > >
> > > > >       up->ops->release_irq(up);
> > > > >
> > > >
> > > > --
> > > >  i.
> > > >
> > >
> > > Thanks,
> > > Yunhui
> > >
> >
> > --
> >  i.
> 
> Thanks,
> Yunhui
> 

-- 
 i.

Re: [External] Re: [PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

[yunhui cui]

Hi Ilpo,

On Mon, May 12, 2025 at 8:03 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Mon, 12 May 2025, yunhui cui wrote:
>
> > Hi Ilpo,
> >
> > On Mon, May 12, 2025 at 6:27 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Mon, 12 May 2025, yunhui cui wrote:
> > >
> > > > Hi Ilpo,
> > > >
> > > > On Tue, May 6, 2025 at 8:00 PM Ilpo Järvinen
> > > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > > >
> > > > > On Tue, 6 May 2025, Yunhui Cui wrote:
> > > > >
> > > > > > 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.
> > > > >
> > > > > Don't expect reader to know the condition how PSLVERR is triggered. I know
> > > > > it's worded out in the other patch but also explain it here.
> > > > >
> > > > > You're only explaining problem and missing what this patch does to solve
> > > > > the problem.
> > > > >
> > > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > > ---
> > > > > >  drivers/tty/serial/8250/8250.h      | 13 +++++++++
> > > > > >  drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
> > > > > >  2 files changed, 35 insertions(+), 21 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > > > > > index b861585ca02a..6f97ff3a197d 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 a913135d5217..1666b965f6a0 100644
> > > > > > --- a/drivers/tty/serial/8250/8250_port.c
> > > > > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > > > > @@ -1357,9 +1357,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);
> > > > > > @@ -2137,25 +2136,22 @@ 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;
> > > > > > +     unsigned long flags;
> > > > > >
> > > > > >       serial8250_rpm_get(up);
> > > > > >
> > > > > > +     uart_port_lock_irqsave(port, &flags);
> > > > > >       lsr = serial_port_in(port, UART_LSR);
> > > > > > +     if (lsr & UART_LSR_DR)
> > > > > > +             status = serial_port_in(port, UART_RX);
> > > > > > +     uart_port_unlock_irqrestore(port, flags);
> > > > > >
> > > > > > -     if (!(lsr & UART_LSR_DR)) {
> > > > > > -             status = NO_POLL_CHAR;
> > > > > > -             goto out;
> > > > > > -     }
> > > > > > -
> > > > > > -     status = serial_port_in(port, UART_RX);
> > > > > > -out:
> > > > > >       serial8250_rpm_put(up);
> > > > > >       return status;
> > > > >
> > > > > Not a problem that originates from you, but IMO calling this variable
> > > > > "status" is quite misleading when it is the character (or NO_POLL_CHAR
> > > > > is no character is present).
> > > > >
> > > > > >  }
> > > > > >
> > > > > > -
> > > > > >  static void serial8250_put_poll_char(struct uart_port *port,
> > > > > >                        unsigned char c)
> > > > > >  {
> > > > > > @@ -2264,13 +2260,17 @@ 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).
> > > > > > +      * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> > > > > > +      * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
> > > > >
> > > > > I don't understand what the last two lines mean and I don't see the
> > > > > connection to the code that is below the comment either, could you try to
> > > > > rephrase the comment.
> > > >
> > > > The original intention was to check UART_LSR_DR first when reading
> > > > UART_RX. However, the purpose of serial_port_in(port, UART_RX) here is
> > > > to clear the interrupt, such as the interrupt caused by RX_TIMEOUT.
> > >
> > > I understood the first sentence in the comment but the rest of it is very
> > > cryptic and has many grammar issues too. Also, the extent of passive voice
> > > there makes it hard to know who does what (UART / kernel).
> > >
> > > > The logic for clearing the interrupt in the interrupt handling
> > > > function of RX_TIMEOUT is !UART_LSR_DR. And to avoid PSLVERR, we need
> > > > to check UART_LSR_DR first. To meet the requirements of both, the FIFO
> > > > needs to be disabled.
> > >
> > > The grammar is so broken, it failed to convey that message.
> >
> > The purpose of serial_port_in(port, UART_RX) is to clear interrupts
> > such as rx_timeout. In dw8250_handle_irq(), serial_port_in(p, UART_RX)
> > is called when the LSR does not have the UART_LSR_DR bit set.
> >
> > To avoid PSLVERR when the FIFO is enabled, serial_in(up, UART_RX)
> > should be called only when the LSR has the UART_LSR_DR bit set.
> >
> > These two logics are clearly contradictory. Therefore, both
> > serial8250_clear_fifos() and serial_port_in(port, UART_RX) are placed
> > under the protection of port->lock.
> >
> > If you believe this is not a potential issue, that's fine. I can
> > remove this patch in the next patchset version.
>
> No, my goal is not to get this removed from the patch series.
>
> I meant that the comment wording needs to be fixed for the next version
> such that it is understandable for those that are not deeply familiar with
> what is related to PSLVERR. Currently even I struggle to follow what's
> written into that comment (unless I read heavily between lines and base
> guesses on the extra knowledge I've about how this entire patchset relates
> to PSLVERR).
>

I plan to change the commit message as follows:
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.


The comment here will be changed as follows:
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()).

What do you think?

> > > > Therefore, we should put serial8250_clear_fifos() and the execution of
> > > > serial_port_in(port, UART_RX) without checking UART_LSR_DR under the
> > > > port->lock.
> > > >
> > > > >
> > > > > >        */
> > > > > >       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);
> > > > > >
> > > > > > @@ -2429,15 +2429,14 @@ 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);
> > > > > > +     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;
> > > > > > @@ -2519,7 +2518,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
> > > > > > @@ -2527,6 +2525,14 @@ 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);
> > > > > > +     /*
> > > > > > +      * 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);
> > > > > >
> > > > > >  #ifdef CONFIG_SERIAL_8250_RSA
> > > > > >       /*
> > > > > > @@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
> > > > > >       disable_rsa(up);
> > > > > >  #endif
> > > > > >
> > > > > > -     /*
> > > > > > -      * Read data port to reset things, and then unlink from
> > > > > > -      * the IRQ chain.
> > > > > > -      */
> > > > > > -     serial_port_in(port, UART_RX);
> > > > > >       serial8250_rpm_put(up);
> > > > > >
> > > > > >       up->ops->release_irq(up);
> > > > > >
> > > > >
> > > > > --
> > > > >  i.
> > > > >
> > > >
> > > > Thanks,
> > > > Yunhui
> > > >
> > >
> > > --
> > >  i.
> >
> > Thanks,
> > Yunhui
> >
>
> --
>  i.

Thanks,
Yunhui