Re: [PATCH] serial: 8250_dw: Prefer SRBR in bogus RX timeout workaround if available

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Wed, 1 Jul 2026, Yicong Yang wrote:
> On 6/29/26 11:56 PM, Ilpo Järvinen wrote:
> > On Mon, 29 Jun 2026, Yicong Yang wrote:
> > 
> >> The DW uart could get into the cases where a bogus RX timeout
> >> interrupt is asserted but no available data. This could be
> >> workaround by doing a bogus read.
> >>
> >> Currently the driver's using the standard RBR (receive buffer
> >> register) for this bogus read. However the reading of RBR
> >> in this case is allowed to raise a hardware error if vendor
> >> choose to implement in this way (our platform). It's also
> >> allowed to do the bogus read using SRBR (shadow RBR) for
> >> workaround which won't raise the hardware error. So change
> >> to use the SRBR to workaround the issue if it's available.
> >>
> >> Signed-off-by: Yicong Yang <yang.yicong@picoheart.com>
> >> ---
> >>  drivers/tty/serial/8250/8250_dw.c    | 15 +++++++++++++--
> >>  drivers/tty/serial/8250/8250_dwlib.c |  3 +++
> >>  drivers/tty/serial/8250/8250_dwlib.h |  4 ++++
> >>  3 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> >> index 84ffba045ffa..fea7dfa30e78 100644
> >> --- a/drivers/tty/serial/8250/8250_dw.c
> >> +++ b/drivers/tty/serial/8250/8250_dw.c
> >> @@ -440,8 +440,19 @@ static int dw8250_handle_irq(struct uart_port *p)
> >>  	if (!up->dma && rx_timeout) {
> >>  		status = serial_lsr_in(up);
> >>  
> >> -		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> >> -			serial_port_in(p, UART_RX);
> >> +		if (!(status & (UART_LSR_DR | UART_LSR_BI))) {
> >> +			/*
> >> +			 * Do the bogus read from Shadow RBR (SRBR) if provided.
> >> +			 * Both RBR and SRBR are supported ways to workaround
> > 
> > Hi,
> > 
> > I've not come across this being mentioned anywhere else than in kernel 
> > related context, but you seem to imply there something that tells about 
> > "supported ways"?
> > 
> 
> in fact we got this workaround method that's documented in the synopsys
> dw_uart issue report, so comment "supported" here. quote the related part
> of the workaround below:
> 
>   [read USR[3] for the rx fifo status]
>   If the read value is '0' (indicating the RX FIFO is empty), do a dummy
>   read of the RBR or shadow RBR (SRBR) register without processing the
>   invalid read data.
> 
>   ...Note: Reading the RBR results in a PSLVERR immediately if
>   REG_TIMEOUT_WIDTH parameter is set to '0'. Whereas, reading SRBR does
>   not result in PSLVERR.

Okay, thanks.

> > If there's no such thing, I'm bit hesitant to declare presence of SRBR 
> > guarantees reading it universally solves this issue.
> > 
> > BUT, I suppose it does on your HW since you must have hit this code path, 
> > and thus PSLVERR, yourself to actually discover this issue so I assume 
> > you've then also confirmed that this patch solves the issue in your case 
> > which isn't entirely without merit either.
> > 
> 
> actually it's hard to reproduce, only once at boot time in our several
> hundreds reboot test. we locate the code line here by the backtrace of
> the system error. the issue hasn't reproduced yet on the patched kernel
> (hundred reboots so far, but still going on), and I suppose it's correct
> as provided by the IP designer.

For testing, you could have logged when this condition is actually hit 
in the first place. I usually do that to increase my own confidence if the 
problem is particularly hard to reproduce.

I don't know if it always triggers PSLVERR or if that's some % of !(status 
& (UART_LSR_DR | UART_LSR_BI)) cases because RBR read is racy with Rx but 
that could also be determined if similar debug is applied without the 
patch, I guess.

But with clearly more boots and it working, that's probably enough as 
evidence together with the issue report offering it as a workaround.

-- 
 i.