qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Anthony Liguori <aliguori@us.ibm.com>
To: qemu-devel@nongnu.org
Cc: Anthony Liguori <aliguori@us.ibm.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: [Qemu-devel] [PATCH 1/2] serial: fix retry logic
Date: Mon,  2 Apr 2012 09:39:38 -0500	[thread overview]
Message-ID: <1333377579-7513-2-git-send-email-aliguori@us.ibm.com> (raw)
In-Reply-To: <1333377579-7513-1-git-send-email-aliguori@us.ibm.com>

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

  reply	other threads:[~2012-04-02 14:39 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-02 14:39 [Qemu-devel] [PATCH 0/2] serial: some fixes for retry logic Anthony Liguori
2012-04-02 14:39 ` Anthony Liguori [this message]
2012-04-02 16:18   ` [Qemu-devel] [PATCH 1/2] serial: fix " Stefano Stabellini
2012-04-02 16:56     ` Anthony Liguori
2012-04-02 14:39 ` [Qemu-devel] [PATCH 2/2] serial: clear LSR.TEMT when populating the TSR Anthony Liguori
2012-04-02 15:55 ` [Qemu-devel] [PATCH 0/2] serial: some fixes for retry logic Anthony Liguori

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1333377579-7513-2-git-send-email-aliguori@us.ibm.com \
    --to=aliguori@us.ibm.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefano.stabellini@eu.citrix.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).