diff mbox series

[v2,2/2] hw/lpc-uart.c: Handle read failure case in UART communication

Message ID 20250410004324.89383-6-abhishek@linux.ibm.com
State Accepted
Headers show
Series Fixed UART handling | expand

Commit Message

ABHISHEK SINGH TOMAR April 10, 2025, 12:43 a.m. UTC 
From: Abhishek Singh Tomar <abhishek@linux.ibm.com>

Added logic to handle cases where the Line Status Register (LSR)
reads 0xFF, which may indicate an error in reading the
register through LPC or the presence of multiple simultaneous UART errors.
previously, This false read of set bit lead to soft lock or hand in older
production systems.

In such scenarios, processing data read/write operations does
not make sense. The function now returns `false` to signal
the failure and halt further operations.

Signed-off-by: Abhishek Singh Tomar <abhishek@linux.ibm.com>
---
 hw/lpc-uart.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Aditya Gupta April 11, 2025, 9:41 a.m. UTC | #1 
On 25/04/10 06:13AM, ABHISHEK SINGH TOMAR wrote:
> From: Abhishek Singh Tomar <abhishek@linux.ibm.com>
> <...snip...>
>
> -	if (uart_read(REG_LSR) & LSR_THRE) {
> +	reg = uart_read(REG_LSR);
> +	if (reg & LSR_THRE && reg != 0xff) {

Nitpick: Maybe we can have: ((reg & LSR_THRE) && (reg != 0xff)). That
seems more clear at what is being compared.

Thanks,
- Aditya G
Reza Arbab May 16, 2025, 3:06 p.m. UTC | #2 
On Fri, Apr 11, 2025 at 03:11:45PM +0530, Aditya Gupta wrote:
>On 25/04/10 06:13AM, ABHISHEK SINGH TOMAR wrote:
>> From: Abhishek Singh Tomar <abhishek@linux.ibm.com>
>> <...snip...>
>>
>> -	if (uart_read(REG_LSR) & LSR_THRE) {
>> +	reg = uart_read(REG_LSR);
>> +	if (reg & LSR_THRE && reg != 0xff) {
>
>Nitpick: Maybe we can have: ((reg & LSR_THRE) && (reg != 0xff)). That
>seems more clear at what is being compared.

I think the most readable is to use two if statements:

	reg = uart_read(REG_LSR);
	if (reg == 0xff)
		return false;

	if (reg & LSR_THRE) {
		...
	}

Fixed the patch up as above and applied to master.
diff mbox series

Patch

diff --git a/hw/lpc-uart.c b/hw/lpc-uart.c
index a5ffc5f22..33955b29c 100644
--- a/hw/lpc-uart.c
+++ b/hw/lpc-uart.c
@@ -98,10 +98,12 @@  static inline void uart_write(unsigned int reg, uint8_t val)
 
 static bool uart_check_tx_room(void)
 {
+	uint8_t reg;
+
 	if (tx_room)
 		return true;
-
-	if (uart_read(REG_LSR) & LSR_THRE) {
+	reg = uart_read(REG_LSR);
+	if (reg & LSR_THRE && reg != 0xff) {
 		/* FIFO is 16 entries */
 		tx_room = 16;
 		tx_full = false;