Re: [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> It can never become negative; reflect this in the type of the field
> and simplify the conditions.

OK, yes, the one case that could make it go -ve was removed in fcfb4d6
somewhere after 1.4.0.


> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/char/serial.c         | 12 ++++++++----
>  include/hw/char/serial.h |  2 +-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 6d815b5..e65e9e0 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -229,7 +229,7 @@ static gboolean serial_xmit(GIOChannel *chan, 
> GIOCondition cond, void *opaque)
>  
>      do {
>          assert(!(s->lsr & UART_LSR_TEMT));
> -        if (s->tsr_retry <= 0) {
> +        if (s->tsr_retry == 0) {
>              assert(!(s->lsr & UART_LSR_THRE));
>  
>              if (s->fcr & UART_FCR_FE) {
> @@ -252,7 +252,7 @@ static gboolean serial_xmit(GIOChannel *chan, 
> GIOCondition cond, 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 < MAX_XMIT_RETRY &&
>                  qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
>                                        serial_xmit, s) > 0) {
>                  s->tsr_retry++;
> @@ -330,7 +330,7 @@ static void serial_ioport_write(void *opaque, hwaddr 
> addr, uint64_t val,
>              s->lsr &= ~UART_LSR_THRE;
>              s->lsr &= ~UART_LSR_TEMT;
>              serial_update_irq(s);
> -            if (s->tsr_retry <= 0) {
> +            if (s->tsr_retry == 0) {
>                  serial_xmit(NULL, G_IO_OUT, s);
>              }
>          }
> @@ -639,6 +639,10 @@ static int serial_post_load(void *opaque, int version_id)
>      if (s->thr_ipending == -1) {
>          s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
>      }
> +    if (s->tsr_retry > MAX_XMIT_RETRY) {
> +        s->tsr_retry = MAX_XMIT_RETRY;
> +    }

Good that makes it safe even after receiving the case that could be -1.

Reviewed-by: Dr. David Alan Gilbert <address@hidden>


>      s->last_break_enable = (s->lcr >> 6) & 1;
>      /* Initialize fcr via setter to perform essential side-effects */
>      serial_write_fcr(s, s->fcr_vmstate);
> @@ -685,7 +689,7 @@ static const VMStateDescription vmstate_serial_tsr = {
>      .minimum_version_id = 1,
>      .needed = serial_tsr_needed,
>      .fields = (VMStateField[]) {
> -        VMSTATE_INT32(tsr_retry, SerialState),
> +        VMSTATE_UINT32(tsr_retry, SerialState),
>          VMSTATE_UINT8(thr, SerialState),
>          VMSTATE_UINT8(tsr, SerialState),
>          VMSTATE_END_OF_LIST()
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 15beb6b..ef90615 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -55,7 +55,7 @@ struct SerialState {
>      int last_break_enable;
>      int it_shift;
>      int baudbase;
> -    int tsr_retry;
> +    unsigned tsr_retry;
>      uint32_t wakeup;
>  
>      /* Time when the last byte was successfully sent out of the tsr */
> -- 
> 2.5.5
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK