Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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