Re: [PATCH 2/2] serial: 8250_dw: Add ti,tda54-uart quirk to skip empty FIFO read

[Moteen Shah <m-shah@ti.com>]

On 11/09/26 15:11, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] The quirk avoids reading an empty FIFO in the IRQ handler, but generic 8250 core code performs unconditional empty FIFO reads during port startup and shutdown, which will still trigger a data abort on this hardware.
>
> Pre-existing issues:
> - [High] pm_runtime_enable() is called after serial port registration, violating callback timing rules.
> --
>
> commit 31fb31567ebe4c6e4b9eb7b2826e35722f3e46f5
> Author: Moteen Shah <m-shah@ti.com>
> serial: 8250_dw: Add ti,tda54-uart quirk to skip empty FIFO read
>
> This commit adds a quirk to the 8250_dw driver for the ti,tda54-uart. The
> quirk skips an empty FIFO read during an RX_TIMEOUT interrupt condition. This
> is intended to avoid triggering a data abort on the TDA54 IP, which crashes
> when reading an empty FIFO.
>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index 51d026f20825a..f97cc3dfbab4a 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
> [ ... ]
>
>> @@ -436,7 +437,7 @@ static int dw8250_handle_irq(struct uart_port *p)
>>   	 * This problem has only been observed so far when not in DMA mode
>>   	 * so we limit the workaround only to non-DMA mode.
>>   	 */
>> -	if (!up->dma && rx_timeout) {
>> +	if (!(quirks & DW_UART_QUIRK_SKIP_RX_TIMEOUT_READ) && !up->dma && rx_timeout) {
> [Severity: High]
> The quirk avoids reading an empty FIFO in the IRQ handler, but will this
> fully protect the hardware from data aborts?
>
> The generic 8250 core code performs unconditional empty FIFO reads during
> port startup and shutdown. For example, serial8250_clear_interrupts()
> unconditionally reads UART_RX, and serial8250_do_shutdown() does the same
> immediately after clearing FIFOs.
>
> Won't opening or closing the TTY device still trigger a data abort on the
> TDA54 IP due to these core empty FIFO reads?


Actually this issue shouldn't come up

"reading an empty FIFO (by RBR) will be fine when the fifo is disabled"[0].

In the case of clear_interrupts the issue is fixed by the following 
commit 7f8fdd4dbffc ("serial: 8250: fix panic due to PSLVERR")

I will send another version of this V1 after pulling in the changes from [1]

[0] 
https://lore.kernel.org/linux-serial/ab256e4a-a9cc-410e-b889-f75fb01e77c7@picoheart.com/
[1] 
https://lore.kernel.org/linux-serial/20260908141524.20302-1-yang.yicong@picoheart.com/


Regards,
Moteen

>
>>   		status = serial_lsr_in(up);
>>   
>>   		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> [ ... ]
>
>> @@ -888,6 +889,11 @@ static const struct dw8250_platform_data dw8250_ultrarisc_dp1000_data = {
>>   	.quirks = DW_UART_QUIRK_CPR_VALUE,
>>   };
>>   
>> +static const struct dw8250_platform_data dw8250_tda54 = {
>> +	.usr_reg = DW_UART_USR,
>> +	.quirks = DW_UART_QUIRK_SKIP_RX_TIMEOUT_READ,
>> +};
>> +
>>   static const struct of_device_id dw8250_of_match[] = {
>>   	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
>>   	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
>> @@ -895,6 +901,7 @@ static const struct of_device_id dw8250_of_match[] = {
>>   	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
>>   	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_skip_set_rate_data },
>>   	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_skip_set_rate_data },
>> +	{ .compatible = "ti,tda54-uart", .data = &dw8250_tda54 },
> [Severity: High]
> This is a pre-existing issue, but does dw8250_probe() violate the runtime PM
> callback timing rules, which could cause a boot deadlock when this new
> compatible device is probed?
>
> dw8250_probe() calls serial8250_register_8250_port(), which synchronously
> invokes the dw8250_do_pm() callback. This happens before pm_runtime_enable()
> is called at the end of the probe function:
>
> drivers/tty/serial/8250/8250_dw.c:dw8250_probe() {
> 	...
> 	data->data.line = serial8250_register_8250_port(up);
> 	if (data->data.line < 0)
> 		return data->data.line;
>
> 	platform_set_drvdata(pdev, data);
>
> 	pm_runtime_enable(dev);
> 	...
> }
>
> Since dw8250_do_pm() uses pm_runtime_get_sync() before runtime PM is enabled,
> could this cause a circular dependency and hung worker threads during device
> registration?
>
>>   	{ .compatible = "ultrarisc,dp1000-uart", .data = &dw8250_ultrarisc_dp1000_data },
>>   	{ /* Sentinel */ }
>>   };