linux-kernel - Re: [External] Re: [PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEEQ3w==dO2i+ZSsRZG0L1S+ccHSJQ-aUa9KE638MwnBM4+Jvw@mail.gmail.com>
Date: Fri, 11 Jul 2025 10:19:16 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: arnd@...db.de, andriy.shevchenko@...ux.intel.com, 
	benjamin.larsson@...exis.eu, gregkh@...uxfoundation.org, 
	heikki.krogerus@...ux.intel.com, ilpo.jarvinen@...ux.intel.com, 
	jirislaby@...nel.org, jkeeping@...usicbrands.com, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	markus.mayer@...aro.org, matt.porter@...aro.org, namcao@...utronix.de, 
	paulmck@...nel.org, pmladek@...e.com, schnelle@...ux.ibm.com, 
	sunilvl@...tanamicro.com, tim.kryger@...aro.org, stable@...r.kernel.org
Subject: Re: [External] Re: [PATCH v9 2/4] serial: 8250_dw: fix PSLVERR on RX_TIMEOUT

Hi John,

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

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

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

Thanks,
Yunhui