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

[PATCH v4 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 v4 2/4] serial: 8250: introduce serial8250_discard_data()

[Yunhui Cui]

To prevent triggering PSLVERR, it is necessary to check whether the
UART_LSR_DR bit of UART_LSR is set before reading UART_RX.
Ensure atomicity of UART_LSR and UART_RX, put serial8250_discard_data()
under port->lock.

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

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b861585ca02a..5a106cf88207 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -162,6 +162,21 @@ 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 unsigned int serial8250_discard_data(struct uart_8250_port *up)
+{
+	u16 lsr;
+
+	lsr = serial_in(up, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		return serial_in(up, UART_RX);
+
+	return 0;
+}
+
 /*
  * For the 16C950
  */
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index a913135d5217..802ac50357c0 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,21 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
 static int serial8250_get_poll_char(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
-	int status;
+	int status = NO_POLL_CHAR;
 	u16 lsr;
 
 	serial8250_rpm_get(up);
 
+	uart_port_lock_irqsave(port, &flags);
 	lsr = serial_port_in(port, UART_LSR);
-
-	if (!(lsr & UART_LSR_DR)) {
-		status = NO_POLL_CHAR;
-		goto out;
-	}
-
-	status = serial_port_in(port, UART_RX);
+	if ((lsr & UART_LSR_DR))
+		status = serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 out:
 	serial8250_rpm_put(up);
 	return status;
 }
 
-
 static void serial8250_put_poll_char(struct uart_port *port,
 			 unsigned char c)
 {
@@ -2264,13 +2259,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 +2428,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 +2517,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 +2524,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 +2540,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 v4 3/4] serial: 8250: 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..07f9be074b4b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
+#include <linux/lockdep.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
@@ -112,6 +113,13 @@ static void dw8250_force_idle(struct uart_port *p)
 	struct uart_8250_port *up = up_to_u8250p(p);
 	unsigned int lsr;
 
+	/*
+	 * serial_in(p, UART_RX) should be under port->lock, but we can't add
+	 * it to avoid AA deadlock as we're unsure if serial_out*(...UART_LCR)
+	 * is under port->lock.
+	 */
+	lockdep_assert_held_once(&p->lock);
+
 	serial8250_clear_and_reinit_fifos(up);
 
 	/*
-- 
2.39.2

[PATCH v4 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 07f9be074b4b..1e364280a108 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -273,6 +273,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 	unsigned int quirks = d->pdata->quirks;
 	unsigned int status;
 	unsigned long flags;
+	unsigned char old_fcr;
 
 	/*
 	 * There are ways to get Designware-based UARTs into a state where
@@ -288,9 +289,19 @@ 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) {
+				old_fcr = serial_in(up, UART_FCR);
+				serial_out(up, UART_FCR, old_fcr & ~1);
+			}
+
 			(void) p->serial_in(p, UART_RX);
 
+			if (up->fcr & UART_FCR_ENABLE_FIFO)
+				serial_out(up, UART_FCR, old_fcr);
+		}
+
 		uart_port_unlock_irqrestore(p, flags);
 	}
 
-- 
2.39.2

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

[Jiri Slaby]

On 25. 04. 25, 8:24, Yunhui Cui wrote:
> 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 | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 07f9be074b4b..1e364280a108 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -273,6 +273,7 @@ static int dw8250_handle_irq(struct uart_port *p)
>   	unsigned int quirks = d->pdata->quirks;
>   	unsigned int status;
>   	unsigned long flags;
> +	unsigned char old_fcr;

No more unsigned char, please. Use u8.

> @@ -288,9 +289,19 @@ 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) {
> +				old_fcr = serial_in(up, UART_FCR);
> +				serial_out(up, UART_FCR, old_fcr & ~1);

s/1/UART_FCR_ENABLE_FIFO/

> +			}
> +
>   			(void) p->serial_in(p, UART_RX);
>   
> +			if (up->fcr & UART_FCR_ENABLE_FIFO)
> +				serial_out(up, UART_FCR, old_fcr);
> +		}
> +
>   		uart_port_unlock_irqrestore(p, flags);
>   	}
>   


-- 
js
suse labs

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

[Jiri Slaby]

On 25. 04. 25, 8:41, Jiri Slaby wrote:
> On 25. 04. 25, 8:24, Yunhui Cui wrote:
>> 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 | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/ 
>> serial/8250/8250_dw.c
>> index 07f9be074b4b..1e364280a108 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -273,6 +273,7 @@ static int dw8250_handle_irq(struct uart_port *p)
>>       unsigned int quirks = d->pdata->quirks;
>>       unsigned int status;
>>       unsigned long flags;
>> +    unsigned char old_fcr;
> 
> No more unsigned char, please. Use u8.
> 
>> @@ -288,9 +289,19 @@ 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) {
>> +                old_fcr = serial_in(up, UART_FCR);

Wait, read(FCR) actually means read(IIR). FCR is write only. Or is DW 
special in this?

>> +                serial_out(up, UART_FCR, old_fcr & ~1);
> 
> s/1/UART_FCR_ENABLE_FIFO/
> 
>> +            }
>> +
>>               (void) p->serial_in(p, UART_RX);
>> +            if (up->fcr & UART_FCR_ENABLE_FIFO)
>> +                serial_out(up, UART_FCR, old_fcr);
>> +        }
>> +
>>           uart_port_unlock_irqrestore(p, flags);
>>       }
> 
> 

-- 
js
suse labs

Re: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard_data()

[Jiri Slaby]

On 25. 04. 25, 8:24, Yunhui Cui wrote:
> To prevent triggering PSLVERR, it is necessary to check whether the
> UART_LSR_DR bit of UART_LSR is set before reading UART_RX.
> Ensure atomicity of UART_LSR and UART_RX, put serial8250_discard_data()
> under port->lock.
> 
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>   drivers/tty/serial/8250/8250.h      | 15 +++++++++++
>   drivers/tty/serial/8250/8250_port.c | 42 ++++++++++++++---------------
>   2 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index b861585ca02a..5a106cf88207 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -162,6 +162,21 @@ 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 unsigned int serial8250_discard_data(struct uart_8250_port *up)
> +{
> +	u16 lsr;

Is lsr really 16-bit?

> +
> +	lsr = serial_in(up, UART_LSR);
> +	if (lsr & UART_LSR_DR)
> +		return serial_in(up, UART_RX);

Why does a discard function return a value at all?

> +
> +	return 0;
> +}
> +
>   /*
>    * For the 16C950
>    */
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index a913135d5217..802ac50357c0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
...
> @@ -2137,25 +2136,21 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>   static int serial8250_get_poll_char(struct uart_port *port)
>   {
>   	struct uart_8250_port *up = up_to_u8250p(port);
> -	int status;
> +	int status = NO_POLL_CHAR;
>   	u16 lsr;
>   
>   	serial8250_rpm_get(up);
>   
> +	uart_port_lock_irqsave(port, &flags);
>   	lsr = serial_port_in(port, UART_LSR);
> -
> -	if (!(lsr & UART_LSR_DR)) {
> -		status = NO_POLL_CHAR;
> -		goto out;
> -	}
> -
> -	status = serial_port_in(port, UART_RX);
> +	if ((lsr & UART_LSR_DR))

Too many parentheses.

> +		status = serial_port_in(port, UART_RX);
> +	uart_port_unlock_irqrestore(port, flags);

thanks,
-- 
js
suse labs

Re: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard_data()

[John Ogness]

On 2025-04-25, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> To prevent triggering PSLVERR, it is necessary to check whether the
> UART_LSR_DR bit of UART_LSR is set before reading UART_RX.
> Ensure atomicity of UART_LSR and UART_RX, put serial8250_discard_data()
> under port->lock.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  drivers/tty/serial/8250/8250.h      | 15 +++++++++++
>  drivers/tty/serial/8250/8250_port.c | 42 ++++++++++++++---------------
>  2 files changed, 36 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index a913135d5217..802ac50357c0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2137,25 +2136,21 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>  static int serial8250_get_poll_char(struct uart_port *port)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
> -	int status;
> +	int status = NO_POLL_CHAR;
>  	u16 lsr;
>  
>  	serial8250_rpm_get(up);
>  
> +	uart_port_lock_irqsave(port, &flags);
>  	lsr = serial_port_in(port, UART_LSR);

The ->poll_get_char() callback is used for kdb/kgdb.

Adding a spinlock here could lead to deadlock. However, I see that
serial8250_rpm_get() is already in there, which goes down a rabbit hole
of possible issues. So I guess we really don't care about possible
kdb/kgdb deadlocks for now.

I can look into cleaning this up with my next 8250 nbcon console
series. So for now, I am OK with you adding the spin_lock() in this
callback.

John Ogness

Re: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard_data()

[kernel test robot]

Hi Yunhui,

kernel test robot noticed the following build errors:

[auto build test ERROR on soc/for-next]
[cannot apply to tty/tty-testing tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.15-rc3 next-20250424]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yunhui-Cui/serial-8250-introduce-serial8250_discard_data/20250425-142655
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20250425062425.68761-2-cuiyunhui%40bytedance.com
patch subject: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard_data()
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250426/202504261249.RVGiOFHl-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250426/202504261249.RVGiOFHl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504261249.RVGiOFHl-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/tty/serial/8250/8250_port.c: In function 'serial8250_get_poll_char':
>> drivers/tty/serial/8250/8250_port.c:2146:39: error: 'flags' undeclared (first use in this function)
    2146 |         uart_port_lock_irqsave(port, &flags);
         |                                       ^~~~~
   drivers/tty/serial/8250/8250_port.c:2146:39: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/tty/serial/8250/8250_port.c:2151:1: warning: label 'out' defined but not used [-Wunused-label]
    2151 | out:
         | ^~~


vim +/flags +2146 drivers/tty/serial/8250/8250_port.c

  2131	
  2132	#ifdef CONFIG_CONSOLE_POLL
  2133	/*
  2134	 * Console polling routines for writing and reading from the uart while
  2135	 * in an interrupt or debug context.
  2136	 */
  2137	
  2138	static int serial8250_get_poll_char(struct uart_port *port)
  2139	{
  2140		struct uart_8250_port *up = up_to_u8250p(port);
  2141		int status = NO_POLL_CHAR;
  2142		u16 lsr;
  2143	
  2144		serial8250_rpm_get(up);
  2145	
> 2146		uart_port_lock_irqsave(port, &flags);
  2147		lsr = serial_port_in(port, UART_LSR);
  2148		if ((lsr & UART_LSR_DR))
  2149			status = serial_port_in(port, UART_RX);
  2150		uart_port_unlock_irqrestore(port, flags);
> 2151	out:
  2152		serial8250_rpm_put(up);
  2153		return status;
  2154	}
  2155	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard_data()

[kernel test robot]

Hi Yunhui,

kernel test robot noticed the following build errors:

[auto build test ERROR on soc/for-next]
[cannot apply to tty/tty-testing tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.15-rc3 next-20250424]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yunhui-Cui/serial-8250-introduce-serial8250_discard_data/20250425-142655
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20250425062425.68761-2-cuiyunhui%40bytedance.com
patch subject: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard_data()
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20250426/202504261327.CEtXsr6g-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250426/202504261327.CEtXsr6g-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504261327.CEtXsr6g-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/tty/serial/8250/8250_port.c:2146:32: error: use of undeclared identifier 'flags'
    2146 |         uart_port_lock_irqsave(port, &flags);
         |                                       ^
   drivers/tty/serial/8250/8250_port.c:2150:36: error: use of undeclared identifier 'flags'
    2150 |         uart_port_unlock_irqrestore(port, flags);
         |                                           ^
   2 errors generated.


vim +/flags +2146 drivers/tty/serial/8250/8250_port.c

  2131	
  2132	#ifdef CONFIG_CONSOLE_POLL
  2133	/*
  2134	 * Console polling routines for writing and reading from the uart while
  2135	 * in an interrupt or debug context.
  2136	 */
  2137	
  2138	static int serial8250_get_poll_char(struct uart_port *port)
  2139	{
  2140		struct uart_8250_port *up = up_to_u8250p(port);
  2141		int status = NO_POLL_CHAR;
  2142		u16 lsr;
  2143	
  2144		serial8250_rpm_get(up);
  2145	
> 2146		uart_port_lock_irqsave(port, &flags);
  2147		lsr = serial_port_in(port, UART_LSR);
  2148		if ((lsr & UART_LSR_DR))
  2149			status = serial_port_in(port, UART_RX);
  2150		uart_port_unlock_irqrestore(port, flags);
  2151	out:
  2152		serial8250_rpm_put(up);
  2153		return status;
  2154	}
  2155	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[yunhui cui]

Hi js,


On Fri, Apr 25, 2025 at 2:43 PM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 25. 04. 25, 8:41, Jiri Slaby wrote:
> > On 25. 04. 25, 8:24, Yunhui Cui wrote:
> >> 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 | 13 ++++++++++++-
> >>   1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/
> >> serial/8250/8250_dw.c
> >> index 07f9be074b4b..1e364280a108 100644
> >> --- a/drivers/tty/serial/8250/8250_dw.c
> >> +++ b/drivers/tty/serial/8250/8250_dw.c
> >> @@ -273,6 +273,7 @@ static int dw8250_handle_irq(struct uart_port *p)
> >>       unsigned int quirks = d->pdata->quirks;
> >>       unsigned int status;
> >>       unsigned long flags;
> >> +    unsigned char old_fcr;
> >
> > No more unsigned char, please. Use u8.
> >
> >> @@ -288,9 +289,19 @@ 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) {
> >> +                old_fcr = serial_in(up, UART_FCR);
>
> Wait, read(FCR) actually means read(IIR). FCR is write only. Or is DW
> special in this?

Indeed, the valid bits of the FCR are write-only. It seems that here
we can only do serial_out(up, UART_FCR, up->fcr); What do you think?

>
> >> +                serial_out(up, UART_FCR, old_fcr & ~1);
> >
> > s/1/UART_FCR_ENABLE_FIFO/
> >
> >> +            }
> >> +
> >>               (void) p->serial_in(p, UART_RX);
> >> +            if (up->fcr & UART_FCR_ENABLE_FIFO)
> >> +                serial_out(up, UART_FCR, old_fcr);
> >> +        }
> >> +
> >>           uart_port_unlock_irqrestore(p, flags);
> >>       }
> >
> >
>
> --
> js
> suse labs
>

