Hi.
On 03/05/2019 13:10, Rautkoski Kimmo EXT wrote:
-----Original Message-----
From: gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx>
Sent: Monday, 29 April, 2019 17:19
To: Rautkoski Kimmo EXT <ext-kimmo.rautkoski@xxxxxxxxxxx>
Cc: linux-serial@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] serial: 8250: Fix TX interrupt handling condition
On Fri, Apr 26, 2019 at 12:06:13PM +0000, Rautkoski Kimmo EXT wrote:
Interrupt handler checked THRE bit (transmitter holding register
empty) in LSR to detect if TX fifo is empty.
In case when there is only receive interrupts the TX handling
got called because THRE bit in LSR is set when there is no
transmission (FIFO empty). TX handling caused TX stop, which in
RS-485 half-duplex mode actually resets receiver FIFO. This is not
desired during reception because of possible data loss.
The fix is to use IIR instead of LSR to detect the TX fifo status.
This ensures that TX handling is only called when there is really
an interrupt for THRE and not when there is only RX interrupts.
Signed-off-by: Kimmo Rautkoski <ext-kimmo.rautkoski@xxxxxxxxxxx>
---
drivers/tty/serial/8250/8250_port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index d2f3310..91ca0ca 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1875,7 +1875,7 @@ int serial8250_handle_irq(struct uart_port *port,
unsigned int iir)
status = serial8250_rx_chars(up, status);
}
serial8250_modem_status(up);
- if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE))
+ if ((!up->dma || up->dma->tx_err) && (iir & UART_IIR_THRI))
serial8250_tx_chars(up);
This feels wrong to me, can someone else test this to verify that it
really does work properly? I don't have access to any 8250 devices at
the moment :(
thanks,
greg k-h
Thanks for checking this. There is indeed a problem with the patch. Interrupt ID in Interrupt Identification register is actually 3 bits, so the check should be different: ((iir & UART_IIR_ID) == UART_IIR_THRI)
I'll send v2 of the patch soon.
Rather than switching to the IIR which can have other meanings for
devices with FIFOs (eg. FIFO space avail), or may be flaky (see
UART_BUG_THRE), could you continue to use the LSR THRE bit but also
check for up->ier & UART_IER_THRI. This is set in __start_tx and cleared
in __do_stop_tx.
It's really just doing in software what the IIR hardware should in
theory be doing for a regular 8250.
Disclaimer: I don't have any such devices either.
Regards,
Ian
BR,
Kimmo
