From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0U0v-0005kK-GO for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:40:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0U0r-0007Ek-3Y for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:40:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58564) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0U0q-0007Ee-RC for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:40:29 -0500 Date: Mon, 15 Dec 2014 11:40:19 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20141215114018.GC5502@work-vm> References: <1418388243-1886-1-git-send-email-pbonzini@redhat.com> <1418388243-1886-3-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1418388243-1886-3-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: imammedo@redhat.com, andrey@xdel.ru, qemu-devel@nongnu.org, dslutz@verizon.com, batuzovk@ispras.ru * Paolo Bonzini (pbonzini@redhat.com) wrote: > - assert THRE cleared and FIFO not empty (if enabled) before > sending a character. Also assert TEMT cleared, since it is > the combination of THRE && transmitter shift register empty. > > - raise THRI immediately after setting THRE > > - check THRE to see if another character has to be sent, > which makes the assertions more obvious and also means TEMT > has to be set as soon as the loop ends > > - clear TEMT together with THRE even in the non-FIFO case > > There are certainly a couple bugfixes in here, but nothing that > squashes known bugs. > > 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..4bce268 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)); > + assert(!(s->lsr & UART_LSR_THRE)); > + > if (s->tsr_retry <= 0) { > if (s->fcr & UART_FCR_FE) { > - if (fifo8_is_empty(&s->xmit_fifo)) { > - return FALSE; > - } > + assert(!fifo8_is_empty(&s->xmit_fifo)); That's undoing dslutz@verizon.com's dffacd46 - Fix emptyness checking See, http://permalink.gmane.org/gmane.comp.emulators.qemu/262412 I don't think your assumptions are safe because of that qemu_chr_fe_add_watch. Dave > 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 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK