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

[Yicong Yang]

On 9/17/26 6:47 PM, Kumar, Udit wrote:
> Hi Yicong,
> 
> On 9/8/2026 7:45 PM, 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.
>>
> 
> Thanks for the patch. If I understand correctly, the IP you are using
> does not allow reading an empty FIFO, so you decided to read the SRBR
> instead.

yes but under certain condition, see blow.

> 
> Does this same case applies when you open a port without any data in the
> FIFO:
> https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/tty/serial/8250/8250_port.c#L710
> ?
> 
> If yes then I am wondering, how the condition mentioned above is bypassed

reading an empty FIFO (by RBR) will be fine when the fifo is disabled.
so I think serial8250_clear_interrupts() you referred is always invoked
when the FIFO is disabled.

I also find a similar issue related to serial8250_clear_interrupts()
that trigger the PSLVERR when reading the empty FIFO when the FIFO
is enabled. this is fixed by 7f8fdd4dbffc ("serial: 8250: fix panic due to PSLVERR")

thanks.

> 
> Thanks
> Udit
> 
> 
>> Signed-off-by: Yicong Yang <yang.yicong@xxxxxxxxxxxxx>
>> ---
>> Change since v1:
>> - refine the comments and type per Andy
>> Link: https://lore.kernel.org/linux-serial/20260907074035.77796-1-yang.yicong@xxxxxxxxxxxxx/
>>
>> Change since v1:
>> - simply the workaround path per Ilpo and Andy
>> - run ~48k reboot tests and didn't reproduce the issue
>> Link: https://lore.kernel.org/linux-serial/20260629075510.32854-1-yang.yicong@xxxxxxxxxxxxx/
>>
>>  drivers/tty/serial/8250/8250_dw.c    | 9 ++++++++-
>>  drivers/tty/serial/8250/8250_dwlib.c | 5 +++++
>>  drivers/tty/serial/8250/8250_dwlib.h | 5 +++++
>>  3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index 51d026f20825..b8a0bca8b536 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -439,8 +439,15 @@ static int dw8250_handle_irq(struct uart_port *p)
>>  	if (!up->dma && rx_timeout) {
>>  		status = serial_lsr_in(up);
>>  
>> +		/*
>> +		 * Do the bogus read from Shadow RBR (SRBR) if provided.
>> +		 * Both RBR and SRBR are supported ways to workaround
>> +		 * this problem. But read RBR can cause hardware error
>> +		 * (PSLVERR) if hardware choose to implement in this way
>> +		 * (implement REG_TIMEOUT_WIDTH to 0), whereas SRBR won't.
>> +		 */
>>  		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
>> -			serial_port_in(p, UART_RX);
>> +			serial_port_in(p, d->data.srbr);
>>  	}
>>  
>>  	/* Manually stop the Rx DMA transfer when acting as flow controller */
>> diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
>> index 8859e66d2d71..9bb02a4ab11f 100644
>> --- a/drivers/tty/serial/8250/8250_dwlib.c
>> +++ b/drivers/tty/serial/8250/8250_dwlib.c
>> @@ -247,5 +247,10 @@ void dw8250_setup_port(struct uart_port *p)
>>  
>>  	if (reg & DW_UART_CPR_SIR_MODE)
>>  		up->capabilities |= UART_CAP_IRDA;
>> +
>> +	if (reg & DW_UART_CPR_SHADOW)
>> +		pd->srbr = DW_UART_SRBR_0;
>> +	else
>> +		pd->srbr = UART_RX;
>>  }
>>  EXPORT_SYMBOL_GPL(dw8250_setup_port);
>> diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
>> index 1fe52332e774..ee7a07fac0f6 100644
>> --- a/drivers/tty/serial/8250/8250_dwlib.h
>> +++ b/drivers/tty/serial/8250/8250_dwlib.h
>> @@ -13,6 +13,7 @@
>>  #include "8250.h"
>>  
>>  /* Offsets for the DesignWare specific registers */
>> +#define DW_UART_SRBR_0	0x0c /* Shadow Receive Buffer Register */
>>  #define DW_UART_USR	0x1f /* UART Status Register */
>>  #define DW_UART_DMASA	0xa8 /* DMA Software Ack */
>>  #define DW_UART_TCR	0xac /* Transceiver Control Register (RS485) */
>> @@ -90,6 +91,10 @@ struct dw8250_port_data {
>>  
>>  	/* RS485 variables */
>>  	bool			hw_rs485_support;
>> +
>> +	/* Register offsets */
>> +	/* Shadow RBR (if not defined RBR will be used) */
>> +	int		srbr;
>>  };
>>  
>>  void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, const struct ktermios *old);