From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58737) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Ygm-00079Z-K8 for qemu-devel@nongnu.org; Mon, 15 Dec 2014 11:40:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0Ygf-0005e8-FA for qemu-devel@nongnu.org; Mon, 15 Dec 2014 11:40:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54357) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Ygf-0005du-5Y for qemu-devel@nongnu.org; Mon, 15 Dec 2014 11:39:57 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sBFGduTg028324 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 15 Dec 2014 11:39:56 -0500 Received: from playground.com (ovpn-112-26.ams2.redhat.com [10.36.112.26]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sBFGcXPQ031674 for ; Mon, 15 Dec 2014 11:39:55 -0500 From: Paolo Bonzini Date: Mon, 15 Dec 2014 17:38:24 +0100 Message-Id: <1418661511-22348-41-git-send-email-pbonzini@redhat.com> In-Reply-To: <1418661511-22348-1-git-send-email-pbonzini@redhat.com> References: <1418661511-22348-1-git-send-email-pbonzini@redhat.com> Subject: [Qemu-devel] [PULL 40/47] serial: clean up THRE/TEMT handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org - assert TEMT is cleared before sending a character; we'll get one from TSR if tsr_retry > 0, from the FIFO or THR otherwise - assert THRE cleared and FIFO not empty (if enabled) before fetching a character to send. This effectively reverts dffacd46, but the check makes no sense and commit f702e62 (serial: change retry logic to avoid concurrency, 2014-07-11) must have made it unnecessary. The commit message for f702e62 talks about multiple calls to qemu_chr_fe_add_watch triggering s->tsr_retry >= MAX_XMIT_RETRY, but other failures were possible. For example, if you have multiple calls, the subsequent ones will see s->tsr_retry == 0 and will find THRE and/or TEMT on entry. - for clarity, raise THRI immediately after the code sets THRE - check THRE to see if another character has to be sent. This makes the assertions more obvious and also means TEMT has to be set as soon as the loop ends. It makes the loop send both TSR and THR if flow-control happens in non-FIFO mode. Previously, THR would be lost. - clear TEMT together with THRE even in the non-FIFO case The last two items are bugfixes, but they were just found by inspection and do not squash known bugs. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Paolo Bonzini --- hw/char/serial.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/hw/char/serial.c b/hw/char/serial.c index 8c42d03..9adb126 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -224,21 +224,23 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque) SerialState *s = opaque; do { + assert(!(s->lsr & UART_LSR_TEMT)); if (s->tsr_retry <= 0) { + assert(!(s->lsr & UART_LSR_THRE)); + if (s->fcr & UART_FCR_FE) { - if (fifo8_is_empty(&s->xmit_fifo)) { - return FALSE; - } + assert(!fifo8_is_empty(&s->xmit_fifo)); s->tsr = fifo8_pop(&s->xmit_fifo); if (!s->xmit_fifo.num) { s->lsr |= UART_LSR_THRE; } - } else if ((s->lsr & UART_LSR_THRE)) { - return FALSE; } else { s->tsr = s->thr; s->lsr |= UART_LSR_THRE; - s->lsr &= ~UART_LSR_TEMT; + } + if ((s->lsr & UART_LSR_THRE) && !s->thr_ipending) { + s->thr_ipending = 1; + serial_update_irq(s); } } @@ -256,17 +258,13 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque) } else { s->tsr_retry = 0; } + /* Transmit another byte if it is already available. It is only possible when FIFO is enabled and not empty. */ - } while ((s->fcr & UART_FCR_FE) && !fifo8_is_empty(&s->xmit_fifo)); + } while (!(s->lsr & UART_LSR_THRE)); s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - - if (s->lsr & UART_LSR_THRE) { - s->lsr |= UART_LSR_TEMT; - s->thr_ipending = 1; - serial_update_irq(s); - } + s->lsr |= UART_LSR_TEMT; return FALSE; } @@ -323,10 +321,10 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val, fifo8_pop(&s->xmit_fifo); } fifo8_push(&s->xmit_fifo, s->thr); - s->lsr &= ~UART_LSR_TEMT; } s->thr_ipending = 0; s->lsr &= ~UART_LSR_THRE; + s->lsr &= ~UART_LSR_TEMT; serial_update_irq(s); if (s->tsr_retry <= 0) { serial_xmit(NULL, G_IO_OUT, s); -- 1.8.3.1