Thanks,
Yunhui

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

[yunhui cui]

On Sun, Apr 27, 2025 at 7:17 PM yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi js,
>
>
> On Fri, Apr 25, 2025 at 2:43 PM Jiri Slaby <jirislaby@kernel.org> wrote:
> >
> > On 25. 04. 25, 8:41, Jiri Slaby wrote:
> > > On 25. 04. 25, 8:24, Yunhui Cui wrote:
> > >> 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 | 13 ++++++++++++-
> > >>   1 file changed, 12 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/
> > >> serial/8250/8250_dw.c
> > >> index 07f9be074b4b..1e364280a108 100644
> > >> --- a/drivers/tty/serial/8250/8250_dw.c
> > >> +++ b/drivers/tty/serial/8250/8250_dw.c
> > >> @@ -273,6 +273,7 @@ static int dw8250_handle_irq(struct uart_port *p)
> > >>       unsigned int quirks = d->pdata->quirks;
> > >>       unsigned int status;
> > >>       unsigned long flags;
> > >> +    unsigned char old_fcr;
> > >
> > > No more unsigned char, please. Use u8.
> > >
> > >> @@ -288,9 +289,19 @@ 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) {
> > >> +                old_fcr = serial_in(up, UART_FCR);
> >
> > Wait, read(FCR) actually means read(IIR). FCR is write only. Or is DW
> > special in this?
>
> Indeed, the valid bits of the FCR are write-only. It seems that here
> we can only do serial_out(up, UART_FCR, up->fcr); What do you think?

I looked through the DW databook and found that we can use the SFE
register. However, it is not guaranteed that all UARTs in the dw
series have this register.



>
> >
> > >> +                serial_out(up, UART_FCR, old_fcr & ~1);
> > >
> > > s/1/UART_FCR_ENABLE_FIFO/
> > >
> > >> +            }
> > >> +
> > >>               (void) p->serial_in(p, UART_RX);
> > >> +            if (up->fcr & UART_FCR_ENABLE_FIFO)
> > >> +                serial_out(up, UART_FCR, old_fcr);
> > >> +        }
> > >> +
> > >>           uart_port_unlock_irqrestore(p, flags);
> > >>       }
> > >
> > >
> >
> > --
> > js
> > suse labs
> >
>
> Thanks,
> Yunhui

