Re: [Qemu-devel] [PATCH 1/2] serial: fix retry logic

[Anthony Liguori <aliguori@us.ibm.com>]

On 04/02/2012 11:18 AM, Stefano Stabellini wrote:
> On Mon, 2 Apr 2012, Anthony Liguori wrote:
>> I'm not sure if the retry logic has ever worked when not using FIFO mode.  I
>> found this while writing a test case although code inspection confirms it is
>> definitely broken.
>>
>> The TSR retry logic will never actually happen because it is guarded by an
>> 'if (s->tsr_rety>  0)' but this is the only place that can ever make the
>> variable greater than zero.  That effectively makes the retry logic an 'if (0)'.
>>
>> I believe this is a typo and the intention was>= 0.
>
> I agree if you, I don't think there can be another explanation.

Thanks for the confirmation.  It's old code so I'm a bit surprised it hasn't 
been noticed yet :-)

>> Once this is fixed though,
>> I see double transmits with my test case.  This is because in the non FIFO
>> case, serial_xmit may get invoked while LSR.THRE is still high because the
>> character was processed but the retransmit timer was still active.
>
> If that is the case then this problem must be independent from the tsr_retry
> bug, considering that the code path you are changing is only taken when
> tsr_retry<= 0, right?

The double transmit is triggered by the xmit retry timer.  That timer will never 
get armed if tsr_retry < 0.

BTW, my test case requires a character device backend to return a short read. 
That's the only way to trigger this (as would be the case with serial device 
passthrough).

>
>> We can handle this by simply checking for LSR.THRE and returning early.
>> It's
>> possible that the FIFO paths also need some attention.
>
> The manual states: "In the FIFO mode this bit is set when the XMIT FIFO
> is empty; it is cleared when at least 1 byte is written to the XMIT
> FIFO", therefore I would return early if UART_LSR_THRE is set no matter
> if we are in FIFO mode or not.

I'll try to add FIFO mode to my test case and trigger the problem.  There's a 
bit more going on in the FIFO paths so it's not clear to me yet if it's needed here.

Regards,

Anthony Liguori

>