Hi, > -----Original Message----- > From: Ian Arkver <ian.arkver.dev@xxxxxxxxx> > Sent: Friday, 03 May, 2019 17:57 > To: Rautkoski Kimmo EXT <ext-kimmo.rautkoski@xxxxxxxxxxx>; > gregkh@xxxxxxxxxxxxxxxxxxx > Cc: linux-serial@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] serial: 8250: Fix TX interrupt handling condition > > 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. > This would work too, I have tested it on my setup. Maybe this is a better approach in the sense that it does not break implementations with buggy 8250. Thank you for pointing this out. I'll prepare v3 patch with this change. BR, Kimmo > Disclaimer: I don't have any such devices either. > > Regards, > Ian > > > > > BR, > > Kimmo > >