Re: [External] Re: [PATCH v4 2/4] serial: 8250: introduce serial8250_discard_data()

[yunhui cui]

Hi js,

On Fri, Apr 25, 2025 at 2:49 PM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 25. 04. 25, 8:24, Yunhui Cui wrote:
> > To prevent triggering PSLVERR, it is necessary to check whether the
> > UART_LSR_DR bit of UART_LSR is set before reading UART_RX.
> > Ensure atomicity of UART_LSR and UART_RX, put serial8250_discard_data()
> > under port->lock.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >   drivers/tty/serial/8250/8250.h      | 15 +++++++++++
> >   drivers/tty/serial/8250/8250_port.c | 42 ++++++++++++++---------------
> >   2 files changed, 36 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > index b861585ca02a..5a106cf88207 100644
> > --- a/drivers/tty/serial/8250/8250.h
> > +++ b/drivers/tty/serial/8250/8250.h
> > @@ -162,6 +162,21 @@ 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 unsigned int serial8250_discard_data(struct uart_8250_port *up)
> > +{
> > +     u16 lsr;
>
> Is lsr really 16-bit?

The data book I have shows a 32 - bit width, but actually only the
lower 8 bits are valid. Using u16 is fine, and the current code all
uses u16 lsr.

>
> > +
> > +     lsr = serial_in(up, UART_LSR);
> > +     if (lsr & UART_LSR_DR)
> > +             return serial_in(up, UART_RX);
>
> Why does a discard function return a value at all?

Update in next version.

>
> > +
> > +     return 0;
> > +}
> > +
> >   /*
> >    * For the 16C950
> >    */
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index a913135d5217..802ac50357c0 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> ...
> > @@ -2137,25 +2136,21 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> >   static int serial8250_get_poll_char(struct uart_port *port)
> >   {
> >       struct uart_8250_port *up = up_to_u8250p(port);
> > -     int status;
> > +     int status = NO_POLL_CHAR;
> >       u16 lsr;
> >
> >       serial8250_rpm_get(up);
> >
> > +     uart_port_lock_irqsave(port, &flags);
> >       lsr = serial_port_in(port, UART_LSR);
> > -
> > -     if (!(lsr & UART_LSR_DR)) {
> > -             status = NO_POLL_CHAR;
> > -             goto out;
> > -     }
> > -
> > -     status = serial_port_in(port, UART_RX);
> > +     if ((lsr & UART_LSR_DR))
>
> Too many parentheses.

Okay.

>
> > +             status = serial_port_in(port, UART_RX);
> > +     uart_port_unlock_irqrestore(port, flags);
>
> thanks,
> --
> js
> suse labs

Thanks,
Yunhui