Hi,
On 10/06/19 10:00 PM, Vignesh Raghavendra wrote:
> 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.
>
I haven't been able to find a fix yet. Please feel free to send a
revert. I will work on finding a fix for 8250 OMAP.
--
Regards
Vignesh
