[Qemu-devel] [PATCH 0/2] serial: some fixes for retry logic

[Qemu-devel] [PATCH 0/2] serial: some fixes for retry logic

[Anthony Liguori]

Hi,

These are a few fixes I found while writing a unit test case for the serial
device.  I'm still working on the actual unit test but thought I'd send out the
fixes now.

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

[Anthony Liguori]

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.  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.

We can handle this by simply checking for LSR.THRE and returning early.  It's
possible that the FIFO paths also need some attention.

Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/serial.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/serial.c b/hw/serial.c
index c0ee55d..b499bca 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -327,6 +327,8 @@ static void serial_xmit(void *opaque)
             s->tsr = fifo_get(s,XMIT_FIFO);
             if (!s->xmit_fifo.count)
                 s->lsr |= UART_LSR_THRE;
+        } else if ((s->lsr & UART_LSR_THRE)) {
+            return;
         } else {
             s->tsr = s->thr;
             s->lsr |= UART_LSR_THRE;
@@ -337,7 +339,7 @@ static void serial_xmit(void *opaque)
         /* in loopback mode, say that we just received a char */
         serial_receive1(s, &s->tsr, 1);
     } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
-        if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
+        if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
             s->tsr_retry++;
             qemu_mod_timer(s->transmit_timer,  new_xmit_ts + s->char_transmit_time);
             return;
-- 
1.7.5.4

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

[Stefano Stabellini]

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.


> 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?


> 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.

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

[Anthony Liguori]

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

>

[Qemu-devel] [PATCH 2/2] serial: clear LSR.TEMT when populating the TSR

[Anthony Liguori]

We never actually clear the TEMT (transmit sending register empty) flag when
populating the TSR.  We set the flag, but since it's never cleared, setting it
is sort of pointless..

I found this with a unit test case.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/serial.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/serial.c b/hw/serial.c
index b499bca..a421d1e 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -332,6 +332,7 @@ static void serial_xmit(void *opaque)
         } else {
             s->tsr = s->thr;
             s->lsr |= UART_LSR_THRE;
+            s->lsr &= ~UART_LSR_TEMT;
         }
     }
 
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 0/2] serial: some fixes for retry logic

[Anthony Liguori]

On 04/02/2012 09:39 AM, Anthony Liguori wrote:
> Hi,
>
> These are a few fixes I found while writing a unit test case for the serial
> device.  I'm still working on the actual unit test but thought I'd send out the
> fixes now.

I've applied these since they're pretty trivial bug fixes.

Regards,

Anthony Liguori

>