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

[Yicong Yang]

On 7/1/26 4:45 PM, Ilpo Järvinen wrote:
> On Wed, 1 Jul 2026, Yicong Yang wrote:
> 
>> On 7/1/26 2:18 AM, Ilpo Järvinen wrote:
>>> 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@xxxxxxxxxxxxx>
>>>>>> ---
>>>>>>  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.
>>
>> thanks for the kind and useful guidance :) did add a log message in the
>> workaround branch so we'll know if the issue hits.
>>
>>>
>>> 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.
>>
>> as the PSLVERR is caused by the reading of RBR so even in the latter case
>> we should still using the SRBR here if availabe to avoid the PSLVERR as
>> long as we enter this branch, I suppose this should be the safest way.
> 
> I was not commenting on how to fix it, but about probabilty of hitting 
> PSLVERR when taking this branch.
> 
> Rx coming in (it's racing with the driver's code) may make the read not 
> result in PSLVERR even when this branch is taken. It could be this does no 
> matter and taking this branch in practice always results in PSLVERR if RBR 
> is read, which means even any hit of this branch confirms SRBR does indeed 
> work.
> 
> But if RBR reads do not always lead to PSLVERR because incoming Rx makes 
> RBR read "safe" it's less clear how many times the branch should be taken 
> before confidently declaring the fix works.
> 

thanks for the detailed explanation, now I get the point here.

thanks.