| Message ID | 1418388243-1886-3-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
* 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 <pbonzini@redhat.com> > --- > 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
On 15/12/2014 12:40, Dr. David Alan Gilbert wrote: >> > 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. I think it's safe because: - serial_xmit is called from outside only after resetting TEMT and THRE and pushing a character on the FIFO - serial_xmit iterates a second time over do...while() only if the FIFO is not empty (both before and after this patch; this patch only changes the condition that is used) - if qemu_chr_fe_add_watch is called, the next call will have tsr_retry >= 1 and thus the "if" would be skipped. Note that in the middle we had commit f702e62 (serial: change retry logic to avoid concurrency, 2014-07-11) that fixed some messy behavior of qemu_chr_fe_add_watch. The commit message talks about multiple calls to qemu_chr_fe_add_watch triggering s->tsr_retry >= MAX_XMIT_RETRY but this is not the only possible failure. If you have multiple calls, the subsequent ones will see s->tsr_retry == 0 and will find (s->lsr & UART_LSR_THRE) != 0 on entry. But this should really never happen. (Thanks for making me think more about it. :)) Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 15/12/2014 12:40, Dr. David Alan Gilbert wrote: > >> > 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. > > I think it's safe because: > > - serial_xmit is called from outside only after resetting TEMT and THRE > and pushing a character on the FIFO Are you sure about TEMT? My reading of serial_ioport_write is that if !FCR_FE then TEMT isn't cleared. > - serial_xmit iterates a second time over do...while() only if the FIFO > is not empty (both before and after this patch; this patch only changes > the condition that is used) > > - if qemu_chr_fe_add_watch is called, the next call will have tsr_retry > >= 1 and thus the "if" would be skipped. > > Note that in the middle we had commit f702e62 (serial: change retry > logic to avoid concurrency, 2014-07-11) that fixed some messy behavior > of qemu_chr_fe_add_watch. The commit message talks about multiple calls > to qemu_chr_fe_add_watch triggering s->tsr_retry >= MAX_XMIT_RETRY but > this is not the only possible failure. If you have multiple calls, the > subsequent ones will see s->tsr_retry == 0 and will find (s->lsr & > UART_LSR_THRE) != 0 on entry. But this should really never happen. > > (Thanks for making me think more about it. :)) Ah yes, that changed things around a lot. Dave > > Paolo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 15/12/2014 16:21, Dr. David Alan Gilbert wrote: >> > >> > - serial_xmit is called from outside only after resetting TEMT and THRE >> > and pushing a character on the FIFO > Are you sure about TEMT? My reading of serial_ioport_write is that if > !FCR_FE then TEMT isn't cleared. This patch changes that. Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 15/12/2014 16:21, Dr. David Alan Gilbert wrote: > >> > > >> > - serial_xmit is called from outside only after resetting TEMT and THRE > >> > and pushing a character on the FIFO > > Are you sure about TEMT? My reading of serial_ioport_write is that if > > !FCR_FE then TEMT isn't cleared. > > This patch changes that. Oh yeh! Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Paolo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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)); 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);
- 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 <pbonzini@redhat.com> --- hw/char/serial.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)