Hi,
On 09/06/19 5:20 PM, Oliver Barta wrote:
> Hello,
>
> there is a small issue in serial8250_handle_irq function.
>
> status = serial_port_in(port, UART_LSR);
>
> if (status & (UART_LSR_DR | UART_LSR_BI) &&
> iir & UART_IIR_RDI) {
> if (!up->dma || handle_rx_dma(up, iir))
> status = serial8250_rx_chars(up, status);
> }
>
> The line status register is read unconditionally but the contained
> error flags are processed only if UART_IIR_RDI bit is set in iir
> variable which was updated by a read of the interrupt identification
> register which happened before the read of LSR. It is unlikely but
> under certain timing conditions (steps for testing below) it may
> happen that some error flags are set while the UART_IIR_RDI bit is
> cleared in iir variable. In this case the error information (e.g.
> framing error or parity error) obtained from LSR will be lost as the
> bits in the hardware register are cleared by the read.
>
Hmm, I see a different behavior on 8250 OMAP UARTs. LSR status does not
clear/reset back, until associated character is read from UART_RX
register (TRM has this behavior documented). So unconditional read of
LSR register w/o processing received character seems ok for OMAP UART.
This seems to be different wrt Designware UART.
>
> This problem was introduced by commit
>
> 2e9fe539108320820016f78ca7704a7342788380 serial: 8250: Don't service
> RX FIFO if interrupts are disabled
>
> I see two possible solutions, either reverting this change or saving
> the error flags with
>
> up->lsr_saved_flags |= status & LSR_SAVE_FLAGS;
>
> in case they are not processed right away as done at several other
> places. I would vote for reverting it but I would like to hear a
> second opinion first.
>
>
> The commit message explains that the original change was required
> because some OMAP UART driver disables UART RX FIFO interrupts as
> response to a throttling request and it needs to be ensured that UART
> RX FIFO processing is actually stopped if the corresponding interrupts
> are disabled. I'm not sure if this is the right way to handle a
> throttling request. Based on the documentation in Documentation/serial
> I would expect a throttling request to only trigger hardware flow
> control to inform the sender that it should stop sending data. It
> should be the responsibility of the sender to act accordingly.
8250 OMAP UART support automatic HW flow control, which means HW manages
RTS signal automatically depending of FIFO level. Flow control is
asserted as soon as pre defined RX FIFO threshold is reached and signal
is deasserted once driver drains RX FIFO.
I am not sure if there is a way to assert signal manually when auto flow
control is enabled. Need to explore this option.
> Stopping processing of the input FIFO seems problematic to me as this
> FIFO is typically small and will likely overflow if input processing
> is stopped immediately.
>
Not really, idea is to stop servicing RX FIFO interrupt and allow RX
FIFO to fill up. Once, FIFO level reaches threshold (3/4 of FIFO in 8250
OMAP UART) flow control line is automatically asserted by UART HW due to
autoRTS.
Also, even though driver disables the interrupt in throttle callback, it
would still service currently pending interrupt which would drain FIFO
completely at least once and then wait for FIFO to fill up to 3/4 before
flow control is asserted. 8250 OMAP UART as 64 byte FIFO and seems to
handle this situation well w/o any HW FIFO overruns in my testing.
I understand that LSR register read as different effect on DesignWare
platforms. Will experiment bit more and come up with a more generic fix
to fix throttling problem within next couple of days.
Otherwise will send a revert for the offending patch.
Regards
Vignesh
>
> Steps to trigger this issue:
> 1) Send a data packet which is larger than the TX FIFO.
> 2) Trigger an external function generator on the outgoing data packet
> which is programmed to send a character with a parity error with a
> specific delay relative to the outgoing packet so that the character
> is received while the UART_IIR_THRI interrupt is processed in order to
> reload the TX FIFO. In particular it needs to be received between the
> read of IIR and LSR. (During my testing I added a delay of several
> micro seconds between both read operations to increase the probability
> for this to happen.)
> 3) Check if parity error information was properly processed.
>
> I was testing on an Intel SOC with integrated DesignWare 16550A
> compatible UART without DMA support.
>
> Best regards,
> Oliver Barta
>
--
Regards
Vignesh
